Fix BL-15558 preserve bloompub font simulation by hatton · Pull Request #7644 · BloomBooks/BloomDesktop

hatton · 2026-01-31T15:58:54Z

Also, give the user a chance to read notes like ones about font embedding.

This change is

devin-ai-integration

Devin Review found 1 potential issue.

View issue and 5 additional flags in Devin Review.

src/BloomBrowserUI/publish/commonPublish/PublishProgressDialog.tsx

src/BloomExe/Publish/Epub/EpubMaker.cs

devin-ai-integration

Devin Review found 1 new potential issue.

View issue and 8 additional flags in Devin Review.

src/BloomExe/Publish/Epub/EpubMaker.cs

devin-ai-integration

Devin Review found 1 new potential issue.

View issue and 8 additional flags in Devin Review.

src/BloomBrowserUI/publish/commonPublish/PublishProgressDialog.tsx

devin-ai-integration

Devin Review found 1 new potential issue.

View issue and 8 additional flags in Devin Review.

src/BloomBrowserUI/publish/commonPublish/PublishProgressDialog.tsx

JohnThomson

@JohnThomson reviewed 4 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hatton).

a discussion (no related file):
Already a big improvement. I think it could go as-is, but I'll let you consider a couple of suggestions.

src/BloomBrowserUI/publish/commonPublish/PublishProgressDialog.tsx line 140 at r6 (raw file):

                        if (progressEvent.progressKind === "Note") {
                            setInterestingMessageEncountered(true);
                        }

Sort of messy. How about
case "Warning":
setErrorEncountered(true);
// fall-through
case "Note":
setInterestingMessageEncountered(true);
// fall-through
case "Progress":

src/BloomExe/Publish/Epub/EpubMaker.cs line 3005 at r6 (raw file):

            // missing styles, matching what happens in Bloom's editor (See BL-15558).
            if (string.IsNullOrEmpty(group.Normal))
                return;

A few fonts exist only in Italic versions (AI noted 'Baron') or only in Bold (Google Fonts Alfa Slab One). Presumably the browser will attempt to synthesize normal from italic or bold if it must? Does that mean we need to include the group.Italic or group.Bold paths (with the appropriate weight/style values) if those are all that exist? (Might unfortunately complicate what you're doing with normalFacesAdded...I'm not sure).

andrew-polk

@andrew-polk reviewed 3 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @hatton).

src/BloomBrowserUI/publish/commonPublish/PublishProgressDialog.tsx line 128 at r6 (raw file):

            const html = `<span class='${progressEvent.progressKind}'>${e.message}</span><br/>`;
            if (e.id == "message") {
                switch (progressEvent.progressKind) {

I think this is clearer:

                switch (progressEvent.progressKind) {
                    case "Error":
                    case "Warning":
                        setErrorEncountered(true);
                    // deliberately fall through
                    case "Note":
                        setInterestingMessageEncountered(true);
                    // deliberately fall through
                    case "Progress":
                        setAccumulatedMessages(
                            (oldMessages) => oldMessages + html,
                        );
                        break;
                    case "Instruction":
                        setInstructionMessage(e.message);
                }

src/BloomExe/Publish/Epub/EpubMaker.cs line 3001 at r6 (raw file):

            }

            // Step 4: no exact face. Don't fake a bold/italic face by pointing at some other file.

I think this is step 3, not 4.
Note that we refer to it above. So that comment should also be changed to "3".

andrew-polk

@andrew-polk reviewed 1 file.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @hatton).

hatton

@hatton made 5 comments.
Reviewable status: 2 of 4 files reviewed, 5 unresolved discussions (waiting on @andrew-polk and @JohnThomson).

a discussion (no related file):

Previously, JohnThomson (John Thomson) wrote…

Already a big improvement. I think it could go as-is, but I'll let you consider a couple of suggestions.

Done. I'd actually rather reviews just merge though rather than get into polishing. Making things a little clearer, when no human will probably ever read this, isn't really worth the time it takes to write up and change.

src/BloomBrowserUI/publish/commonPublish/PublishProgressDialog.tsx line 128 at r6 (raw file):

Previously, andrew-polk wrote…

I think this is clearer:

                switch (progressEvent.progressKind) {
                    case "Error":
                    case "Warning":
                        setErrorEncountered(true);
                    // deliberately fall through
                    case "Note":
                        setInterestingMessageEncountered(true);
                    // deliberately fall through
                    case "Progress":
                        setAccumulatedMessages(
                            (oldMessages) => oldMessages + html,
                        );
                        break;
                    case "Instruction":
                        setInstructionMessage(e.message);
                }

Done

