BL-15642 Intro Page Settings by hatton · Pull Request #7557 · BloomBooks/BloomDesktop

hatton · 2025-12-30T23:21:34Z

This change is

Copilot

Pull request overview

This PR implements a Page Settings dialog (BL-15642) that allows users to customize per-page appearance properties including background color, page number color, and page number background color. The settings are stored as CSS custom properties on the page element's inline style attribute.

Key Changes:

Added a new Page Settings dialog accessible via a settings button in the page editing interface
Implemented backend support for saving page-level CSS custom properties
Restructured appearance theme CSS to allow page-level overrides of page number styling

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
yarn.lock	Added empty yarn.lock file (autogenerated file header only)
src/content/bookLayout/pageNumbers.less	Added support for `--pageNumber-color` CSS variable and included commented-out experimental code for automatic page number background matching
src/content/appearanceThemes/appearance-theme-zero-margin-ebook.css	Moved `--pageNumber-background-color` declaration to allow page settings to override it
src/content/appearanceThemes/appearance-theme-rounded-border-ebook.css	Moved `--pageNumber-background-color` declaration to allow page settings to override it
src/content/appearanceThemes/appearance-theme-default.css	Added default `--pageNumber-color` variable definition
src/content/appearanceMigrations/efl-zeromargin1/customBookStyles.css	Changed page number background from white to transparent for migration compatibility
src/BloomExe/web/controllers/EditingViewApi.cs	Added API endpoint handler for showing page settings dialog
src/BloomExe/Edit/EditingView.cs	Added `SaveAndOpenPageSettingsDialog` method and click handler
src/BloomExe/Book/HtmlDom.cs	Added logic to save/restore page-level inline style attributes
src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx	New component implementing the page settings dialog with color pickers for page and page number customization
src/BloomBrowserUI/bookEdit/js/origami.ts	Added page settings button to the above-page control container and wired up click handler
src/BloomBrowserUI/bookEdit/editViewFrame.ts	Exported `showPageSettingsDialog` function for use across the application
src/BloomBrowserUI/bookEdit/css/origamiEditing.less	Added styling for page settings button and changed container layout to space-between

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

Copilot · 2025-12-30T23:25:57Z

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx

+const setCurrentPageBackgroundColor = (color: string): void => {
+    const page = getCurrentPageElement();
+    page.style.setProperty("--page-background-color", color);
+    page.style.setProperty("--marginBox-background-color", color);
+};
+
+const getPageNumberColor = (): string => {
+    const page = getCurrentPageElement();
+
+    const inline = normalizeToHexOrEmpty(
+        page.style.getPropertyValue("--pageNumber-color"),
+    );
+    if (inline) return inline;
+
+    const computed = normalizeToHexOrEmpty(
+        getComputedStyleForPage(page).getPropertyValue("--pageNumber-color"),
+    );
+    return computed || "#000000";
+};
+
+const setPageNumberColor = (color: string): void => {
+    const page = getCurrentPageElement();
+    page.style.setProperty("--pageNumber-color", color);
+};
+
+const getPageNumberBackgroundColor = (): string => {
+    const page = getCurrentPageElement();
+
+    const inline = normalizeToHexOrEmpty(
+        page.style.getPropertyValue("--pageNumber-background-color"),
+    );
+    if (inline) return inline;
+
+    const computed = normalizeToHexOrEmpty(
+        getComputedStyleForPage(page).getPropertyValue(
+            "--pageNumber-background-color",
+        ),
+    );
+    return computed || "";
+};
+
+const setPageNumberBackgroundColor = (color: string): void => {
+    const page = getCurrentPageElement();
+    page.style.setProperty("--pageNumber-background-color", color);
+};


When setting CSS custom properties to empty strings, consider using removeProperty instead of setProperty with an empty value. This is more explicit and ensures the property is actually removed from the inline style, allowing computed styles and CSS rules to take effect properly.

For example, when a color is empty or transparent (normalized to ""), the current code calls setProperty("--page-background-color", ""). According to the pattern used elsewhere in the codebase (e.g., StyleEditor.ts line 683), the preferred approach is to use removeProperty when clearing a value.

Consider checking if the color is empty and using:

page.style.removeProperty("--page-background-color") when color is empty

page.style.setProperty("--page-background-color", color) when color has a value

This applies to all three setter functions: setCurrentPageBackgroundColor, setPageNumberColor, and setPageNumberBackgroundColor.

