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/canvasControlRegistry.ts line 12 at r16 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Presets does not convey that to me...suggests some sort of initial state that might change. canvasControlAvailability?

[GPT-5.4] Agreed. The live module is now canvasControlAvailabilityRules.ts, and I also removed the leftover canvasControlAvailabilityPresets.ts duplicate so there is no second file reintroducing the old name.

[hatton]

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

Previously, JohnThomson (John Thomson) wrote…

This function is the implementation of one particular menu item. It doesn't seem to me to belong in the registry

[GPT-5.4] Agreed. The registry is back to declarative data; the concrete single-command behavior lives with the other control implementations instead of in the per-element configuration file.

[hatton]

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

Previously, JohnThomson (John Thomson) wrote…

ICanvasCommandArrangement? ICanvasCommandConfiguration? I cannot think of a sense in which it is an element definition.
(At one point I wanted the file to be canvasCommandOrganization, but that suggests a bureacracy of people.)

[GPT-5.4] Renamed this away from element definition. The interface is now ICanvasElementControlConfiguration, which better matches what the data actually represents.

[hatton]

[GPT-5.4] Fixed. GetCreativeCommonsInfo() no longer uses dynamic. It now reads the expected Creative Commons properties via validated reflection on LicenseInfo, so the cross-assembly name-based fallback still works but missing or mismatched members fail with a clear InvalidOperationException instead of a RuntimeBinderException.

[hatton]

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

Previously, JohnThomson (John Thomson) wrote…

Both this and the file name made me expect to find the mapping, but not the actual functions that implement the behavior. Maybe we could spell out that for each thing you can do to any kind of canvas element, this registry specifies the code that actually does that to the canvas element, the text, icon, and hint that should be used if the command is exposed as a menu item or control bar button, and where relevant which component in the panel implements it.

[GPT-5.4] Clarified this in code and docs. canvasElementControlRegistry.ts now describes itself as the per-type control configuration map, and the README explicitly points readers to canvasControlRegistry.ts for the concrete command and panel implementations, labels, icons, hints, and menu-row construction.

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

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 4 additional findings in Devin Review.

[devin-ai-integration]

🔴 Infinite recursion in EditingModel.GetXmlDocumentForEditScreenWebPage internal overload

The new internal overload of GetXmlDocumentForEditScreenWebPage(string pageUrl, string pageListUrl) at line 1393 calls itself recursively instead of calling the private method with the same signature at line 1347. Both methods have the signature (string, string), so the internal method resolves to itself, causing a StackOverflowException at runtime whenever UpdateCurrentPageDebugView or EditingView.ChangePage calls it.

Recursive call chain

Line 1393 defines:

internal HtmlDom GetXmlDocumentForEditScreenWebPage(string pageUrl, string pageListUrl)
{
    return GetXmlDocumentForEditScreenWebPage(pageUrl, pageListUrl);
}

This is identical in signature to itself and will recurse infinitely. It should delegate to the private method at src/BloomExe/Edit/EditingModel.cs:1347, but C# method resolution picks the most accessible matching overload — which is itself.

Prompt for agents

In src/BloomExe/Edit/EditingModel.cs, the `internal` overload at line 1393 has the exact same signature as the `private` method at line 1347, causing infinite recursion. The simplest fix is to remove the `internal` overload entirely and make the `private` method at line 1347 `internal` instead. Alternatively, rename the private method to something like `GetXmlDocumentForEditScreenWebPageCore` and have both the public and internal overloads call that renamed method. The callers of the internal overload are `UpdateCurrentPageDebugView` (line 1398) and `EditingView.ChangePage` (line 362 of EditingView.cs).

---

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

[hatton]

[GPT-5.4] Fixed. I removed the duplicate internal overload and changed the original implementation to internal, so both UpdateCurrentPageDebugView() and the other internal caller now reach the real method instead of recursing. I also validated src/BloomExe/Edit/EditingModel.cs locally and there are no file-level errors.

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