diff --git a/.changeset/shell-measures-its-container.md b/.changeset/shell-measures-its-container.md new file mode 100644 index 0000000000..bf4554e8bc --- /dev/null +++ b/.changeset/shell-measures-its-container.md @@ -0,0 +1,30 @@ +--- +"@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/builder": patch +"@nextlyhq/eslint-config": patch +"@nextlyhq/module-specifiers": patch +"@nextlyhq/plugin-form-builder": patch +"@nextlyhq/plugin-page-builder": patch +"@nextlyhq/plugin-sdk": 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/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. + +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. 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..d36073a23e 100644 --- a/e2e/tests/shell/shell.spec.ts +++ b/e2e/tests/shell/shell.spec.ts @@ -229,6 +229,75 @@ 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. + // + // 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 + 20); + + 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..9e19c7d0bb 100644 --- a/packages/builder/src/builder-shell.test.tsx +++ b/packages/builder/src/builder-shell.test.tsx @@ -12,30 +12,112 @@ * 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, + fitsFullShell, + 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; + +/** + * 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 P6_PADDING = 48; + +/** + * `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 => { + // `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") + ? P6_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); + } + + /** 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 +134,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 +158,7 @@ function stubViewport(matches: boolean) { } function renderShell(props: Partial[0]> = {}) { - stubViewport(true); + stubContainerFits(true); const onExit = vi.fn(); const result = render( @@ -168,7 +257,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 +348,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 +387,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( { ); }); + 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 @@ -513,7 +644,7 @@ describe("an embedded host with nowhere to exit to", () => { // 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 +653,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 +668,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 +678,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 +692,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 +710,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 +725,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 +740,152 @@ 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("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. + // + // 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 + P6_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 + // 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 +949,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..a10a1f0397 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, @@ -170,7 +171,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 @@ -178,18 +185,81 @@ 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; + // 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. + // + // 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. + // + // 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. + // + // `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(entry.contentRect.width)); + }); + next.observe(node); + observer.current = next; }, []); - return fits; + React.useEffect( + () => () => { + observer.current?.disconnect(); + observer.current = null; + }, + [] + ); + + return [measure, fits]; } /** @@ -387,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; } /** @@ -453,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. @@ -896,7 +977,7 @@ export function BuilderShell({ store, ...props }: BuilderShellProps) { fallbackStore.current ??= browserStore(); const resolvedStore = store ?? fallbackStore.current; const [preferences, update, loadCount] = usePreferences(resolvedStore); - const fitsFullShell = useFitsFullShell(); + const [measureShell, shellFits] = useFitsFullShell(); return ( @@ -905,56 +986,73 @@ 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 ? ( -
-

- 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 @@ -971,27 +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. */} - 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; } /** 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) { />