Seems to be done in SetOrRemoveCustomProperty?

[codex] Yes, confirmed. The current code uses setOrRemoveCustomProperty for the page color custom properties.

[codex] Thanks, agreed. This is already addressed in the current implementation path: PageSettingsConfigrPages.tsx uses setOrRemoveCustomProperty() and calls removeProperty() when the normalized value is empty/transparent. No additional change needed for this comment.

src/content/bookLayout/pageNumbers.less

JohnThomson

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

a discussion (no related file):
Some of my comments are high-level structural things, and this may not be the place to address them. But you've asked me to look for places code quality and architecture could be improved.
Most of the rest are not of great importance, but you should definitely consider whether you want this button on Game pages and if so whether it successfully coexists with the Game controls.

src/content/appearanceThemes/appearance-theme-rounded-border-ebook.css line 31 at r2 (raw file):

[class*="Device"].numberedPage:not(.bloom-interactive-page) {
    --pageNumber-extra-height: 0mm !important; /* we put the page number on top of the image so we don't need a margin boost */
    --pageNumber-background-color: #ffffff; /* I'm not clear why this is white, but all I did in this change is to move it so that it can be overridden by page settings */

Possibly because it's otherwise transparent, and this theme often puts it on top of an image, where it might not be legible without a background color?

src/BloomBrowserUI/react_components/color-picking/colorPickerDialog.tsx line 289 at r2 (raw file):

    // The MUI backdrop is rendered outside the dialog tree, so we use a body class
    // to suppress it while the color picker is open.

Seems to me to need explanation: why do we want to suppress the backdrop for this dialog?

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 31 at r2 (raw file):

import tinycolor from "tinycolor2";

let isOpenAlready = false;

Just isOpen? or isPageSettingsDialogOpen? The code here seems to entirely manage this variable, whereas 'already' suggests that something else might have caused it to be open before this code got started.

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 41 at r2 (raw file):

};

const getCurrentPageElement = (): HTMLElement => {

There's probably an existing function (quite likely more than one) that you could import to do this.

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 73 at r2 (raw file):

const getComputedStyleForPage = (page: HTMLElement): CSSStyleDeclaration => {
    const view = page.ownerDocument.defaultView;

Deserves a comment. Why would a page not have an ownerDocument? Why would it give a more useful answer for getComputedStyle()?

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 162 at r2 (raw file):

            "--pageNumber-background-color",
        ),
    );

Should it have the same special cases as getCurrentPageBackgroundColor? Or better, just call that? If not, it might be helpful to explain why.

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 390 at r2 (raw file):

                                    applyPageSettings(settings);
                                    return;
                                }

It feels wrong that we should have to handle two different kinds of argument in onChange. Maybe there's a good reason Configr is implemented that way, but I think it would be better to make it return one or the other. Maybe ConfigrPane could take a that tells it what "s" should be and could parse the string itself if need be? Or at least there could be a wrapper like that so every client doesn't have to do what you are doing here.

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 422 at r2 (raw file):

                                        }
                                        disabled={false}
                                    />

The structure feels wrong here. The knowledge that each control is a ConfigrCustomStringInput wrapping a ColorDisplayButton is both duplicated three times and split between this code and the individul functions, and the knowledge of how to get each label is used twice each. Consider making a single function that combines a ConfigrCustomStringInput and a nested ColorDisplayButton, with just enough props to implement the three variations, including looking up the label. You could then either call that three times here, or define the three methods each to call it with appropriate arguments, and here just call the three functions.

src/BloomBrowserUI/bookEdit/js/origami.ts line 360 at r2 (raw file):

