Improve the sizing and positioning of special page elements (BL-16141) by JohnThomson · Pull Request #7834 · BloomBooks/BloomDesktop

JohnThomson · 2026-04-10T16:44:10Z

This change is

devin-ai-integration

Devin Review found 1 potential issue.

View 3 additional findings in Devin Review.

devin-ai-integration · 2026-04-10T16:47:32Z

src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.tsx

+    const sourceContentRect = getVisibleContentRect(sourceElement);
+    const width = Math.max(1, Math.ceil(sourceContentRect.width));
+    const height = Math.max(1, Math.ceil(sourceContentRect.height));
+    canvasElement.style.width = `${width}px`;
+    canvasElement.style.height = `${height}px`;
+
+    const probeCanvasElement = document.createElement("div");
+    probeCanvasElement.classList.add(kCanvasElementClass);
+    probeCanvasElement.style.width = `${width}px`;
+    probeCanvasElement.style.height = `${height}px`;
+    const probeContent = canvasContent.cloneNode(true) as HTMLElement;
+    probeCanvasElement.appendChild(probeContent);
+    measurementHost.appendChild(probeCanvasElement);
+
+    const probeContentRect = getVisibleContentRect(probeContent);
+    const probeCanvasRect = probeCanvasElement.getBoundingClientRect();
+    const contentOffsetLeft = probeContentRect.left - probeCanvasRect.left;
+    const contentOffsetTop = probeContentRect.top - probeCanvasRect.top;
+
+    probeCanvasElement.remove();
+
+    const pageRect = page.getBoundingClientRect();
+    const scale = EditableDivUtils.getPageScale();
+    canvasElement.style.left =
+        (sourceContentRect.left - pageRect.left) / scale -
+        contentOffsetLeft +
+        "px";
+    canvasElement.style.top =
+        (sourceContentRect.top - pageRect.top) / scale -
+        contentOffsetTop +
+        "px";


🔴 Page scale not accounted for in setCanvasElementSizeAndPositionFromVisibleContent

The new setCanvasElementSizeAndPositionFromVisibleContent function uses getVisibleContentRect() which internally relies on range.getClientRects() and getBoundingClientRect() — both of which return coordinates in viewport space (i.e., post-CSS-transform, scaled). These scaled values are then used directly for CSS properties (style.width, style.height) that operate in unscaled (layout) coordinates, and contentOffsetLeft/contentOffsetTop (also scaled) are subtracted from already-unscaled position values.

Compare with the old code path at customXmatterPage.tsx:195-215 which correctly uses clientWidth (unscaled) for sizing and getComputedStyle margins (unscaled) for offset adjustments. When EditableDivUtils.getPageScale() returns anything other than 1.0 (common with zoom or full-bleed pages), this bug causes canvas elements to be incorrectly sized (too large or too small) and mispositioned.

Size bug (lines 330-333)

sourceContentRect.width/height are scaled but used directly as CSS dimensions:

const width = Math.max(1, Math.ceil(sourceContentRect.width)); // scaled canvasElement.style.width = `${width}px`; // should be unscaled

Should be sourceContentRect.width / scale.

Position bug (lines 352-359)

contentOffsetLeft/Top are scaled (from getBoundingClientRect() differences) but subtracted from unscaled values:

canvasElement.style.left = (sourceContentRect.left - pageRect.left) / scale - // unscaled contentOffsetLeft + // scaled — should also be divided by scale "px";

Prompt for agents

The function setCanvasElementSizeAndPositionFromVisibleContent at customXmatterPage.tsx:322-360 does not account for page scale when computing sizes and positions. All values from getVisibleContentRect() (which uses getBoundingClientRect/getClientRects) are in viewport (scaled) coordinates, but CSS style properties (width, height, left, top) need unscaled values. The fix needs to: 1. Divide sourceContentRect.width and sourceContentRect.height by scale before using them for CSS width/height (lines 330-333) and for the probe dimensions (lines 337-338) 2. Divide contentOffsetLeft and contentOffsetTop by scale before subtracting them in the position calculations (lines 352-359) The scale value should be obtained from EditableDivUtils.getPageScale() and applied consistently. Compare with the existing old code path at lines 192-216 which correctly uses unscaled measurements (clientWidth, getComputedStyle margins) for sizing and divides only getBoundingClientRect values by scale.

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

