Make canvas tool selection more consistent (BL-15977)#7831
Make canvas tool selection more consistent (BL-15977)#7831StephenMcConnel wants to merge 1 commit intomasterfrom
Conversation
![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 3 files
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/pageChooser/PageChooserDialog.tsx">
<violation number="1" location="src/BloomBrowserUI/pageChooser/PageChooserDialog.tsx:433">
P1: The button-click path (JSX `dataToolId` prop at line ~730) still uses `selectedTemplatePageDiv.getAttribute("data-tool-id")` directly, which returns `""` for canvas pages where the attribute is on the child element. This means clicking the "Add Page" button won't set the correct `dataToolId` for canvas pages, even though double-clicking now does. The JSX `dataToolId` prop should also use `getToolId(selectedTemplatePageDiv)` to match.</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.
| convertWholeBookCheckbox ? convertWholeBookCheckbox.checked : false, | ||
| props.forChooseLayout ? -1 : 1, | ||
| selectedPageDiv.getAttribute("data-tool-id") ?? "", | ||
| getToolId(selectedPageDiv), |
There was a problem hiding this comment.
P1: The button-click path (JSX dataToolId prop at line ~730) still uses selectedTemplatePageDiv.getAttribute("data-tool-id") directly, which returns "" for canvas pages where the attribute is on the child element. This means clicking the "Add Page" button won't set the correct dataToolId for canvas pages, even though double-clicking now does. The JSX dataToolId prop should also use getToolId(selectedTemplatePageDiv) to match.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/BloomBrowserUI/pageChooser/PageChooserDialog.tsx, line 433:
<comment>The button-click path (JSX `dataToolId` prop at line ~730) still uses `selectedTemplatePageDiv.getAttribute("data-tool-id")` directly, which returns `""` for canvas pages where the attribute is on the child element. This means clicking the "Add Page" button won't set the correct `dataToolId` for canvas pages, even though double-clicking now does. The JSX `dataToolId` prop should also use `getToolId(selectedTemplatePageDiv)` to match.</comment>
<file context>
@@ -409,7 +430,7 @@ export const PageChooserDialog: React.FunctionComponent<
convertWholeBookCheckbox ? convertWholeBookCheckbox.checked : false,
props.forChooseLayout ? -1 : 1,
- selectedPageDiv.getAttribute("data-tool-id") ?? "",
+ getToolId(selectedPageDiv),
getToolId(selectedPageDiv),
);
</file context>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 dataToolId prop uses old direct attribute read, missing new canvas-element fallback logic
The PR updated the double-click path in executeDoubleClickActionAsync (line 433) to use getToolId(selectedPageDiv) for the dataToolId argument, which now includes fallback logic to find data-tool-id on a nested canvas element. However, the dataToolId prop passed to SelectedTemplatePageControls (lines 730-734) still reads the attribute directly from the page div: selectedTemplatePageDiv.getAttribute("data-tool-id") ?? "". This means the button-click path (via SelectedTemplatePageControls → props.onSubmit at selectedTemplatePageControls.tsx:278) sends an empty dataToolId to C# for pages where data-tool-id lives on the canvas element rather than the page div. C# at AddOrChangePageApi.cs:193-195 only sets data-tool-id when dataToolId is non-empty, so the attribute will be missing on the newly added/changed page. The requiredTool prop on the same component (line 728) correctly uses getToolId(), making the inconsistency clear.
(Refers to lines 730-734)
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
🚩 Removal of data-tool-id setting in C# HandleToggleCustomCover and its implications
The PR removes pageElt.SetAttribute("data-tool-id", "canvas") (old line 197) and the dataToolId preservation logic (old lines 216, 253-256) from HandleToggleCustomCover in EditingViewApi.cs. Previously, data-tool-id was set on the page element in C# when switching to custom mode with saved state, and preserved across the SaveThen page rebuild. Now it's no longer set. This is consistent with the PR's approach of not relying on data-tool-id on the page div for custom xmatter pages. The showCanvasTool() calls in the JS code (customXmatterPage.tsx:332, 477, 531) handle canvas tool activation instead. The data-tool-id attribute on the page div was used by toolbox.ts:125 for adjustToolListForPage, but for custom covers, showCanvasTool() handles this directly. Worth confirming this doesn't affect page thumbnail rendering in PageThumbnailList.cs:228.
(Refers to lines 194-198)
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
This change is