Refactor canvas elements: registry-driven controls

[hatton]

This PR delivers the Canvas refactor to a registry-driven control architecture and carries it through with behavior fixes and expanded end-to-end coverage.

What Changed

• Introduced a declarative canvas control system for the toolbox panel, mini-toolbar, and context menu.
• Consolidated command definitions so labels/icons/handlers are sourced from one place, with deterministic section ordering and spacing.
• Replaced monolithic CanvasElementManager logic with focused modules under js/canvasElementManager (selection UI, pointer/drag interactions, geometry/positioning, clipboard, duplication, alternates, background image handling, editing suspension, etc.).
• Tightened cross-bundle boundaries (dependency-light constants and bridge utilities) to reduce import-cycle and bundling risk.

Functional Fixes Included

• Hardened background-image setup/resizing behavior in the refactored path.
• Improved custom SVG menu icon rendering/alignment in context controls.

Test and Tooling Work

• Added/expanded dedicated Canvas Playwright coverage with shared fixtures/helpers and broad scenario specs:
  • drag/drop, select/move/resize/crop
  • context toolbar/menu and availability rules
  • clipboard/paste and duplication/child-bubble behavior
  • background/canvas resize interactions
  • regression and shared-mode cleanup checks
• Added fail-fast checks for non-canvas page context in canvas e2e runs.

---

This change is 

---

[hatton]

src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlAvailabilityPresets.ts line 23 at r16 (raw file):

Previously, hatton (John Hatton) wrote…

[GPT-5.4] Addressed. Paste image now checks real clipboard-image availability instead of only the selected element state. I added a new common/clipboardHasImage endpoint, pass hasClipboardImage into the canvas control context when the menu opens, gate pasteImage on that fact, and added a canvas availability regression test for the disabled/enabled states.

John Hatton chose not to take on this issue in this PR.

[hatton]

John Hatton chose not to take on this issue in this PR.

[JohnThomson]

@JohnThomson reviewed 3 files and made 4 comments.
Reviewable status: 20 of 197 files reviewed, 35 unresolved discussions (waiting on hatton).

---

src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasElementUtils.ts line 1 at r19 (raw file):

// Cross-frame bridge utilities for canvas element code.

I asked an AI how it would improve naming and code organization, and it's top suggestion was that this file wants a more specific name; it's not really a generic place to put utils. It suggested canvasElementBridge. I see it's not a new file, but while we're moving everything and breaking our history, it might be worth doing.

---

src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementManagerPublicFunctions.ts line 5 at r19 (raw file):

//
// This file exists to keep CanvasElementManager.ts smaller and to reduce coupling
// between the page bundle and toolbox UI code.

Seems very similar in purpose to canvasElementUtils (which might become canvasElementBridge). Since it's quite small I'd be inclined to merge them, unless we can improve the names enough to make it clear what functions to look for (and add) in each.

---

src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementCanvasResizeAdjustments.ts line 1 at r19 (raw file):

import { Bubble, Comical, TailSpec } from "comicaljs";

File name is awkward. Just canvasElementResizeAdjustment?

---

src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementCanvasResizeAdjustments.ts line 19 at r19 (raw file):

}