${getPageSettingsButtonHtml()}\
</div>`,
        );

Do you want this button in Game pages, which is where the previously empty version is used? If so this probably needs attention, since I think the Games code uses this whole element to render the Start/Correct/Wrong/Play control.
(Maybe it's time to re-implement the origami on/off switch in React, and then we could have a single react control responsible for the whole decision about what to show in this space?)

src/BloomBrowserUI/bookEdit/js/origami.ts line 391 at r2 (raw file):

function pageSettingsButtonClickHandler(e: Event) {
    e.preventDefault();
    post("editView/showPageSettingsDialog");

It looks like we go through C# here just so we can save the page first. Is that essential? I would think it would be better to save afterwards, if this action even calls for a Save, so the page thumbnail can update, especially if the background color changed.

src/BloomExe/Edit/EditingView.cs line 1928 at r2 (raw file):

        }

        private void _pageSettingsButton_Click(object sender, EventArgs e)

not used? Maybe there was a C# button in early testing?

JohnThomson · 2026-02-06T20:54:19Z

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx

+const setCurrentPageBackgroundColor = (color: string): void => {
+    const page = getCurrentPageElement();
+    page.style.setProperty("--page-background-color", color);
+    page.style.setProperty("--marginBox-background-color", color);
+};
+
+const getPageNumberColor = (): string => {
+    const page = getCurrentPageElement();
+
+    const inline = normalizeToHexOrEmpty(
+        page.style.getPropertyValue("--pageNumber-color"),
+    );
+    if (inline) return inline;
+
+    const computed = normalizeToHexOrEmpty(
+        getComputedStyleForPage(page).getPropertyValue("--pageNumber-color"),
+    );
+    return computed || "#000000";
+};
+
+const setPageNumberColor = (color: string): void => {
+    const page = getCurrentPageElement();
+    page.style.setProperty("--pageNumber-color", color);
+};
+
+const getPageNumberBackgroundColor = (): string => {
+    const page = getCurrentPageElement();
+
+    const inline = normalizeToHexOrEmpty(
+        page.style.getPropertyValue("--pageNumber-background-color"),
+    );
+    if (inline) return inline;
+
+    const computed = normalizeToHexOrEmpty(
+        getComputedStyleForPage(page).getPropertyValue(
+            "--pageNumber-background-color",
+        ),
+    );
+    return computed || "";
+};
+
+const setPageNumberBackgroundColor = (color: string): void => {
+    const page = getCurrentPageElement();
+    page.style.setProperty("--pageNumber-background-color", color);
+};


Seems to be done in SetOrRemoveCustomProperty?

devin-ai-integration

Devin Review found 4 potential issues.

View 4 additional findings in Devin Review.

devin-ai-integration · 2026-03-06T00:11:12Z

src/BloomBrowserUI/bookEdit/workspaceRoot.ts

+export function showEditViewPageSettingsDialog() {
+    // Book pages are first (0-4); Page > Colors is index 5.
+    showBookSettingsDialog(5);
+}


🟡 Page settings button navigates to wrong key - "colors" is a nested page key, not a top-level page key

When the page settings button is clicked (via origami.ts), it calls post("editView/showPageSettingsDialog") which ultimately calls showBookSettingsDialog("colors") at src/BloomBrowserUI/bookEdit/editViewFrame.ts:271. This passes "colors" as initiallySelectedTopLevelPageKey to ConfigrPane. However, "colors" is the pageKey of a ConfigrPage nested inside the "pageArea" ConfigrArea (src/BloomBrowserUI/bookEdit/bookAndPageSettings/PageSettingsConfigrPages.tsx:395). The prop initiallySelectedTopLevelPageKey likely expects a top-level key like "pageArea", not a nested page key. This means the dialog may not navigate to the Colors page as intended when opened from the page settings button.

Was this helpful? React with 👍 or 👎 to provide feedback.

[codex] I investigated this in the vendored @sillsdev/config-r code. initiallySelectedTopLevelPageIndex is applied to a flattened list of pages across areas, not area index. Since book area has 5 pages, index 5 correctly selects Page > Colors. So no code change here.

src/BloomBrowserUI/bookEdit/js/origami.ts

devin-ai-integration · 2026-03-06T00:11:14Z

src/BloomExe/Book/HtmlDom.cs

+            // Allow saving per-page CSS custom properties (e.g. --page-background-color) stored on the page div.
+            // If missing, remove any previously-saved style.
+            var style = edittedPageDiv.GetAttribute("style");
+            if (string.IsNullOrEmpty(style))
+                destinationPageDiv.RemoveAttribute("style");
+            else
+                destinationPageDiv.SetAttribute("style", style);


🚩 HtmlDom.cs now persists the style attribute, which is a significant behavioral change

The addition at HtmlDom.cs:1918-1924 copies the style attribute from the edited page div to the destination (saved) page div. Previously, inline styles on the page div were not persisted across saves. This is essential for the new page-level color settings (which are stored as CSS custom properties in the inline style), but it also means ANY inline style on the page div will now be saved. If some editing-only code temporarily adds inline styles to the page div (e.g., for visual feedback during editing), those could now leak into the saved HTML. The bloom-ui cleanup code above (classNamesToUnion) only removes elements, not attributes. Worth verifying no other code path adds temporary inline styles to the page div.

Was this helpful? React with 👍 or 👎 to provide feedback.

[codex] Agreed. I changed persistence in HtmlDom.ProcessPageAfterEditing() to only store managed page color custom properties (--page-background-color, --marginBox-background-color, --pageNumber-color, --pageNumber-background-color) and drop unrelated style entries.

src/content/appearanceMigrations/efl-zeromargin1/customBookStyles.css

devin-ai-integration

Devin Review found 3 new potential issues.

View 10 additional findings in Devin Review.

DistFiles/localization/es/BloomMediumPriority.xlf

src/BloomBrowserUI/bookEdit/workspaceRoot.ts

devin-ai-integration · 2026-03-06T02:28:21Z

src/BloomBrowserUI/react_components/color-picking/colorPickerDialog.tsx

                    `}