JohnThomson

@JohnThomson made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion.

JohnThomson · 2026-04-10T16:53:55Z

src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.tsx

+    const sourceContentRect = getVisibleContentRect(sourceElement);
+    const width = Math.max(1, Math.ceil(sourceContentRect.width));
+    const height = Math.max(1, Math.ceil(sourceContentRect.height));
+    canvasElement.style.width = `${width}px`;
+    canvasElement.style.height = `${height}px`;
+
+    const probeCanvasElement = document.createElement("div");
+    probeCanvasElement.classList.add(kCanvasElementClass);
+    probeCanvasElement.style.width = `${width}px`;
+    probeCanvasElement.style.height = `${height}px`;
+    const probeContent = canvasContent.cloneNode(true) as HTMLElement;
+    probeCanvasElement.appendChild(probeContent);
+    measurementHost.appendChild(probeCanvasElement);
+
+    const probeContentRect = getVisibleContentRect(probeContent);
+    const probeCanvasRect = probeCanvasElement.getBoundingClientRect();
+    const contentOffsetLeft = probeContentRect.left - probeCanvasRect.left;
+    const contentOffsetTop = probeContentRect.top - probeCanvasRect.top;
+
+    probeCanvasElement.remove();
+
+    const pageRect = page.getBoundingClientRect();
+    const scale = EditableDivUtils.getPageScale();
+    canvasElement.style.left =
+        (sourceContentRect.left - pageRect.left) / scale -
+        contentOffsetLeft +
+        "px";
+    canvasElement.style.top =
+        (sourceContentRect.top - pageRect.top) / scale -
+        contentOffsetTop +
+        "px";


cubic-dev-ai

2 issues found across 1 file (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/toolbox/canvas/customXmatterPage.spec.ts">

<violation number="1" location="src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.spec.ts:79">
P2: Pass `false` here; the current assertion expects a border on an element that has none.</violation>

<violation number="2" location="src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.spec.ts:118">
P1: These layout assertions need a real-browser test setup or mocked geometry; jsdom won't exercise the sizing logic here.</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.}

cubic-dev-ai · 2026-04-10T20:42:23Z

src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.spec.ts

@@ -0,0 +1,268 @@
+import { afterEach, describe, expect, it } from "vitest";


P1: These layout assertions need a real-browser test setup or mocked geometry; jsdom won't exercise the sizing logic here.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.spec.ts, line 118: <comment>These layout assertions need a real-browser test setup or mocked geometry; jsdom won't exercise the sizing logic here.</comment> <file context> @@ -0,0 +1,268 @@ + + const rect = getVisibleContentRectForTest(container); + + expect(rect.width).toBeGreaterThan(0); + expect(rect.height).toBeGreaterThan(0); + }); </file context>

cubic-dev-ai · 2026-04-10T20:42:23Z

src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.spec.ts

