Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 18 additions & 10 deletions src/BloomBrowserUI/publish/commonPublish/PublishProgressDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ export const PublishProgressDialog: React.FunctionComponent<{
const [accumulatedMessages, setAccumulatedMessages] = useState("");

const [errorEncountered, setErrorEncountered] = useState(false);
const [interestingMessageEncountered, setInterestingMessageEncountered] =
useState(false);

const {
setProgressState: setProgressStateProp,
Expand All @@ -47,6 +49,7 @@ export const PublishProgressDialog: React.FunctionComponent<{
setAccumulatedMessages("");
setInstructionMessage(undefined);
setErrorEncountered(false);
setInterestingMessageEncountered(false);
}, [setProgressStateProp, setClosePendingProp]);

//Note, originally this was just a function, closeIfNoError().
Expand All @@ -55,12 +58,8 @@ export const PublishProgressDialog: React.FunctionComponent<{
// update we notice that and see about closing.
React.useEffect(() => {
if (props.closePending) {
if (errorEncountered) {
setProgressStateProp(() =>
errorEncountered
? ProgressState.Done
: ProgressState.Closed,
);
if (errorEncountered || interestingMessageEncountered) {
setProgressStateProp(ProgressState.Done);
// Although we may be in state 'Done' and thus not actually closed yet,
// we're no longer in the state that closePending is meant to handle,
// where we've finished but don't yet have enough information to know
Expand All @@ -75,11 +74,15 @@ export const PublishProgressDialog: React.FunctionComponent<{
}
}, [
props.closePending,
errorEncountered,
setProgressStateProp,
setClosePendingProp,
closeAndResetDialog,
interestingMessageEncountered,
]);

// Kick off the server-side task only after the websocket is ready,
// so we don't miss early progress messages.
React.useEffect(() => {
props.setProgressState(ProgressState.Working);
// we need to be ready to listen to progress messages from the server,
Expand All @@ -103,7 +106,12 @@ export const PublishProgressDialog: React.FunctionComponent<{
},
);
});
}, [props.apiForStartingTask, props.generation]); // Every time the start API endpoint changes, we basically restart the component
}, [
props.apiForStartingTask,
props.generation,
props.setProgressState,
props.webSocketClientContext,
]); // Every time the start API endpoint changes, we basically restart the component