-                    open={props.open === undefined ? open : props.open}
+                    hideBackdrop={true}
+                    BackdropProps={{
+                        invisible: true,
+                    }}
+                    slotProps={{
+                        backdrop: {
+                            invisible: true,
+                        },
+                    }}
+                    open={dialogOpen}
                    ref={dlgRef}
                    onClose={(
                        _event,
                        reason: "backdropClick" | "escapeKeyDown",
                    ) => {
-                        if (reason === "backdropClick")
-                            onClose(DialogResult.OK);
+                        if (eyedropperActive) {
+                            return;
+                        }
                        if (reason === "escapeKeyDown")


🚩 Color picker dialog backdrop click no longer closes with OK

The old ColorPickerDialog closed with DialogResult.OK on backdrop clicks (if (reason === 'backdropClick') onClose(DialogResult.OK)). The new code sets hideBackdrop={true} and removes the backdropClick handler entirely. Combined with the Global styles that make .MuiBackdrop-root transparent, this means clicking outside the color picker dialog no longer closes it. Users must now use the OK or Cancel buttons. This is a deliberate behavioral change (needed for the eyedropper workflow where the dialog hides), but it changes the UX for all color picker uses — not just the new page settings ones.

Was this helpful? React with 👍 or 👎 to provide feedback.

[GPT-5.4] Good catch. I changed the dialog to keep an invisible backdrop instead of hiding it entirely, and restored backdrop clicks to close with OK when the eyedropper is not active. The outer-dialog backdrop suppression stays in place for nested-dialog and eyedropper interactions.

hatton

@hatton made 1 comment.
Reviewable status: 3 of 76 files reviewed, 20 unresolved discussions (waiting on JohnThomson).

src/BloomBrowserUI/react_components/color-picking/colorPickerDialog.tsx line 289 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Seems to me to need explanation: why do we want to suppress the backdrop for this dialog?

[codex] The purpose is to avoid interference while picking colors/eyedropper interactions and to prevent layered backdrop behavior across nested dialogs. Existing comments already explain this, and the CSS note also states the intent (minimal interference). I did not change behavior here.

- Action: No code change (documentation intent already present).

hatton

@hatton made 1 comment.
Reviewable status: 3 of 76 files reviewed, 20 unresolved discussions (waiting on JohnThomson).

a discussion (no related file):

Previously, JohnThomson (John Thomson) wrote…

Some of my comments are high-level structural things, and this may not be the place to address them. But you've asked me to look for places code quality and architecture could be improved.
Most of the rest are not of great importance, but you should definitely consider whether you want this button on Game pages and if so whether it successfully coexists with the Game controls.

Will do, TODO

hatton · 2026-03-06T02:49:06Z

Previously, hatton (John Hatton) wrote…

Will do, TODO

Will do, TODO [codex] I kept the button on game pages (as discussed) and also expanded visibility to non-custom pages. The game control container flow remains unchanged (GameTool still appends into .above-page-control-container). This should coexist, but a quick manual check on a game page is still recommended.

hatton · 2026-03-06T02:49:57Z

src/content/appearanceThemes/appearance-theme-rounded-border-ebook.css line 31 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Possibly because it's otherwise transparent, and this theme often puts it on top of an image, where it might not be legible without a background color?

Will do, TODO [codex] Agreed. I treated this as intentional legibility behavior and aligned migration behavior accordingly by restoring white for efl-zeromargin1.

hatton · 2026-03-06T02:50:00Z

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 41 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

There's probably an existing function (quite likely more than one) that you could import to do this.

Will do, TODO [codex] Agreed in principle. In the current branch, this helper now lives in PageSettingsConfigrPages.tsx and is reused by the integrated dialog flow. I did not find an additional shared helper that would reduce duplication further without adding cross-module coupling.

hatton · 2026-03-06T02:50:03Z

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 390 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

It feels wrong that we should have to handle two different kinds of argument in onChange. Maybe there's a good reason Configr is implemented that way, but I think it would be better to make it return one or the other. Maybe ConfigrPane could take a that tells it what "s" should be and could parse the string itself if need be? Or at least there could be a wrapper like that so every client doesn't have to do what you are doing here.

Will do, TODO [codex] Good catch. In the vendored @sillsdev/config-r currently used by this branch, ConfigrPane/ContentPane onChange already returns a plain object (ConfigrValues), not a JSON string. I removed the now-unnecessary local string/object normalization paths from our dialogs so we no longer branch per client.

hatton · 2026-03-06T02:50:05Z

src/BloomBrowserUI/bookEdit/js/origami.ts line 360 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Do you want this button in Game pages, which is where the previously empty version is used? If so this probably needs attention, since I think the Games code uses this whole element to render the Start/Correct/Wrong/Play control.
(Maybe it's time to re-implement the origami on/off switch in React, and then we could have a single react control responsible for the whole decision about what to show in this space?)

Will do, TODO [codex] I kept the button on game pages and preserved the container behavior used by GameTool. The implementation still allows game controls to append to .above-page-control-container. Manual verification on a game page is still advisable.

hatton · 2026-03-06T02:50:08Z

src/BloomExe/Edit/EditingView.cs line 1928 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

not used? Maybe there was a C# button in early testing?

Will do, TODO [codex] Agreed. I removed the unused _pageSettingsButton_Click method.

hatton · 2026-03-06T02:50:27Z

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 31 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Just isOpen? or isPageSettingsDialogOpen? The code here seems to entirely manage this variable, whereas 'already' suggests that something else might have caused it to be open before this code got started.

Will do, TODO [codex] The referenced standalone PageSettingsDialog.tsx was replaced by the integrated Book/Page settings dialog implementation. The current code path uses BookAndPageSettingsDialog.tsx with existing naming patterns there. I did not make a naming-only change in this pass.

hatton · 2026-03-06T02:50:31Z

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 162 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Should it have the same special cases as getCurrentPageBackgroundColor? Or better, just call that? If not, it might be helpful to explain why.

Will do, TODO [codex] I left this as-is. The page-number background intentionally has different semantics from page background and defaults to unset/transparent unless explicitly themed or set. Reusing page-background logic here would risk conflating those behaviors.

hatton · 2026-03-06T02:50:34Z

src/BloomBrowserUI/bookEdit/js/origami.ts line 391 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

It looks like we go through C# here just so we can save the page first. Is that essential? I would think it would be better to save afterwards, if this action even calls for a Save, so the page thumbnail can update, especially if the background color changed.

Will do, TODO [codex] I kept save-before-open. This was a conscious decision for this branch to ensure consistent state entering settings and avoid introducing workflow regression late in the cycle.

devin-ai-integration

Devin Review found 2 new potential issues.

View 10 additional findings in Devin Review.

src/BloomBrowserUI/bookEdit/bookAndPageSettings/PageSettingsConfigrPages.tsx

devin-ai-integration · 2026-03-06T15:23:33Z

src/BloomBrowserUI/bookEdit/js/origami.ts

            .getElementsByClassName("bloom-page")[0]
            ?.getAttribute("data-tool-id") === "game"
    ) {
-        return $("<div class='above-page-control-container bloom-ui'></div>");
+        return $(
+            `<div class='above-page-control-container bloom-ui'>\
+${getPageSettingsButtonHtml()}\
+</div>`,
+        );


🚩 Page settings button appears on ALL page types including drag activities

The getAbovePageControlContainer function in origami.ts:383-424 now adds a page settings button to every page, including drag activity pages (data-tool-id='game'). For drag activities, the origami controls are intentionally hidden, but the page settings button is still shown. This may or may not be desired—page color settings on game pages might not be meaningful, or they might be useful. Worth confirming this is the intended behavior.

(Refers to lines 388-397)

Was this helpful? React with 👍 or 👎 to provide feedback.

[GPT-5.4] Agreed. I removed the Page Settings button from game pages and now leave that container empty for GameTool to populate, so the new control no longer competes with Start/Correct/Wrong/Play.

devin-ai-integration

Devin Review found 2 new potential issues.

View 10 additional findings in Devin Review.

src/BloomBrowserUI/bookEdit/bookAndPageSettings/BookAndPageSettingsDialog.tsx

devin-ai-integration · 2026-03-06T17:39:02Z

src/BloomBrowserUI/bookEdit/bookAndPageSettings/BookAndPageSettingsDialog.tsx

+    function saveSettingsAndCloseDialog() {
+        const latestSettings = settingsToReturnLater;
+        if (latestSettings) {
+            applyPageSettings(
+                parsePageSettingsFromConfigrValue(latestSettings),
+            );
+
+            const settingsToPost =
+                removePageSettingsFromConfigrSettings(latestSettings);
+            // If nothing changed, we don't get any...and don't need to make this call.
+            postJson("book/settings", settingsToPost);
+        }
+
+        closeDialogAndClearOpenFlag();
+        // todo: how do we make the pageThumbnailList reload? It's in a different browser, so
+        // we can't use a global. It listens to websocket, but we currently can only listen,
+        // we cannot send.
+    }


🟡 Page settings not saved when user only changes page colors without touching any book setting

In saveSettingsAndCloseDialog (line 301-318), page settings are only applied and book settings only posted if settingsToReturnLater is truthy (line 303). settingsToReturnLater is set inside the ConfigrPane onChange callback via setTimeout. If for some reason ConfigrPane never fires onChange (e.g., the user opens and immediately clicks OK without touching anything), settingsToReturnLater stays undefined and page settings are never persisted. This is actually the expected behavior — if nothing changed, there's nothing to save. However, the live-preview applyPageSettings in the onChange handler means page DOM changes are applied during editing but if settingsToReturnLater is still undefined at save time (a narrow race with the setTimeout), the final applyPageSettings at line 304 won't run. In practice, ConfigrPane typically fires onChange immediately, so this is unlikely but represents a subtle timing issue.

Was this helpful? React with 👍 or 👎 to provide feedback.

[GPT-5.4] Good catch. I kept the deferred React state update, but now also capture the latest Configr payload in a ref synchronously and use that on save. That closes the narrow timing hole where page settings could be previewed but not persisted if OK was clicked before the queued state update ran.

The rebase had to deal with "Single Browser" and conceivably "More Vite Dev". Afterwards I couldn't get into the edit tab without errors, so I gave it the whole console log and it made these changes. This should be carefully reviewed.

devin-ai-integration

Devin Review found 3 new potential issues.

View 4 additional findings in Devin Review.

devin-ai-integration · 2026-03-12T22:28:23Z

src/content/bookLayout/pageNumbers.less

+        color: var(--pageNumber-color);
+        -webkit-text-stroke: 1px var(--pageNumber-outline-color);
+        paint-order: stroke fill;


🚩 Unconditional -webkit-text-stroke on all page numbers is a visual change for existing books

The new CSS at src/content/bookLayout/pageNumbers.less:27 adds -webkit-text-stroke: 1px var(--pageNumber-outline-color) to ALL .numberedPage:after pseudo-elements. The default --pageNumber-outline-color is white (set at src/content/appearanceThemes/appearance-theme-default.css:44). With paint-order: stroke fill, the stroke renders behind the fill, so it only visually extends ~0.5px outside glyph boundaries. On white backgrounds this is invisible, but on dark/colored backgrounds (e.g., ebook themes like rounded-border-ebook or zero-margin-ebook), page numbers that previously had no outline will now show a visible white halo. This affects all existing books without any user action. If this is intentional for readability on dark backgrounds, consider documenting the visual change. If not, consider gating the stroke behind a CSS variable that defaults to none or 0.

Was this helpful? React with 👍 or 👎 to provide feedback.

[GPT-5.4] Agreed. I left the stroke support in place, but changed the default themes --pageNumber-outline-color to transparent so existing books do not pick up a white halo unless a theme explicitly opts into it.

devin-ai-integration · 2026-03-12T22:28:23Z

src/BloomExe/Edit/EditingView.cs

+        private void _topBarPanel_Click(object sender, EventArgs e)
+        {
+            if (Model.Visible && ModifierKeys == (Keys.Shift | Keys.Control))
+                _model.RethinkPageAndReloadItAndReportIfItFails();
+        }


🚩 Dead code: _topBarPanel_Click handler added but never wired to any event

The _topBarPanel_Click method at src/BloomExe/Edit/EditingView.cs:1740-1744 is added with a comment referencing BL-13120 (a user data loss investigation), but no event handler registration (_topBarPanel.Click += _topBarPanel_Click) exists anywhere in the codebase. This appears to be accidentally included in this PR. It's harmless dead code but adds noise.

Was this helpful? React with 👍 or 👎 to provide feedback.

[GPT-5.4] Agreed. I removed the unused _topBarPanel_Click handler from EditingView.cs.

devin-ai-integration · 2026-03-12T22:28:24Z

src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts

+    if (!isAccordionInitialized(toolbox)) {
+        window.setTimeout(() => setCurrentTool(toolID), 0);
+        return;
+    }


🚩 setCurrentTool retry loop has no maximum retry count

The new isAccordionInitialized guard in src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts:1100-1103 retries with setTimeout(() => setCurrentTool(toolID), 0) if the jQuery UI accordion isn't initialized yet. This recursive setTimeout has no termination guard or retry limit. If the accordion never initializes (edge case in a non-legacy toolbox path where getToolboxReactAdapter returns undefined), this would loop indefinitely. In practice the accordion should initialize quickly, but a maximum retry count would be more robust.

Was this helpful? React with 👍 or 👎 to provide feedback.

[GPT-5.4] Agreed. The retry loop now fails fast instead of spinning forever: after a bounded number of zero-delay retries it throws if the accordion still has not initialized.

Copilot AI review requested due to automatic review settings December 30, 2025 23:21

Copilot started reviewing on behalf of hatton December 30, 2025 23:22 View session

Copilot AI reviewed Dec 30, 2025

View reviewed changes

JohnThomson requested changes Feb 6, 2026

View reviewed changes

hatton force-pushed the BL-15642PageColor branch from ae661db to 5887378 Compare March 6, 2026 00:05

devin-ai-integration bot reviewed Mar 6, 2026

View reviewed changes

hatton commented Mar 6, 2026

View reviewed changes

devin-ai-integration bot reviewed Mar 6, 2026

View reviewed changes

hatton added 10 commits March 11, 2026 14:24

BL-15642 Intro Page Settings

3981e91

See color results on page as you make changes.

5f913e3

Add palette for page backgrounds

67611c8

review fixes

0cb9001

Update configr, work on cancel

4180db9

Add eyedropper to color picker

5cb953f

Add page settings to edit-tab settings dialog

86b22b6

Review fixes

2a8e3cc

Mark stale BookAndPageSettings.Title translations as needs-translation

cefe01a

Review fixes

ba3ac1f

hatton added 7 commits March 11, 2026 14:28

draft skill for replying to reviewable

d50d290

Fixes post rebase

5d1f1a7

The rebase had to deal with "Single Browser" and conceivably "More Vite Dev". Afterwards I couldn't get into the edit tab without errors, so I gave it the whole console log and it made these changes. This should be carefully reviewed.

Add page number outline color support

bbb55b0

Improve color picker responsiveness

b508c10

Show transparency preview in color buttons

c6a2da2

Remove invalid props spread from collections pane

95a9857

Remove unused page number localization entries

c1a26ab

hatton force-pushed the BL-15642PageColor branch from 6791e5d to c1a26ab Compare March 12, 2026 22:17

devin-ai-integration bot reviewed Mar 12, 2026

View reviewed changes

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 30, 2025

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!

Uh oh!

JohnThomson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devin-ai-integration bot Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

devin-ai-integration bot Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hatton 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!

hatton commented Mar 6, 2026

Uh oh!

hatton commented Mar 6, 2026

Uh oh!

hatton commented Mar 6, 2026

Uh oh!

hatton commented Mar 6, 2026

Uh oh!

hatton commented Mar 6, 2026

Uh oh!

hatton commented Mar 6, 2026

Uh oh!

hatton commented Mar 6, 2026

Uh oh!

hatton commented Mar 6, 2026

Uh oh!

hatton commented Mar 6, 2026

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hatton commented Dec 30, 2025 •

edited by devin-ai-integration bot

Loading

devin-ai-integration bot Mar 6, 2026 •

edited

Loading

devin-ai-integration bot Mar 6, 2026 •

edited

Loading