Merge pull request #7644 from BloomBooks/BL-15558FixFontEmbedding

Fix BL-15558 preserve bloompub font simulation

Author: andrew-polk

src/BloomBrowserUI/publish/commonPublish/PublishProgressDialog.tsx

@@ -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,
@@ -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().
@@ -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
@@ -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,
@@ -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,
@@ -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,
                         );

src/BloomExe/FontProcessing/FontFileFinder.cs

@@ -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;
                 }

src/BloomExe/Publish/BloomPub/BloomPubMaker.cs

@@ -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);
                 }
                 // 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

src/BloomExe/Publish/Epub/EpubMaker.cs

@@ -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))
@@ -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 (
@@ -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";
+                }
+                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))
+            {
+                chosenPath = group.Italic;
+                declaredWeight = "normal";
+                declaredStyle = "italic";
+            }
+            else if (wantsBold && !wantsItalic && !string.IsNullOrEmpty(group.Bold))
+            {
+                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
             );

src/BloomTests/Publish/Epub/ExportEpubTests.cs

@@ -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;
@@ -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()
         {