diff --git a/.changeset/drop-refusal-feedback.md b/.changeset/drop-refusal-feedback.md new file mode 100644 index 0000000000..69661fc0c0 --- /dev/null +++ b/.changeset/drop-refusal-feedback.md @@ -0,0 +1,28 @@ +--- +"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 +"@nextlyhq/module-specifiers": patch +--- + +The page-builder canvas now tells an author WHY a drop was refused instead of silently doing nothing. Drop planning returns a discriminated outcome — action, refused with a reason, unchanged, or unresolved — and the drag overlay shows the reason while the drag is still in the air. diff --git a/e2e/tests/canvas/acceptance.spec.ts b/e2e/tests/canvas/acceptance.spec.ts index f8c8d3ce2f..91e18d8136 100644 --- a/e2e/tests/canvas/acceptance.spec.ts +++ b/e2e/tests/canvas/acceptance.spec.ts @@ -41,6 +41,7 @@ import { mapFramePointToHost } from "./coordinate-mapping"; import { CanvasCapabilityError, dragPointerTo, + dragSourceUntilTarget, dragToZoneEdge, dragUntilInsideZone, dragUntilTarget, @@ -111,6 +112,24 @@ async function dragOntoZone(driver: CanvasDriver): Promise { return active; } +/** + * The two sources the invalid-target point needs, each carried onto a zone. + * + * A pair, because either drag alone is satisfied by a canvas that treats every target the same + * way. Both go through the shared transport so the gesture is identical and only the block differs. + */ +async function dragRestrictedOntoZone(driver: CanvasDriver): Promise { + return dragSourceUntilTarget( + driver, + await driver.restrictedDragSourceCentre() + ); +} + +/** A drag an ordinary container will take. */ +async function dragAcceptedOntoZone(driver: CanvasDriver): Promise { + return dragSourceUntilTarget(driver, await driver.acceptedDragSourceCentre()); +} + /** * What a running drag looks like from outside, whichever engine is driving it. * @@ -687,50 +706,39 @@ test.describe("a canvas any Nextly editor could ship", () => { test("shows an explicit state over an invalid target", async ({ request, }) => { - note( - PLAN_POINT.invalidTargetVisible, - "B-7", - "this canvas shows nothing over an illegal target" - ); + note(PLAN_POINT.invalidTargetVisible, "B-7"); await driver.mountTree(await seedPage(request, NESTED_FIXTURE)); - await dragFromPanel(driver); - - // The canvas cannot answer this at all, and that refusal IS the - // shortfall. Asserted as the reader's OWN error type BEFORE the - // expectation is marked, so a broken selector, a missing iframe or a - // failed seed stays a real failure instead of becoming another - // expected one. It also fires the day the capability arrives: this - // line goes red first and forces the target below to be rewritten. - // - // Wrapped in an async thunk because these readers throw SYNCHRONOUSLY: - // `expect(reader())` never receives a promise, so `.rejects` cannot see - // the refusal and the raw error escapes the assertion entirely. - await expect(async () => chrome.readsInvalidTarget()).rejects.toThrow( - CanvasCapabilityError - ); - // Marked only now. Everything above ran unprotected. - test.fail(true, "nothing is shown over an illegal target"); - - // NOT SEPARATING YET, and the reason is in the product rather than here. - // `canDrop` refuses a drop for four reasons, and the only one a panel drag - // can reach is `not-allowed-in-slot`, which needs a slot declaring - // `allowedBlocks`. Measured against the shipped registry: no block declares - // one, so every slot accepts every child and there is no illegal target for - // this drag to enter. The pointer therefore rests somewhere legal, and a - // canvas that answered `false` here forever would satisfy the assertion the - // day the reader starts working. - // - // Closing this needs a block whose slot restricts its children — a product - // decision, not a harness one. Until then the expected failure records a - // capability the canvas lacks and NOT a judgement about what it draws over - // an illegal target, because it is never over one. + // The driver's restricted source is a block restricting its own PARENT, which is what makes an + // illegal target reachable at all: no block in the shipped registry declares an `allowedBlocks` + // slot, so a slot-side refusal has nothing to refuse and a container-side one has everything. + const refused = await dragRestrictedOntoZone(driver); + expect( + refused, + "the refused drag must reach a drop zone before the canvas is read" + ).toBeGreaterThanOrEqual(0); const explicit = await chrome.readsInvalidTarget(); await driver.cancel(); // Showing nothing is not a state. The author cannot tell "you may not drop // here" from "the drag broke", and both read as an unresponsive editor. expect(explicit, "an invalid target must be shown, not implied").toBe(true); + + // The other half, and without it the point is satisfied by a canvas that draws a refusal over + // EVERY target. That canvas tells the author nothing — a signal present everywhere carries no + // information — and it passes every assertion above. + await driver.mountTree(await seedPage(request, NESTED_FIXTURE)); + const accepted = await dragAcceptedOntoZone(driver); + expect( + accepted, + "the permitted drag must reach a drop zone before the canvas is read" + ).toBeGreaterThanOrEqual(0); + const overLegal = await chrome.readsInvalidTarget(); + await driver.cancel(); + expect( + overLegal, + "a target that accepts the block must not be shown as invalid" + ).toBe(false); }); test("autoscrolls toward an edge and stops at the bounds", async ({ diff --git a/e2e/tests/canvas/driver.ts b/e2e/tests/canvas/driver.ts index 3d684b9b97..35d847f548 100644 --- a/e2e/tests/canvas/driver.ts +++ b/e2e/tests/canvas/driver.ts @@ -69,6 +69,25 @@ export interface CanvasDriver { */ dragSourceCentre(): Promise; + /** + * Two sources chosen by whether an ordinary container will TAKE them, for the + * one acceptance point that needs a refused drag and a permitted one. + * + * A pair rather than a single "restricted" reader, because either half alone + * is satisfied by a canvas that behaves identically over both. Showing a + * refusal everywhere and showing it nowhere are different defects and each + * passes the other's test; only running the same drag with the two sources + * separates them. + * + * On the driver for the same reason {@link dragSourceCentre} is: WHICH block a + * canvas refuses is its own structural rule, and a suite that picked one by + * name could not be retargeted by swapping the driver. + */ + restrictedDragSourceCentre(): Promise; + + /** The permitted half of the pair above. */ + acceptedDragSourceCentre(): Promise; + /** A point over the canvas, near its top, in host coordinates. */ canvasCentre(): Promise; @@ -1264,6 +1283,26 @@ export async function jitterAcrossEdge( * In steps rather than one jump, because a single move is a teleport and a * canvas that commits on dwell answers a teleport differently from a gesture. */ +/** + * Start a panel drag at `source` and carry it until a drop zone is active. + * + * The whole sequence in one place because it is three steps that only work together, and a + * per-suite copy gets the middle one wrong invisibly: computing the delta from the SOURCE point + * rather than from where the pointer actually is overshoots by whatever the driver moved to cross + * the activation threshold, and the drag still runs and still ends somewhere plausible. + * + * Returns the zone's ordinal, or -1 when the descent found none. The CALLER asserts on that: + * reaching no zone means different things to a test about refusals and one about geometry. + */ +export async function dragSourceUntilTarget( + driver: CanvasDriver, + source: Point +): Promise { + await driver.startDragAt(source); + await dragPointerTo(driver, await driver.canvasCentre()); + return dragUntilTarget(driver); +} + export async function dragPointerTo( driver: CanvasDriver, target: Point, diff --git a/e2e/tests/canvas/invalid-drop-feedback.test.ts b/e2e/tests/canvas/invalid-drop-feedback.test.ts new file mode 100644 index 0000000000..5d79c40a93 --- /dev/null +++ b/e2e/tests/canvas/invalid-drop-feedback.test.ts @@ -0,0 +1,139 @@ +/** + * A refused drop has to REACH the author, and this is the only check that follows the reason the + * whole way: from the drop rules, through the drag handlers, into something on screen. + * + * The unit suites cover each end of that path and neither covers the middle. `canDrop` returning + * `wrong-parent` and `dropRefusalMessage` turning it into a sentence both stay green when the + * handler stops calling `setRefusal`, or when the status element stops rendering it — and the + * symptom of either is an editor that shows nothing, which is exactly the state this feature + * replaced. + * + * ## The separating property + * + * "The tree does not change" is NOT it. A canvas that draws nothing satisfies that perfectly, and + * that canvas is the one being replaced. What separates a working refusal from a silent one is the + * REASON arriving where the author reads it, so that is what is asserted — against a control drag + * that must reach a target and say nothing. + * + * ## Why no special fixture + * + * The driver's restricted source is a block that restricts its own PARENT, so an ordinary + * container refuses it by the shipped registry's own rule. A fixture manufacturing a slot with an + * allowlist would be testing a document nobody authors. + */ +import { expect, test, type Page } from "@playwright/test"; + +import { dragSourceUntilTarget } from "./driver"; +import type { CanvasFixture } from "./driver"; +import { FLAT_LIST_FIXTURE, seedPage } from "./fixtures"; +import { createPocDriver } from "./poc-driver"; + +test.describe.configure({ timeout: 240_000 }); +// Below roughly 1280px the editor drops the canvas preview entirely, and `mountTree` then times +// out against a canvas that is working. +test.use({ viewport: { width: 2560, height: 1400 } }); + +/** The element dnd-kit positions under the cursor; the chip and the refusal are inside it. */ +const DRAG_OVERLAY = ".nx-pb-drag-overlay"; + +/** + * The sentence `wrong-parent` produces, restated rather than imported. + * + * The plugin does not export `dropRefusalMessage` from any entry, and widening a package's public + * surface to let a test read one string is the wrong trade. Restating it errs in the loud + * direction: a copy change fails here and is confirmed by whoever made it, whereas the exhaustive + * mapping over every reason is the unit suite's job and stays there. + */ +const WRONG_PARENT_TEXT = "This block can only go inside certain containers."; + +/** + * Drag one of the driver's two named sources onto a real drop zone, refusing to return without + * one. + * + * The canvas centre is over dead space as often as not, so reading the overlay straight after + * arriving would measure a drag with no target — and "no refusal shown" is then true for a reason + * that has nothing to do with the feature. + * + * WHICH block each source is belongs to the driver, not here: it is the canvas's own structural + * rule that decides what a container refuses, and naming a block by label in this file would make + * the test unretargetable and duplicate a lookup that already exists. The transport is shared for + * the same reason — the two drags must differ only in the block, so any difference the assertions + * see is the rule rather than the gesture. + */ +async function dragOntoZone( + page: Page, + fixture: CanvasFixture, + pick: "restricted" | "accepted" +): Promise { + const driver = createPocDriver(page); + await driver.mountTree(fixture); + const source = + pick === "restricted" + ? await driver.restrictedDragSourceCentre() + : await driver.acceptedDragSourceCentre(); + const active = await dragSourceUntilTarget(driver, source); + expect( + active, + `the ${pick} drag must reach a drop zone before the overlay is read` + ).toBeGreaterThanOrEqual(0); +} + +test("tells the author which rule refused the drop", async ({ + page, + request, +}) => { + await dragOntoZone( + page, + await seedPage(request, FLAT_LIST_FIXTURE), + "restricted" + ); + + const overlay = page.locator(DRAG_OVERLAY); + await expect( + overlay, + "the drag overlay must be on screen for its contents to be read" + ).toHaveCount(1); + await expect( + overlay.locator("[data-refused]"), + "the overlay must mark itself refused while a rule is stopping the drop" + ).toHaveCount(1); + // The sentence, in the live region. Both halves matter and they break independently: the text is + // what a sighted author reads, and `role="status"` is what carries it to one who is not looking + // at the cursor. + // + // EXACTLY the sentence. The refusal mark beside it is an SVG, so it contributes no text of its + // own, and equality then rejects a region that has accumulated a second reason rather than + // replaced the first. + await expect( + overlay.locator('[role="status"]'), + "the refusal must name the rule where the author reads it" + ).toHaveText(WRONG_PARENT_TEXT); +}); + +test("says nothing over a target that accepts the block", async ({ + page, + request, +}) => { + await dragOntoZone( + page, + await seedPage(request, FLAT_LIST_FIXTURE), + "accepted" + ); + + // The population first. Silence is the expected result here, and silence is also what a drag + // that never started, an overlay that never mounted and a broken selector all produce — so the + // overlay being present is what makes the emptiness below evidence rather than absence. + const overlay = page.locator(DRAG_OVERLAY); + await expect( + overlay, + "the drag overlay must be on screen for its contents to be read" + ).toHaveCount(1); + await expect( + overlay.locator("[data-refused]"), + "the target accepts this block, so nothing may be marked refused" + ).toHaveCount(0); + await expect( + overlay.locator('[role="status"]'), + "the live region must stay empty rather than carry a stale reason" + ).toHaveText(""); +}); diff --git a/e2e/tests/canvas/poc-driver.ts b/e2e/tests/canvas/poc-driver.ts index 8cf9577520..2b0545ee8f 100644 --- a/e2e/tests/canvas/poc-driver.ts +++ b/e2e/tests/canvas/poc-driver.ts @@ -78,6 +78,36 @@ const EDITOR_ROOT = ".nx-pb-editor"; /** A library entry in the left panel; the drag source for a cross-frame drag. */ const LIBRARY_ITEM = ".nx-pb-lib-item"; +/** The library's filter, used to bring a named entry into view rather than scrolling to it. */ +const LIBRARY_SEARCH = "Search blocks"; + +/** + * A block this canvas refuses inside an ordinary container, and one it accepts. + * + * `core/column` declares `parent: ["core/columns"]`, so a container's drop zones refuse it by the + * shipped registry's own rule — no fixture has to manufacture a restricted slot. `core/heading` + * declares no parent and sits in the same panel, so the two differ only in the rule under test. + */ +const RESTRICTED_BLOCK_LABEL = "Column"; +const ACCEPTED_BLOCK_LABEL = "Heading"; + +/** The element dnd-kit positions under the cursor; the chip and any refusal are inside it. */ +const DRAG_OVERLAY = ".nx-pb-drag-overlay"; + +/** The overlay's refusal marker, set only while a rule is stopping the drop. */ +const REFUSED = "[data-refused]"; + +/** Where the refusal's sentence goes, so it reaches an author not watching the cursor. */ +const LIVE_REGION = '[role="status"]'; + +/** + * How long a refusal may take to appear after the drag reaches the target it refuses. + * + * One React commit, so this is generous by two orders of magnitude on purpose: it is the cost of + * reporting "not refused" about a target that IS refused, and only a permitted target ever pays it. + */ +const REFUSAL_RENDER_MS = 2_000; + /** Past dnd-kit's activation distance in one move, so a drag actually starts. */ const DRAG_THRESHOLD_PX = 12; @@ -121,6 +151,32 @@ export function createPocDriver(page: Page): CanvasDriver { }); } + /** + * The centre of the library entry carrying exactly this label. + * + * Filtered for rather than scrolled to. The library is a long scrolling list, so an entry's + * locator resolves happily while its box is off-screen and a drag started there begins nowhere; + * typing into the search remounts each category expanded and puts the match at the top of the + * panel. + * + * EXACT text, because "Column" and "Columns" are both registered blocks and a container refuses + * only one of them. A substring match returns whichever the DOM lists first, so the drag under + * test would be chosen by document order rather than by the rule. + */ + async function libraryEntryCentre(label: string): Promise { + await page.getByLabel(LIBRARY_SEARCH).fill(label); + const entry = page + .locator(LIBRARY_ITEM) + .filter({ has: page.getByText(label, { exact: true }) }); + await expect( + entry, + `exactly one library entry must be labelled "${label}"` + ).toHaveCount(1); + const box = await entry.boundingBox(); + if (!box) throw new Error(`library entry "${label}" has no box`); + return { x: box.x + box.width / 2, y: box.y + box.height / 2 }; + } + /** * Every drop zone's vertical extent in HOST coordinates, document order. * @@ -206,6 +262,14 @@ export function createPocDriver(page: Page): CanvasDriver { return { x: box.x + box.width / 2, y: box.y + box.height / 2 }; }, + async restrictedDragSourceCentre() { + return libraryEntryCentre(RESTRICTED_BLOCK_LABEL); + }, + + async acceptedDragSourceCentre() { + return libraryEntryCentre(ACCEPTED_BLOCK_LABEL); + }, + async canvasCentre() { const box = await page.locator("iframe").boundingBox(); if (!box) throw new Error("canvas iframe has no box"); @@ -699,11 +763,39 @@ export function createPocChromeReader(page: Page): CanvasChromeReader { return { count: inHost + inFrame, host: "document" as const }; }, - readsInvalidTarget(): Promise { - throw new CanvasCapabilityError( - "this canvas shows nothing over an illegal target, so there is no " + - "invalid state to read; absence of an indicator is not a state" - ); + async readsInvalidTarget(): Promise { + // The overlay first, and it THROWS rather than answering. `false` here would mean "this + // canvas shows nothing over an illegal target", and a drag that never started produces the + // same nothing — so a harness fault would be reported as the shortfall this reader exists to + // detect, which is the one confusion it must not make. + const overlay = page.locator(DRAG_OVERLAY); + if ((await overlay.count()) === 0) { + throw new Error( + "no drag overlay is on screen, so there is no drag whose target could be invalid" + ); + } + // Marked AND worded. The marker alone is a state the canvas draws for itself; what the + // requirement asks for is something the author can read, and the two break independently. + // + // WAITED FOR, not counted. `count()` resolves against the DOM as it stands, and the two + // things this reader correlates are written by different systems: the active zone that got + // the drag here is a dnd-kit attribute write on an element inside the canvas frame, while + // the marker is a React commit in the host document. They land in either order, so an + // immediate read reports a refusal that has not rendered yet as no refusal at all — a + // canvas defect, from a race. + // + // The wait is bounded, and the bound is the honest instrument here: nothing signals "this + // canvas has decided and the answer is no", so a permitted target is only distinguishable + // from a slow refusal by having stayed unmarked for longer than a refusal takes to draw. + // It errs toward WAITING — a slow machine costs the timeout rather than a false verdict. + const marked = await overlay + .locator(REFUSED) + .waitFor({ state: "attached", timeout: REFUSAL_RENDER_MS }) + .then(() => true) + .catch(() => false); + if (!marked) return false; + const said = await overlay.locator(LIVE_REGION).innerText(); + return said.trim().length > 0; }, canvasScrollTop(): Promise { diff --git a/packages/plugin-page-builder/src/admin/EditorSurface.tsx b/packages/plugin-page-builder/src/admin/EditorSurface.tsx index 571faf1e38..a6ae3935ec 100644 --- a/packages/plugin-page-builder/src/admin/EditorSurface.tsx +++ b/packages/plugin-page-builder/src/admin/EditorSurface.tsx @@ -10,14 +10,16 @@ */ import { DragDropProvider, DragOverlay } from "@dnd-kit/react"; import { type LucideIcon } from "lucide-react"; +import { useState } from "react"; import { defaultBlockRegistry } from "../core/registry"; import { Canvas } from "./canvas/Canvas"; -import { Monitor, Smartphone, Tablet } from "./icons"; +import { Ban, Monitor, Smartphone, Tablet } from "./icons"; import { InvalidSlotBanner } from "./InvalidSlotBanner"; import { dragLabel } from "./logic/dragLabel"; -import { planDrop } from "./logic/dropPlan"; +import { planDrop, type DropOutcome, type DropRefusal } from "./logic/dropPlan"; +import { dropRefusalMessage } from "./logic/dropRefusal"; import { BlockLibrary } from "./panels/BlockLibrary"; import { Inspector } from "./panels/Inspector"; import { useEditor } from "./store/EditorProvider"; @@ -28,31 +30,78 @@ const BREAKPOINTS: { id: string; label: string; Icon: LucideIcon }[] = [ { id: "mobile", label: "Mobile", Icon: Smartphone }, ]; +/** The source and target a drag event carries, which is all either handler below reads. */ +interface DragOperation { + source: { id: string | number; data?: unknown } | null; + target: { id: string | number; data?: unknown } | null; +} + export function EditorSurface() { const { state, dispatch } = useEditor(); const root = state.document.root; + /** + * Why the CURRENT target refuses this block, while the drag is still in the air. + * + * Held here rather than derived at render: the overlay renders on every pointer move and + * planning is a tree walk, so it is computed when the target CHANGES — which is exactly when + * `dragover` fires. + */ + const [refusal, setRefusal] = useState(null); - const onDragEnd = (event: { - operation: { - source: { id: string | number; data?: unknown } | null; - target: { id: string | number; data?: unknown } | null; - }; - canceled: boolean; - }) => { - if (event.canceled) return; - const { source, target } = event.operation; - if (!source || !target) return; - const action = planDrop( + const outcomeOf = (operation: DragOperation): DropOutcome => { + const { source, target } = operation; + if (!source || !target) return { kind: "unresolved" }; + return planDrop( source.data ?? {}, target.data ?? {}, root, defaultBlockRegistry ); - if (action) dispatch(action); + }; + + /** + * Feedback lands DURING the drag, not on release. A refusal the author only discovers after + * letting go is the failure this is here to remove: they aim at a container, nothing happens, + * and nothing says why. + */ + const onDragOver = (event: { operation: DragOperation }) => { + const outcome = outcomeOf(event.operation); + setRefusal(outcome.kind === "refused" ? outcome.reason : null); + }; + + /** + * The first target needs its own read, because `dragover` cannot report it. + * + * `setDropTarget` returns early when the identifier is unchanged, and dispatches only once the + * operation is already `dragging`. A target resolved while the drag is still initialising + * therefore sets the identifier WITHOUT emitting — and every later resolution of that same + * target takes the early return. So a drag that begins over a refusing container would say + * nothing until the pointer left and came back, which is exactly the case a node dragged inside + * its own formatted parent hits first. + */ + const onDragStart = (event: { operation: DragOperation }) => { + const outcome = outcomeOf(event.operation); + setRefusal(outcome.kind === "refused" ? outcome.reason : null); + }; + + const onDragEnd = (event: { + operation: DragOperation; + canceled: boolean; + }) => { + // Cleared on EVERY end, cancels included: the overlay unmounts but this state does not, and a + // refusal left behind would be the message shown at the start of the next drag. + setRefusal(null); + if (event.canceled) return; + const outcome = outcomeOf(event.operation); + if (outcome.kind === "action") dispatch(outcome.action); }; return ( - +
@@ -99,26 +148,108 @@ export function EditorSurface() {
- + {/* + * Named, because everything inside it is an anonymous inline-styled div otherwise. The chip + * and the refusal below it are the editor's only feedback during a drag, and neither the + * stylesheet nor anything reading the surface has a handle on them without a class on the + * element dnd-kit positions. + */} + {source => (
- ⠿ - {dragLabel(source?.data ?? {}, root, defaultBlockRegistry)} + + ⠿ + {dragLabel(source?.data ?? {}, root, defaultBlockRegistry)} + + {/* + * Mounted for the WHOLE drag and only its text changes. A polite live region that + * enters the accessibility tree already carrying its first message is not reliably + * announced, so a region created at the moment of refusal can stay silent for the one + * case it exists to speak for. Empty, it renders nothing and occupies no space. + * + * Polite rather than assertive: this text changes on every target the pointer crosses, + * and an assertive region would interrupt on each one. + */} + + {refusal ? ( + <> + {/* + * An icon rather than a character. The obvious glyph for this is U+20E0 + * COMBINING ENCLOSING CIRCLE BACKSLASH, which is an enclosing MARK: it has no + * form of its own and needs a base character to enclose, so standing alone it + * draws a dotted-circle placeholder or nothing at all depending on the font. + * A component renders the same everywhere and contributes no text to the live + * region beside it. + */} + + {dropRefusalMessage(refusal)} + + ) : null} +
)} diff --git a/packages/plugin-page-builder/src/admin/icons.tsx b/packages/plugin-page-builder/src/admin/icons.tsx index 2484913557..dbcda67b50 100644 --- a/packages/plugin-page-builder/src/admin/icons.tsx +++ b/packages/plugin-page-builder/src/admin/icons.tsx @@ -9,6 +9,7 @@ import { ArrowDown, ArrowUp, + Ban, ChevronDown, ChevronRight, Copy, @@ -51,6 +52,7 @@ export function blockIcon(name: string | undefined): LucideIcon { export { ArrowDown, ArrowUp, + Ban, ChevronDown, ChevronRight, Copy, diff --git a/packages/plugin-page-builder/src/admin/logic/dropPlan.test.ts b/packages/plugin-page-builder/src/admin/logic/dropPlan.test.ts index 4850654888..f9e1ddb27e 100644 --- a/packages/plugin-page-builder/src/admin/logic/dropPlan.test.ts +++ b/packages/plugin-page-builder/src/admin/logic/dropPlan.test.ts @@ -1,3 +1,11 @@ +/** + * Every assertion here reads the outcome's KIND, and a refusal also reads its REASON. + * + * Asserting only that no action came back would not separate these cases: a refused drop, a no-op + * and an unresolvable target are all "nothing to dispatch", so a test written that way passes on + * an implementation that has confused any one of them with the other two — and the distinction + * each case exists to pin is the one thing it cannot see. + */ import { describe, expect, it } from "vitest"; import { defaultBlockRegistry } from "../../core/registry"; @@ -19,50 +27,43 @@ const reg = defaultBlockRegistry; describe("planDrop", () => { it("plans an ADD from the library at the target index", () => { const root = tree(); - const action = planDrop( + const outcome = planDrop( { kind: "library", blockType: "core/heading" }, { kind: "dropzone", parentId: root.id, slot: "default", index: 1 }, root, reg ); - expect(action).toEqual({ - type: "ADD", - parentId: root.id, - slot: "default", - nodeType: "core/heading", - index: 1, + expect(outcome).toEqual({ + kind: "action", + action: { + type: "ADD", + parentId: root.id, + slot: "default", + nodeType: "core/heading", + index: 1, + }, }); }); - it("rejects an ADD into a non-container", () => { - const root = tree(); - const heading = root.slots!.default![0]; - expect( - planDrop( - { kind: "library", blockType: "core/paragraph" }, - { kind: "dropzone", parentId: heading.id, slot: "default", index: 0 }, - root, - reg - ) - ).toBeNull(); - }); - it("plans a MOVE and adjusts the index for a downward same-slot move", () => { const root = tree(); const a = root.slots!.default![0]; // index 0 // drop A into gap index 3 (after C) → after removal, target becomes 2 - const action = planDrop( + const outcome = planDrop( { kind: "node", nodeId: a.id }, { kind: "dropzone", parentId: root.id, slot: "default", index: 3 }, root, reg ); - expect(action).toEqual({ - type: "MOVE", - id: a.id, - parentId: root.id, - slot: "default", - index: 2, + expect(outcome).toEqual({ + kind: "action", + action: { + type: "MOVE", + id: a.id, + parentId: root.id, + slot: "default", + index: 2, + }, }); }); @@ -70,66 +71,117 @@ describe("planDrop", () => { const root = tree(); const c = root.slots!.default![2]; // index 2 // drop C into gap index 0 (before A) - const action = planDrop( + const outcome = planDrop( { kind: "node", nodeId: c.id }, { kind: "dropzone", parentId: root.id, slot: "default", index: 0 }, root, reg ); - expect(action).toEqual({ - type: "MOVE", - id: c.id, - parentId: root.id, - slot: "default", - index: 0, + expect(outcome).toEqual({ + kind: "action", + action: { + type: "MOVE", + id: c.id, + parentId: root.id, + slot: "default", + index: 0, + }, }); }); - it("treats a drop adjacent to the source as a no-op", () => { - const root = tree(); - const b = root.slots!.default![1]; // index 1 - // gap index 1 (before B) and gap index 2 (after B) are both no-ops - expect( - planDrop( - { kind: "node", nodeId: b.id }, - { kind: "dropzone", parentId: root.id, slot: "default", index: 1 }, - root, - reg - ) - ).toBeNull(); - expect( - planDrop( - { kind: "node", nodeId: b.id }, - { kind: "dropzone", parentId: root.id, slot: "default", index: 2 }, - root, - reg - ) - ).toBeNull(); - }); + describe("the three outcomes that are not an action", () => { + /** + * The point of the group: one fixture reaches each of them, and every assertion below names a + * DIFFERENT kind. An implementation that collapsed any two would fail here rather than pass + * three tests with one value. + */ + it("REFUSES an ADD into a non-container, and says which rule", () => { + const root = tree(); + const heading = root.slots!.default![0]; + expect( + planDrop( + { kind: "library", blockType: "core/paragraph" }, + { kind: "dropzone", parentId: heading.id, slot: "default", index: 0 }, + root, + reg + ) + ).toEqual({ kind: "refused", reason: "not-a-container" }); + }); - it("rejects dropping a container into its own descendant", () => { - const inner = makeNode("core/heading", { text: "x" }); - const outer = makeNode("core/container", {}, undefined, { - default: [inner], + it("REFUSES a block the container's slot does not admit, and says which rule", () => { + // `core/columns` admits only `core/column`, so a heading aimed at it is refused by the + // slot's allowlist rather than by the container being unable to hold children at all — + // a different reason, and the author's remedy differs with it. + const columns = makeNode("core/columns", {}, undefined, { default: [] }); + const root = makeNode("core/container", {}, undefined, { + default: [columns], + }); + expect( + planDrop( + { kind: "library", blockType: "core/heading" }, + { kind: "dropzone", parentId: columns.id, slot: "default", index: 0 }, + root, + reg + ) + ).toEqual({ kind: "refused", reason: "not-allowed-in-slot" }); }); - const root = makeNode("core/container", {}, undefined, { - default: [outer], + + it("REFUSES dropping a container into its own descendant", () => { + const innerC = makeNode("core/container", {}, undefined, { default: [] }); + const outer = makeNode("core/container", {}, undefined, { + default: [innerC], + }); + const root = makeNode("core/container", {}, undefined, { + default: [outer], + }); + expect( + planDrop( + { kind: "node", nodeId: outer.id }, + { kind: "dropzone", parentId: innerC.id, slot: "default", index: 0 }, + root, + reg + ) + ).toEqual({ kind: "refused", reason: "into-itself" }); }); - // try to move `outer` into its own child container? use a nested container - const innerC = makeNode("core/container", {}, undefined, { default: [] }); - const outer2 = makeNode("core/container", {}, undefined, { - default: [innerC], + + it("reports a drop adjacent to the source as UNCHANGED, not refused", () => { + const root = tree(); + const b = root.slots!.default![1]; // index 1 + // gap index 1 (before B) and gap index 2 (after B) both land B where it already is + for (const index of [1, 2]) { + expect( + planDrop( + { kind: "node", nodeId: b.id }, + { kind: "dropzone", parentId: root.id, slot: "default", index }, + root, + reg + ) + ).toEqual({ kind: "unchanged" }); + } }); - const root2 = makeNode("core/container", {}, undefined, { - default: [outer2], + + it("reports a target the document does not hold as UNRESOLVED, not refused", () => { + const root = tree(); + expect( + planDrop( + { kind: "library", blockType: "core/heading" }, + { + kind: "dropzone", + parentId: "no-such-node", + slot: "default", + index: 0, + }, + root, + reg + ) + ).toEqual({ kind: "unresolved" }); + }); + + it("reports a drag that ended off any drop zone as UNRESOLVED", () => { + const root = tree(); + expect( + planDrop({ kind: "library", blockType: "core/heading" }, {}, root, reg) + ).toEqual({ kind: "unresolved" }); }); - expect( - planDrop( - { kind: "node", nodeId: outer2.id }, - { kind: "dropzone", parentId: innerC.id, slot: "default", index: 0 }, - root2, - reg - ) - ).toBeNull(); }); }); diff --git a/packages/plugin-page-builder/src/admin/logic/dropPlan.ts b/packages/plugin-page-builder/src/admin/logic/dropPlan.ts index 9017f4629d..89ebd675ff 100644 --- a/packages/plugin-page-builder/src/admin/logic/dropPlan.ts +++ b/packages/plugin-page-builder/src/admin/logic/dropPlan.ts @@ -9,7 +9,7 @@ import type { BlockRegistry } from "../../core/registry"; import { findNode } from "../../core/tree"; import type { BlockNode } from "../../core/types"; -import { canDrop } from "./dropRules"; +import { canDrop, type DropReason } from "./dropRules"; import { locateNode } from "./locate"; export interface DragSource { @@ -36,59 +36,104 @@ export type DropAction = } | { type: "MOVE"; id: string; parentId: string; slot: string; index: number }; +/** + * Why this planner refused a drop: every reason `canDrop` can give, plus the one only a MOVE can + * hit. + * + * Derived from `DropReason` rather than restated, so a rule added to `canDrop` reaches the author + * without a second list having to be remembered. `into-itself` is not among them because it is not + * a property of the types involved — the same block type is a perfectly legal child of that + * container; what refuses it is this particular node being an ancestor of that particular target. + */ +export type DropRefusal = DropReason | "into-itself"; + +/** + * What a drag onto a target amounts to. Four outcomes, because there are four questions and + * collapsing any two of them loses the one thing the canvas needs. + * + * A single absent value cannot carry three of them — a refused drop, a drop that changes nothing, + * and a target this planner cannot identify all reduce to "no action", and a caller holding that + * cannot tell a rejection from a no-op. A refusal then has nowhere to put its reason and the + * canvas has nothing to draw, so the author releases into dead space and the editor says nothing. + */ +export type DropOutcome = + /** Dispatch it. */ + | { kind: "action"; action: DropAction } + /** A rule says no, and says which. The author is owed this one. */ + | { kind: "refused"; reason: DropRefusal } + /** A legal drop that would leave the tree exactly as it is. Nothing to do, nothing to explain. */ + | { kind: "unchanged" } + /** No source or target this planner can resolve — not a drop attempt it has an opinion about. */ + | { kind: "unresolved" }; + export function planDrop( source: DragSource, target: DropTarget, root: BlockNode, registry: BlockRegistry -): DropAction | null { +): DropOutcome { if ( target.kind !== "dropzone" || target.parentId == null || target.slot == null ) { - return null; + return { kind: "unresolved" }; } const parent = findNode(root, target.parentId); - if (!parent) return null; + // The zone names a node the document does not hold, so there is no place to judge rather than a + // place that refuses. Telling the author a rule stopped them would name a cause that does not + // exist. + if (!parent) return { kind: "unresolved" }; const index = target.index ?? 0; if (source.kind === "library" && source.blockType) { - if (!canDrop(parent.type, target.slot, source.blockType, registry).ok) - return null; + const check = canDrop(parent.type, target.slot, source.blockType, registry); + if (!check.ok) return { kind: "refused", reason: check.reason }; return { - type: "ADD", - parentId: target.parentId, - slot: target.slot, - nodeType: source.blockType, - index, + kind: "action", + action: { + type: "ADD", + parentId: target.parentId, + slot: target.slot, + nodeType: source.blockType, + index, + }, }; } if (source.kind === "node" && source.nodeId) { const moving = findNode(root, source.nodeId); - if (!moving) return null; - // Cannot drop a node into itself or one of its descendants. - if (findNode(moving, target.parentId)) return null; - if (!canDrop(parent.type, target.slot, moving.type, registry).ok) - return null; + if (!moving) return { kind: "unresolved" }; + // Cannot drop a node into itself or one of its descendants. A refusal rather than a no-op: the + // author aimed at somewhere real and a rule stopped them, which is exactly the case that has + // to say so. + if (findNode(moving, target.parentId)) + return { kind: "refused", reason: "into-itself" }; + const check = canDrop(parent.type, target.slot, moving.type, registry); + if (!check.ok) return { kind: "refused", reason: check.reason }; let toIndex = index; const loc = locateNode(root, source.nodeId); if (loc && loc.parentId === target.parentId && loc.slot === target.slot) { - // Dropping into a gap adjacent to the source is a no-op. - if (index === loc.index || index === loc.index + 1) return null; + // Dropping into a gap adjacent to the source is a no-op. Distinct from a refusal: the drop + // is allowed and simply lands where the block already is, so drawing a refusal here would + // tell the author a legal move is forbidden. + if (index === loc.index || index === loc.index + 1) + return { kind: "unchanged" }; // Removing the source first shifts later gaps down by one. if (index > loc.index) toIndex = index - 1; } return { - type: "MOVE", - id: source.nodeId, - parentId: target.parentId, - slot: target.slot, - index: toIndex, + kind: "action", + action: { + type: "MOVE", + id: source.nodeId, + parentId: target.parentId, + slot: target.slot, + index: toIndex, + }, }; } - return null; + return { kind: "unresolved" }; } diff --git a/packages/plugin-page-builder/src/admin/logic/dropRefusal.test.ts b/packages/plugin-page-builder/src/admin/logic/dropRefusal.test.ts new file mode 100644 index 0000000000..900453ce3d --- /dev/null +++ b/packages/plugin-page-builder/src/admin/logic/dropRefusal.test.ts @@ -0,0 +1,44 @@ +import { describe, expect, it } from "vitest"; + +import { defaultBlockRegistry } from "../../core/registry"; +import { makeNode } from "../../core/tree"; +import "../../render/blocks"; // register core blocks + +import { planDrop } from "./dropPlan"; +import { DROP_REFUSALS, dropRefusalMessage } from "./dropRefusal"; + +describe("dropRefusalMessage", () => { + it("has a distinct sentence for every reason", () => { + // Asserted by MEMBERSHIP rather than by a count: two reasons sharing a sentence keeps the + // total right while telling the author the same thing about different rules, which is the + // generic message a `default` arm would have produced. + const messages = DROP_REFUSALS.map(dropRefusalMessage); + expect(new Set(messages).size).toBe(DROP_REFUSALS.length); + for (const message of messages) expect(message.length).toBeGreaterThan(0); + }); + + it("explains a refusal taken from the planner, not from its own key list", () => { + // `DROP_REFUSALS` is derived from `MESSAGES`, so looping over it and asserting each key has a + // message cannot fail — it asks the record about itself. What the compiler ALREADY guarantees + // is the other half: `Record` will not build if a reason has no sentence. + // + // So the thing left to observe is that the two ends meet at runtime: a reason produced by a + // real drag, carried through `planDrop`, resolves to the sentence the author reads. + const columns = makeNode("core/columns", {}, undefined, { default: [] }); + const root = makeNode("core/container", {}, undefined, { + default: [columns], + }); + const outcome = planDrop( + { kind: "library", blockType: "core/heading" }, + { kind: "dropzone", parentId: columns.id, slot: "default", index: 0 }, + root, + defaultBlockRegistry + ); + expect(outcome.kind).toBe("refused"); + if (outcome.kind !== "refused") return; + expect(DROP_REFUSALS).toContain(outcome.reason); + expect(dropRefusalMessage(outcome.reason)).toBe( + "This container doesn’t accept this kind of block." + ); + }); +}); diff --git a/packages/plugin-page-builder/src/admin/logic/dropRefusal.ts b/packages/plugin-page-builder/src/admin/logic/dropRefusal.ts new file mode 100644 index 0000000000..5f400206b0 --- /dev/null +++ b/packages/plugin-page-builder/src/admin/logic/dropRefusal.ts @@ -0,0 +1,53 @@ +/** + * What the canvas TELLS an author whose drop was refused (spec §9). + * + * The drop rules already know which rule stopped a drag; this turns that into the sentence shown + * beside the cursor. Separate from `dropRules` because the two answer different questions — one + * decides, one explains — and the wording changes far more often than the rules do. + * + * ## Why a Record and not a switch + * + * A `switch` with a `default` arm absorbs a reason nobody has written a sentence for: the author + * is told something generic about a rule that had a specific thing to say, and nothing fails. An + * exhaustive `Record` over the union makes the compiler demand a sentence for every member, so a + * rule added to `DropReason` cannot reach the canvas unexplained. + * + * ## Why these words + * + * Addressed to the author, about what is on the screen. "Slot", "allowlist" and "parent + * restriction" are what the registry calls these things; an author sees a container, a block, and + * a place a block will not go. Each sentence names the block or the container rather than the + * mechanism, because the author's next action is to aim somewhere else and the sentence has to + * tell them where not to. + * + * Pure → unit-tested. + */ +import type { DropRefusal } from "./dropPlan"; + +const MESSAGES: Record = { + // The container's own type is not one this editor can resolve, so it draws as an unknown-block + // placeholder with no slots. Phrased as the container being unavailable rather than as a rule, + // because there is no rule the author could satisfy. + "unknown-parent": "This container isn’t available in the editor.", + "not-a-container": "This block can’t hold other blocks.", + // The zone names a slot the container does not declare, which an author cannot act on. Said as a + // stale target rather than as a refusal they could avoid. + "unknown-slot": "This drop area is no longer part of the layout.", + "not-allowed-in-slot": "This container doesn’t accept this kind of block.", + "wrong-parent": "This block can only go inside certain containers.", + "into-itself": "A block can’t be moved inside itself.", +}; + +/** The sentence for a refusal. Total over `DropRefusal`, so there is no unexplained refusal. */ +export function dropRefusalMessage(reason: DropRefusal): string { + return MESSAGES[reason]; +} + +/** + * Every reason this module can explain, exported so a test can enumerate the union at runtime. + * + * A type cannot be iterated, so exhaustiveness over `DropRefusal` is only checkable against the + * keys the compiler already forced to be complete. Derived from `MESSAGES` rather than written out + * again — a second list is the drift this package has a rule about. + */ +export const DROP_REFUSALS = Object.keys(MESSAGES) as DropRefusal[]; diff --git a/packages/plugin-page-builder/src/admin/logic/dropRules.ts b/packages/plugin-page-builder/src/admin/logic/dropRules.ts index 0e6d0e7186..02ae7d8aa9 100644 --- a/packages/plugin-page-builder/src/admin/logic/dropRules.ts +++ b/packages/plugin-page-builder/src/admin/logic/dropRules.ts @@ -16,16 +16,34 @@ import { import type { BlockRegistry } from "../../core/registry"; import { slotAdmits } from "../../core/slot-allow"; -export interface DropCheck { - ok: boolean; - reason?: - | "unknown-parent" - | "not-a-container" - | "unknown-slot" - | "not-allowed-in-slot" - /** The CHILD restricts which parents it may sit under, and this is not one. */ - | "wrong-parent"; -} +/** + * Which rule refused a drop. + * + * Named rather than inlined because the reason travels: the canvas has to tell the author WHICH + * rule stopped the drop, and a caller that only reads `ok` throws that away at the one point where + * it is still known. + */ +export type DropReason = + | "unknown-parent" + | "not-a-container" + | "unknown-slot" + | "not-allowed-in-slot" + /** The CHILD restricts which parents it may sit under, and this is not one. */ + | "wrong-parent"; + +/** + * A refusal carries its reason by construction. + * + * As two members rather than one shape with an optional field, because an optional `reason` makes + * `{ ok: false }` on its own type-check: a refusal with nothing to say about itself becomes + * expressible, and every caller wanting the reason has to handle an absence no code path produces. + * + * `reason?: undefined` on the accepting member keeps `canDrop(...).reason` readable without first + * narrowing on `ok` — the reason is `undefined` there because an accepted drop has none. + */ +export type DropCheck = + | { ok: true; reason?: undefined } + | { ok: false; reason: DropReason }; export function canDrop( parentType: string,