src/BloomBrowserUI/publish/commonPublish/PublishProgressDialog.tsx line 140 at r6 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Sort of messy. How about
case "Warning":
setErrorEncountered(true);
// fall-through
case "Note":
setInterestingMessageEncountered(true);
// fall-through
case "Progress":

went with Andrews. Sigh this is a waste of time.

src/BloomExe/Publish/Epub/EpubMaker.cs line 3001 at r6 (raw file):

Previously, andrew-polk wrote…

I think this is step 3, not 4.
Note that we refer to it above. So that comment should also be changed to "3".

Done.

src/BloomExe/Publish/Epub/EpubMaker.cs line 3005 at r6 (raw file):

Previously, JohnThomson (John Thomson) wrote…

A few fonts exist only in Italic versions (AI noted 'Baron') or only in Bold (Google Fonts Alfa Slab One). Presumably the browser will attempt to synthesize normal from italic or bold if it must? Does that mean we need to include the group.Italic or group.Bold paths (with the appropriate weight/style values) if those are all that exist? (Might unfortunately complicate what you're doing with normalFacesAdded...I'm not sure).

I don't know. I've done all here that want to do, this is dragging on far beyond its benefit.

Copilot

Pull request overview

Updates font-face generation to better preserve Bloom/BloomReader font synthesis behavior when specific faces are missing, and keeps publish progress dialogs open when notes/warnings should be visible to the user.

Changes:

Adjust EpubMaker.AddFontFace to choose embedded font files in a way that allows browsers/readers to synthesize missing bold/italic axes (and dedupe normal/normal faces).
Add unit tests covering the new face-selection behavior for missing bold/italic combinations.
Update publish progress dialog behavior so “Note” messages prevent auto-close (giving users time to read important notes).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
src/BloomTests/Publish/Epub/ExportEpubTests.cs	Adds tests for new `AddFontFace` face-selection/synthesis behavior.
src/BloomExe/Publish/Epub/EpubMaker.cs	Implements new font-face selection logic and introduces `normalFacesAdded` tracking.
src/BloomExe/Publish/BloomPub/BloomPubMaker.cs	Updates BloomPUB font embedding to use the new `AddFontFace` signature.
src/BloomBrowserUI/publish/commonPublish/PublishProgressDialog.tsx	Prevents auto-close when “Note” messages occur; refines progress-state handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-03T23:58:57Z

src/BloomExe/Publish/Epub/EpubMaker.cs

+                else if (!string.IsNullOrEmpty(group.Italic))
+                {
+                    chosenPath = group.Italic;
+                    declaredWeight = "normal";
+                    declaredStyle = "italic";
+                }


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).

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

Copilot · 2026-02-03T23:58:57Z

src/BloomExe/Publish/BloomPub/BloomPubMaker.cs

                var group = fontFileFinder.GetGroupForFont(font.fontFamily);
                if (group != null)
                {
-                    EpubMaker.AddFontFace(sb, font, group);
+                    EpubMaker.AddFontFace(sb, font, group, normalFacesAdded);


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).

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:

Copilot · 2026-02-03T23:58:57Z

src/BloomExe/Publish/Epub/EpubMaker.cs

+                    declaredStyle = "normal";
+                }
+            }
+            else if (wantsItalic && !wantsBold && !string.IsNullOrEmpty(group.Italic))


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

hallucination

Copilot · 2026-02-03T23:58:57Z

src/BloomExe/Publish/Epub/EpubMaker.cs

+                declaredWeight = "normal";
+                declaredStyle = "italic";
+            }
+            else if (wantsBold && !wantsItalic && !string.IsNullOrEmpty(group.Bold))


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

hallucination

JohnThomson

@JohnThomson reviewed 2 files and all commit messages, and resolved 3 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @andrew-polk and @hatton).

andrew-polk

@andrew-polk reviewed 2 files and all commit messages, and resolved 2 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @hatton).

andrew-polk · 2026-02-04T15:38:50Z

Copilot review has now chimed in with several bugs (if they are accurate...). I'm trying to grok them now.

andrew-polk

@andrew-polk made 3 comments, resolved 2 discussions, and dismissed @copilot[bot] from 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hatton).

andrew-polk · 2026-02-04T17:14:33Z

src/BloomExe/Publish/BloomPub/BloomPubMaker.cs

                var group = fontFileFinder.GetGroupForFont(font.fontFamily);
                if (group != null)
                {
-                    EpubMaker.AddFontFace(sb, font, group);
+                    EpubMaker.AddFontFace(sb, font, group, normalFacesAdded);


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:

andrew-polk · 2026-02-04T17:14:34Z

src/BloomExe/Publish/Epub/EpubMaker.cs

+                    declaredStyle = "normal";
+                }
+            }
+            else if (wantsItalic && !wantsBold && !string.IsNullOrEmpty(group.Italic))


hallucination

