From cf1ad052349a305c748ae524e4fc91a1c7e2f77c Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Mon, 17 Aug 2026 00:57:06 +0500 Subject: [PATCH 1/7] fix(plugin-page-builder): keep the media picker inside the shell's active state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The picker's content is portalled to document.body, so the shell's `hidden` and `inert` never reach it — both apply to the subtree, and a portal leaves it. Below the supported width the inspector is inert but stays mounted, so an open picker went on floating over the "needs a wider screen" notice, fully interactive, on top of the message telling the author the editor is unavailable. Derived from the shell's own active state rather than closed by an effect. Writing `open` false would discard the author's intent, and widening the window should bring the picker back exactly as it brings back everything else the shell deliberately keeps mounted. Deriving also leaves one source of truth rather than a copy that can fall out of step. --- .../src/admin/controls/MediaControl.tsx | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/plugin-page-builder/src/admin/controls/MediaControl.tsx b/packages/plugin-page-builder/src/admin/controls/MediaControl.tsx index 509c0c0bf4..e1778f2c23 100644 --- a/packages/plugin-page-builder/src/admin/controls/MediaControl.tsx +++ b/packages/plugin-page-builder/src/admin/controls/MediaControl.tsx @@ -7,6 +7,7 @@ * dataProvider. A manual URL + alt (accessibility) remain available as a fallback. */ import { MediaPickerDialog, type Media } from "@nextlyhq/admin"; +import { useShellIsActive } from "@nextlyhq/builder/shell"; import { Button, Input } from "@nextlyhq/ui"; import { useState } from "react"; @@ -26,6 +27,22 @@ const str = (v: unknown): string => (typeof v === "string" ? v : ""); export function MediaControl({ value, onChange, label }: ControlProps) { const v = (value ?? {}) as MediaValue; const [open, setOpen] = useState(false); + /* + * The picker renders through a portal, so the shell cannot contain it. + * + * When the viewport is too narrow the shell hides and inerts the inspector + * while deliberately keeping it MOUNTED, which preserves half-finished work. + * Both of those reach the subtree only; this dialog's content is portalled to + * `document.body`, outside it, so it stayed visible and clickable on top of + * the "needs a wider screen" notice. + * + * DERIVED rather than closed by an effect. An effect would write `open` + * false, which discards the author's intent: widening the window again + * should bring back the picker they had opened, exactly as it brings back + * everything else the shell kept mounted. Deriving also means there is no + * second source of truth to fall out of step with the shell. + */ + const shellIsActive = useShellIsActive(); const onSelect = (media: Media[]) => { const m = media[0]; @@ -81,7 +98,7 @@ export function MediaControl({ value, onChange, label }: ControlProps) { /> Date: Mon, 17 Aug 2026 01:18:54 +0500 Subject: [PATCH 2/7] fix(builder): measure the shell's container rather than the viewport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shell sizes to its container — h-full w-full, no viewport units — while asking matchMedia about the window. Those agree only when the shell fills the screen, so an editor embedded in a narrow admin column on a wide display was told it fitted and compressed the rail, panel and canvas past their minimums; a wide column on a narrow display got the opposite. Neither direction is reachable by choosing a different breakpoint, because it is the wrong quantity. A ResizeObserver replaces the media query. The observer follows whichever root is currently VISIBLE, and that is the part worth stating: the editor stays mounted behind the narrow notice so unsaved slot state survives, and its wrapper is display:contents when visible and hidden when narrow, so observing it reports width 0 for the whole narrow state and the shell could never measure its way back above the threshold. The notice holds the observer while narrow, the shell root while wide, and exactly one of them is on screen at a time. A callback ref rather than an effect, since the observed element is replaced when the answer flips. contentRect rather than getBoundingClientRect, because canvas zoom scales the latter against a CSS-pixel minimum. A missing ResizeObserver falls back to rendering fully, matching the initial state's reasoning that a shell which cannot measure should not show a notice it has no evidence for. The harness takes an optional container width so a test can give the shell a narrow box inside a wide window, which is the arrangement that tells the two measurements apart. --- .../src/app/builder-shell/harness.tsx | 22 ++- .../playground/src/app/builder-shell/page.tsx | 27 ++- e2e/tests/shell/shell.spec.ts | 61 ++++++ packages/builder/src/builder-shell.test.tsx | 173 ++++++++++++++---- packages/builder/src/builder-shell.tsx | 115 +++++++++++- 5 files changed, 348 insertions(+), 50 deletions(-) diff --git a/apps/playground/src/app/builder-shell/harness.tsx b/apps/playground/src/app/builder-shell/harness.tsx index 8a404767dd..48940ca4c9 100644 --- a/apps/playground/src/app/builder-shell/harness.tsx +++ b/apps/playground/src/app/builder-shell/harness.tsx @@ -26,9 +26,27 @@ import "@nextlyhq/builder/styles.css"; * Slot contents are deliberately inert. Anything interactive here would be * testing the harness rather than the shell. */ -export function BuilderShellHarness() { +export function BuilderShellHarness({ + containerWidth, +}: { + /** + * Constrain the shell's container without touching the window. + * + * Omitted, the shell fills the viewport as before. Supplied, it gets a box + * narrower than the window — the arrangement that separates measuring the + * container from measuring the viewport, which no viewport-sized harness + * can distinguish. + */ + containerWidth?: number; +}) { return ( -
+
{ // Recorded in the DOM rather than navigating: the assertion is that diff --git a/apps/playground/src/app/builder-shell/page.tsx b/apps/playground/src/app/builder-shell/page.tsx index 625b3baa19..70a8ed2625 100644 --- a/apps/playground/src/app/builder-shell/page.tsx +++ b/apps/playground/src/app/builder-shell/page.tsx @@ -23,12 +23,35 @@ import { BuilderShellHarness } from "./harness"; * The slot contents are intentionally inert. Anything interactive here would be * testing the harness. */ -export default function BuilderShellHarnessRoute() { +export default async function BuilderShellHarnessRoute({ + searchParams, +}: { + searchParams: Promise>; +}) { // GATED, for the reason the style-fixture plugin had to be: a dev-only route // under `src/app/` is always reachable in `pnpm dev:app` and indistinguishable // from product. That exact shape put a test-double plugin into a // contributor's real plugins list. Same env-var gate as // `NEXTLY_E2E_STYLE_FIXTURE`, so the two are recognisable as one convention. if (process.env.NEXTLY_E2E_SHELL_HARNESS !== "1") notFound(); - return ; + + /* + * An optional CONTAINER width, so a test can give the shell a narrow box + * inside a wide window. + * + * That combination is the one the shell used to get wrong and the one no + * viewport-sized harness can express: it sizes to its container, so a wide + * window around a narrow column is precisely where measuring the window + * reports "fits" while the layout is being compressed past its minimums. + * + * Read on the SERVER and passed down, so the first client render already has + * it — resolving it in the browser instead would render full-width once and + * narrow afterwards, which is a hydration mismatch and also hides the very + * first measurement the test is about. + */ + const raw = (await searchParams).container; + const parsed = Number(typeof raw === "string" ? raw : ""); + const container = Number.isFinite(parsed) && parsed > 0 ? parsed : undefined; + + return ; } diff --git a/e2e/tests/shell/shell.spec.ts b/e2e/tests/shell/shell.spec.ts index a74b2923f3..7631ada627 100644 --- a/e2e/tests/shell/shell.spec.ts +++ b/e2e/tests/shell/shell.spec.ts @@ -229,6 +229,67 @@ test.describe("a viewport below the supported width", () => { }); }); +test.describe("a narrow CONTAINER inside a wide window", () => { + // The window is comfortably above the threshold throughout this block. Every + // assertion below is about the shell's own box, which is the quantity it + // sizes to (`h-full w-full`, no viewport units anywhere). + test.use({ viewport: { width: 1600, height: 900 } }); + + test("shows the notice when its column is too narrow to hold the layout", async ({ + page, + }) => { + // The defect this replaced: `matchMedia` reported the WINDOW, so an editor + // embedded in a narrow admin column on a wide screen was told it fitted and + // compressed the rail, panel and canvas past their minimums. The window + // here is 1600 and the container is below the threshold, so a viewport + // measurement and a container measurement give opposite answers — which is + // what makes this the separating case rather than a second narrow test. + await page.goto(`/builder-shell?container=${MIN_SHELL_WIDTH - 100}`); + + await expect(page.getByText(/needs a wider screen/i)).toBeVisible(); + await expect(page.getByRole("region", { name: "Canvas" })).toHaveCount(0); + }); + + test("renders the full layout when its column is wide enough", async ({ + page, + }) => { + // The positive control. Without it, a shell that showed the notice + // unconditionally — or one whose measurement never resolved — would satisfy + // the assertion above perfectly. + await page.goto(`/builder-shell?container=${MIN_SHELL_WIDTH + 200}`); + + await expect(page.getByRole("region", { name: "Canvas" })).toBeVisible(); + await expect(page.getByText(/needs a wider screen/i)).toHaveCount(0); + }); + + test("recovers when the container grows back", async ({ page }) => { + // THE DEADLOCK, and the reason the observer follows the VISIBLE root. + // + // The editor stays mounted behind the notice so unsaved slot state + // survives, and its wrapper is `display: contents` when visible and + // `hidden` when narrow. Observing THAT element reports width 0 while the + // notice is up, so `0 >= MIN_SHELL_WIDTH` stays false and the shell can + // never measure its way back — narrowing once would disable the editor for + // the rest of the session. + // + // This has to run in a browser. The unit suite's ResizeObserver is a fake + // that reports the width the test sets regardless of which element is + // observed, so it answers identically for the hidden wrapper and the + // visible root — verified by writing the deadlock deliberately, which left + // the unit file green. + await page.goto(`/builder-shell?container=${MIN_SHELL_WIDTH - 100}`); + await expect(page.getByText(/needs a wider screen/i)).toBeVisible(); + + // Grow the CONTAINER, not the window: the window never changed. + await page.getByTestId("shell-container").evaluate((node, width) => { + node.style.width = `${width}px`; + }, MIN_SHELL_WIDTH + 200); + + await expect(page.getByRole("region", { name: "Canvas" })).toBeVisible(); + await expect(page.getByText(/needs a wider screen/i)).toHaveCount(0); + }); +}); + test.describe("the readiness diagnostic itself", () => { test.use({ viewport: { width: 1440, height: 900 } }); diff --git a/packages/builder/src/builder-shell.test.tsx b/packages/builder/src/builder-shell.test.tsx index d16a6c1b7c..c0945ab48c 100644 --- a/packages/builder/src/builder-shell.test.tsx +++ b/packages/builder/src/builder-shell.test.tsx @@ -12,30 +12,90 @@ * labelled control rather than a glyph, and that preferences survive a remount * through the port rather than through `localStorage` reached for directly. */ -import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { + act, + cleanup, + fireEvent, + render, + screen, +} from "@testing-library/react"; import * as React from "react"; import { renderToString } from "react-dom/server"; import { afterEach, describe, expect, it, vi } from "vitest"; import { BuilderShell } from "./builder-shell"; import { CommandPalette } from "./command-palette"; -import { DEFAULT_PREFERENCES, type PreferenceStore } from "./shell-state"; +import { + DEFAULT_PREFERENCES, + MIN_SHELL_WIDTH, + type PreferenceStore, +} from "./shell-state"; afterEach(cleanup); /** - * `react-resizable-panels` measures its group with a `ResizeObserver`, which - * jsdom does not implement. Stubbed as an inert observer rather than one that - * reports sizes: a fake that invented dimensions would let a layout assertion - * pass against numbers this file made up, which is the failure these tests are - * written to avoid. The panels mount; their SIZES are Playwright's to check. + * The width every observed element reports, in CSS pixels. + * + * The shell decides whether it fits by measuring its CONTAINER, so this is the + * input to that decision and a test that wants the narrow branch sets it. + */ +let observedWidth = MIN_SHELL_WIDTH + 160; + +/** + * `ResizeObserver`, which jsdom does not implement. + * + * Previously stubbed INERT, on the reasoning that a fake reporting invented + * dimensions would let a layout assertion pass against numbers this file made + * up. That reasoning still holds for LAYOUT and no longer covers everything: + * the shell now derives fits-or-not from the observed width, so an inert + * observer does not abstain from that question — it answers it, permanently + * "fits", and the narrow branch becomes unreachable. + * + * So this reports ONE number the test sets, and nothing else. Panel sizes + * remain Playwright's to check; what is decided here is a comparison against a + * threshold, which is exactly the kind of thing a unit test can settle. */ -class InertResizeObserver { - observe() {} - unobserve() {} - disconnect() {} +class DrivenResizeObserver { + private static live = new Set(); + private targets = new Set(); + + constructor(private readonly callback: ResizeObserverCallback) {} + + observe(target: Element) { + this.targets.add(target); + DrivenResizeObserver.live.add(this); + // A real observer delivers an initial observation on `observe`, which is + // what lets a test set the width before rendering and have the first + // measurement already carry it. + this.deliver(); + } + + unobserve(target: Element) { + this.targets.delete(target); + } + + disconnect() { + this.targets.clear(); + DrivenResizeObserver.live.delete(this); + } + + private deliver() { + const entries = [...this.targets].map( + target => + ({ + target, + contentRect: { width: observedWidth, height: 900 }, + }) as unknown as ResizeObserverEntry + ); + if (entries.length > 0) this.callback(entries, this as never); + } + + /** Re-deliver to everything currently observing, as a resize would. */ + static redeliver() { + for (const observer of DrivenResizeObserver.live) observer.deliver(); + } } -vi.stubGlobal("ResizeObserver", InertResizeObserver); +vi.stubGlobal("ResizeObserver", DrivenResizeObserver); function memoryStore(initial: string | null = null): PreferenceStore & { value: string | null; @@ -52,16 +112,23 @@ function memoryStore(initial: string | null = null): PreferenceStore & { } /** - * jsdom has no `matchMedia`, and the shell asks it whether the viewport can - * carry the full layout. Stubbed to the supported case so the tests exercise - * the shell rather than the narrow-viewport message; the one test that wants - * the other answer stubs it itself. + * Set whether the shell's CONTAINER can carry the full layout. + * + * Named for the container rather than the viewport because that is now the + * quantity: the shell sizes to its container (`h-full w-full`, no viewport + * units), and a wide window around a narrow column used to report "fits" while + * the layout was being compressed past its minimums. + * + * `matchMedia` is still stubbed because jsdom lacks it and other code reaches + * for it, but the shell no longer asks it anything. */ -function stubViewport(matches: boolean) { +function stubContainerFits(fits: boolean) { + observedWidth = fits ? MIN_SHELL_WIDTH + 160 : MIN_SHELL_WIDTH - 100; + DrivenResizeObserver.redeliver(); vi.stubGlobal( "matchMedia", vi.fn(() => ({ - matches, + matches: fits, addEventListener: vi.fn(), removeEventListener: vi.fn(), })) @@ -69,7 +136,7 @@ function stubViewport(matches: boolean) { } function renderShell(props: Partial[0]> = {}) { - stubViewport(true); + stubContainerFits(true); const onExit = vi.fn(); const result = render( @@ -168,7 +235,7 @@ describe("panels the host cannot fill", () => { const store = memoryStore( JSON.stringify({ ...DEFAULT_PREFERENCES, leftPanel: "layers" }) ); - stubViewport(true); + stubContainerFits(true); render( { // The port is what lets these become durable server-side prefs later. A // component reaching for `localStorage` makes that a rewrite. const store = memoryStore(); - stubViewport(true); + stubContainerFits(true); render(); fireEvent.click(screen.getByRole("button", { name: "Tokens" })); @@ -259,7 +326,7 @@ describe("preferences", () => { const store = memoryStore( JSON.stringify({ ...DEFAULT_PREFERENCES, leftPanel: "fonts" }) ); - stubViewport(true); + stubContainerFits(true); render( { // `onLayoutChanged` never fires under an inert ResizeObserver, which is // exactly why the Playwright spec is the one that caught it. const store = memoryStore(); - stubViewport(true); + stubContainerFits(true); render(); fireEvent.click(screen.getByRole("button", { name: "Tokens" })); @@ -298,7 +365,7 @@ describe("preferences", () => { // A preference outlives the release that wrote it. Restoring a removed // panel leaves a region rendering nothing, with no obvious way back. const store = memoryStore(JSON.stringify({ leftPanel: "history" })); - stubViewport(true); + stubContainerFits(true); render( { // The editor also mounts as a FIELD inside an entry form, where the author is // already on the page they would be sent back to. An inert button there // teaches them that leaving does nothing. - stubViewport(true); + stubContainerFits(true); render(); expect(screen.queryByRole("button", { name: "Exit editor" })).toBeNull(); @@ -522,7 +589,7 @@ describe("an embedded host with nowhere to exit to", () => { it("still renders the editor itself", () => { // The positive control for the assertion above: without it, a shell that // failed to render anything would satisfy "no exit button" perfectly. - stubViewport(true); + stubContainerFits(true); render(
@@ -537,7 +604,7 @@ describe("a viewport too narrow for the shell", () => { it("says where to edit instead of compressing", () => { // An editor that merely gets cramped is worse than one that says it needs a // wider screen: the author otherwise discovers the limit by failing a task. - stubViewport(false); + stubContainerFits(false); render(); expect(screen.getByText(/needs a wider screen/i)).toBeTruthy(); @@ -547,7 +614,7 @@ describe("a viewport too narrow for the shell", () => { it("still offers a way out", () => { // The one control that must survive every degraded state: an author who // opened the editor on a narrow screen has to be able to leave it. - stubViewport(false); + stubContainerFits(false); const onExit = vi.fn(); render(); @@ -561,7 +628,7 @@ describe("a viewport too narrow for the shell", () => { // there; keeping the button with no handler is worse, because it looks // operable. Asserting only the button's absence would pass on the version // that still promises an escape, so the sentence is asserted too. - stubViewport(false); + stubContainerFits(false); render(); expect(screen.getByText(/needs a wider screen/i)).toBeTruthy(); @@ -579,7 +646,7 @@ describe("a viewport too narrow for the shell", () => { // Queried by test id rather than by role: the subtree is `hidden`, so a // role query correctly refuses to see it and would report the unmounted // case and the hidden case identically. - stubViewport(false); + stubContainerFits(false); render(

the caller's canvas

@@ -594,7 +661,7 @@ describe("a viewport too narrow for the shell", () => { // The positive control for the test above. Keeping the slots mounted is // only correct while they are also unreachable — a tab order that runs // through an editor nobody can see is worse than the unmount was. - stubViewport(false); + stubContainerFits(false); render(

the caller's canvas

@@ -609,13 +676,47 @@ describe("a viewport too narrow for the shell", () => { expect(screen.queryByRole("region", { name: "Canvas" })).toBeNull(); }); + it("re-renders the editor when the measured width comes back up", () => { + // Asserts the hook is WIRED — that a later measurement reaches the render + // — and deliberately does NOT claim to cover the deadlock this fix exists + // for. Saying so here because the two are easy to confuse and the stronger + // reading is the tempting one. + // + // The deadlock is that observing the editor's own wrapper reports width 0 + // whenever the notice is up, because that wrapper is `display: contents` + // when visible and `hidden` when narrow, so the shell could never measure + // its way back above the threshold. That is REAL BROWSER GEOMETRY. The + // observer here is a fake that reports whatever width the test sets, + // regardless of which element is being observed, so it reports the same + // number for the wrapper and for the visible root — measured by writing + // the deadlock deliberately, and this file stayed green. + // + // Which element carries the observer is therefore Playwright's to check, + // where elements have boxes. See `e2e/tests/shell/shell.spec.ts`. + stubContainerFits(false); + render( + +

the caller's canvas

+ + ); + expect(screen.queryByText(/wider screen/i)).not.toBeNull(); + expect(screen.queryByRole("region", { name: "Canvas" })).toBeNull(); + + act(() => { + stubContainerFits(true); + }); + + expect(screen.queryByRole("region", { name: "Canvas" })).not.toBeNull(); + expect(screen.queryByText(/wider screen/i)).toBeNull(); + }); + it("keeps a portalling slot child from opening over the notice", () => { // `hidden` and `inert` only reach what renders INSIDE the wrapper. A dialog in a slot portals // to the document body and escapes both, so it would float over the narrow-screen notice, // fully interactive. The shell publishes its own answer instead, and the palette takes it as // its default — note NO `enabled` prop here, because a caller forced to pass one would be // re-deriving MIN_SHELL_WIDTH for itself. - stubViewport(false); + stubContainerFits(false); render( { // `enabled` NARROWS the shell's state rather than replacing it. A host passing a condition of // its own — `enabled={!readOnly}` — would otherwise re-enable the portalling palette on a // viewport where the shell has hidden everything else, which is the case it exists to cover. - stubViewport(false); + stubContainerFits(false); render( { // key, focused nothing a person could see, and — where the host shares the // shortcut manager — took the keystroke from whatever binding of its own // would otherwise have handled it. - stubViewport(false); + stubContainerFits(false); render(

the caller's canvas

@@ -679,7 +780,7 @@ describe("a viewport too narrow for the shell", () => { // The className is how the host places the shell in its own layout — a grid // area, a height, a border. Dropping it on this path let the notice escape // the box the shell had been given. - stubViewport(false); + stubContainerFits(false); const { container } = render( { // so a host that swaps stores — signing into a second workspace, promoting // a memory store to a persisted one — went on writing to the one it had // replaced. - stubViewport(true); + stubContainerFits(true); const first = memoryStore(); const second = memoryStore(); diff --git a/packages/builder/src/builder-shell.tsx b/packages/builder/src/builder-shell.tsx index 035c729dc1..5722ced447 100644 --- a/packages/builder/src/builder-shell.tsx +++ b/packages/builder/src/builder-shell.tsx @@ -153,6 +153,15 @@ export interface BuilderShellProps { className?: string; } +/** + * A measuring ref that observes nothing, for the root that is not visible. + * + * Module scope so its identity is stable: an inline arrow would be a new + * function every render, and the merged ref built from it would detach and + * reattach the element on each one. + */ +const NO_MEASURE = (_node: HTMLElement | null): void => undefined; + /** A store that forgets, for a server render or a host that supplies its own. */ const NO_STORAGE: PreferenceStore = { read: () => null, @@ -178,18 +187,73 @@ function browserStore(): PreferenceStore { * the supported case and correcting after mount makes both first renders * identical. */ -function useFitsFullShell(): boolean { +function useFitsFullShell(): [(node: HTMLElement | null) => void, boolean] { const [fits, setFits] = React.useState(true); + const observer = React.useRef(null); - React.useEffect(() => { - const query = window.matchMedia(`(min-width: ${MIN_SHELL_WIDTH}px)`); - const sync = () => setFits(query.matches); - sync(); - query.addEventListener("change", sync); - return () => query.removeEventListener("change", sync); + /* + * A CALLBACK ref, not an effect over a `useRef`. + * + * The observed element is REPLACED when the answer flips — the notice and the + * shell root are different nodes, and only one of them is on screen at a time + * — so an effect keyed on mount would observe whichever rendered first and go + * on observing it after React swapped it out. + */ + const measure = React.useCallback((node: HTMLElement | null) => { + observer.current?.disconnect(); + observer.current = null; + if (node === null) return; + // jsdom implements no `ResizeObserver`. Falling back to `fits` rather than + // to a notice matches the initial state's reasoning: a shell that cannot + // measure renders fully rather than showing a message it has no evidence + // for. + if (typeof ResizeObserver === "undefined") { + setFits(true); + return; + } + const next = new ResizeObserver(entries => { + const entry = entries[entries.length - 1]; + if (entry === undefined) return; + // `contentRect`, not `getBoundingClientRect`: a transformed ancestor — + // this editor has canvas zoom — scales the latter, and the number is + // being compared against a minimum expressed in CSS pixels. + setFits(entry.contentRect.width >= MIN_SHELL_WIDTH); + }); + next.observe(node); + observer.current = next; }, []); - return fits; + React.useEffect( + () => () => { + observer.current?.disconnect(); + observer.current = null; + }, + [] + ); + + return [measure, fits]; +} + +/** + * Attach one element to a ref object and a callback ref at once. + * + * The shell root is read by the stylesheet and by F6 ownership through a ref + * OBJECT, and measured through a callback ref, and an element carries one + * `ref` attribute. Both arguments are stable — a `useRef` result and a + * `useCallback` result — so the merged ref is stable too and React does not + * detach and reattach it on every render. + */ +function useMergedRef( + object: React.MutableRefObject, + callback: (node: HTMLElement | null) => void +): (node: HTMLDivElement | null) => void { + return React.useCallback( + (node: HTMLDivElement | null) => { + object.current = node; + callback(node); + }, + [object, callback] + ); } /** @@ -582,9 +646,18 @@ function ShellRegions({ className, active, loadCount, + measureShell, }: Omit & { preferences: ShellPreferences; update: (change: (current: ShellPreferences) => ShellPreferences) => void; + /** + * Observes this subtree's own root while it is the visible one. + * + * Passed in rather than created here so the notice branch can hold it + * instead when the shell is hidden — the observer has to follow whichever + * root has a box, and only the parent renders both. + */ + measureShell: (node: HTMLElement | null) => void; /** * Whether this subtree is the one the author is using. * @@ -615,6 +688,9 @@ function ShellRegions({ useRegionCycling(regionRefs, chromeRef, active); const onKeyDownCapture = useSeparatorRegionEscape(regionRefs, active); useDesignSystemStylesheet(chromeRef); + // One element, three readers: the stylesheet and F6 ownership take the ref + // object, the container measurement takes the callback. + const rootRef = useMergedRef(chromeRef, measureShell); /** * Whether the host can fill a panel. One predicate, so the rail's disabled @@ -655,7 +731,7 @@ function ShellRegions({ return (
@@ -907,6 +983,18 @@ export function BuilderShell({ store, ...props }: BuilderShellProps) { {!fitsFullShell ? (
From fde03721180cc972195bc8ff460426c8fd91e97a Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Mon, 17 Aug 2026 01:20:23 +0500 Subject: [PATCH 3/7] chore: add changeset for container-based shell sizing --- .changeset/shell-measures-its-container.md | 29 ++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 .changeset/shell-measures-its-container.md diff --git a/.changeset/shell-measures-its-container.md b/.changeset/shell-measures-its-container.md new file mode 100644 index 0000000000..4ca30f934e --- /dev/null +++ b/.changeset/shell-measures-its-container.md @@ -0,0 +1,29 @@ +--- +"nextly": patch +"create-nextly-app": patch +"@nextlyhq/admin": patch +"@nextlyhq/admin-css": patch +"@nextlyhq/blocks-engine": patch +"@nextlyhq/blocks-react": patch +"@nextlyhq/ui": patch +"@nextlyhq/adapter-drizzle": patch +"@nextlyhq/adapter-postgres": patch +"@nextlyhq/adapter-mysql": patch +"@nextlyhq/adapter-sqlite": patch +"@nextlyhq/storage-s3": patch +"@nextlyhq/storage-uploadthing": patch +"@nextlyhq/storage-vercel-blob": patch +"@nextlyhq/plugin-form-builder": patch +"@nextlyhq/plugin-page-builder": patch +"@nextlyhq/plugin-seo": patch +"@nextlyhq/plugin-sdk": patch +"@nextlyhq/eslint-config": patch +"@nextlyhq/prettier-config": patch +"@nextlyhq/telemetry": patch +"@nextlyhq/tsconfig": patch +"@nextlyhq/builder": patch +--- + +The page editor now decides whether it fits by measuring the space it was actually given, rather than the size of the browser window. Embedded as a field inside a form on a wide screen, it used to conclude it had room and squeeze the rail, panel and canvas below the widths they need; it now shows the "needs a wider screen" message in that case, and goes back to the full layout as soon as the space grows again. + +The media picker also no longer floats over that message. It opens in a layer outside the editor, so hiding the editor left an open picker visible and clickable on top of the notice saying the editor was unavailable. From c23439d75d3600bcd0151f2750580b8da93e073c Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Mon, 17 Aug 2026 01:38:15 +0500 Subject: [PATCH 4/7] refactor(builder): decide the fit through the exported predicate The measuring hook repeated `width >= MIN_SHELL_WIDTH` inline while shell-state already exports and tests `fitsFullShell` as that answer. Two implementations of one question agree the day they are written, and these would have first disagreed at exactly the boundary the helper's own tests pin, with each side looking correct on its own. The local boolean was also called `fitsFullShell`, which shadowed the helper and is why the duplication was easy to write. It is `shellFits` now, so the name belongs to the predicate. The helper's parameter is renamed from `viewportWidth` to `width`: the quantity is the width of the box the shell was given, which is no longer the window's. --- packages/builder/src/builder-shell.test.tsx | 38 +++++++++++++++++++++ packages/builder/src/builder-shell.tsx | 32 +++++++++++------ packages/builder/src/shell-state.ts | 12 +++++-- 3 files changed, 69 insertions(+), 13 deletions(-) diff --git a/packages/builder/src/builder-shell.test.tsx b/packages/builder/src/builder-shell.test.tsx index c0945ab48c..98d90ed232 100644 --- a/packages/builder/src/builder-shell.test.tsx +++ b/packages/builder/src/builder-shell.test.tsx @@ -27,6 +27,7 @@ import { BuilderShell } from "./builder-shell"; import { CommandPalette } from "./command-palette"; import { DEFAULT_PREFERENCES, + fitsFullShell, MIN_SHELL_WIDTH, type PreferenceStore, } from "./shell-state"; @@ -676,6 +677,43 @@ describe("a viewport too narrow for the shell", () => { expect(screen.queryByRole("region", { name: "Canvas" })).toBeNull(); }); + it("decides at the boundary the exported predicate defines", () => { + // ONE implementation of "does this fit". The hook used to repeat + // `width >= MIN_SHELL_WIDTH` inline, which is a second answer to a question + // `shell-state` already exports and tests — and the two would first diverge + // at exactly the boundary those tests pin. + // + // Asserted AT the boundary rather than well inside it: the shell must agree + // with `fitsFullShell(MIN_SHELL_WIDTH) === true`, so a hook that had drifted + // to a strict `>` shows the notice here while the helper's own tests stay + // green. + observedWidth = MIN_SHELL_WIDTH; + expect(fitsFullShell(observedWidth)).toBe(true); + render( + +

canvas

+ + ); + + expect(screen.queryByRole("region", { name: "Canvas" })).not.toBeNull(); + expect(screen.queryByText(/wider screen/i)).toBeNull(); + }); + + it("refuses one pixel below that boundary", () => { + // The other side, so the test above cannot be satisfied by a shell that + // renders fully at every width. + observedWidth = MIN_SHELL_WIDTH - 1; + expect(fitsFullShell(observedWidth)).toBe(false); + render( + +

canvas

+ + ); + + expect(screen.queryByText(/wider screen/i)).not.toBeNull(); + expect(screen.queryByRole("region", { name: "Canvas" })).toBeNull(); + }); + it("re-renders the editor when the measured width comes back up", () => { // Asserts the hook is WIRED — that a later measurement reaches the render // — and deliberately does NOT claim to cover the deadlock this fix exists diff --git a/packages/builder/src/builder-shell.tsx b/packages/builder/src/builder-shell.tsx index 5722ced447..8c03a0df0a 100644 --- a/packages/builder/src/builder-shell.tsx +++ b/packages/builder/src/builder-shell.tsx @@ -26,6 +26,7 @@ import * as React from "react"; import { devWarnOnce } from "./dev-warn"; import { DEFAULT_PREFERENCES, + fitsFullShell, LEFT_PANELS, MIN_CANVAS_WIDTH, MIN_SHELL_WIDTH, @@ -179,7 +180,13 @@ function browserStore(): PreferenceStore { } /** - * Whether the viewport can carry the full shell, tracked as it changes. + * Whether the shell's own CONTAINER can carry the full layout, as it changes. + * + * The container rather than the viewport, because that is what the shell sizes + * to — `h-full w-full`, no viewport units anywhere. The two agree only when the + * shell happens to fill the window, so an editor embedded in a narrow column on + * a wide display was told it fitted and compressed its regions past their + * minimums. * * Starts as `true` on purpose. The alternative is measuring during render, * which the server cannot do — it would emit the narrow-viewport message and the @@ -217,7 +224,12 @@ function useFitsFullShell(): [(node: HTMLElement | null) => void, boolean] { // `contentRect`, not `getBoundingClientRect`: a transformed ancestor — // this editor has canvas zoom — scales the latter, and the number is // being compared against a minimum expressed in CSS pixels. - setFits(entry.contentRect.width >= MIN_SHELL_WIDTH); + // + // The comparison itself comes from `shell-state`, which exports and tests + // it. Repeating `>= MIN_SHELL_WIDTH` here would be a second answer to one + // question, and the two would first disagree exactly at the boundary the + // helper's tests pin. + setFits(fitsFullShell(entry.contentRect.width)); }); next.observe(node); observer.current = next; @@ -972,7 +984,7 @@ export function BuilderShell({ store, ...props }: BuilderShellProps) { fallbackStore.current ??= browserStore(); const resolvedStore = store ?? fallbackStore.current; const [preferences, update, loadCount] = usePreferences(resolvedStore); - const [measureShell, fitsFullShell] = useFitsFullShell(); + const [measureShell, shellFits] = useFitsFullShell(); return ( @@ -981,7 +993,7 @@ export function BuilderShell({ store, ...props }: BuilderShellProps) { them depends on this and should not make it someone else's setup step. Radix nests providers safely — a host with its own keeps its delay. */} - {!fitsFullShell ? ( + {!shellFits ? ( diff --git a/packages/builder/src/shell-state.ts b/packages/builder/src/shell-state.ts index 6c0b449782..c30215b5af 100644 --- a/packages/builder/src/shell-state.ts +++ b/packages/builder/src/shell-state.ts @@ -95,9 +95,15 @@ export const MIN_CANVAS_WIDTH = 480; */ export const MIN_SHELL_WIDTH = 1280; -/** Whether the viewport can carry the full shell. */ -export function fitsFullShell(viewportWidth: number): boolean { - return viewportWidth >= MIN_SHELL_WIDTH; +/** + * Whether a box of this width can carry the full shell. + * + * The parameter is the width of the space the shell was GIVEN, not the + * window's. The shell sizes to its container, so the two differ whenever it is + * embedded — a narrow column on a wide display being the case that matters. + */ +export function fitsFullShell(width: number): boolean { + return width >= MIN_SHELL_WIDTH; } /** From b72477292d1dce1de9ffd5bbaf921eeea8ca325b Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Mon, 17 Aug 2026 01:54:09 +0500 Subject: [PATCH 5/7] fix(builder): measure the border box so padding cannot move the threshold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The observer follows whichever root is visible, and those roots do not carry the same padding: the narrow notice is p-6, the shell root has none. contentRect excludes padding, so the same container measured 48px narrower while the notice was up. That reopened the deadlock inside a 48px band. A container growing back to anywhere between the threshold and the threshold plus that padding stayed below it and the notice never left, while a fresh render at the same width showed the editor — behaviour depending on how a width was arrived at rather than on the width. The border box is a property of the container rather than of whichever root happens to be observing it, so padding can change on either without moving the threshold. Like contentRect and unlike getBoundingClientRect it is a layout size, so canvas zoom does not scale it against a CSS-pixel minimum. The fallback is offsetWidth, which is the same box; falling back to contentRect would reinstate the sensitivity. Both recovery tests now grow into that band. Growing comfortably past the threshold clears it whether or not the padding is subtracted, so neither test could previously tell the two implementations apart. The unit observer models the padding difference for the same reason. --- e2e/tests/shell/shell.spec.ts | 10 +++- packages/builder/src/builder-shell.test.tsx | 65 ++++++++++++++++++--- packages/builder/src/builder-shell.tsx | 33 +++++++++-- 3 files changed, 96 insertions(+), 12 deletions(-) diff --git a/e2e/tests/shell/shell.spec.ts b/e2e/tests/shell/shell.spec.ts index 7631ada627..d36073a23e 100644 --- a/e2e/tests/shell/shell.spec.ts +++ b/e2e/tests/shell/shell.spec.ts @@ -281,9 +281,17 @@ test.describe("a narrow CONTAINER inside a wide window", () => { await expect(page.getByText(/needs a wider screen/i)).toBeVisible(); // Grow the CONTAINER, not the window: the window never changed. + // + // Grown to JUST above the threshold rather than comfortably past it. The + // narrow notice carries padding the shell root does not, so a measurement + // read from the CONTENT box reports this container narrower than it is + // while the notice is up, and recovery into that band never happens. A + // target of `+200` clears the threshold with or without the padding + // subtracted, so it cannot tell the two implementations apart — which is + // exactly what an earlier version of this test failed to do. await page.getByTestId("shell-container").evaluate((node, width) => { node.style.width = `${width}px`; - }, MIN_SHELL_WIDTH + 200); + }, MIN_SHELL_WIDTH + 20); await expect(page.getByRole("region", { name: "Canvas" })).toBeVisible(); await expect(page.getByText(/needs a wider screen/i)).toHaveCount(0); diff --git a/packages/builder/src/builder-shell.test.tsx b/packages/builder/src/builder-shell.test.tsx index 98d90ed232..c597c09a38 100644 --- a/packages/builder/src/builder-shell.test.tsx +++ b/packages/builder/src/builder-shell.test.tsx @@ -42,6 +42,13 @@ afterEach(cleanup); */ let observedWidth = MIN_SHELL_WIDTH + 160; +/** + * The narrow notice's horizontal padding, in CSS pixels — Tailwind `p-6` on + * both sides. The shell root carries none, which is the asymmetry that makes + * the choice of measured box observable. + */ +const NOTICE_PADDING = 48; + /** * `ResizeObserver`, which jsdom does not implement. * @@ -81,13 +88,22 @@ class DrivenResizeObserver { } private deliver() { - const entries = [...this.targets].map( - target => - ({ - target, - contentRect: { width: observedWidth, height: 900 }, - }) as unknown as ResizeObserverEntry - ); + const entries = [...this.targets].map(target => { + // The two roots the observer follows do NOT have the same padding — the + // narrow notice is `p-6`, the shell root has none — and `contentRect` + // excludes padding while `borderBoxSize` includes it. Modelling that here + // is what lets this file tell a border-box measurement from a content-box + // one; a stub reporting the same number for both cannot, and would pass + // on an implementation whose recovery threshold moves by the padding. + const padding = (target as HTMLElement).className?.includes("p-6") + ? NOTICE_PADDING + : 0; + return { + target, + borderBoxSize: [{ inlineSize: observedWidth, blockSize: 900 }], + contentRect: { width: observedWidth - padding, height: 900 }, + } as unknown as ResizeObserverEntry; + }); if (entries.length > 0) this.callback(entries, this as never); } @@ -714,6 +730,41 @@ describe("a viewport too narrow for the shell", () => { expect(screen.queryByRole("region", { name: "Canvas" })).toBeNull(); }); + it("recovers into the band where the notice's own padding would hide it", () => { + // The narrow band, and the only widths that separate a border-box + // measurement from a content-box one. + // + // The notice is `p-6` and the shell root is not, so `contentRect` reports + // this container 48px narrower while the notice is up. A container growing + // back to anywhere in [MIN_SHELL_WIDTH, MIN_SHELL_WIDTH + 48) therefore + // measures below the threshold and the notice never leaves — while a fresh + // render at that same width shows the editor, because the shell root is + // what gets observed first. Behaviour that depends on how a width was + // ARRIVED AT rather than on the width. + // + // Growing to a comfortably wide value cannot see this: it clears the + // threshold with or without the padding subtracted, which is why the first + // version of the recovery test above passed on the broken implementation. + const inBand = MIN_SHELL_WIDTH + 20; + expect(inBand).toBeLessThan(MIN_SHELL_WIDTH + NOTICE_PADDING); + + stubContainerFits(false); + render( + +

canvas

+ + ); + expect(screen.queryByText(/wider screen/i)).not.toBeNull(); + + act(() => { + observedWidth = inBand; + DrivenResizeObserver.redeliver(); + }); + + expect(screen.queryByRole("region", { name: "Canvas" })).not.toBeNull(); + expect(screen.queryByText(/wider screen/i)).toBeNull(); + }); + it("re-renders the editor when the measured width comes back up", () => { // Asserts the hook is WIRED — that a later measurement reaches the render // — and deliberately does NOT claim to cover the deadlock this fix exists diff --git a/packages/builder/src/builder-shell.tsx b/packages/builder/src/builder-shell.tsx index 8c03a0df0a..83d912baa6 100644 --- a/packages/builder/src/builder-shell.tsx +++ b/packages/builder/src/builder-shell.tsx @@ -221,15 +221,40 @@ function useFitsFullShell(): [(node: HTMLElement | null) => void, boolean] { const next = new ResizeObserver(entries => { const entry = entries[entries.length - 1]; if (entry === undefined) return; - // `contentRect`, not `getBoundingClientRect`: a transformed ancestor — - // this editor has canvas zoom — scales the latter, and the number is - // being compared against a minimum expressed in CSS pixels. + // The BORDER box, and this is load-bearing rather than a detail. // + // The observer follows whichever root is visible, and those two roots do + // not have the same padding: the narrow notice is `p-6` and the shell + // root has none. `contentRect` excludes padding, so the same container + // measures 48px narrower while the notice is up — which reopens the + // deadlock this hook exists to close, just inside a 48px band. A + // container sitting between the threshold and the threshold plus its + // padding would show the notice for ever after narrowing once, while a + // fresh render at that same width shows the editor. Behaviour that + // depends on how a width was ARRIVED AT is the defect, not the width. + // + // Reading the border box makes the measurement a property of the + // container rather than of whichever root happens to be observing it, so + // padding can change on either without moving the threshold. Like + // `contentRect` and unlike `getBoundingClientRect`, it is a layout size, + // so a transformed ancestor — this editor has canvas zoom — does not + // scale it against a minimum expressed in CSS pixels. + // + // `offsetWidth` is the same box, for anything not reporting + // `borderBoxSize`; falling back to `contentRect` would reinstate exactly + // the padding sensitivity above. + const border = entry.borderBoxSize?.[0]?.inlineSize; + const width = + border ?? + (entry.target instanceof HTMLElement + ? entry.target.offsetWidth + : entry.contentRect.width); + // The comparison itself comes from `shell-state`, which exports and tests // it. Repeating `>= MIN_SHELL_WIDTH` here would be a second answer to one // question, and the two would first disagree exactly at the boundary the // helper's tests pin. - setFits(fitsFullShell(entry.contentRect.width)); + setFits(fitsFullShell(width)); }); next.observe(node); observer.current = next; From 97c9b9dac1206b558dde87899fa38bfd82c6b335 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Mon, 17 Aug 2026 02:15:45 +0500 Subject: [PATCH 6/7] chore: cover every lockstep package in the changeset The list was copied from an existing changeset rather than derived, so it named 23 of the 24 packages that version together and omitted @nextlyhq/module-specifiers, which was added to the workspace after the file it was copied from. A package left out of the group is bumped with no changelog entry. Regenerated from the workspace, which is what the release checker compares against. --- .changeset/shell-measures-its-container.md | 27 +++++++++++----------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/.changeset/shell-measures-its-container.md b/.changeset/shell-measures-its-container.md index 4ca30f934e..bf4554e8bc 100644 --- a/.changeset/shell-measures-its-container.md +++ b/.changeset/shell-measures-its-container.md @@ -1,27 +1,28 @@ --- -"nextly": patch -"create-nextly-app": patch +"@nextlyhq/adapter-drizzle": patch +"@nextlyhq/adapter-mysql": patch +"@nextlyhq/adapter-postgres": patch +"@nextlyhq/adapter-sqlite": patch "@nextlyhq/admin": patch "@nextlyhq/admin-css": patch "@nextlyhq/blocks-engine": patch "@nextlyhq/blocks-react": patch -"@nextlyhq/ui": patch -"@nextlyhq/adapter-drizzle": patch -"@nextlyhq/adapter-postgres": patch -"@nextlyhq/adapter-mysql": patch -"@nextlyhq/adapter-sqlite": patch -"@nextlyhq/storage-s3": patch -"@nextlyhq/storage-uploadthing": patch -"@nextlyhq/storage-vercel-blob": patch +"@nextlyhq/builder": patch +"@nextlyhq/eslint-config": patch +"@nextlyhq/module-specifiers": patch "@nextlyhq/plugin-form-builder": patch "@nextlyhq/plugin-page-builder": patch -"@nextlyhq/plugin-seo": patch "@nextlyhq/plugin-sdk": patch -"@nextlyhq/eslint-config": patch +"@nextlyhq/plugin-seo": patch "@nextlyhq/prettier-config": patch +"@nextlyhq/storage-s3": patch +"@nextlyhq/storage-uploadthing": patch +"@nextlyhq/storage-vercel-blob": patch "@nextlyhq/telemetry": patch "@nextlyhq/tsconfig": patch -"@nextlyhq/builder": patch +"@nextlyhq/ui": patch +"create-nextly-app": patch +"nextly": patch --- The page editor now decides whether it fits by measuring the space it was actually given, rather than the size of the browser window. Embedded as a field inside a form on a wide screen, it used to conclude it had room and squeeze the rail, panel and canvas below the widths they need; it now shows the "needs a wider screen" message in that case, and goes back to the full layout as soon as the space grows again. From 86467a75a027eed6aebfc3e3a6f1a62eaee7020a Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Mon, 17 Aug 2026 02:57:23 +0500 Subject: [PATCH 7/7] fix(builder): measure one wrapper whose content box is the regions' space MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two readings of the container were wrong for the same underlying reason: the observed element kept being something other than the box the regions get. Following whichever root was visible made the answer depend on that root's padding — the notice is p-6 and the shell root is not — so a container growing back into a 48px band never recovered. Reading the border box fixed that asymmetry and introduced its mirror, because a border box includes padding or a border the caller put on className, decoration the regions never receive: a 1280px root with p-6 reported that it fitted while leaving 1232px to lay out in. One always-rendered wrapper now carries the caller's className and nothing else, and its content box is measured. That box is the space inside the host's decoration, which is what its children are given, and it is the same element in both states so no branch's padding can move the threshold. The notice's own padding sits inside it and is invisible to the measurement. The conditional measuring ref and the ref merge it required are gone with it. The F6 ambiguity count now counts ACTIVE shells rather than mounted ones. Per-container measurement lets siblings disagree about whether they fit, so a form can hold one usable editor beside several behind their notices; counting every mount left the only editor that could answer declining because it saw more than one, while the others declined as disabled, and the key reached nothing. --- packages/builder/src/builder-shell.test.tsx | 104 ++++++- packages/builder/src/builder-shell.tsx | 304 +++++++++----------- 2 files changed, 231 insertions(+), 177 deletions(-) diff --git a/packages/builder/src/builder-shell.test.tsx b/packages/builder/src/builder-shell.test.tsx index c597c09a38..9e19c7d0bb 100644 --- a/packages/builder/src/builder-shell.test.tsx +++ b/packages/builder/src/builder-shell.test.tsx @@ -43,11 +43,16 @@ afterEach(cleanup); let observedWidth = MIN_SHELL_WIDTH + 160; /** - * The narrow notice's horizontal padding, in CSS pixels — Tailwind `p-6` on - * both sides. The shell root carries none, which is the asymmetry that makes - * the choice of measured box observable. + * Horizontal padding a `p-6` class contributes, in CSS pixels — Tailwind's + * 1.5rem on both sides. + * + * The measured wrapper carries the CALLER's `className` and nothing else, so a + * caller adding padding there reduces what its children get by this much. The + * narrow notice also has `p-6`, inside the wrapper, where it is invisible to + * the measurement — which is the whole point of measuring one element rather + * than whichever branch is showing. */ -const NOTICE_PADDING = 48; +const P6_PADDING = 48; /** * `ResizeObserver`, which jsdom does not implement. @@ -89,14 +94,14 @@ class DrivenResizeObserver { private deliver() { const entries = [...this.targets].map(target => { - // The two roots the observer follows do NOT have the same padding — the - // narrow notice is `p-6`, the shell root has none — and `contentRect` - // excludes padding while `borderBoxSize` includes it. Modelling that here - // is what lets this file tell a border-box measurement from a content-box - // one; a stub reporting the same number for both cannot, and would pass - // on an implementation whose recovery threshold moves by the padding. + // `contentRect` excludes padding and `borderBoxSize` includes it, and the + // difference is the whole question here: what the regions get is the + // CONTENT box of the element the caller styled. Modelling both is what + // lets this file tell those two readings apart — a stub reporting one + // number for both would pass on either, which is how a measurement that + // over-reports the available space by the caller's padding would ship. const padding = (target as HTMLElement).className?.includes("p-6") - ? NOTICE_PADDING + ? P6_PADDING : 0; return { target, @@ -570,6 +575,48 @@ describe("which shell F6 belongs to", () => { ); }); + it("stays reachable when the other shells are behind their notices", () => { + // Only became possible once each shell measured its OWN container. Two + // page-builder fields in columns of different widths can disagree about + // whether they fit, so a form can hold one usable editor beside several + // showing the narrow notice. + // + // Counting every MOUNTED shell there leaves the one editor that can answer + // seeing more than one and declining, while the others decline because + // their binding is disabled — and F6 reaches nothing at all. A shell behind + // its notice is not a candidate for the key, so it is not part of the + // ambiguity either. + // + // jsdom cannot give the two shells different real widths, so the narrow one + // is rendered while the observer reports a narrow number and the wide one + // after it changes — which is the same end state: one active, one not. + observedWidth = MIN_SHELL_WIDTH - 100; + render( + +

narrow canvas

+ + ); + + act(() => { + observedWidth = MIN_SHELL_WIDTH + 160; + }); + render( + +

wide canvas

+ + ); + + // Exactly one shell is usable. + expect(screen.queryAllByText(/wider screen/i)).toHaveLength(1); + const rails = screen.getAllByRole("navigation", { name: "Editor panels" }); + expect(rails).toHaveLength(1); + + (document.activeElement as HTMLElement | null)?.blur(); + fireEvent.keyDown(document, { key: "F6" }); + + expect(document.activeElement).toBe(rails[0]); + }); + it("declines that entry when a second shell makes it ambiguous", () => { // With two mounted, a press from nowhere names neither — and answering it // anyway is decided by whichever registered last, which is how a form with @@ -730,6 +777,39 @@ describe("a viewport too narrow for the shell", () => { expect(screen.queryByRole("region", { name: "Canvas" })).toBeNull(); }); + it("subtracts padding the CALLER put on the shell", () => { + // The regions are laid out inside the caller's decoration, not across it. + // A root at exactly the threshold with `p-6` leaves 48px less than the + // layout needs, so reporting that it fits recreates the compression this + // whole change exists to prevent — measuring the border box says "1280" + // while the regions are handed 1232. + observedWidth = MIN_SHELL_WIDTH; + render( + +

canvas

+ + ); + + expect(screen.queryByText(/wider screen/i)).not.toBeNull(); + expect(screen.queryByRole("region", { name: "Canvas" })).toBeNull(); + }); + + it("does not subtract padding that belongs to the notice", () => { + // The other side, and the reason the measured element is a wrapper rather + // than whichever branch is showing. The notice's own `p-6` sits INSIDE the + // measured box, so it must not move the threshold — an earlier version + // measured the notice itself and could not recover into a 48px band. + observedWidth = MIN_SHELL_WIDTH; + render( + +

canvas

+ + ); + + expect(screen.queryByRole("region", { name: "Canvas" })).not.toBeNull(); + expect(screen.queryByText(/wider screen/i)).toBeNull(); + }); + it("recovers into the band where the notice's own padding would hide it", () => { // The narrow band, and the only widths that separate a border-box // measurement from a content-box one. @@ -746,7 +826,7 @@ describe("a viewport too narrow for the shell", () => { // threshold with or without the padding subtracted, which is why the first // version of the recovery test above passed on the broken implementation. const inBand = MIN_SHELL_WIDTH + 20; - expect(inBand).toBeLessThan(MIN_SHELL_WIDTH + NOTICE_PADDING); + expect(inBand).toBeLessThan(MIN_SHELL_WIDTH + P6_PADDING); stubContainerFits(false); render( diff --git a/packages/builder/src/builder-shell.tsx b/packages/builder/src/builder-shell.tsx index 83d912baa6..a10a1f0397 100644 --- a/packages/builder/src/builder-shell.tsx +++ b/packages/builder/src/builder-shell.tsx @@ -154,15 +154,6 @@ export interface BuilderShellProps { className?: string; } -/** - * A measuring ref that observes nothing, for the root that is not visible. - * - * Module scope so its identity is stable: an inline arrow would be a new - * function every render, and the merged ref built from it would detach and - * reattach the element on each one. - */ -const NO_MEASURE = (_node: HTMLElement | null): void => undefined; - /** A store that forgets, for a server render or a host that supplies its own. */ const NO_STORAGE: PreferenceStore = { read: () => null, @@ -221,40 +212,40 @@ function useFitsFullShell(): [(node: HTMLElement | null) => void, boolean] { const next = new ResizeObserver(entries => { const entry = entries[entries.length - 1]; if (entry === undefined) return; - // The BORDER box, and this is load-bearing rather than a detail. + // The CONTENT box of the measuring wrapper, which is by construction the + // space the regions will be laid out in. + // + // Getting this right took two wrong answers, and both were wrong for the + // same underlying reason — the observed element kept being something + // other than the box the regions actually get. // - // The observer follows whichever root is visible, and those two roots do - // not have the same padding: the narrow notice is `p-6` and the shell - // root has none. `contentRect` excludes padding, so the same container - // measures 48px narrower while the notice is up — which reopens the - // deadlock this hook exists to close, just inside a 48px band. A - // container sitting between the threshold and the threshold plus its - // padding would show the notice for ever after narrowing once, while a - // fresh render at that same width shows the editor. Behaviour that - // depends on how a width was ARRIVED AT is the defect, not the width. + // Observing whichever ROOT was visible made the answer depend on that + // root's own padding: the notice is `p-6` and the shell root is not, so + // the content box reported the same container 48px narrower while the + // notice was up, and a container growing back into that band could never + // recover. Switching to the border box fixed the asymmetry and introduced + // the opposite error, because a border box INCLUDES whatever padding or + // border the caller put on `className` — decoration the regions never + // receive — so a 1280px root with `p-6` reported that it fitted while + // leaving 1232px to lay out in. // - // Reading the border box makes the measurement a property of the - // container rather than of whichever root happens to be observing it, so - // padding can change on either without moving the threshold. Like - // `contentRect` and unlike `getBoundingClientRect`, it is a layout size, - // so a transformed ancestor — this editor has canvas zoom — does not - // scale it against a minimum expressed in CSS pixels. + // Both disappear once the measured element is a single always-rendered + // wrapper that carries the caller's `className` and nothing else. Its + // content box is the space inside the caller's decoration, which is + // exactly what its children are given, and it is the same element in both + // states so no branch's padding can move the threshold. The notice's own + // `p-6` is inside it and cannot be seen from here. // - // `offsetWidth` is the same box, for anything not reporting - // `borderBoxSize`; falling back to `contentRect` would reinstate exactly - // the padding sensitivity above. - const border = entry.borderBoxSize?.[0]?.inlineSize; - const width = - border ?? - (entry.target instanceof HTMLElement - ? entry.target.offsetWidth - : entry.contentRect.width); + // `contentRect` rather than `getBoundingClientRect` for the reason it + // always was: both are layout sizes, so a transformed ancestor — this + // editor has canvas zoom — does not scale the number against a minimum + // expressed in CSS pixels. // The comparison itself comes from `shell-state`, which exports and tests // it. Repeating `>= MIN_SHELL_WIDTH` here would be a second answer to one // question, and the two would first disagree exactly at the boundary the // helper's tests pin. - setFits(fitsFullShell(width)); + setFits(fitsFullShell(entry.contentRect.width)); }); next.observe(node); observer.current = next; @@ -271,28 +262,6 @@ function useFitsFullShell(): [(node: HTMLElement | null) => void, boolean] { return [measure, fits]; } -/** - * Attach one element to a ref object and a callback ref at once. - * - * The shell root is read by the stylesheet and by F6 ownership through a ref - * OBJECT, and measured through a callback ref, and an element carries one - * `ref` attribute. Both arguments are stable — a `useRef` result and a - * `useCallback` result — so the merged ref is stable too and React does not - * detach and reattach it on every render. - */ -function useMergedRef( - object: React.MutableRefObject, - callback: (node: HTMLElement | null) => void -): (node: HTMLDivElement | null) => void { - return React.useCallback( - (node: HTMLDivElement | null) => { - object.current = node; - callback(node); - }, - [object, callback] - ); -} - /** * Whether the shell around this subtree is currently interactive. * @@ -488,22 +457,33 @@ function useDesignSystemStylesheet( } /** - * How many shells are mounted right now. + * How many shells are ACTIVE right now — mounted and wide enough to be used. * * Read only to decide whether "focus is nowhere in particular" identifies a - * shell unambiguously. With one on the page it does; with several it does not, - * and the tie has to be declined rather than won by whichever registered last. + * shell unambiguously. With one usable editor on the page it does; with several + * it does not, and the tie has to be declined rather than won by whichever + * registered last. + * + * Active rather than merely mounted, and that distinction only became + * observable once each shell measured its OWN container: siblings in columns of + * different widths can now disagree about whether they fit, so a form can hold + * one usable editor beside several showing their narrow notices. Counting every + * mount there makes the one editor that CAN answer decline, because it sees + * more than one — and the others decline too, since their binding is disabled. + * F6 would reach nothing at all. A shell behind its notice is not a candidate + * for the key, so it is not part of the ambiguity either. */ -let mountedShells = 0; +let activeShells = 0; -function useMountedShellCount(): () => number { +function useActiveShellCount(active: boolean): () => number { React.useEffect(() => { - mountedShells += 1; + if (!active) return; + activeShells += 1; return () => { - mountedShells -= 1; + activeShells -= 1; }; - }, []); - return () => mountedShells; + }, [active]); + return () => activeShells; } /** @@ -554,7 +534,7 @@ function useRegionCycling( rootRef: React.RefObject, enabled: boolean ): void { - const shellCount = useMountedShellCount(); + const shellCount = useActiveShellCount(enabled); // Read through a ref so the binding's `when` sees the CURRENT answer. The // manager checks it at press time, and a value captured when the binding was // registered would keep reporting whatever was true then. @@ -683,18 +663,9 @@ function ShellRegions({ className, active, loadCount, - measureShell, }: Omit & { preferences: ShellPreferences; update: (change: (current: ShellPreferences) => ShellPreferences) => void; - /** - * Observes this subtree's own root while it is the visible one. - * - * Passed in rather than created here so the notice branch can hold it - * instead when the shell is hidden — the observer has to follow whichever - * root has a box, and only the parent renders both. - */ - measureShell: (node: HTMLElement | null) => void; /** * Whether this subtree is the one the author is using. * @@ -725,9 +696,6 @@ function ShellRegions({ useRegionCycling(regionRefs, chromeRef, active); const onKeyDownCapture = useSeparatorRegionEscape(regionRefs, active); useDesignSystemStylesheet(chromeRef); - // One element, three readers: the stylesheet and F6 ownership take the ref - // object, the container measurement takes the callback. - const rootRef = useMergedRef(chromeRef, measureShell); /** * Whether the host can fill a panel. One predicate, so the rail's disabled @@ -768,7 +736,7 @@ function ShellRegions({ return (
- {!shellFits ? ( -
-

- The page editor needs a wider screen -

- {/* - * The escape sentence and the button below it are ONE UNIT with the - * handler: all three present, or all three absent. - * - * Keeping the copy while dropping the control would instruct the - * author to go somewhere and then offer nothing to get there — and - * keeping a button with no handler is worse, because it looks - * operable. A host that supplies no `onExit` has no destination to - * offer, and an embedded one needs none: the author is already - * inside the surrounding form and can scroll to the rest of it. - */} - {props.onExit ? ( - <> + {/* + * THE measured element, and the only one. + * + * Always rendered, whichever branch is showing, so it always has a box + * to measure — which is what makes the decision reversible. Observing a + * branch instead made the answer depend on that branch's own padding + * and, in the editor's case, on a wrapper that is `display: contents` + * when visible and `hidden` when narrow and therefore reports 0. + * + * It carries the caller's `className` and nothing else of its own, so + * its CONTENT box is the space inside whatever padding, border or grid + * area the host gave the shell — which is precisely the space its + * children have to lay out in. Both branches are sized from it rather + * than from the host directly, so neither can disagree with the + * measurement. + */} +
+ {!shellFits ? ( +
+

+ The page editor needs a wider screen +

+ {/* + * The escape sentence and the button below it are ONE UNIT with the + * handler: all three present, or all three absent. + * + * Keeping the copy while dropping the control would instruct the + * author to go somewhere and then offer nothing to get there — and + * keeping a button with no handler is worse, because it looks + * operable. A host that supplies no `onExit` has no destination to + * offer, and an embedded one needs none: the author is already + * inside the surrounding form and can scroll to the rest of it. + */} + {props.onExit ? ( + <> +

+ Editing a layout needs at least {MIN_SHELL_WIDTH}px. On a + smaller screen you can still edit this page's content + from the admin. +

+ + + ) : (

- Editing a layout needs at least {MIN_SHELL_WIDTH}px. On a - smaller screen you can still edit this page's content - from the admin. + Editing a layout needs at least {MIN_SHELL_WIDTH}px.

- - - ) : ( -

- Editing a layout needs at least {MIN_SHELL_WIDTH}px. -

- )} -
- ) : null} + )} +
+ ) : null} - {/* The editor stays MOUNTED while the notice is up, rather than being + {/* The editor stays MOUNTED while the notice is up, rather than being swapped out for it. Returning the notice instead unmounted every slot the caller had @@ -1096,34 +1069,35 @@ export function BuilderShell({ store, ...props }: BuilderShellProps) { the editor goes on occupying memory while hidden, which is the deliberate trade: an author's unsaved work is worth more than the allocation. */} -