+        it("detects elements with no border", () => {
+            const el = document.createElement("div");
+            el.style.border = "none";
+            expect(testElementBorderVisibility(el, true)).toBe(true);


P2: Pass false here; the current assertion expects a border on an element that has none.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.spec.ts, line 79: <comment>Pass `false` here; the current assertion expects a border on an element that has none.</comment> <file context> @@ -0,0 +1,268 @@ + it("detects elements with no border", () => { + const el = document.createElement("div"); + el.style.border = "none"; + expect(testElementBorderVisibility(el, true)).toBe(true); + }); + </file context>

devin-ai-integration

Devin Review found 3 potential issues.

View 3 additional findings in Devin Review.

devin-ai-integration · 2026-04-10T20:44:43Z

src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.spec.ts

🚩 Test file tests reimplemented helpers rather than actual production code

The test file (customXmatterPage.spec.ts) re-implements hasVisibleBorder as testElementBorderVisibility and getVisibleContentRect as getVisibleContentRectForTest, then tests these local copies rather than the actual production functions (which are not exported). This means the tests don't actually validate the production code — they only validate the test's own reimplementations. If the production code diverges from these copies, the tests would still pass. Consider exporting the helper functions (perhaps via a @visibleForTesting pattern) or using an internal-module testing approach so the actual functions are exercised.

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

devin-ai-integration · 2026-04-10T20:44:44Z

src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.tsx

+    const probeContent = canvasContent.cloneNode(true) as HTMLElement;
+    probeCanvasElement.appendChild(probeContent);
+    measurementHost.appendChild(probeCanvasElement);
+
+    const probeContentRect = getVisibleContentRect(probeContent);


🔴 Probe measures content offset with margin, but margin is zeroed afterward, causing mispositioned elements

In setCanvasElementSizeAndPositionFromVisibleContent, the probe clone (probeContent) retains the original element's margin when measuring contentOffsetLeft/contentOffsetTop. The positioning formula then subtracts this offset: CE.left = (sourceContent.left - page.left) / scale - contentOffsetLeft. This positions the CE so that content at the original margin offset would appear at the correct location. However, immediately after at customXmatterPage.tsx:191, ceContent.style.margin = "0" is applied, which removes the margin and shifts the content to offset 0 inside the CE. The result is that the content appears shifted left/up by the margin amount.

Concrete example trace

Source element with margin-left: 20px, text at viewport position 20px:

sourceContentRect.left = 20

Probe measures contentOffsetLeft = 20 (margin)

CE.left = 20/1 - 20 = 0

After margin = "0", content sits at CE.left + 0 = 0

Expected position: 20. Actual: 0.

The fix is to set probeContent.style.margin = "0" before measuring the probe, so the measured offset matches the final state after margin removal.

Suggested change

const probeContent = canvasContent.cloneNode(true) as HTMLElement;

probeCanvasElement.appendChild(probeContent);

measurementHost.appendChild(probeCanvasElement);

const probeContentRect = getVisibleContentRect(probeContent);

const probeContent = canvasContent.cloneNode(true) as HTMLElement;

probeContent.style.margin = "0";

probeCanvasElement.appendChild(probeContent);

measurementHost.appendChild(probeCanvasElement);

const probeContentRect = getVisibleContentRect(probeContent);

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

devin-ai-integration · 2026-04-10T20:44:46Z

src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.spec.ts

+        it("detects elements with no border", () => {
+            const el = document.createElement("div");
+            el.style.border = "none";
+            expect(testElementBorderVisibility(el, true)).toBe(true);


🟡 Test uses wrong expected value: expects 'no border' element to have visible border

The test "detects elements with no border" sets el.style.border = "none" and then asserts expect(testElementBorderVisibility(el, true)).toBe(true). The second argument true means "expected to have a visible border", but the element has border: none so hasVisibleBorder will be false. Thus testElementBorderVisibility(el, true) returns false === true → false, and the assertion expect(false).toBe(true) will fail. The second argument should be false (expect no visible border).

Suggested change

expect(testElementBorderVisibility(el, true)).toBe(true);

expect(testElementBorderVisibility(el, false)).toBe(true);

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

Improve the sizing and positioning of special page elements (BL-16141)

ab40e7f

devin-ai-integration bot reviewed Apr 10, 2026

View reviewed changes

Tweaks to make it work right on a scaled page

c47cd70

JohnThomson commented Apr 10, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

Add unit tests

dbb1691

cubic-dev-ai bot reviewed Apr 10, 2026

View reviewed changes

devin-ai-integration bot reviewed Apr 10, 2026

View reviewed changes

		@@ -0,0 +1,268 @@
		import { afterEach, describe, expect, it } from "vitest";

	expect(testElementBorderVisibility(el, true)).toBe(true);
	expect(testElementBorderVisibility(el, false)).toBe(true);

Uh oh!

Conversation

JohnThomson commented Apr 10, 2026 • edited by hatton Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

JohnThomson Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

JohnThomson left a comment

Choose a reason for hiding this comment

Uh oh!

JohnThomson Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot Apr 10, 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!

cubic-dev-ai bot Apr 10, 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!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

JohnThomson commented Apr 10, 2026 •

edited by hatton

Loading

cubic-dev-ai bot Apr 10, 2026 •

edited

Loading

cubic-dev-ai bot Apr 10, 2026 •

edited

Loading