BL-15642 Page Settings with background color#7787
Conversation
# Conflicts: # src/BloomBrowserUI/bookEdit/js/CanvasElementContextControls.tsx # src/BloomBrowserUI/package.json # src/BloomBrowserUI/react_components/color-picking/colorPicker.tsx # src/BloomBrowserUI/react_components/color-picking/colorPickerDialog.tsx # src/BloomBrowserUI/yarn.lock
(it introduces complexities)
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // NOTE: tools without a "data-toolId" attribute (such as the More tool) cannot be the "currentTool." | ||
| let idx = 0; | ||
| const toolbox = $("#toolbox"); | ||
| ensureJQueryAccordionActivationHook(toolbox); | ||
|
|
||
| if (!isAccordionInitialized(toolbox)) { | ||
| const elapsedTime = Date.now() - waitStartTime; | ||
| if (elapsedTime >= maxMillisecondsToWaitForAccordionInitialization) { | ||
| const fallbackToolId = | ||
| (toolbox.find("> h3").first().attr("data-toolId") as | ||
| | string | ||
| | undefined) ?? toolID; | ||
| console.error( | ||
| `Toolbox accordion did not initialize within ${elapsedTime}ms while activating ${toolID || "the default tool"}. Falling back without waiting for the accordion UI.`, | ||
| ); | ||
| if (fallbackToolId) { | ||
| switchTool(fallbackToolId); | ||
| } | ||
| return; | ||
| } | ||
| const retryDelay = Math.min( | ||
| initialAccordionInitializationRetryDelayInMilliseconds * | ||
| (retryCount + 1), | ||
| maximumAccordionInitializationRetryDelayInMilliseconds, | ||
| ); | ||
| // A 0ms retry loop can burn through the whole retry budget before jQuery finishes | ||
| // initializing on a slow startup, so back off a little and cap the total wait time. | ||
| window.setTimeout( | ||
| () => setCurrentTool(toolID, retryCount + 1, waitStartTime), | ||
| retryDelay, | ||
| ); |
There was a problem hiding this comment.
📝 Info: Accordion initialization retry uses linear backoff with cap, preventing busy-wait
The setCurrentTool function now retries when the jQuery accordion is not yet initialized, using a delay that grows linearly (10ms * (retryCount + 1)) capped at 50ms, with a total wait time cap of 3 seconds. The latestSetCurrentToolRequestToken pattern ensures that if a new setCurrentTool call comes in during retries, old retries are abandoned. After timeout, it falls back to calling switchTool without the accordion UI, which may result in the tool being activated without the accordion visually reflecting the selection. The ensureJQueryAccordionActivationHook is called early (line 1166) so that even if startup gives up waiting, later user clicks still properly trigger switchTool() once the accordion is ready. This is a reasonable robustness improvement.
Was this helpful? React with 👍 or 👎 to provide feedback.
devin-ai-integration: undefined invalidatePendingJQueryAccordionActivationRetries call in toolbox.ts. Removed the stale call from ensureToolEnabled so enabling a tool no longer risks a runtime ReferenceError. devin-ai-integration: setCurrentTool retry loop had no explicit supersession mechanism. Added a single pending retry timeout plus request tokens so newer tool-activation requests cancel or invalidate older retries.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Report the active Vite dev-server port in instance metadata. Preserve that metadata in the Bloom automation helpers. Allow watchBloomExe to reuse the Vite port from a running Bloom instance in the same worktree.
…y have a special order.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/BloomBrowserUI/bookEdit/css/origamiEditing.less">
<violation number="1" location="src/BloomBrowserUI/bookEdit/css/origamiEditing.less:201">
P2: Delete this commented-out block instead of leaving it in the file. The `page-settings-button` class has no remaining live references anywhere in the codebase (the TSX usage in `AbovePageControls.tsx` is also commented out), so this is pure dead code.
(Based on your team's feedback about removing obsolete or unused code paths.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| } | ||
| } | ||
|
|
||
| // .page-settings-button { |
There was a problem hiding this comment.
P2: Delete this commented-out block instead of leaving it in the file. The page-settings-button class has no remaining live references anywhere in the codebase (the TSX usage in AbovePageControls.tsx is also commented out), so this is pure dead code.
(Based on your team's feedback about removing obsolete or unused code paths.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/BloomBrowserUI/bookEdit/css/origamiEditing.less, line 201:
<comment>Delete this commented-out block instead of leaving it in the file. The `page-settings-button` class has no remaining live references anywhere in the codebase (the TSX usage in `AbovePageControls.tsx` is also commented out), so this is pure dead code.
(Based on your team's feedback about removing obsolete or unused code paths.) </comment>
<file context>
@@ -197,40 +197,41 @@
- &:hover {
- opacity: 0.8;
- }
+// .page-settings-button {
+// display: flex;
+// align-items: center;
</file context>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
# Conflicts: # src/BloomBrowserUI/bookEdit/bookSettings/BookSettingsDialog.tsx # src/BloomBrowserUI/bookEdit/js/CanvasElementContextControls.tsx
Only persist page-level settings when their final values differ from the state at dialog open. Restore the original page snapshot before previewing config-r changes so untouched page values continue following theme changes. Add focused dialog tests for theme-only edits and reverted page-setting edits.
The Book and Page Settings dialog was assuming that the page iframe body and the editable .bloom-page root became available at the same time. In one observed CDP failure, the body existed first, so the dialog mounted and tried to read page settings before the live page DOM was actually ready. Move the bloom-page lookup and readiness wait into src/BloomBrowserUI/utils/shared.ts so callers can ask for the actual editable page root instead of treating iframe body readiness as sufficient. Keep page-settings behavior strict when the page is missing, and add focused tests for the shared helper and the page-settings consumer.
| <source xml:lang="en">Settings</source> | ||
| <note>ID: CollectionSettingsDialog.CollectionSettingsWindowTitle</note> | ||
| </trans-unit> |
There was a problem hiding this comment.
🔴 Old XLF entry CollectionSettingsDialog.CollectionSettingsWindowTitle not marked obsolete after L10N ID change
The C# CollectionSettingsDialog.Designer.cs changed its SetLocalizingId from CollectionSettingsDialog.CollectionSettingsWindowTitle to CollectionSettingsDialog.Title (src/BloomExe/Collection/CollectionSettingsDialog.Designer.cs:652), and the text from "Settings" to "Collection Settings". A new XLF entry was correctly added at DistFiles/localization/en/Bloom.xlf:553-556, but the old entry at line 549-551 was not marked obsolete. Per AGENTS.md: "Don't change the content or ID of an existing XLF entry unless it is new (marked translate='no'). Instead, mark the old one with a note saying it is 'obsolete as of ' and make a new entry with a different ID." The BookSettings.Title entry in BloomMediumPriority.xlf:734 was correctly handled with an "Obsolete as of 6.4" note, but this one was missed.
(Refers to lines 549-552)
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Move the rounded ebook theme's dark surround into a separate page-frame-color so page-background-color consistently means the user-facing page surface. Make marginBox-background-color default to page-background-color in theme CSS, stop page settings from writing or persisting a duplicate page-level margin box color override, and keep legacy inline marginBox colors readable for backward compatibility. Also remove stale theme-name plumbing from the page settings code and update the focused page settings and dialog-saving tests. Validation: yarn test ./bookEdit/bookAndPageSettings/PageSettingsConfigrPages.spec.ts ./bookEdit/bookAndPageSettings/BookAndPageSettingsDialog.saving.test.tsx
Move the zero-margin ebook default outline color from the page-number pseudo-element to the page element so Book and Page Settings overrides affect the rendered page number. Add focused appearance settings regression tests covering rounded-border and zero-margin themes.
Turn on the alpha slider for the standalone text color pickers used by the format dialog, audio highlight settings, and canvas toolbox. This keeps the commit scoped to the picker call sites that were cleanly separable from other in-progress page settings work. Validation: yarn test ./bookEdit/bookAndPageSettings/PageSettingsConfigrPages.spec.ts
Add the missing English strings for the new page settings labels and descriptions in BloomMediumPriority.xlf. This covers the current-page area description plus the page number outline/background color text so the page settings UI stops reporting missing English localization entries.
There was a problem hiding this comment.
1 issue found across 15 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/BloomBrowserUI/bookEdit/bookAndPageSettings/PageSettingsConfigrPages.tsx">
<violation number="1" location="src/BloomBrowserUI/bookEdit/bookAndPageSettings/PageSettingsConfigrPages.tsx:538">
P2: The block comment above this line still says "hide the page number color group for now" and suggests it might come back as a book setting — but the group is now active. Delete or update the stale comment so it doesn't mislead future readers.
(Based on your team's feedback about removing obsolete comments when behavior changes.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| * We could add this back in the future, perhaps as a book settings feature | ||
| * instead of a page settings feature. | ||
| */ | ||
| const PageNumberConfigrInputs: React.FunctionComponent<{ |
There was a problem hiding this comment.
P2: The block comment above this line still says "hide the page number color group for now" and suggests it might come back as a book setting — but the group is now active. Delete or update the stale comment so it doesn't mislead future readers.
(Based on your team's feedback about removing obsolete comments when behavior changes.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/BloomBrowserUI/bookEdit/bookAndPageSettings/PageSettingsConfigrPages.tsx, line 538:
<comment>The block comment above this line still says "hide the page number color group for now" and suggests it might come back as a book setting — but the group is now active. Delete or update the stale comment so it doesn't mislead future readers.
(Based on your team's feedback about removing obsolete comments when behavior changes.) </comment>
<file context>
@@ -576,70 +535,70 @@ const PageConfigrInputs: React.FunctionComponent<{
-// </>
-// );
-// };
+const PageNumberConfigrInputs: React.FunctionComponent<{
+ disabled?: boolean;
+ onColorPickerVisibilityChanged?: (open: boolean) => void;
</file context>
| const settingsToPost = | ||
| removePageSettingsFromConfigrSettings(latestSettings); | ||
| // If nothing changed, we don't get any...and don't need to make this call. | ||
| postJson("book/settings", settingsToPost); |
There was a problem hiding this comment.
🚩 Book settings always posted even for page-only color changes
In the new combined dialog, saveSettingsAndCloseDialog (BookAndPageSettingsDialog.tsx:307-333) always calls postJson("book/settings", settingsToPost) whenever latestSettings is truthy. Since ConfigrPane's onChange fires on ANY setting change (including page-only color changes), latestSettings will always be set after any interaction. The comment at line 324 ('If nothing changed, we don't get any...and don't need to make this call') is misleading—it was carried over from the old BookSettingsDialog where only book-level settings existed. Now, a user who only changes page colors will still trigger a full book settings POST to the server. This is probably harmless since posting unchanged book settings should be idempotent, but it's worth noting as a potential efficiency concern.
Was this helpful? React with 👍 or 👎 to provide feedback.
| onClose={() => cancelAndCloseDialog()} | ||
| onCancel={() => cancelAndCloseDialog()} |
There was a problem hiding this comment.
🚩 Behavioral change: backdrop click now cancels instead of accepting
The old BookSettingsDialog had onClose={closeDialog} which meant backdrop click closed the dialog without restoring settings (effectively accepting current state). The new dialog at line 398 wires onClose={() => cancelAndCloseDialog()}, so backdrop click now cancels and restores the initial page attributes. This is a deliberate semantic change—users who previously expected backdrop click to save their changes will now find it reverts them. The change also fixes an old bug where onClose (old line 411) did not reset isOpenAlready, which could permanently prevent reopening the dialog.
Was this helpful? React with 👍 or 👎 to provide feedback.
getPageNumberBackgroundColor()to read computed theme value when no inline override exists (commit ffb4e2f)setTimeout(..., 0)tosetTimeout(..., 50)and replacethrow new Error(...)withconsole.error(...)+ graceful returnThis change is