From 400a193ada86fcfcd7b908955edfb48b51af946e Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sat, 15 Aug 2026 14:14:40 +0500 Subject: [PATCH 1/7] feat(plugin-page-builder): tell an author why a drop was refused --- .../src/admin/EditorSurface.tsx | 83 +++++-- .../src/admin/logic/dropPlan.test.ts | 202 +++++++++++------- .../src/admin/logic/dropPlan.ts | 93 +++++--- .../src/admin/logic/dropRefusal.test.ts | 44 ++++ .../src/admin/logic/dropRefusal.ts | 53 +++++ .../src/admin/logic/dropRules.ts | 38 +++- 6 files changed, 387 insertions(+), 126 deletions(-) create mode 100644 packages/plugin-page-builder/src/admin/logic/dropRefusal.test.ts create mode 100644 packages/plugin-page-builder/src/admin/logic/dropRefusal.ts diff --git a/packages/plugin-page-builder/src/admin/EditorSurface.tsx b/packages/plugin-page-builder/src/admin/EditorSurface.tsx index 571faf1e38..b181bc0195 100644 --- a/packages/plugin-page-builder/src/admin/EditorSurface.tsx +++ b/packages/plugin-page-builder/src/admin/EditorSurface.tsx @@ -10,6 +10,7 @@ */ import { DragDropProvider, DragOverlay } from "@dnd-kit/react"; import { type LucideIcon } from "lucide-react"; +import { useState } from "react"; import { defaultBlockRegistry } from "../core/registry"; @@ -17,7 +18,8 @@ import { Canvas } from "./canvas/Canvas"; import { 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,59 @@ 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); + }; + + 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 ( - +
@@ -102,14 +132,22 @@ export function EditorSurface() { {source => (
- ⠿ + {refusal ? "⃠" : "⠿"} {dragLabel(source?.data ?? {}, root, defaultBlockRegistry)} + {refusal ? ( + // Announced politely rather than asserted: this text changes on every target the + // pointer crosses, and an assertive live region would interrupt on each one. + + {dropRefusalMessage(refusal)} + + ) : null}
)} 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..564f66d022 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. + * + * These cases used to assert `toBeNull()`, and three of them still would: a refused drop, a + * no-op and an unresolvable target all returned the same `null`. So each test passed on an + * implementation that had confused it with the other two, and the property they were written to + * separate was the one thing they could not 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..f2d1793178 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. + * + * `null` used to stand for three of these at once, at six separate sites — a refused drop, a drop + * that changes nothing, and a target this planner could not identify. A caller could therefore + * never tell a rejection from a no-op, so a refusal had nowhere to put its reason and the canvas + * had nothing to draw: the author released into dead space and the editor said 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..01b68eb0b8 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: `{ ok: false }` on its own used to + * type-check, so a refusal with nothing to say about itself was expressible, and a caller wanting + * the reason had to handle an absence that no code path actually 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, From 56a12fdf70b40783cb3e1b6e26c8318072360bf0 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sat, 15 Aug 2026 14:28:39 +0500 Subject: [PATCH 2/7] chore: add changeset for the drop refusal feedback --- .changeset/drop-refusal-feedback.md | 28 +++++++++++++++++++ .../src/admin/logic/dropPlan.test.ts | 8 +++--- .../src/admin/logic/dropPlan.ts | 8 +++--- .../src/admin/logic/dropRules.ts | 6 ++-- 4 files changed, 39 insertions(+), 11 deletions(-) create mode 100644 .changeset/drop-refusal-feedback.md 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/packages/plugin-page-builder/src/admin/logic/dropPlan.test.ts b/packages/plugin-page-builder/src/admin/logic/dropPlan.test.ts index 564f66d022..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,10 +1,10 @@ /** * Every assertion here reads the outcome's KIND, and a refusal also reads its REASON. * - * These cases used to assert `toBeNull()`, and three of them still would: a refused drop, a - * no-op and an unresolvable target all returned the same `null`. So each test passed on an - * implementation that had confused it with the other two, and the property they were written to - * separate was the one thing they could not see. + * 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"; diff --git a/packages/plugin-page-builder/src/admin/logic/dropPlan.ts b/packages/plugin-page-builder/src/admin/logic/dropPlan.ts index f2d1793178..89ebd675ff 100644 --- a/packages/plugin-page-builder/src/admin/logic/dropPlan.ts +++ b/packages/plugin-page-builder/src/admin/logic/dropPlan.ts @@ -51,10 +51,10 @@ 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. * - * `null` used to stand for three of these at once, at six separate sites — a refused drop, a drop - * that changes nothing, and a target this planner could not identify. A caller could therefore - * never tell a rejection from a no-op, so a refusal had nowhere to put its reason and the canvas - * had nothing to draw: the author released into dead space and the editor said nothing. + * 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. */ diff --git a/packages/plugin-page-builder/src/admin/logic/dropRules.ts b/packages/plugin-page-builder/src/admin/logic/dropRules.ts index 01b68eb0b8..02ae7d8aa9 100644 --- a/packages/plugin-page-builder/src/admin/logic/dropRules.ts +++ b/packages/plugin-page-builder/src/admin/logic/dropRules.ts @@ -34,9 +34,9 @@ export type DropReason = /** * A refusal carries its reason by construction. * - * As two members rather than one shape with an optional field: `{ ok: false }` on its own used to - * type-check, so a refusal with nothing to say about itself was expressible, and a caller wanting - * the reason had to handle an absence that no code path actually produces. + * 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. From 32365f1c2361264b815cd5dde2dfd73cd5d7a58a Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sat, 15 Aug 2026 14:50:29 +0500 Subject: [PATCH 3/7] fix(plugin-page-builder): keep the refusal readable and announced --- .../src/admin/EditorSurface.tsx | 103 +++++++++++++----- 1 file changed, 73 insertions(+), 30 deletions(-) diff --git a/packages/plugin-page-builder/src/admin/EditorSurface.tsx b/packages/plugin-page-builder/src/admin/EditorSurface.tsx index b181bc0195..9585ca380b 100644 --- a/packages/plugin-page-builder/src/admin/EditorSurface.tsx +++ b/packages/plugin-page-builder/src/admin/EditorSurface.tsx @@ -135,39 +135,82 @@ export function EditorSurface() { data-refused={refusal ? "true" : undefined} style={{ display: "inline-flex", - alignItems: "center", - gap: 6, - padding: "6px 12px", - borderRadius: "var(--radius)", - // Refusal reads as a colour AND as words. Colour alone excludes anyone who cannot - // distinguish these two, and the sentence is the part that says which rule stopped - // the drop, which no colour can carry. - background: refusal - ? "var(--nx-destructive)" - : "var(--nx-primary)", - color: refusal - ? "var(--nx-destructive-foreground)" - : "var(--nx-primary-foreground)", - fontSize: 13, - fontWeight: 600, - boxShadow: "0 8px 24px rgb(0 0 0 / 0.25)", + flexDirection: "column", + alignItems: "flex-start", + gap: 4, pointerEvents: "none", - whiteSpace: "nowrap", }} > - {refusal ? "⃠" : "⠿"} - {dragLabel(source?.data ?? {}, root, defaultBlockRegistry)} - {refusal ? ( - // Announced politely rather than asserted: this text changes on every target the - // pointer crosses, and an assertive live region would interrupt on each one. - - {dropRefusalMessage(refusal)} - - ) : null} + + ⠿ + {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 ? ( + <> + + ⃠ + + {dropRefusalMessage(refusal)} + + ) : null} +
)} From 609b3014ec8cde0ea11d0205c10732566f303c66 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sat, 15 Aug 2026 15:17:00 +0500 Subject: [PATCH 4/7] fix(plugin-page-builder): report the target a drag starts on --- .../src/admin/EditorSurface.tsx | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/packages/plugin-page-builder/src/admin/EditorSurface.tsx b/packages/plugin-page-builder/src/admin/EditorSurface.tsx index 9585ca380b..b259a118ec 100644 --- a/packages/plugin-page-builder/src/admin/EditorSurface.tsx +++ b/packages/plugin-page-builder/src/admin/EditorSurface.tsx @@ -69,6 +69,21 @@ export function EditorSurface() { 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; @@ -82,7 +97,11 @@ export function EditorSurface() { }; return ( - +
@@ -152,7 +171,8 @@ export function EditorSurface() { color: "var(--nx-primary-foreground)", fontSize: 13, fontWeight: 600, - boxShadow: "0 8px 24px rgb(0 0 0 / 0.25)", + boxShadow: + "0 8px 24px color-mix(in srgb, var(--nx-shadow-color) 25%, transparent)", whiteSpace: "nowrap", }} > @@ -197,7 +217,8 @@ export function EditorSurface() { color: "var(--nx-foreground)", fontSize: 12, fontWeight: 500, - boxShadow: "0 8px 24px rgb(0 0 0 / 0.25)", + boxShadow: + "0 8px 24px color-mix(in srgb, var(--nx-shadow-color) 25%, transparent)", } : undefined } From b389d4763a322a80a4c5288d30db6dd4a88eb785 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sun, 16 Aug 2026 10:57:43 +0500 Subject: [PATCH 5/7] test(plugin-page-builder): follow a drop refusal to the author The drop rules and the sentence they produce are each unit-covered; nothing covered the path between them, and every intermediate can break with both suites green. The symptom is an editor that shows nothing, which is the state this feature replaced. Names the drag overlay so its contents can be read: everything inside it is an anonymous inline-styled div otherwise. --- .../canvas/invalid-drop-feedback.test.ts | 157 ++++++++++++++++++ .../src/admin/EditorSurface.tsx | 8 +- 2 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 e2e/tests/canvas/invalid-drop-feedback.test.ts 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..5621be23b7 --- /dev/null +++ b/e2e/tests/canvas/invalid-drop-feedback.test.ts @@ -0,0 +1,157 @@ +/** + * 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 "Column" needs no special fixture + * + * `core/column` declares `parent: ["core/columns"]`, so every drop zone in an ordinary + * `core/container` refuses it with `wrong-parent`. The refusal is the shipped registry's own, not + * one arranged here — a fixture that manufactured a restricted slot would be testing a document + * nobody authors. + */ +import { expect, test, type Page } from "@playwright/test"; + +import { dragUntilTarget } from "./driver"; +import type { CanvasFixture, Point } 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."; + +/** + * The centre of one named library entry. + * + * Searched for rather than scrolled to. The library is a long scrolling list, so an entry's box + * can be off-screen while the locator resolves happily — and the search box remounts each category + * expanded, which puts the match at the top of the panel where a drag can start from it. + * + * Exact text, because "Column" and "Columns" are both registered blocks and only one of them is + * refused by a container. + */ +async function libraryItemCentre(page: Page, label: string): Promise { + await page.getByLabel("Search blocks").fill(label); + const item = page + .locator(".nx-pb-lib-item") + .filter({ has: page.getByText(label, { exact: true }) }); + await expect( + item, + `exactly one library entry must be labelled "${label}"` + ).toHaveCount(1); + const box = await item.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 }; +} + +/** + * Drag a named library entry onto a real drop zone, and refuse 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. + */ +async function dragLibraryBlockOntoZone( + page: Page, + fixture: CanvasFixture, + label: string +): Promise { + const driver = createPocDriver(page); + await driver.mountTree(fixture); + const source = await libraryItemCentre(page, label); + const target = await driver.canvasCentre(); + await driver.startDragAt(source); + await driver.moveBy(target.x - source.x, target.y - source.y); + const active = await dragUntilTarget(driver); + expect( + active, + `the "${label}" 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 dragLibraryBlockOntoZone( + page, + await seedPage(request, FLAT_LIST_FIXTURE), + "Column" + ); + + 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. + // + // Containment rather than equality, because the region also holds an `aria-hidden` refusal mark. + // A screen reader does not announce that glyph and `textContent` does see it, so an exact match + // here would be asserting the decoration rather than the sentence. + await expect( + overlay.locator('[role="status"]'), + "the refusal must name the rule where the author reads it" + ).toContainText(WRONG_PARENT_TEXT); +}); + +test("says nothing over a target that accepts the block", async ({ + page, + request, +}) => { + await dragLibraryBlockOntoZone( + page, + await seedPage(request, FLAT_LIST_FIXTURE), + "Heading" + ); + + // 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]"), + "a container accepts a heading, 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/packages/plugin-page-builder/src/admin/EditorSurface.tsx b/packages/plugin-page-builder/src/admin/EditorSurface.tsx index b259a118ec..3de5c65a88 100644 --- a/packages/plugin-page-builder/src/admin/EditorSurface.tsx +++ b/packages/plugin-page-builder/src/admin/EditorSurface.tsx @@ -148,7 +148,13 @@ 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 => (
Date: Sun, 16 Aug 2026 11:04:27 +0500 Subject: [PATCH 6/7] test(e2e): graduate the invalid-target acceptance point The reader threw unconditionally and the case expected that throw, so the capability arriving changed nothing: its own comment predicted the line would go red first, and it could not, because the refusal it was waiting for was never reachable by the drag it performed. A block restricting its own parent reaches a refusal with no allowlisted slot, so the driver now supplies a refused source and a permitted one. Both are needed: either drag alone is satisfied by a canvas that treats every target the same way. --- e2e/tests/canvas/acceptance.spec.ts | 92 +++++++++++-------- e2e/tests/canvas/driver.ts | 19 ++++ .../canvas/invalid-drop-feedback.test.ts | 63 +++++-------- e2e/tests/canvas/poc-driver.ts | 77 +++++++++++++++- 4 files changed, 169 insertions(+), 82 deletions(-) diff --git a/e2e/tests/canvas/acceptance.spec.ts b/e2e/tests/canvas/acceptance.spec.ts index f8c8d3ce2f..ac25714647 100644 --- a/e2e/tests/canvas/acceptance.spec.ts +++ b/e2e/tests/canvas/acceptance.spec.ts @@ -50,7 +50,7 @@ import { settledTarget, settledValue, } from "./driver"; -import type { CanvasChromeReader, CanvasDriver } from "./driver"; +import type { CanvasChromeReader, CanvasDriver, Point } from "./driver"; import { createPocChromeReader, createPocDriver } from "./poc-driver"; test.describe.configure({ timeout: 240_000 }); @@ -111,6 +111,33 @@ async function dragOntoZone(driver: CanvasDriver): Promise { return active; } +/** + * Carry a NAMED panel source onto a drop zone and report which zone it reached. + * + * Returns rather than asserts, so the caller says what reaching no zone means to it. The two + * wrappers below pick the source by whether the canvas accepts it — the pair the invalid-target + * point needs, since either drag alone is satisfied by a canvas that treats both the same way. + */ +async function dragSourceOntoZone( + driver: CanvasDriver, + source: Point +): Promise { + const target = await driver.canvasCentre(); + await driver.startDragAt(source); + await driver.moveBy(target.x - source.x, target.y - source.y); + return dragUntilTarget(driver); +} + +/** A drag no ordinary container will take. */ +async function dragRestrictedOntoZone(driver: CanvasDriver): Promise { + return dragSourceOntoZone(driver, await driver.restrictedDragSourceCentre()); +} + +/** A drag it will. */ +async function dragAcceptedOntoZone(driver: CanvasDriver): Promise { + return dragSourceOntoZone(driver, await driver.acceptedDragSourceCentre()); +} + /** * What a running drag looks like from outside, whichever engine is driving it. * @@ -687,50 +714,41 @@ 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. + // An earlier version of this test dragged whatever the panel listed first and expected the + // reader to refuse, because the only refusal reachable then was `not-allowed-in-slot` and no + // block declares an `allowedBlocks` slot. A block restricting its own PARENT reaches a refusal + // without any such slot, so the illegal target this point is about is now enterable — which is + // what the driver's restricted source supplies. + 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..fca3d9d176 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; diff --git a/e2e/tests/canvas/invalid-drop-feedback.test.ts b/e2e/tests/canvas/invalid-drop-feedback.test.ts index 5621be23b7..04c0adf27b 100644 --- a/e2e/tests/canvas/invalid-drop-feedback.test.ts +++ b/e2e/tests/canvas/invalid-drop-feedback.test.ts @@ -15,17 +15,16 @@ * 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 "Column" needs no special fixture + * ## Why no special fixture * - * `core/column` declares `parent: ["core/columns"]`, so every drop zone in an ordinary - * `core/container` refuses it with `wrong-parent`. The refusal is the shipped registry's own, not - * one arranged here — a fixture that manufactured a restricted slot would be testing a document - * nobody authors. + * 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 { dragUntilTarget } from "./driver"; -import type { CanvasFixture, Point } from "./driver"; +import type { CanvasFixture } from "./driver"; import { FLAT_LIST_FIXTURE, seedPage } from "./fixtures"; import { createPocDriver } from "./poc-driver"; @@ -48,51 +47,35 @@ const DRAG_OVERLAY = ".nx-pb-drag-overlay"; const WRONG_PARENT_TEXT = "This block can only go inside certain containers."; /** - * The centre of one named library entry. - * - * Searched for rather than scrolled to. The library is a long scrolling list, so an entry's box - * can be off-screen while the locator resolves happily — and the search box remounts each category - * expanded, which puts the match at the top of the panel where a drag can start from it. - * - * Exact text, because "Column" and "Columns" are both registered blocks and only one of them is - * refused by a container. - */ -async function libraryItemCentre(page: Page, label: string): Promise { - await page.getByLabel("Search blocks").fill(label); - const item = page - .locator(".nx-pb-lib-item") - .filter({ has: page.getByText(label, { exact: true }) }); - await expect( - item, - `exactly one library entry must be labelled "${label}"` - ).toHaveCount(1); - const box = await item.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 }; -} - -/** - * Drag a named library entry onto a real drop zone, and refuse to return without one. + * 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. */ -async function dragLibraryBlockOntoZone( +async function dragOntoZone( page: Page, fixture: CanvasFixture, - label: string + pick: "restricted" | "accepted" ): Promise { const driver = createPocDriver(page); await driver.mountTree(fixture); - const source = await libraryItemCentre(page, label); + const source = + pick === "restricted" + ? await driver.restrictedDragSourceCentre() + : await driver.acceptedDragSourceCentre(); const target = await driver.canvasCentre(); await driver.startDragAt(source); await driver.moveBy(target.x - source.x, target.y - source.y); const active = await dragUntilTarget(driver); expect( active, - `the "${label}" drag must reach a drop zone before the overlay is read` + `the ${pick} drag must reach a drop zone before the overlay is read` ).toBeGreaterThanOrEqual(0); } @@ -100,10 +83,10 @@ test("tells the author which rule refused the drop", async ({ page, request, }) => { - await dragLibraryBlockOntoZone( + await dragOntoZone( page, await seedPage(request, FLAT_LIST_FIXTURE), - "Column" + "restricted" ); const overlay = page.locator(DRAG_OVERLAY); @@ -132,10 +115,10 @@ test("says nothing over a target that accepts the block", async ({ page, request, }) => { - await dragLibraryBlockOntoZone( + await dragOntoZone( page, await seedPage(request, FLAT_LIST_FIXTURE), - "Heading" + "accepted" ); // The population first. Silence is the expected result here, and silence is also what a drag @@ -148,7 +131,7 @@ test("says nothing over a target that accepts the block", async ({ ).toHaveCount(1); await expect( overlay.locator("[data-refused]"), - "a container accepts a heading, so nothing may be marked refused" + "the target accepts this block, so nothing may be marked refused" ).toHaveCount(0); await expect( overlay.locator('[role="status"]'), diff --git a/e2e/tests/canvas/poc-driver.ts b/e2e/tests/canvas/poc-driver.ts index 8cf9577520..00a2f53adb 100644 --- a/e2e/tests/canvas/poc-driver.ts +++ b/e2e/tests/canvas/poc-driver.ts @@ -78,6 +78,28 @@ 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"]'; + /** Past dnd-kit's activation distance in one move, so a drag actually starts. */ const DRAG_THRESHOLD_PX = 12; @@ -113,6 +135,32 @@ export function createPocDriver(page: Page): CanvasDriver { * `getComputedStyle` reports `"none"` there and `DOMMatrixReadOnly` parses * that to the identity, whose `a` is already 1. */ + /** + * 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 }; + } + async function frameScale(): Promise { return page.evaluate(() => { const frame = document.querySelector("iframe"); @@ -206,6 +254,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 +755,22 @@ 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. + if ((await overlay.locator(REFUSED).count()) === 0) return false; + const said = await overlay.locator(LIVE_REGION).innerText(); + return said.trim().length > 0; }, canvasScrollTop(): Promise { From 0d6c0826c49e983f1273cca791b72d337db0e40a Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sun, 16 Aug 2026 11:21:45 +0500 Subject: [PATCH 7/7] fix(plugin-page-builder): draw the refusal mark with an icon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit U+20E0 is an enclosing MARK with no form of its own, so standing alone it drew a dotted-circle placeholder or nothing depending on the font. A component renders the same everywhere and contributes no text to the live region beside it, which lets the suite assert the sentence exactly. The invalid-target reader waited on nothing: the active zone is a dnd-kit attribute write inside the canvas frame and the refusal is a React commit in the host document, so an immediate count reported a refusal that had not rendered as no refusal at all. Both new drags now go through the shared stepped transport, which measures from where the pointer actually is rather than from the source point — the overshoot that helper exists to prevent. --- e2e/tests/canvas/acceptance.spec.ts | 38 ++++++---------- e2e/tests/canvas/driver.ts | 20 +++++++++ .../canvas/invalid-drop-feedback.test.ts | 19 ++++---- e2e/tests/canvas/poc-driver.ts | 43 +++++++++++++++---- .../src/admin/EditorSurface.tsx | 22 +++++++--- .../plugin-page-builder/src/admin/icons.tsx | 2 + 6 files changed, 96 insertions(+), 48 deletions(-) diff --git a/e2e/tests/canvas/acceptance.spec.ts b/e2e/tests/canvas/acceptance.spec.ts index ac25714647..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, @@ -50,7 +51,7 @@ import { settledTarget, settledValue, } from "./driver"; -import type { CanvasChromeReader, CanvasDriver, Point } from "./driver"; +import type { CanvasChromeReader, CanvasDriver } from "./driver"; import { createPocChromeReader, createPocDriver } from "./poc-driver"; test.describe.configure({ timeout: 240_000 }); @@ -112,30 +113,21 @@ async function dragOntoZone(driver: CanvasDriver): Promise { } /** - * Carry a NAMED panel source onto a drop zone and report which zone it reached. + * The two sources the invalid-target point needs, each carried onto a zone. * - * Returns rather than asserts, so the caller says what reaching no zone means to it. The two - * wrappers below pick the source by whether the canvas accepts it — the pair the invalid-target - * point needs, since either drag alone is satisfied by a canvas that treats both the same way. + * 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 dragSourceOntoZone( - driver: CanvasDriver, - source: Point -): Promise { - const target = await driver.canvasCentre(); - await driver.startDragAt(source); - await driver.moveBy(target.x - source.x, target.y - source.y); - return dragUntilTarget(driver); -} - -/** A drag no ordinary container will take. */ async function dragRestrictedOntoZone(driver: CanvasDriver): Promise { - return dragSourceOntoZone(driver, await driver.restrictedDragSourceCentre()); + return dragSourceUntilTarget( + driver, + await driver.restrictedDragSourceCentre() + ); } -/** A drag it will. */ +/** A drag an ordinary container will take. */ async function dragAcceptedOntoZone(driver: CanvasDriver): Promise { - return dragSourceOntoZone(driver, await driver.acceptedDragSourceCentre()); + return dragSourceUntilTarget(driver, await driver.acceptedDragSourceCentre()); } /** @@ -717,11 +709,9 @@ test.describe("a canvas any Nextly editor could ship", () => { note(PLAN_POINT.invalidTargetVisible, "B-7"); await driver.mountTree(await seedPage(request, NESTED_FIXTURE)); - // An earlier version of this test dragged whatever the panel listed first and expected the - // reader to refuse, because the only refusal reachable then was `not-allowed-in-slot` and no - // block declares an `allowedBlocks` slot. A block restricting its own PARENT reaches a refusal - // without any such slot, so the illegal target this point is about is now enterable — which is - // what the driver's restricted source supplies. + // 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, diff --git a/e2e/tests/canvas/driver.ts b/e2e/tests/canvas/driver.ts index fca3d9d176..35d847f548 100644 --- a/e2e/tests/canvas/driver.ts +++ b/e2e/tests/canvas/driver.ts @@ -1283,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 index 04c0adf27b..5d79c40a93 100644 --- a/e2e/tests/canvas/invalid-drop-feedback.test.ts +++ b/e2e/tests/canvas/invalid-drop-feedback.test.ts @@ -23,7 +23,7 @@ */ import { expect, test, type Page } from "@playwright/test"; -import { dragUntilTarget } from "./driver"; +import { dragSourceUntilTarget } from "./driver"; import type { CanvasFixture } from "./driver"; import { FLAT_LIST_FIXTURE, seedPage } from "./fixtures"; import { createPocDriver } from "./poc-driver"; @@ -56,7 +56,9 @@ const WRONG_PARENT_TEXT = "This block can only go inside certain containers."; * * 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 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, @@ -69,10 +71,7 @@ async function dragOntoZone( pick === "restricted" ? await driver.restrictedDragSourceCentre() : await driver.acceptedDragSourceCentre(); - const target = await driver.canvasCentre(); - await driver.startDragAt(source); - await driver.moveBy(target.x - source.x, target.y - source.y); - const active = await dragUntilTarget(driver); + const active = await dragSourceUntilTarget(driver, source); expect( active, `the ${pick} drag must reach a drop zone before the overlay is read` @@ -102,13 +101,13 @@ test("tells the author which rule refused the drop", async ({ // what a sighted author reads, and `role="status"` is what carries it to one who is not looking // at the cursor. // - // Containment rather than equality, because the region also holds an `aria-hidden` refusal mark. - // A screen reader does not announce that glyph and `textContent` does see it, so an exact match - // here would be asserting the decoration rather than the sentence. + // 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" - ).toContainText(WRONG_PARENT_TEXT); + ).toHaveText(WRONG_PARENT_TEXT); }); test("says nothing over a target that accepts the block", async ({ diff --git a/e2e/tests/canvas/poc-driver.ts b/e2e/tests/canvas/poc-driver.ts index 00a2f53adb..2b0545ee8f 100644 --- a/e2e/tests/canvas/poc-driver.ts +++ b/e2e/tests/canvas/poc-driver.ts @@ -100,6 +100,14 @@ 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; @@ -135,6 +143,14 @@ export function createPocDriver(page: Page): CanvasDriver { * `getComputedStyle` reports `"none"` there and `DOMMatrixReadOnly` parses * that to the identity, whose `a` is already 1. */ + async function frameScale(): Promise { + return page.evaluate(() => { + const frame = document.querySelector("iframe"); + if (!(frame instanceof HTMLElement)) return 1; + return new DOMMatrixReadOnly(getComputedStyle(frame).transform).a; + }); + } + /** * The centre of the library entry carrying exactly this label. * @@ -161,14 +177,6 @@ export function createPocDriver(page: Page): CanvasDriver { return { x: box.x + box.width / 2, y: box.y + box.height / 2 }; } - async function frameScale(): Promise { - return page.evaluate(() => { - const frame = document.querySelector("iframe"); - if (!(frame instanceof HTMLElement)) return 1; - return new DOMMatrixReadOnly(getComputedStyle(frame).transform).a; - }); - } - /** * Every drop zone's vertical extent in HOST coordinates, document order. * @@ -768,7 +776,24 @@ export function createPocChromeReader(page: Page): CanvasChromeReader { } // 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. - if ((await overlay.locator(REFUSED).count()) === 0) return false; + // + // 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; }, diff --git a/packages/plugin-page-builder/src/admin/EditorSurface.tsx b/packages/plugin-page-builder/src/admin/EditorSurface.tsx index 3de5c65a88..a6ae3935ec 100644 --- a/packages/plugin-page-builder/src/admin/EditorSurface.tsx +++ b/packages/plugin-page-builder/src/admin/EditorSurface.tsx @@ -15,7 +15,7 @@ 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, type DropOutcome, type DropRefusal } from "./logic/dropPlan"; @@ -213,7 +213,9 @@ export function EditorSurface() { // and 6.90:1 dark, clearing the 3:1 a non-text boundary needs in both modes, // and the sentence says which rule applied where no colour could. display: "inline-flex", - alignItems: "baseline", + // Centred rather than on the baseline: an SVG's baseline is its bottom edge, + // so a baseline-aligned icon rides above the text it sits beside. + alignItems: "center", gap: 6, maxWidth: 260, padding: "5px 10px", @@ -231,9 +233,19 @@ export function EditorSurface() { > {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,