andrew-polk · 2026-02-04T17:14:34Z

src/BloomExe/Publish/Epub/EpubMaker.cs

+                declaredWeight = "normal";
+                declaredStyle = "italic";
+            }
+            else if (wantsBold && !wantsItalic && !string.IsNullOrEmpty(group.Bold))


hallucination

Copilot · 2026-02-04T17:14:46Z

@andrew-polk I've opened a new pull request, #7655, to work on those changes. Once the pull request is ready, I'll request review from you.

Also, give the user a chance to read notes like ones about font embedding.

hatton

@hatton made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hatton).

hatton · 2026-02-05T22:45:22Z

src/BloomExe/Publish/Epub/EpubMaker.cs

+                else if (!string.IsNullOrEmpty(group.Italic))
+                {
+                    chosenPath = group.Italic;
+                    declaredWeight = "normal";
+                    declaredStyle = "italic";
+                }


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

andrew-polk

@andrew-polk reviewed 2 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hatton).

devin-ai-integration bot reviewed Jan 31, 2026

View reviewed changes

src/BloomBrowserUI/publish/commonPublish/PublishProgressDialog.tsx Show resolved Hide resolved

hatton force-pushed the BL-15558FixFontEmbedding branch from 087af99 to 76fe486 Compare January 31, 2026 23:39

devin-ai-integration bot reviewed Jan 31, 2026

View reviewed changes

src/BloomExe/Publish/Epub/EpubMaker.cs Show resolved Hide resolved

hatton force-pushed the BL-15558FixFontEmbedding branch from 76fe486 to cb97b21 Compare February 1, 2026 00:01

devin-ai-integration bot reviewed Feb 2, 2026

View reviewed changes

src/BloomExe/Publish/Epub/EpubMaker.cs Outdated Show resolved Hide resolved

hatton force-pushed the BL-15558FixFontEmbedding branch 2 times, most recently from 41a1936 to aebe807 Compare February 2, 2026 18:12

devin-ai-integration bot reviewed Feb 2, 2026

View reviewed changes

src/BloomBrowserUI/publish/commonPublish/PublishProgressDialog.tsx Outdated Show resolved Hide resolved

hatton force-pushed the BL-15558FixFontEmbedding branch from aebe807 to f0c394b Compare February 3, 2026 00:21

devin-ai-integration bot reviewed Feb 3, 2026

View reviewed changes

src/BloomBrowserUI/publish/commonPublish/PublishProgressDialog.tsx Show resolved Hide resolved

hatton force-pushed the BL-15558FixFontEmbedding branch from f0c394b to 7dd1877 Compare February 3, 2026 21:12

JohnThomson approved these changes Feb 3, 2026

View reviewed changes

andrew-polk reviewed Feb 3, 2026

View reviewed changes

hatton force-pushed the BL-15558FixFontEmbedding branch from 7dd1877 to cdaecb5 Compare February 3, 2026 23:51

hatton marked this pull request as ready for review February 3, 2026 23:51

Copilot AI review requested due to automatic review settings February 3, 2026 23:51

Copilot started reviewing on behalf of hatton February 3, 2026 23:51 View session

hatton commented Feb 3, 2026

View reviewed changes

Copilot AI reviewed Feb 3, 2026

View reviewed changes

JohnThomson approved these changes Feb 4, 2026

View reviewed changes

andrew-polk reviewed Feb 4, 2026

View reviewed changes

andrew-polk requested changes Feb 4, 2026

View reviewed changes

Copilot AI mentioned this pull request Feb 4, 2026

Fix font file embedding mismatch between FontFileFinder and EpubMaker #7655

Closed

Fix BL-15558 preserve bloompub font simulation

9e858ba

Also, give the user a chance to read notes like ones about font embedding.

hatton force-pushed the BL-15558FixFontEmbedding branch from cdaecb5 to 9e858ba Compare February 5, 2026 22:45

hatton commented Feb 5, 2026

View reviewed changes

andrew-polk approved these changes Feb 5, 2026

View reviewed changes

andrew-polk merged commit 9fa1ca3 into Version6.3 Feb 5, 2026
1 of 2 checks passed

andrew-polk deleted the BL-15558FixFontEmbedding branch February 5, 2026 22:52

Uh oh!

Conversation

hatton commented Jan 31, 2026 • edited by devin-ai-integration bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JohnThomson left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-polk left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-polk left a comment

Choose a reason for hiding this comment

Uh oh!

hatton left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnThomson left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-polk left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-polk commented Feb 4, 2026

Uh oh!

andrew-polk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Feb 4, 2026

Uh oh!

hatton left a comment

Choose a reason for hiding this comment

hatton commented Jan 31, 2026 •

edited by devin-ai-integration bot

Loading