useSubscribeToWebSocketForEvent(
props.webSocketClientContext,
Expand All @@ -122,10 +130,10 @@ export const PublishProgressDialog: React.FunctionComponent<{
case "Warning":
setErrorEncountered(true);
// deliberately fall through
case "Progress":

// eslint-disable-next-line no-fallthrough
case "Note":
setInterestingMessageEncountered(true);
// deliberately fall through
case "Progress":
setAccumulatedMessages(
(oldMessages) => oldMessages + html,
);
Expand Down
4 changes: 2 additions & 2 deletions src/BloomExe/FontProcessing/FontFileFinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,10 @@ public string GetFileForFont(string fontName, string fontStyle, string fontWeigh
{
if (!string.IsNullOrEmpty(group.BoldItalic))
return group.BoldItalic;
else if (!string.IsNullOrEmpty(group.Bold))
return group.Bold;
else if (!string.IsNullOrEmpty(group.Italic))
return group.Italic;
else if (!string.IsNullOrEmpty(group.Bold))
return group.Bold;
else if (!string.IsNullOrEmpty(group.Normal))
return group.Normal;
}
Expand Down
3 changes: 2 additions & 1 deletion src/BloomExe/Publish/BloomPub/BloomPubMaker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -930,14 +930,15 @@ out HashSet<string> badFonts
}
// Create the fonts.css file, which tells the browser where to find the fonts for those families.
var sb = new StringBuilder();
var normalFacesAdded = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
foreach (var font in fontsWanted.OrderBy(x => x.ToString()))
{
if (badFonts.Contains(font.fontFamily))
continue;
var group = fontFileFinder.GetGroupForFont(font.fontFamily);
if (group != null)
{
EpubMaker.AddFontFace(sb, font, group);
EpubMaker.AddFontFace(sb, font, group, normalFacesAdded);
Comment on lines 937 to +941
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BloomPubMaker embeds font files using PublishHelper.CheckFontsForEmbedding (which uses FontFileFinder.GetFileForFont). Since EpubMaker.AddFontFace may now choose a different face file for a bold+italic request (preferring Italic over Bold when BoldItalic is missing), fonts.css can reference a face file that wasn't copied into the BloomPUB folder. Please ensure the file-copy list matches the face that AddFontFace emits (e.g., align GetFileForFont / embedding selection with AddFontFace’s cascade).

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this and the comment in EpubMaker are two sides of the same coin.

I had copilot evaluate these and it agreed this is a bug.
It wants to fix it by swapping the processing order in FontFileFinder.GetFileForFont:
image.png

}
// We don't need (or want) a rule to use Andika instead.
// The reader typically WILL use Andika, because we have a rule making it the default font
Expand Down
137 changes: 123 additions & 14 deletions src/BloomExe/Publish/Epub/EpubMaker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2791,6 +2791,7 @@ out HashSet<string> badFonts
}
}
var sb = new StringBuilder();
var normalFacesAdded = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
foreach (var font in _fontsUsedInBook.OrderBy(x => x.ToString()))
{
if (badFonts.Contains(font.fontFamily))
Expand All @@ -2816,7 +2817,14 @@ out HashSet<string> badFonts
// The fonts.css file is stored in a subfolder as are the font files. They are in different
// subfolders, and the reference to the font file has to take the relative path to fonts.css
// into account.
AddFontFace(sb, font, group, "../" + kFontsFolder + "/", true);
AddFontFace(
sb,
font,
group,
normalFacesAdded,
"../" + kFontsFolder + "/",
true
);
}
}
if (
Expand Down Expand Up @@ -2889,26 +2897,127 @@ internal static void AddFontFace(
StringBuilder sb,
PublishHelper.FontInfo font,
FontGroup group,
HashSet<string> normalFacesAdded,
string relativePathFromCss = "",
bool sanitizeFileName = false
)
{
var weight = font.fontWeight == "700" ? "bold" : "normal";
string path = null;
if (font.fontStyle == "italic" && font.fontWeight == "700")
path = group.BoldItalic;
if (string.IsNullOrEmpty(path) && font.fontStyle == "italic")
path = group.Italic;
if (string.IsNullOrEmpty(path) && font.fontWeight == "700")
path = group.Bold;
if (string.IsNullOrEmpty(path))
path = group.Normal;
// What we are doing here (See BL-15558)
// On the one hand, we have the fonts that the html/css call for.
// On the other hand, we have the actual fonts (not typefaces, actual "fonts")
// that are available on this machine. When we "add a font face" here, we are
// creating a css rule that points to an actual font file that will be embedded.
// If the font face we want isn't available, the browser can synthesize one.
// So for example if you want italic, but we only have normal, that's fine we
// have no italic font face to add, so we just add the normal one and the browser will
// do its best. This is what is needed for WYSIWYG, because that is what is happening
// in the Editor view. But if you *do* have a font file matching the requested face,
// then we want to emit a font-face rule for it which points to the font file.

var wantsItalic = font.fontStyle == "italic";
var wantsBold = font.fontWeight == "700";
string chosenPath = null;
string declaredWeight = null;
string declaredStyle = null;

// Step 1: Find the best available face for the requested style/weight.
// For bold-italic requests, we prefer: BoldItalic > Italic > Bold > Normal
// This cascade means the browser only synthesizes what's missing (e.g., if we have
// Italic but not BoldItalic, the browser just synthesizes bold on top of real italic).
// For non-combined requests (just bold or just italic), we require an exact match
// or fall back to Normal in Step 3.
if (wantsItalic && wantsBold)
{
// For combined requests, emit the most specific real face we have and let the
// browser synthesize the missing axis.
if (!string.IsNullOrEmpty(group.BoldItalic))
{
chosenPath = group.BoldItalic;
declaredWeight = "bold";
declaredStyle = "italic";
}
// the order of italic vs bold here is intentional,
// the thinking being that bold is easier to synthesize than italic,
// so if we have to pick just one, we prefer to provide the italic face
// and let the browser fake the bold.
else if (!string.IsNullOrEmpty(group.Italic))
{
chosenPath = group.Italic;
declaredWeight = "normal";
declaredStyle = "italic";
}
Comment on lines +2943 to +2948
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AddFontFace can select group.Italic as the embedded face for a bold+italic request when BoldItalic is missing. However the embedding/copy step (PublishHelper.CheckFontsForEmbedding -> FontFileFinder.GetFileForFont) currently prefers group.Bold over group.Italic for bold+italic requests, so the EPUB can end up embedding only the bold file while fonts.css references the italic file (missing in the package). Please make the embedding selection logic match this chosenPath (e.g., change GetFileForFont ordering for bold+italic or embed both Bold and Italic when BoldItalic is unavailable).

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK fixed in the FontFileFinder as per Andrew's suggestion.

else if (!string.IsNullOrEmpty(group.Bold))
{
chosenPath = group.Bold;
declaredWeight = "bold";
declaredStyle = "normal";
}
else if (!string.IsNullOrEmpty(group.Normal))
{
chosenPath = group.Normal;
declaredWeight = "normal";
declaredStyle = "normal";
}
}
else if (wantsItalic && !wantsBold && !string.IsNullOrEmpty(group.Italic))
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Condition is always false because of access to local variable wantsItalic.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hallucination

{
chosenPath = group.Italic;
declaredWeight = "normal";
declaredStyle = "italic";
}
else if (wantsBold && !wantsItalic && !string.IsNullOrEmpty(group.Bold))
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Condition is always false because of access to local variable wantsBold.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hallucination

{
chosenPath = group.Bold;
declaredWeight = "bold";
declaredStyle = "normal";
}
else if (!wantsItalic && !wantsBold && !string.IsNullOrEmpty(group.Normal))
{
chosenPath = group.Normal;
declaredWeight = "normal";
declaredStyle = "normal";
}

if (!string.IsNullOrEmpty(chosenPath))
{
// Step 2: only emit one normal/normal rule per family to keep output deterministic.
// Note: we could still duplicate non-normal faces if a bold-italic request falls back
// to italic/bold that is also directly requested. This is rare and harmless, so we
// do not add broader dedupe logic.
if (declaredStyle == "normal" && declaredWeight == "normal")
{
if (normalFacesAdded.Contains(font.fontFamily))
return;
normalFacesAdded.Add(font.fontFamily);
}
AddFontFace(
sb,
font.fontFamily,
declaredWeight,
declaredStyle ?? font.fontStyle,
chosenPath,
relativePathFromCss,
sanitizeFileName
);
return;
}

// Step 3: no exact face. Don't fake a bold/italic face by pointing at some other file.
// Instead, declare just a normal/normal face so the reading browser can synthesize
// missing styles, matching what happens in Bloom's editor (See BL-15558).
if (string.IsNullOrEmpty(group.Normal))
// If there's no Normal slot for this family, we avoid substituting bold/italic
// here; that would misrepresent the file and still not satisfy a normal request.
return;
if (normalFacesAdded.Contains(font.fontFamily))
return;
normalFacesAdded.Add(font.fontFamily);
AddFontFace(
sb,
font.fontFamily,
weight,
font.fontStyle,
path,
"normal",
"normal",
group.Normal,
relativePathFromCss,
sanitizeFileName
);
Expand Down
109 changes: 109 additions & 0 deletions src/BloomTests/Publish/Epub/ExportEpubTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Xml.Linq;
using Bloom;
using Bloom.Book;
using Bloom.FontProcessing;
using Bloom.Publish;
using Bloom.Publish.Epub;
using Bloom.SafeXml;
Expand Down Expand Up @@ -2560,6 +2561,114 @@ public void AddFontFace_WithNullPath_AddsNothing()
Assert.That(sb.Length, Is.EqualTo(0));
}

[Test]
public void AddFontFace_MissingItalicFace_DoesNotDeclareItalicStyle()
{
var sb = new StringBuilder();
var group = new FontGroup { Normal = "Test Font R.ttf" };
var normalFacesAdded = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
var font = new PublishHelper.FontInfo
{
fontFamily = "Test Font",
fontStyle = "italic",
fontWeight = "400",
};

EpubMaker.AddFontFace(sb, font, group, normalFacesAdded);

var css = sb.ToString();
Assert.That(css, Does.Contain("font-style:normal"));
Assert.That(css, Does.Not.Contain("font-style:italic"));
Assert.That(css, Does.Contain("Test Font R.ttf"));
}

[Test]
public void AddFontFace_BoldItalicRequest_WithOnlyItalic_DeclaresItalicNormalFace()
{
var sb = new StringBuilder();
var group = new FontGroup { Italic = "Test Font I.ttf" };
var normalFacesAdded = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
var font = new PublishHelper.FontInfo
{
fontFamily = "Test Font",
fontStyle = "italic",
fontWeight = "700",
};

EpubMaker.AddFontFace(sb, font, group, normalFacesAdded);

var css = sb.ToString();
Assert.That(css, Does.Contain("font-style:italic"));
Assert.That(css, Does.Contain("font-weight:normal"));
Assert.That(css, Does.Contain("Test Font I.ttf"));
Assert.That(css, Does.Not.Contain("font-weight:bold"));
}

[Test]
public void AddFontFace_BoldItalicRequest_WithOnlyBold_DeclaresBoldNormalStyleFace()
{
var sb = new StringBuilder();
var group = new FontGroup { Bold = "Test Font B.ttf" };
var normalFacesAdded = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
var font = new PublishHelper.FontInfo
{
fontFamily = "Test Font",
fontStyle = "italic",
fontWeight = "700",
};

EpubMaker.AddFontFace(sb, font, group, normalFacesAdded);

var css = sb.ToString();
Assert.That(css, Does.Contain("font-style:normal"));
Assert.That(css, Does.Contain("font-weight:bold"));
Assert.That(css, Does.Contain("Test Font B.ttf"));
Assert.That(css, Does.Not.Contain("font-style:italic"));
}

[Test]
public void AddFontFace_BoldItalicRequest_WithItalicAndBold_PrefersItalicForSynthesis()
{
var sb = new StringBuilder();
var group = new FontGroup { Bold = "Test Font B.ttf", Italic = "Test Font I.ttf" };
var normalFacesAdded = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
var font = new PublishHelper.FontInfo
{
fontFamily = "Test Font",
fontStyle = "italic",
fontWeight = "700",
};

EpubMaker.AddFontFace(sb, font, group, normalFacesAdded);

var css = sb.ToString();
Assert.That(css, Does.Contain("font-style:italic"));
Assert.That(css, Does.Contain("font-weight:normal"));
Assert.That(css, Does.Contain("Test Font I.ttf"));
Assert.That(css, Does.Not.Contain("Test Font B.ttf"));
}

[Test]
public void AddFontFace_BoldItalicRequest_WithBoldItalic_DeclaresBoldItalicFace()
{
var sb = new StringBuilder();
var group = new FontGroup { BoldItalic = "Test Font BI.ttf" };
var normalFacesAdded = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
var font = new PublishHelper.FontInfo
{
fontFamily = "Test Font",
fontStyle = "italic",
fontWeight = "700",
};

EpubMaker.AddFontFace(sb, font, group, normalFacesAdded);

var css = sb.ToString();
Assert.That(css, Does.Contain("font-style:italic"));
Assert.That(css, Does.Contain("font-weight:bold"));
Assert.That(css, Does.Contain("Test Font BI.ttf"));
}

[Test]
public void MultilingualTitlesOrderedCorrectly()
{
Expand Down