diff --git a/.changeset/shell-contains-overlays.md b/.changeset/shell-contains-overlays.md new file mode 100644 index 0000000000..0b567d479e --- /dev/null +++ b/.changeset/shell-contains-overlays.md @@ -0,0 +1,28 @@ +--- +"@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 +--- + +Dropdowns and pickers opened inside the page editor no longer stay on screen when the editor hides itself. If the editor did not have enough width it would show a notice explaining that, while an open dropdown floated on top of the notice and could still be clicked. diff --git a/apps/playground/src/app/builder-shell/harness.tsx b/apps/playground/src/app/builder-shell/harness.tsx index 48940ca4c9..ace04a3fe3 100644 --- a/apps/playground/src/app/builder-shell/harness.tsx +++ b/apps/playground/src/app/builder-shell/harness.tsx @@ -4,6 +4,13 @@ import { type LeftPanel } from "@nextlyhq/builder"; // The shell comes from its own entry: that is the one carrying `"use client"`, // which is why the root barrel can stay callable from a Server Component. import { BuilderShell } from "@nextlyhq/builder/shell"; +import { + Select, + SelectContent, + SelectItem, + SelectTrigger, + SelectValue, +} from "@nextlyhq/ui"; // The design system's sheet FIRST, then the editor's, which supplements it. // `@nextlyhq/builder/styles.css` deliberately ships neither the `--nx-*` tokens // nor the base reset, because the host already loads a sheet that owns both and @@ -56,7 +63,37 @@ export function BuilderShellHarness({ }} topBar={Top bar slot} breadcrumb={Breadcrumb slot} - inspector={
Inspector slot
} + inspector={ +
+ Inspector slot + {/* + * The ONE interactive thing in this harness, and it earns the + * exception the module docblock makes for inert slots. + * + * Where a portalled overlay LANDS is not decidable without one: it + * leaves the DOM position it was opened from, so the question is + * which container receives it and whether that container is inside + * the subtree the shell hides. A marker cannot answer that, and + * jsdom cannot either, since it computes no styles and would report + * a clipped dropdown and a visible one identically. + * + * Deliberately at the BOTTOM of the inspector: the inspector + * scrolls, so an overlay opened here is where clipping shows. One + * opened mid-panel looks correct whether or not the container + * escaped the scroll box. + */} +
+ +
+ } renderPanel={(panel: LeftPanel) => (
{panel} slot diff --git a/e2e/tests/shell/shell.spec.ts b/e2e/tests/shell/shell.spec.ts index 71d98c6e1e..d012e9830f 100644 --- a/e2e/tests/shell/shell.spec.ts +++ b/e2e/tests/shell/shell.spec.ts @@ -295,6 +295,125 @@ test.describe("a narrow CONTAINER inside a wide window", () => { }); }); +test.describe("overlays opened inside the shell", () => { + test.use({ viewport: { width: 1440, height: 900 } }); + + const HOST = '[data-slot="builder-overlay-host"]'; + + test("land in the shell's own host rather than the document body", async ({ + page, + }) => { + // The containment property. An overlay leaves the DOM position it was + // opened from, so nothing about the region that owns it can reach the + // content — which is why an open dropdown used to survive the shell being + // hidden. Asserted as ANCESTRY rather than by looking for the element, + // since finding it says nothing about where it went. + const shell = createShellDriver(page); + await shell.goto(); + + await page.getByTestId("overlay-trigger").click(); + const content = page.getByTestId("overlay-content"); + await expect(content).toBeVisible(); + + expect( + await content.evaluate((node, host) => node.closest(host) !== null, HOST) + ).toBe(true); + }); + + test("render with a real box when opened at a scrolled panel edge", async ({ + page, + }) => { + // The trigger sits at the BOTTOM of a scrolling inspector, which is where a + // container placed inside that scroll box would cut the dropdown off. + // + // WHAT THIS DOES NOT COVER, measured rather than assumed. Every overlay + // positions itself `fixed`, and `overflow` only clips a fixed descendant + // when an ancestor is a containing block for it — a `transform`, `filter`, + // `contain` or `will-change`. There is none anywhere in the shell today, so + // a container placed inside the inspector would NOT clip either, and this + // test would pass on that placement as readily as on the correct one. It + // cannot currently distinguish them. + // + // What actually guards the placement is the unit assertion that no ancestor + // of the host carries one of those properties + // (`builder-shell.test.tsx`), which is the condition that would make + // clipping possible. This test is the smoke check beside it: the overlay + // opens, is painted, and has a box — which is worth having, and is not the + // clipping guard. + const shell = createShellDriver(page); + await shell.goto(); + + const trigger = page.getByTestId("overlay-trigger"); + await trigger.scrollIntoViewIfNeeded(); + await trigger.click(); + + const content = page.getByTestId("overlay-content"); + await expect(content).toBeVisible(); + + const box = await content.boundingBox(); + expect(box).not.toBeNull(); + if (box === null) return; + expect(box.height).toBeGreaterThan(20); + expect(box.width).toBeGreaterThan(20); + }); + + test("resolve the design-system tokens from inside that host", async ({ + page, + }) => { + // The admin re-applies `nextly-admin` on its own portal root because that + // class selects the token values. This host inherits instead, being nested + // inside the shell, and inheritance is a different path from re-declaring: + // an overlay whose background resolves to nothing renders as unstyled text + // over the canvas, which is legible enough to pass a screenshot glance. + const shell = createShellDriver(page); + await shell.goto(); + + await page.getByTestId("overlay-trigger").click(); + const content = page.getByTestId("overlay-content"); + await expect(content).toBeVisible(); + + // Read through the BROWSER's computed value, never the custom property: + // asking for `--nx-builder-surface` returns the declared token stream + // whether or not anything defines it, which is how an unstyled overlay + // passes a check that looks like this one. + // + // Not routed through the driver's `isUnpainted`, deliberately. That + // predicate takes a whole `ShellRenderMeasurement` and reads the alpha from + // a separator its own measurement produces; supplying that here would mean + // re-deriving the alpha beside it, which is the duplication the driver + // exists to prevent. The narrower question this test needs — did the + // background resolve to anything at all — is answerable without it. + const background = await content.evaluate( + node => getComputedStyle(node).backgroundColor + ); + expect(background.trim()).not.toBe(""); + expect(background.trim()).not.toBe("transparent"); + expect(background).not.toMatch(/(rgba?\([^)]*[,/]\s*0(\.0+)?\s*\))/); + }); + + test("disappear with the shell when its container is too narrow", async ({ + page, + }) => { + // The defect this whole change exists for, end to end. Opened while the + // shell is usable, then the CONTAINER is narrowed underneath it: the + // dropdown must go with the editor rather than floating over the notice + // that says the editor is unavailable. + const shell = createShellDriver(page); + await shell.goto(); + + await page.getByTestId("overlay-trigger").click(); + await expect(page.getByTestId("overlay-content")).toBeVisible(); + + await page.getByTestId("shell-container").evaluate((node, width) => { + node.style.width = `${width}px`; + }, MIN_SHELL_WIDTH - 100); + + await expect(page.getByText(/needs more width/i)).toBeVisible(); + await expect(page.getByTestId("overlay-content")).toBeHidden(); + void shell; + }); +}); + 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 ac9c5db535..cddf8899a1 100644 --- a/packages/builder/src/builder-shell.test.tsx +++ b/packages/builder/src/builder-shell.test.tsx @@ -400,6 +400,109 @@ describe("preferences", () => { }); }); +describe("where overlays inside the shell portal to", () => { + /** The host the shell publishes for portalled overlay content. */ + function overlayHost(): HTMLElement { + const host = document.querySelector( + '[data-slot="builder-overlay-host"]' + ); + if (host === null) throw new Error("the shell rendered no overlay host"); + return host; + } + + it("puts the host inside the subtree that goes inert", () => { + // The whole point. `hidden` and `inert` are attributes on an element and + // reach descendants only, so an overlay portalled to `document.body` could + // never be covered by them — it stayed visible and clickable on top of the + // notice saying the editor was unavailable. Containment is what makes that + // impossible rather than something each control has to remember. + stubContainerFits(false); + render( + +

canvas

+
+ ); + + const inert = overlayHost().closest("[inert]"); + expect(inert).not.toBeNull(); + // And the same element is the one taken out of layout, so portalled + // content inherits `display: none` rather than merely losing pointers. + expect(inert?.hasAttribute("hidden")).toBe(true); + }); + + it("keeps the host out of every scrollable region", () => { + // The failure a naive placement produces, and it is worse than the one + // being fixed because it is silent: the panel, the canvas and the + // inspector are each `overflow-auto`, so an overlay opened near a panel + // edge would be CLIPPED rather than merely misplaced. Asserted + // structurally because jsdom computes no styles — what it can decide is + // that the host is not a descendant of any of them. + renderShell({ renderPanel: panel =>

{panel} panel

}); + fireEvent.click(screen.getByRole("button", { name: "Insert" })); + + const host = overlayHost(); + for (const region of [ + screen.getByRole("region", { name: "Canvas" }), + screen.getByRole("complementary", { name: "Inspector" }), + screen.getByRole("navigation", { name: "Editor panels" }), + ]) { + expect(region.contains(host)).toBe(false); + } + }); + + it("keeps the host out of any ancestor that would trap fixed positioning", () => { + // A guard rather than a comment, because the invariant is fragile by + // construction and its violation is silent. + // + // Every overlay positions itself `fixed`. A `transform`, `filter`, + // `backdrop-filter`, `perspective`, `contain` or `will-change` on an + // ancestor makes that ancestor the containing block instead of the + // viewport, so a centred dialog would centre on the canvas and a dropdown + // would land somewhere unrelated. The same properties create a stacking + // context, which additionally makes every z-index inside the host local + // and unable to compete with admin-level overlays. + // + // A page builder acquires canvas zoom eventually, and the day a `scale-` + // lands on an ancestor of this host is the day overlays quietly + // mispositioned. This fails then rather than in a bug report. + const TRAPS = + /(^|[\s:])(scale|rotate|translate|skew|transform|filter|blur|backdrop|perspective|isolate|contain|will-change|opacity)-/; + + renderShell({ renderPanel: panel =>

{panel} panel

}); + + const offenders: string[] = []; + for ( + let node: HTMLElement | null = overlayHost().parentElement; + node !== null; + node = node.parentElement + ) { + const classes = node.className; + if (typeof classes === "string" && TRAPS.test(classes)) { + offenders.push(`${node.tagName.toLowerCase()}: ${classes}`); + } + } + + expect(offenders).toEqual([]); + }); + + it("walked real ancestors, so the guard above is not vacuous", () => { + // The positive control. A host with no parents, or a selector that found + // nothing, would satisfy the empty-offenders check perfectly. + renderShell(); + + let depth = 0; + for ( + let node: HTMLElement | null = overlayHost().parentElement; + node !== null; + node = node.parentElement + ) { + depth += 1; + } + + expect(depth).toBeGreaterThan(2); + }); +}); + describe("the server render and the first client render agree", () => { it("emits the defaults even when the store has preferences", () => { // The hydration property, and it needs a real server render to state. diff --git a/packages/builder/src/builder-shell.tsx b/packages/builder/src/builder-shell.tsx index bbed6809e6..007fe4cbe8 100644 --- a/packages/builder/src/builder-shell.tsx +++ b/packages/builder/src/builder-shell.tsx @@ -1,6 +1,7 @@ "use client"; import { + PortalProvider, ResizableHandle, ResizablePanel, ResizablePanelGroup, @@ -969,6 +970,14 @@ export function BuilderShell({ store, ...props }: BuilderShellProps) { const resolvedStore = store ?? fallbackStore.current; const [preferences, update, loadCount] = usePreferences(resolvedStore); const [measureShell, shellFits] = useFitsFullShell(); + /* + * Where overlays inside this shell portal to. State rather than a ref, + * because `PortalProvider` has to RE-RENDER once the node exists; a ref + * mutation would leave the provider holding `null` for the life of the mount. + */ + const [overlayHost, setOverlayHost] = React.useState( + null + ); return ( @@ -1077,23 +1086,63 @@ export function BuilderShell({ store, ...props }: BuilderShellProps) { className={shellFits ? "contents" : undefined} > {/* Published as context as well as applied as attributes, because `hidden` and `inert` - only reach what renders INSIDE this wrapper. Slot content that portals to the body - escapes both, and needs to be told rather than contained. */} + only reach what renders INSIDE this wrapper. Slot content that reads this answers for + itself; content that PORTALS is contained by `overlayHost` below instead. */} - + {/* + * `null` until the host mounts, which `usePortalContainer` reads + * as "use the default". That is one commit of the old behaviour + * rather than a crash, and no overlay can be open that early. + */} + + +