export class CanvasElementCanvasResizeAdjustments {

I don't see why you want a class here. We make one instance of it in one place for the purpose of calling one function in one place...and that function just provides a wrapper so that its three callers can process it as a function of CanvasElementManager rather than importing it directly. Looks to me as if it just adds complexity.
There's no reason for pxToNumber to be a member variable. Why not just import it directly from canvasElementCssUtils? Instead, CanvasElementManager imports it as pxToNumberFromCssUtils, re-wraps it as its own function pxToNumber, and then passes that to the constructor of this object...
adjustBackgroundImageSize is harder to analyze, because it has also become a member function of a new class, so I'd have to analyze whether there's a good reason for that. But I doubt it...again, one instance is created, and the instance variables look like functions that are created to wrap calls to member functions of classes created to wrap other functions...
This is not trivial complexity. For example, suppose I'm reading some code that calls pxToNumber, and want to see what it does. With a direct import, ctrl-click takes me straight there. (I may not even need to go...VS Code might well pop up the comment that describes the function.) With the code your AI has written,

• ctrl-click takes me to a member variable
• I will do one search to find what sets that, and get to the constructor
• I will search for references to the constructor to find the one place it is called
• I will look at that call and figure out what it passes as the pxToNumber argument
• I will follow that to find the local function in CAM
• And then follow that to the function I wanted to see
  That's at least six times as much work! And I very much doubt that it will be easier for an AI to figure out, either.

Unless there's a very good reason, I would be greatly in favor of undoing all this business of wrapping functions in singleton classes. For a long time we've been moving away from creating classes unless they really store important state. We're not in C#, where every function has to belong to some class. Modules provide all the namespace management we need. This feels like a move backwards and to increased complexity.

[hatton]

src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasElementUtils.ts line 1 at r19 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I asked an AI how it would improve naming and code organization, and it's top suggestion was that this file wants a more specific name; it's not really a generic place to put utils. It suggested canvasElementBridge. I see it's not a new file, but while we're moving everything and breaking our history, it might be worth doing.

[codex] Renamed the toolbox-to-page bridge from canvasElementUtils.ts to canvasElementPageBridge.ts, updated all imports, and tightened the module and README wording so it is explicitly a bridge instead of a catch-all utils file. This is in b20370c.

[hatton]

src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementManagerPublicFunctions.ts line 5 at r19 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Seems very similar in purpose to canvasElementUtils (which might become canvasElementBridge). Since it's quite small I'd be inclined to merge them, unless we can improve the names enough to make it clear what functions to look for (and add) in each.

[codex] I did not merge these two bridge modules, but I did rename them so their responsibilities are explicit: canvasElementPageBridge.ts is the toolbox-to-page bridge, and CanvasElementToolboxBridge.ts is the page-to-toolbox helper module for things like showCanvasTool(). I also updated the module comments and READMEs to explain that split. This is in b20370c.

[hatton]

src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementCanvasResizeAdjustments.ts line 1 at r19 (raw file):

Previously, JohnThomson (John Thomson) wrote…

File name is awkward. Just canvasElementResizeAdjustment?

[codex] Renamed CanvasElementCanvasResizeAdjustments.ts to CanvasElementResizeAdjustments.ts and updated the manager, README, and spec references to match. This is in b20370c.

[hatton]

src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementCanvasResizeAdjustments.ts line 19 at r19 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I don't see why you want a class here. We make one instance of it in one place for the purpose of calling one function in one place...and that function just provides a wrapper so that its three callers can process it as a function of CanvasElementManager rather than importing it directly. Looks to me as if it just adds complexity.
There's no reason for pxToNumber to be a member variable. Why not just import it directly from canvasElementCssUtils? Instead, CanvasElementManager imports it as pxToNumberFromCssUtils, re-wraps it as its own function pxToNumber, and then passes that to the constructor of this object...
adjustBackgroundImageSize is harder to analyze, because it has also become a member function of a new class, so I'd have to analyze whether there's a good reason for that. But I doubt it...again, one instance is created, and the instance variables look like functions that are created to wrap calls to member functions of classes created to wrap other functions...
This is not trivial complexity. For example, suppose I'm reading some code that calls pxToNumber, and want to see what it does. With a direct import, ctrl-click takes me straight there. (I may not even need to go...VS Code might well pop up the comment that describes the function.) With the code your AI has written,

• ctrl-click takes me to a member variable
• I will do one search to find what sets that, and get to the constructor
• I will search for references to the constructor to find the one place it is called
• I will look at that call and figure out what it passes as the pxToNumber argument
• I will follow that to find the local function in CAM
• And then follow that to the function I wanted to see
  That's at least six times as much work! And I very much doubt that it will be easier for an AI to figure out, either.

Unless there's a very good reason, I would be greatly in favor of undoing all this business of wrapping functions in singleton classes. For a long time we've been moving away from creating classes unless they really store important state. We're not in C#, where every function has to belong to some class. Modules provide all the namespace management we need. This feels like a move backwards and to increased complexity.

[codex] Removed the singleton-class indirection here. The resize logic now lives in a stateless adjustCanvasElementChildrenIfSizeChanged() module function in CanvasElementResizeAdjustments.ts, and CanvasElementManager now calls it directly with adjustBackgroundImageSize plus the shared pxToNumberFromCssUtils helper instead of constructing a wrapper class instance. This is in b20370c.

[JohnThomson]

@JohnThomson made 1 comment.
Reviewable status: 6 of 201 files reviewed, 33 unresolved discussions (waiting on hatton).

---

src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementCanvasResizeAdjustments.ts line 19 at r19 (raw file):

Previously, hatton (John Hatton) wrote…

[codex] Removed the singleton-class indirection here. The resize logic now lives in a stateless adjustCanvasElementChildrenIfSizeChanged() module function in CanvasElementResizeAdjustments.ts, and CanvasElementManager now calls it directly with adjustBackgroundImageSize plus the shared pxToNumberFromCssUtils helper instead of constructing a wrapper class instance. This is in b20370c.

Good start, but we should just import pxToNumber, not require the caller to supply it. There's only one version of this function in our codebase (I hope).
Also, I'm hoping we can get rid of a lot more of these classes that were apparently just made to wrap functions better imported.

[JohnThomson]

@JohnThomson made 1 comment.
Reviewable status: 6 of 201 files reviewed, 34 unresolved discussions (waiting on hatton).

---

src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementManager.ts line 149 at r20 (raw file):

            setupControlFrame: this.setupControlFrame.bind(this),
        });
        this.backgroundImageManager = new CanvasElementBackgroundImageManager({

It appears to me that most of the classes we instantiate here are just wrappers for functions.

• We started with functions X, Y, and Z in CAM which call P and Q
• We moved X, Y, and Z to a new file A, and P and Q to other files B and C
• We should have made the first new file import P and Q, and anything that needs them import X, Y, and Z. If the functions needed access to some state that lives in the CAM singleton, they should make use of a reliable function that finds the one instance (in the right iframe).
• Instead, we made a class in each new file. X, Y, and Z became public methods of Aclass, and P and Q are now public methods of BClass and CClass.
• Now, we need an instance of each of these classes, so the methods can be called. So CAM has to make instances of AClass, BClass, and CClass.
• But, X, Y, and Z still can't call P and Q, because they are member functions of Bclass and CClass, and we don't want every caller creating instances.
• So class A is made to have member variables that point at functions P and Q
• And CAM gets new functions P and Q, which just wrap calls to BInstance.P and CInstance.Q
• And then passes it's own P and Q wrappers to the constructor for A.
  Something that should have been three simple imports has become a mess of classes, member variables, wrapper functions, constructors, and passing functions all over the place.
  Now, most of these methods started out as member functions of CAM. So it will take a bit of fiddling to share them instead using import and export. Maybe, some of the new classes really deserve to become objects that own some of the CAM state. But we should minimize this pattern where, instead of seeing 'this.X' and being able to ctrl-click and go right to the implementation, I have to go a member variable that holds function X, then to the constructor that initializes it, then to the caller of that constructor, then to the local function that wraps a call to a member of another class, and then finally to the code I want to see. That's a huge downgrade of readability and increase in complexity.

[hatton]

src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementManager.ts line 149 at r20 (raw file):

Previously, JohnThomson (John Thomson) wrote…

It appears to me that most of the classes we instantiate here are just wrappers for functions.

• We started with functions X, Y, and Z in CAM which call P and Q
• We moved X, Y, and Z to a new file A, and P and Q to other files B and C
• We should have made the first new file import P and Q, and anything that needs them import X, Y, and Z. If the functions needed access to some state that lives in the CAM singleton, they should make use of a reliable function that finds the one instance (in the right iframe).
• Instead, we made a class in each new file. X, Y, and Z became public methods of Aclass, and P and Q are now public methods of BClass and CClass.
• Now, we need an instance of each of these classes, so the methods can be called. So CAM has to make instances of AClass, BClass, and CClass.
• But, X, Y, and Z still can't call P and Q, because they are member functions of Bclass and CClass, and we don't want every caller creating instances.
• So class A is made to have member variables that point at functions P and Q
• And CAM gets new functions P and Q, which just wrap calls to BInstance.P and CInstance.Q
• And then passes it's own P and Q wrappers to the constructor for A.
  Something that should have been three simple imports has become a mess of classes, member variables, wrapper functions, constructors, and passing functions all over the place.
  Now, most of these methods started out as member functions of CAM. So it will take a bit of fiddling to share them instead using import and export. Maybe, some of the new classes really deserve to become objects that own some of the CAM state. But we should minimize this pattern where, instead of seeing 'this.X' and being able to ctrl-click and go right to the implementation, I have to go a member variable that holds function X, then to the constructor that initializes it, then to the caller of that constructor, then to the local function that wraps a call to a member of another class, and then finally to the code I want to see. That's a huge downgrade of readability and increase in complexity.

[GPT-5.4]: I went back and did the broader cleanup. Besides the earlier pxToNumber simplification, CanvasElementSelectionUi.ts and CanvasElementBackgroundImageManager.ts are now direct helper modules used from CanvasElementManager, so there are two fewer constructor-injected wrapper classes / host objects in the path. I left CanvasElementHandleDragInteractions.ts, CanvasElementPointerInteractions.ts, and CanvasElementEditingSuspension.ts as classes because they carry substantial mutable interaction state and event lifecycle, and flattening those looked more likely to make the code harder to follow rather than easier.

[hatton]

src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementCanvasResizeAdjustments.ts line 19 at r19 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Good start, but we should just import pxToNumber, not require the caller to supply it. There's only one version of this function in our codebase (I hope).
Also, I'm hoping we can get rid of a lot more of these classes that were apparently just made to wrap functions better imported.

[GPT-5.4]: I agreed with the pxToNumber point and fixed that by having CanvasElementResizeAdjustments.ts import it directly. I also followed through on the broader concern instead of stopping there: CanvasElementSelectionUi.ts and CanvasElementBackgroundImageManager.ts are now direct helper modules used from CanvasElementManager, so there are fewer wrapper classes / host objects in the path. I did not flatten the drag/pointer/suspension controllers because those still own significant mutable state and event lifecycle, and flattening them looked more likely to hurt readability than help it.

[hatton]

[codex] I changed the registry’s image fallback to use getImageFromCanvasElement(ctx.canvasElement) instead of probing a canvas element through getImageFromContainer(...). That keeps the compatibility behavior but makes the intent explicit at the call site John flagged.

@hatton made 2 comments.
Reviewable status: 6 of 201 files reviewed, 34 unresolved discussions (waiting on JohnThomson).

---

src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlRegistry.ts line 729 at r16 (raw file):

: could this go at the start of the file?

[Hatton] Not necessarily without complicating things. Unlike c#, ts seems to sensitive to order. I don't want to discuss this one any more.

[hatton]

[GPT-5.4] I followed up on the latest remaining bot-reported issue directly in the branch. The recursive EditingModel.GetXmlDocumentForEditScreenWebPage(string, string) overload is gone now: the real implementation was made internal, the duplicate self-recursing overload was removed, and the file validates cleanly locally. The earlier image-fallback concern in this discussion was already addressed with getImageFromCanvasElement(ctx.canvasElement).

[devin-ai-integration]

Devin Review found 3 new potential issues.

View 3 additional findings in Devin Review.

[devin-ai-integration]

🔴 Missing wrapWithRequestPageContentDelay around async DOM mutation in canvasControlTextMenuItems.ts

The new makeFieldTypeMenuItem in canvasControlTextMenuItems.ts uses getString() (an async API call) and then sets editable.innerHTML = content in the callback, without wrapping the operation in wrapWithRequestPageContentDelay. The old code in the deleted CanvasElementContextControls.tsx:988-1024 explicitly wrapped this identical operation with wrapWithRequestPageContentDelay(…, "setCanvasFieldValueFromDataBook"). This violates the repository's src/BloomBrowserUI/AGENTS.md rule: "When code makes changes to the editable page dom using asynchronous operations, it should use wrapWithRequestPageContentDelay." Without this wrapper, a concurrent requestPageContent call could save the page before the async field-type content has been written, losing the user's field-type change.

Prompt for agents

The getString() call at canvasControlTextMenuItems.ts:347-357 asynchronously fetches a data-book value and then mutates editable.innerHTML. The old implementation in the deleted CanvasElementContextControls.tsx wrapped this exact pattern in wrapWithRequestPageContentDelay to prevent page-content save races. Import wrapWithRequestPageContentDelay from bloomEditing.ts and wrap the getString callback's DOM mutation, similar to how the old setEditableContentFromKnownDataBookValueIfAny function did it. The delay ID was "setCanvasFieldValueFromDataBook".

---

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

[devin-ai-integration]

🚩 EditingModel.cs adds two new GetXmlDocumentForEditScreenWebPage overloads that are never called

Two new methods were added to EditingModel.cs (lines 1353-1423): an internal overload taking pageUrl and pageListUrl parameters, and a public parameterless overload. Neither is called anywhere in the codebase (confirmed by grep). They duplicate the logic already in EditingView.cs for constructing the workspace root HTML. These appear to be scaffolding for a future change or were added by mistake. They are dead code and can be removed to avoid confusion.

---

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

[devin-ai-integration]

🚩 e2e test helpers and page-bundle e2e API surface shipped in production code

The editablePage.ts file now exports ~12 e2e* functions (lines 167-287) that directly manipulate CanvasElementManager internals (delete, duplicate, set active, override canExpandToFillSpace, etc.). These are exposed on the editablePageBundle window object and shipped in production builds. While not a correctness bug, they provide an attack surface for in-page scripts and add dead code to production. Consider gating these behind a build-time flag or environment check so they are stripped from release builds.

---

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

[JohnThomson]

@JohnThomson made 2 comments and resolved 31 discussions.
Reviewable status: 4 of 207 files reviewed, 6 unresolved discussions (waiting on hatton).

---

src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementManager.ts line 75 at r16 (raw file):

Previously, hatton (John Hatton) wrote…

[GPT-5.4] Cleaned this up in the current branch. CanvasElementManager now imports CanvasElementAlternates and CanvasElementPositioning as namespaces and calls those modules directly, instead of aliasing each import and wrapping it back to the original name. That keeps the public CanvasElementManager entry points where we still need them, but removes the rename-back noise you called out.

It fixed some of them, no doubt; but for example I still see seven methods imported with different names from CanvasElementSelectionUi, and at least some of them are then re-wrapped with the original names. Some do more than a completely trivial call of the renamed method, but it still smells. Maybe there's a good reason, but it's too hard to follow the call chain across files in Reviewable.

---

src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementManager.ts line 149 at r20 (raw file):

Previously, hatton (John Hatton) wrote…

[GPT-5.4]: I went back and did the broader cleanup. Besides the earlier pxToNumber simplification, CanvasElementSelectionUi.ts and CanvasElementBackgroundImageManager.ts are now direct helper modules used from CanvasElementManager, so there are two fewer constructor-injected wrapper classes / host objects in the path. I left CanvasElementHandleDragInteractions.ts, CanvasElementPointerInteractions.ts, and CanvasElementEditingSuspension.ts as classes because they carry substantial mutable interaction state and event lifecycle, and flattening those looked more likely to make the code harder to follow rather than easier.

Still feels like a mess here. Maybe an interactive review will make sense of it.