From 68d684f0801f077085a27b5a5fb83b71a87f1ede Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sat, 15 Aug 2026 03:35:57 +0500 Subject: [PATCH 1/8] fix(plugin-page-builder): hold the drop target across a switch margin A pointer resting near a drop-zone boundary changed target on every small movement, because zones were ranked by the default collision detection and nothing made the zone already held harder to displace than a fresh one. Rank the interleaved zones with a detector of their own. Eligibility is delegated to the default detection unchanged, so no zone starts claiming a pointer it did not claim before; only the ordering among eligible zones is replaced, by the pointer's vertical distance to each zone's centre in pixels, with the current target credited a fixed margin. Every zone reports ONE collision type, and that is what makes the margin reachable at all. `sortCollisions` compares priority, then type, then value, so a detector that reports pointer containment inside a zone and shape overlap outside it changes tier exactly where the pointer crosses a zone edge, and a margin expressed in the value is never consulted there. Priority is passed through untouched, so a zone's container depth still decides between the identical rectangles of a nested container's gap and its parent's. Distance is vertical only: interleaved zones span their container's full width, so the horizontal term is common to every zone competing on value, and folding it in under a square root would stop it cancelling. The two `test.fail` markers on the hysteresis assertions come off in this same commit. `test.fail` inverts a result, so leaving them would turn both canvas specs red on a working implementation and removing them ahead of the change would turn them red for real; neither ordering leaves main green. --- .changeset/drag-target-switch-margin.md | 28 ++ e2e/tests/canvas/acceptance.spec.ts | 13 +- e2e/tests/canvas/scenarios.spec.ts | 29 +- packages/plugin-page-builder/package.json | 2 + .../src/admin/canvas/DropZone.tsx | 15 + .../src/admin/canvas/zoneCollision.test.ts | 263 ++++++++++++++++++ .../src/admin/canvas/zoneCollision.ts | 120 ++++++++ pnpm-lock.yaml | 6 + 8 files changed, 450 insertions(+), 26 deletions(-) create mode 100644 .changeset/drag-target-switch-margin.md create mode 100644 packages/plugin-page-builder/src/admin/canvas/zoneCollision.test.ts create mode 100644 packages/plugin-page-builder/src/admin/canvas/zoneCollision.ts diff --git a/.changeset/drag-target-switch-margin.md b/.changeset/drag-target-switch-margin.md new file mode 100644 index 0000000000..2613b96001 --- /dev/null +++ b/.changeset/drag-target-switch-margin.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 +--- + +Hold the drop target across a switch margin while dragging on the page-builder canvas, so a pointer resting near a zone boundary no longer flips between insertion points on every small movement. diff --git a/e2e/tests/canvas/acceptance.spec.ts b/e2e/tests/canvas/acceptance.spec.ts index f8c8d3ce2f..0c4a4240c5 100644 --- a/e2e/tests/canvas/acceptance.spec.ts +++ b/e2e/tests/canvas/acceptance.spec.ts @@ -466,12 +466,7 @@ test.describe("a canvas any Nextly editor could ship", () => { test("holds its target through a jitter at a zone boundary", async ({ request, }) => { - note( - PLAN_POINT.targetSwitchHysteresis, - "B-7", - "this canvas has no switch margin: bracketed at an edge, the target " + - "flips on every 2px crossing" - ); + note(PLAN_POINT.targetSwitchHysteresis, "B-7"); await driver.mountTree(await seedPage(request, FLAT_LIST_FIXTURE)); // Onto a zone, then to that zone's EDGE. Both halves are preconditions with // teeth. Jittering from dead space counts the indicator appearing and @@ -546,12 +541,6 @@ test.describe("a canvas any Nextly editor could ship", () => { "the indicator must be visible to measure whether it moves" ).toBeGreaterThanOrEqual(0); - // Marked only now. Everything above ran unprotected, so a failed seed, a - // drag that never reached a zone, a target that never moved, or a probe the - // runner was too slow to take stays a real outcome of its own rather than - // becoming another expected failure. - test.fail(true, "the target flips on every 2px crossing of a zone edge"); - // The log's FIRST entry is the state when recording began, not a change, so // anything after it is motion the jitter caused. Comparing the whole log // instead asserts an array that can never be empty, which would keep this diff --git a/e2e/tests/canvas/scenarios.spec.ts b/e2e/tests/canvas/scenarios.spec.ts index b4369073d1..652ecba4af 100644 --- a/e2e/tests/canvas/scenarios.spec.ts +++ b/e2e/tests/canvas/scenarios.spec.ts @@ -325,17 +325,23 @@ test("scenario 4: a steady drag over variable-height blocks never reverses", asy }); /** - * Expected failure: this canvas has no target-switch hysteresis. + * The target is sticky across a margin, so a 2px jitter cannot move it. * - * The requirement is a sticky target with an 8-12px dead-zone margin or a - * >100ms dwell, so that oscillating the pointer 2px around any boundary never - * flips the indicator. Observed here, with the edge bracketed to 1px and the - * samples taken on opposite sides of it: `[1,2,1,2,...]`, a flip on every move. + * The requirement is an 8-12px dead-zone margin or a >100ms dwell, so that + * oscillating the pointer 2px around any boundary never flips the indicator. + * The canvas satisfies the distance half: a zone holds the target until a + * challenger is nearer by the full margin, which no 2px move can achieve. * - * Both halves of the method are load-bearing. Jittering anywhere other than a - * bracketed edge measures the middle of one zone's catchment, and sampling P - * and P+2 rather than P-2 and P+2 keeps both samples on the same side; either - * reports a stable indicator on a canvas that has none. + * Both halves of the method are load-bearing, and they are what make a green + * run mean anything. Jittering anywhere other than a bracketed edge measures + * the middle of one zone's catchment, and sampling P and P+2 rather than P-2 + * and P+2 keeps both samples on the same side; either one reports a stable + * indicator whether or not the margin exists, so the test would keep passing if + * the margin were removed. + * + * The fixture alternates 400px and 24px siblings deliberately: the 24px pitch + * is the narrowest spacing the margin has to stay inside, since a margin wider + * than the spacing would carry the target past the next zone's centre. */ test("scenario 4b: a 2px jitter at a zone edge keeps the indicator stable", async ({ page, @@ -434,11 +440,6 @@ test("scenario 4b: a 2px jitter at a zone edge keeps the indicator stable", asyn `the drag must find a target at all: ${JSON.stringify(observed)}` ).toBe(true); - test.fail( - true, - "no target-switch hysteresis: the indicator flips on every 2px move" - ); - // The log's first entry is the state when recording began, so anything after // it is a change the jitter caused. -1 is NOT excluded: an indicator that // vanishes and returns is the same defect seen from the other side, and it diff --git a/packages/plugin-page-builder/package.json b/packages/plugin-page-builder/package.json index 1f83a638bd..766690d67e 100644 --- a/packages/plugin-page-builder/package.json +++ b/packages/plugin-page-builder/package.json @@ -53,6 +53,8 @@ "test:integration": "vitest run --config vitest.integration.config.ts" }, "dependencies": { + "@dnd-kit/abstract": "0.5.0", + "@dnd-kit/collision": "0.5.0", "@dnd-kit/dom": "0.5.0", "@dnd-kit/react": "0.5.0", "@nextlyhq/blocks-engine": "workspace:*", diff --git a/packages/plugin-page-builder/src/admin/canvas/DropZone.tsx b/packages/plugin-page-builder/src/admin/canvas/DropZone.tsx index bf56b8bd04..01363d9443 100644 --- a/packages/plugin-page-builder/src/admin/canvas/DropZone.tsx +++ b/packages/plugin-page-builder/src/admin/canvas/DropZone.tsx @@ -14,8 +14,18 @@ import { useDragDropMonitor, useDroppable } from "@dnd-kit/react"; import { createContext, useContext, useState, type ReactNode } from "react"; +import { createZoneCollisionDetector } from "./zoneCollision"; + const BLOCK_TYPE = "nx-block"; +/** + * Shared by every interleaved zone, and built once at module scope because it + * closes over nothing per-zone: the detector reads the incumbent target from + * the drag operation it is handed, so there is no state to keep and no reason + * to give each zone its own instance. + */ +const zoneCollisionDetector = createZoneCollisionDetector(); + /** * How deeply nested the container owning these zones is. * @@ -86,6 +96,11 @@ export function DropZone({ // is the only thing that can settle two IDENTICAL rectangles — which is // exactly what a nested container's edge gap and its parent's gap are. collisionPriority: depth, + // Only the interleaved zones rank this way. They are the ones that compete + // with a sibling zone for the same pointer, so they are the ones a switch + // margin means anything for; a container's single "drop here" zone has no + // sibling to flip to, and keeps the default ranking. + collisionDetector: empty ? undefined : zoneCollisionDetector, }); if (empty) { diff --git a/packages/plugin-page-builder/src/admin/canvas/zoneCollision.test.ts b/packages/plugin-page-builder/src/admin/canvas/zoneCollision.test.ts new file mode 100644 index 0000000000..821850b4f4 --- /dev/null +++ b/packages/plugin-page-builder/src/admin/canvas/zoneCollision.test.ts @@ -0,0 +1,263 @@ +/** + * The property under test is the WIDTH of the switch margin, in pixels of + * pointer travel, read out of the same sort the canvas ranks with. + * + * Two things this deliberately does not do, because each of them is a way to + * pass while measuring something else: + * + * - **It never compares two `value` numbers directly.** `sortCollisions` reads + * `priority` and `type` first, so a margin can be correct in `value` and + * never reach the comparison. Every ranking assertion below goes through the + * real `sortCollisions`, which is the only thing that can show the margin is + * actually consulted. + * - **It sweeps the pointer and observes where the winner changes**, rather + * than asserting an arithmetic identity. The margin is a claim about pointer + * movement, so it is measured in pointer movement. + */ +import { + CollisionType, + sortCollisions, + type Collision, +} from "@dnd-kit/abstract"; +import { describe, expect, it } from "vitest"; + +import { + TARGET_SWITCH_BAND_PX, + ZONE_COLLISION_TYPE, + zoneCollisionValue, + zoneDistancePx, +} from "./zoneCollision"; + +/** Two adjacent zones from the flat-list geometry: 60px apart, centred on the + * insertion line each one marks. */ +const ZONE_A_CENTRE_Y = 120; +const ZONE_B_CENTRE_Y = 180; +const ZONE_PITCH_PX = ZONE_B_CENTRE_Y - ZONE_A_CENTRE_Y; + +/** + * One ranking round, assembled the way the collision observer assembles it: + * a collision per zone, then `sortCollisions`, then the winner is the head. + * + * `priority` is equal for both because sibling zones share a container and + * therefore a depth. That equality is what puts the decision on the tiers this + * module controls, and it is the real arrangement rather than a convenience. + */ +function winnerAt({ + pointerY, + currentTargetId, + bandPx = TARGET_SWITCH_BAND_PX, + typeFor = () => ZONE_COLLISION_TYPE, +}: { + pointerY: number; + currentTargetId: string | null; + bandPx?: number; + typeFor?: (id: string) => CollisionType; +}): string { + const collisions: Collision[] = [ + { id: "a", centre: ZONE_A_CENTRE_Y }, + { id: "b", centre: ZONE_B_CENTRE_Y }, + ].map(zone => ({ + id: zone.id, + priority: 1, + type: typeFor(zone.id), + value: zoneCollisionValue({ + distancePx: zoneDistancePx(pointerY, zone.centre), + isCurrentTarget: currentTargetId === zone.id, + bandPx, + }), + })); + + collisions.sort(sortCollisions); + const winner = collisions[0]?.id; + if (typeof winner !== "string") { + throw new Error("a ranking round must produce a winner"); + } + return winner; +} + +/** + * Walk the pointer one pixel at a time, carrying the winner forward as the + * incumbent, and report where the target first changes. + * + * Carrying it forward is the whole point: the margin only exists relative to a + * target that is already held, so a sweep that recomputes from no incumbent + * measures the midpoint and reports no margin at all. + */ +function firstSwitchY({ + from, + to, + startTargetId, + bandPx = TARGET_SWITCH_BAND_PX, + typeFor, +}: { + from: number; + to: number; + startTargetId: string; + bandPx?: number; + typeFor?: (id: string) => CollisionType; +}): number | null { + const step = Math.sign(to - from); + let target = startTargetId; + for (let y = from; y !== to + step; y += step) { + const winner = winnerAt({ + pointerY: y, + currentTargetId: target, + bandPx, + typeFor, + }); + if (winner !== target) return y; + target = winner; + } + return null; +} + +describe("zoneDistancePx", () => { + it("is the vertical gap to the zone centre, unsigned", () => { + expect(zoneDistancePx(100, 120)).toBe(20); + expect(zoneDistancePx(140, 120)).toBe(20); + expect(zoneDistancePx(120, 120)).toBe(0); + }); +}); + +describe("the switch margin, measured in pointer travel", () => { + it("is exactly TARGET_SWITCH_BAND_PX wide across the real sort", () => { + // Down from A's centre to B's centre, then back. The two crossings sit + // symmetrically about the midpoint, so their separation IS the margin. + const down = firstSwitchY({ + from: ZONE_A_CENTRE_Y, + to: ZONE_B_CENTRE_Y, + startTargetId: "a", + }); + const up = firstSwitchY({ + from: ZONE_B_CENTRE_Y, + to: ZONE_A_CENTRE_Y, + startTargetId: "b", + }); + + expect(down, "the target must eventually move to b").not.toBeNull(); + expect(up, "the target must eventually move back to a").not.toBeNull(); + // Integer stepping resolves each crossing to within a pixel, so the width + // derived from two of them carries both errors. + expect(Number(down) - Number(up)).toBeCloseTo(TARGET_SWITCH_BAND_PX, -0.5); + }); + + it("lands inside the 8-12px the canvas requires", () => { + const down = Number( + firstSwitchY({ + from: ZONE_A_CENTRE_Y, + to: ZONE_B_CENTRE_Y, + startTargetId: "a", + }) + ); + const up = Number( + firstSwitchY({ + from: ZONE_B_CENTRE_Y, + to: ZONE_A_CENTRE_Y, + startTargetId: "b", + }) + ); + const width = down - up; + + expect(width).toBeGreaterThanOrEqual(8); + expect(width).toBeLessThanOrEqual(12); + }); + + it("collapses to zero when the margin is zero", () => { + // The control that separates "this measures the margin" from "this measures + // some other asymmetry in the sweep". With no margin both crossings must + // fall on the same pixel. + const down = firstSwitchY({ + from: ZONE_A_CENTRE_Y, + to: ZONE_B_CENTRE_Y, + startTargetId: "a", + bandPx: 0, + }); + const up = firstSwitchY({ + from: ZONE_B_CENTRE_Y, + to: ZONE_A_CENTRE_Y, + startTargetId: "b", + bandPx: 0, + }); + + expect(Number(down) - Number(up)).toBeLessThanOrEqual(1); + }); + + it("holds the target through a jitter narrower than the margin", () => { + // The canvas-level symptom, stated at the ranking layer: a pointer bracketed + // at the midpoint and moved by less than the margin must not change target. + const midpoint = ZONE_A_CENTRE_Y + ZONE_PITCH_PX / 2; + let target = "a"; + const seen = new Set(); + for (const y of [midpoint, midpoint + 2, midpoint - 2, midpoint + 2]) { + target = winnerAt({ pointerY: y, currentTargetId: target }); + seen.add(target); + } + + expect([...seen], "a 2px jitter must not move the target").toEqual(["a"]); + }); +}); + +describe("what the uniform collision tier is load-bearing for", () => { + it("loses the margin entirely when the tier is allowed to vary", () => { + // The mutation this design exists to survive. Reporting containment inside a + // zone and a lower tier outside it is what the default detection does, and + // `sortCollisions` reads the tier BEFORE the value, so the incumbent is + // outranked the moment the pointer enters the challenger's 6px rect and the + // margin is never consulted. If this ever stops failing, the uniform tier + // has stopped doing anything and the margin is decorative. + const varyingTier = (id: string): CollisionType => + // b claims containment as soon as the pointer is within its rect. + id === "b" + ? CollisionType.PointerIntersection + : CollisionType.ShapeIntersection; + + const down = firstSwitchY({ + from: ZONE_A_CENTRE_Y, + to: ZONE_B_CENTRE_Y, + startTargetId: "a", + typeFor: varyingTier, + }); + + // b outranks a on the tier at every position, so the target moves on the + // very first step rather than after half a pitch plus half a margin. + expect(down).toBe(ZONE_A_CENTRE_Y); + }); + + it("is the tier a zone already reports with the pointer inside it", () => { + expect(ZONE_COLLISION_TYPE).toBe(CollisionType.PointerIntersection); + }); +}); + +describe("zoneCollisionValue", () => { + it("ranks the nearer zone higher", () => { + const near = zoneCollisionValue({ + distancePx: 5, + isCurrentTarget: false, + bandPx: 10, + }); + const far = zoneCollisionValue({ + distancePx: 25, + isCurrentTarget: false, + bandPx: 10, + }); + + expect(near).toBeGreaterThan(far); + }); + + it("credits the incumbent exactly the margin, in pixels", () => { + const challenger = zoneCollisionValue({ + distancePx: 30, + isCurrentTarget: false, + bandPx: 10, + }); + const incumbent = zoneCollisionValue({ + distancePx: 30, + isCurrentTarget: true, + bandPx: 10, + }); + + // Linear and negated, so the credit reads back as a plain pixel difference + // rather than something that has to be inverted to be interpreted. + expect(incumbent - challenger).toBe(10); + }); +}); diff --git a/packages/plugin-page-builder/src/admin/canvas/zoneCollision.ts b/packages/plugin-page-builder/src/admin/canvas/zoneCollision.ts new file mode 100644 index 0000000000..9cc3147119 --- /dev/null +++ b/packages/plugin-page-builder/src/admin/canvas/zoneCollision.ts @@ -0,0 +1,120 @@ +/** + * Ranking for the drop zones interleaved between a slot's children. + * + * Zones are 6px tall and centred exactly on the insertion line they mark, so + * "which zone should own the pointer" is a question about one number: the + * vertical gap between the pointer and each zone's centre. This module answers + * it in pixels and adds a switch margin, so a pointer resting near a boundary + * keeps the target it already has instead of flipping on every small movement. + * + * Two properties of `sortCollisions` in `@dnd-kit/abstract` shape everything + * here, because it compares `priority`, THEN `type`, and only THEN `value`: + * + * - **The margin has to live in `value`, so `type` must not vary.** A detector + * that reports pointer containment inside a zone and shape overlap outside it + * changes tier as the pointer crosses the zone's own edge, and a margin + * expressed in `value` is never consulted across that change — the incumbent + * is outranked before its margin is read. Reporting ONE type for every zone + * is what keeps the comparison on the tier the margin can reach. + * - **`priority` is left alone.** A zone carries its container's depth as + * `collisionPriority`, which the collision observer writes over whatever a + * detector returns, so depth stays the primary key and a nested container + * keeps winning against the identical rectangle of its parent's gap. + */ +import { CollisionType, type CollisionDetector } from "@dnd-kit/abstract"; +import { defaultCollisionDetection } from "@dnd-kit/collision"; + +/** + * How much closer a challenging zone must be before the target moves to it. + * + * This is the full width of the switch margin in pixels of pointer movement, + * not a half-width: releasing the incumbent needs `distance - BAND` to beat the + * challenger, which moves the switch point half a band past the midpoint going + * one way and half a band short of it coming back. + */ +export const TARGET_SWITCH_BAND_PX = 10; + +/** + * The single collision tier every interleaved zone reports. + * + * Pointer containment rather than a lower tier, because that is what a zone + * already reports at the position where a target is most often held: with the + * pointer inside its 6px rect. Pinning the tier there keeps that case ranking + * as it does without this module, and lifts only the case where a zone was + * previously demoted for not containing the pointer. + */ +export const ZONE_COLLISION_TYPE = CollisionType.PointerIntersection; + +/** + * The pointer's distance from a zone, measured along the axis the zone divides. + * + * Vertical only, and that is narrower than a straight line on purpose. Zones + * interleaved in block flow span their container's full width, so every zone + * competing on `value` is the same horizontal distance from the pointer; a + * two-axis distance would fold that shared term in under a square root, where + * it stops cancelling and starts distorting the vertical comparison it is + * irrelevant to. + */ +export function zoneDistancePx(pointerY: number, zoneCentreY: number): number { + return Math.abs(pointerY - zoneCentreY); +} + +/** + * A zone's rank among its siblings: higher wins, and the unit is pixels. + * + * Negated because `sortCollisions` orders descending while the better candidate + * is the NEARER one. Kept linear rather than reciprocal so that the margin + * subtracted below is a distance the pointer can actually travel — the width of + * the switch margin equals `bandPx` exactly, at any distance and any zone + * spacing, which is what makes it assertable in pixels of pointer movement. + */ +export function zoneCollisionValue({ + distancePx, + isCurrentTarget, + bandPx, +}: { + distancePx: number; + isCurrentTarget: boolean; + bandPx: number; +}): number { + return -(isCurrentTarget ? distancePx - bandPx : distancePx); +} + +/** + * Rank zones by pointer distance, holding the current target across a margin. + * + * Eligibility is delegated rather than reimplemented: the default detection + * decides WHETHER a zone is in play at all, exactly as it does without this + * module, and only the ranking among the zones it admits is replaced. A zone + * the default detection rejects is still rejected here, so no zone starts + * claiming a pointer it would not have claimed before. + */ +export function createZoneCollisionDetector( + bandPx: number = TARGET_SWITCH_BAND_PX +): CollisionDetector { + return input => { + const eligible = defaultCollisionDetection(input); + if (!eligible) return null; + + const { droppable, dragOperation } = input; + const centre = droppable.shape?.center; + const pointer = dragOperation.position.current; + // Without a measured rectangle or a pointer there is no distance to rank + // by, so the delegated result stands rather than being replaced by a + // fabricated one. + if (!centre || !pointer) return eligible; + + return { + id: droppable.id, + priority: eligible.priority, + // One tier for every zone. See the module comment: a varying tier + // outranks the margin below before it is ever compared. + type: ZONE_COLLISION_TYPE, + value: zoneCollisionValue({ + distancePx: zoneDistancePx(pointer.y, centre.y), + isCurrentTarget: dragOperation.target?.id === droppable.id, + bandPx, + }), + }; + }; +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index da1977cde1..371866f574 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1310,6 +1310,12 @@ importers: packages/plugin-page-builder: dependencies: + '@dnd-kit/abstract': + specifier: 0.5.0 + version: 0.5.0 + '@dnd-kit/collision': + specifier: 0.5.0 + version: 0.5.0 '@dnd-kit/dom': specifier: 0.5.0 version: 0.5.0 From 36e6556bbfa3b436a9355bcd1a6da0018ee6384a Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sat, 15 Aug 2026 08:54:37 +0500 Subject: [PATCH 2/8] fix(plugin-page-builder): rank every insertion target on one policy The switch margin reached only the drop zones interleaved between a slot's children, leaving three other insertion targets on the default ranking: the "drop here" placeholder of an empty slot, and the `before:` / `append:` targets a formatted slot uses instead of zones. Two ranking paths for one question, and the targets left out kept flipping on a small movement. Move both halves of the decision into one module. Priority and ranking were already one question split across five places, so `collisionPolicy.ts` now owns the depth scale as well, and every `useDroppable` asks it rather than spelling a number. Three corrections to the ranking itself, each of which the zone-only version got away with only because zones span their container: Distance gains a horizontal term, `hypot(max(0, |dx| - halfWidth), dy)`. It is zero anywhere inside a target's width, so ordinary block flow still ranks purely on the axis the insertion point divides. It stops being zero for targets in different columns of a formatted slot, which share a depth and a vertical band and would otherwise tie and be settled by registration order. Eligibility keeps the held target while the pointer stays within its width. The default detection stops reporting a target once the dragged feedback no longer overlaps it, and where targets are spaced farther apart than that feedback is tall, that happens before any neighbour becomes eligible. The margin lives in the ranking, so it can only act on targets still in the ranking: without this the held target is dropped at that edge and the indicator alternates between a target and nothing. Empty zones rank the same way. One per container does not mean no competitor: two adjacent empty containers put their placeholders at the same depth. --- .../src/admin/canvas/CanvasNode.tsx | 23 +- .../src/admin/canvas/DropZone.tsx | 22 +- ...lision.test.ts => collisionPolicy.test.ts} | 189 +++++++++++++-- .../src/admin/canvas/collisionPolicy.ts | 227 ++++++++++++++++++ .../src/admin/canvas/zoneCollision.ts | 120 --------- 5 files changed, 423 insertions(+), 158 deletions(-) rename packages/plugin-page-builder/src/admin/canvas/{zoneCollision.test.ts => collisionPolicy.test.ts} (55%) create mode 100644 packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts delete mode 100644 packages/plugin-page-builder/src/admin/canvas/zoneCollision.ts diff --git a/packages/plugin-page-builder/src/admin/canvas/CanvasNode.tsx b/packages/plugin-page-builder/src/admin/canvas/CanvasNode.tsx index a0f88caa2c..45b6430c78 100644 --- a/packages/plugin-page-builder/src/admin/canvas/CanvasNode.tsx +++ b/packages/plugin-page-builder/src/admin/canvas/CanvasNode.tsx @@ -36,6 +36,10 @@ import { dragSensors } from "../logic/dragSensors"; import { useEditor } from "../store/EditorProvider"; import { QueryLoopSamplePreview } from "./CanvasQueryLoop"; +import { + insertionCollisionDetector, + nodeTargetPriority, +} from "./collisionPolicy"; import { CanvasDepth, DropZone, useCanvasDepth } from "./DropZone"; const BLOCK_TYPE = "nx-block"; @@ -240,8 +244,12 @@ export function CanvasNode({ node }: { node: BlockNode }): ReactNode { index: appendSlot ? (node.slots?.[appendSlot]?.length ?? 0) : 0, }, // Targets this node's OWN slot, so it ranks with the zones INSIDE it rather - // than with its siblings — the same `depth + 1` the slot content is rendered at. - collisionPriority: depth + 1, + // than with its siblings — the same depth the slot content is rendered at. + collisionPriority: nodeTargetPriority(depth, "append"), + // The same ranking every insertion target uses. A formatted slot draws no + // zones, so this target and its neighbouring `before:` targets ARE that + // slot's insertion points and need the switch margin for the same reason. + collisionDetector: insertionCollisionDetector, }); const rootRef = appendSlot ? append.ref : undefined; const className = classFor( @@ -325,7 +333,8 @@ function DraggableNode({ disabled: dropBeforeIndex == null, data: { kind: "dropzone", parentId, slot, index: dropBeforeIndex ?? 0 }, // Targets the slot this node SITS IN, so it ranks with that slot's own zones. - collisionPriority: depth, + collisionPriority: nodeTargetPriority(depth, "before"), + collisionDetector: insertionCollisionDetector, }); // A formatted container itself: "append" target for the formatted slot it declares, since that @@ -345,8 +354,12 @@ function DraggableNode({ index: appendIndex, }, // Targets this node's OWN slot, so it ranks with the zones INSIDE it rather - // than with its siblings — the same `depth + 1` the slot content is rendered at. - collisionPriority: depth + 1, + // than with its siblings — the same depth the slot content is rendered at. + collisionPriority: nodeTargetPriority(depth, "append"), + // The same ranking every insertion target uses. A formatted slot draws no + // zones, so this target and its neighbouring `before:` targets ARE that + // slot's insertion points and need the switch margin for the same reason. + collisionDetector: insertionCollisionDetector, }); const className = classFor( diff --git a/packages/plugin-page-builder/src/admin/canvas/DropZone.tsx b/packages/plugin-page-builder/src/admin/canvas/DropZone.tsx index b8b4f0528c..488e060b4f 100644 --- a/packages/plugin-page-builder/src/admin/canvas/DropZone.tsx +++ b/packages/plugin-page-builder/src/admin/canvas/DropZone.tsx @@ -14,18 +14,10 @@ import { useDragDropMonitor, useDroppable } from "@dnd-kit/react"; import { createContext, useContext, useState, type ReactNode } from "react"; -import { createZoneCollisionDetector } from "./zoneCollision"; +import { insertionCollisionDetector, zonePriority } from "./collisionPolicy"; const BLOCK_TYPE = "nx-block"; -/** - * Shared by every interleaved zone, and built once at module scope because it - * closes over nothing per-zone: the detector reads the incumbent target from - * the drag operation it is handed, so there is no state to keep and no reason - * to give each zone its own instance. - */ -const zoneCollisionDetector = createZoneCollisionDetector(); - /** * How deeply nested the container owning these zones is. * @@ -103,12 +95,12 @@ export function DropZone({ // is what "the innermost container owns the drop target" asks for, and it // is the only thing that can settle two IDENTICAL rectangles — which is // exactly what a nested container's edge gap and its parent's gap are. - collisionPriority: depth, - // Only the interleaved zones rank this way. They are the ones that compete - // with a sibling zone for the same pointer, so they are the ones a switch - // margin means anything for; a container's single "drop here" zone has no - // sibling to flip to, and keeps the default ranking. - collisionDetector: empty ? undefined : zoneCollisionDetector, + collisionPriority: zonePriority(depth), + // Empty zones rank the same way, because "one per container" does not mean + // "no competitor": two adjacent containers that are both empty put their + // placeholders at the same depth, and those compete for the same pointer + // exactly as two zones in one slot do. + collisionDetector: insertionCollisionDetector, }); if (empty) { diff --git a/packages/plugin-page-builder/src/admin/canvas/zoneCollision.test.ts b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts similarity index 55% rename from packages/plugin-page-builder/src/admin/canvas/zoneCollision.test.ts rename to packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts index 821850b4f4..2549645eb0 100644 --- a/packages/plugin-page-builder/src/admin/canvas/zoneCollision.test.ts +++ b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts @@ -22,11 +22,32 @@ import { import { describe, expect, it } from "vitest"; import { + INSERTION_COLLISION_TYPE, TARGET_SWITCH_BAND_PX, - ZONE_COLLISION_TYPE, - zoneCollisionValue, - zoneDistancePx, -} from "./zoneCollision"; + insertionCollisionValue, + insertionDistancePx, + isInsertionTargetEligible, + nodeTargetPriority, + zonePriority, +} from "./collisionPolicy"; + +/** + * Zones interleaved in block flow span their container, so the pointer is always + * horizontally inside them and only `y` varies. Fixing that half here keeps the + * margin measurements below about the axis the margin acts on; the horizontal + * term gets its own tests further down. + */ +const ZONE_WIDTH_PX = 800; +const POINTER_X = 400; +function distanceInColumn(pointerY: number, centreY: number): number { + return insertionDistancePx({ + pointerX: POINTER_X, + pointerY, + centreX: POINTER_X, + centreY, + width: ZONE_WIDTH_PX, + }); +} /** Two adjacent zones from the flat-list geometry: 60px apart, centred on the * insertion line each one marks. */ @@ -46,7 +67,7 @@ function winnerAt({ pointerY, currentTargetId, bandPx = TARGET_SWITCH_BAND_PX, - typeFor = () => ZONE_COLLISION_TYPE, + typeFor = () => INSERTION_COLLISION_TYPE, }: { pointerY: number; currentTargetId: string | null; @@ -60,8 +81,8 @@ function winnerAt({ id: zone.id, priority: 1, type: typeFor(zone.id), - value: zoneCollisionValue({ - distancePx: zoneDistancePx(pointerY, zone.centre), + value: insertionCollisionValue({ + distancePx: distanceInColumn(pointerY, zone.centre), isCurrentTarget: currentTargetId === zone.id, bandPx, }), @@ -111,11 +132,11 @@ function firstSwitchY({ return null; } -describe("zoneDistancePx", () => { - it("is the vertical gap to the zone centre, unsigned", () => { - expect(zoneDistancePx(100, 120)).toBe(20); - expect(zoneDistancePx(140, 120)).toBe(20); - expect(zoneDistancePx(120, 120)).toBe(0); +describe("insertionDistancePx", () => { + it("is the vertical gap to the target centre when horizontally inside", () => { + expect(distanceInColumn(100, 120)).toBe(20); + expect(distanceInColumn(140, 120)).toBe(20); + expect(distanceInColumn(120, 120)).toBe(0); }); }); @@ -224,18 +245,18 @@ describe("what the uniform collision tier is load-bearing for", () => { }); it("is the tier a zone already reports with the pointer inside it", () => { - expect(ZONE_COLLISION_TYPE).toBe(CollisionType.PointerIntersection); + expect(INSERTION_COLLISION_TYPE).toBe(CollisionType.PointerIntersection); }); }); -describe("zoneCollisionValue", () => { +describe("insertionCollisionValue", () => { it("ranks the nearer zone higher", () => { - const near = zoneCollisionValue({ + const near = insertionCollisionValue({ distancePx: 5, isCurrentTarget: false, bandPx: 10, }); - const far = zoneCollisionValue({ + const far = insertionCollisionValue({ distancePx: 25, isCurrentTarget: false, bandPx: 10, @@ -245,12 +266,12 @@ describe("zoneCollisionValue", () => { }); it("credits the incumbent exactly the margin, in pixels", () => { - const challenger = zoneCollisionValue({ + const challenger = insertionCollisionValue({ distancePx: 30, isCurrentTarget: false, bandPx: 10, }); - const incumbent = zoneCollisionValue({ + const incumbent = insertionCollisionValue({ distancePx: 30, isCurrentTarget: true, bandPx: 10, @@ -261,3 +282,135 @@ describe("zoneCollisionValue", () => { expect(incumbent - challenger).toBe(10); }); }); + +describe("the horizontal term, which separates columns", () => { + // Targets in different columns of a formatted slot share a depth and a + // vertical band. Ranking them by `y` alone makes equal-height targets TIE, + // and a tie is settled by registration order rather than by where the pointer + // is — so the wrong column wins while the pointer sits inside the right one. + const COLUMN_WIDTH = 300; + const LEFT_CENTRE_X = 150; + const RIGHT_CENTRE_X = 450; + const ROW_CENTRE_Y = 200; + + function columnDistances(pointerX: number): { left: number; right: number } { + const at = (centreX: number): number => + insertionDistancePx({ + pointerX, + pointerY: ROW_CENTRE_Y, + centreX, + centreY: ROW_CENTRE_Y, + width: COLUMN_WIDTH, + }); + return { left: at(LEFT_CENTRE_X), right: at(RIGHT_CENTRE_X) }; + } + + it("does not tie two columns at the same height", () => { + // Inside the left column. Pure vertical distance would report 0 for BOTH. + const { left, right } = columnDistances(LEFT_CENTRE_X); + + expect(left).toBe(0); + expect( + right, + "the far column must not tie with the one under the pointer" + ).toBeGreaterThan(0); + }); + + it("is zero anywhere inside a target's width, not only at its centre", () => { + // The property that keeps ordinary block flow ranking purely on `y`: a + // full-width zone contains the pointer horizontally wherever it is, so the + // horizontal term contributes nothing and cannot distort the comparison. + const atCentre = columnDistances(LEFT_CENTRE_X).left; + const atEdge = columnDistances(LEFT_CENTRE_X + COLUMN_WIDTH / 2).left; + + expect(atCentre).toBe(0); + expect(atEdge).toBe(0); + }); + + it("grows only once the pointer leaves the target", () => { + const justOutside = columnDistances( + LEFT_CENTRE_X + COLUMN_WIDTH / 2 + 10 + ).left; + + expect(justOutside).toBe(10); + }); +}); + +describe("eligibility, which the margin depends on", () => { + // The margin lives in the ranking, so it can only act on targets that are + // still IN the ranking. The default detection stops reporting a target once + // the dragged feedback no longer overlaps it, and where targets are spaced + // farther apart than that feedback is tall, that happens before any + // neighbour becomes eligible — the held target is dropped and the indicator + // alternates between a target and nothing, which is the flicker arriving + // through eligibility rather than through ranking. + it("keeps the held target when the default detection drops it", () => { + expect( + isInsertionTargetEligible({ + hasDefaultCollision: false, + isCurrentTarget: true, + pointerWithinWidth: true, + }) + ).toBe(true); + }); + + it("does not extend that reprieve to a target that is not held", () => { + // Otherwise every target in the document stays in the ranking forever. + expect( + isInsertionTargetEligible({ + hasDefaultCollision: false, + isCurrentTarget: false, + pointerWithinWidth: true, + }) + ).toBe(false); + }); + + it("releases the held target once the pointer leaves its width", () => { + // The bound on the reprieve. Without it, leaving the column, the container + // or the canvas entirely would still show its indicator. + expect( + isInsertionTargetEligible({ + hasDefaultCollision: false, + isCurrentTarget: true, + pointerWithinWidth: false, + }) + ).toBe(false); + }); + + it("admits anything the default detection already admits", () => { + // Eligibility is never NARROWED, so no target stops claiming a pointer it + // claimed before this module existed. + for (const isCurrentTarget of [true, false]) { + for (const pointerWithinWidth of [true, false]) { + expect( + isInsertionTargetEligible({ + hasDefaultCollision: true, + isCurrentTarget, + pointerWithinWidth, + }) + ).toBe(true); + } + } + }); +}); + +describe("the one priority scale", () => { + // Stated as RELATIONSHIPS rather than numbers. A test that restated the + // arithmetic would agree with a policy that had drifted, because both sides + // would be wrong the same way. + it("ranks a container's own append target with the zones inside it", () => { + expect(nodeTargetPriority(3, "append")).toBe(zonePriority(4)); + }); + + it("ranks an insert-before target with the zones of the slot it sits in", () => { + expect(nodeTargetPriority(3, "before")).toBe(zonePriority(3)); + }); + + it("puts a deeper zone above a shallower one", () => { + expect(zonePriority(4)).toBeGreaterThan(zonePriority(3)); + }); + + it("puts a container's append target above the zones holding the container", () => { + expect(nodeTargetPriority(3, "append")).toBeGreaterThan(zonePriority(3)); + }); +}); diff --git a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts new file mode 100644 index 0000000000..f38d62dceb --- /dev/null +++ b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts @@ -0,0 +1,227 @@ +/** + * The single place the canvas decides which insertion target wins a pointer. + * + * Four droppables mark insertion points — the drop zones interleaved between a + * slot's children, the "drop here" placeholder of an empty slot, and the + * `before:` / `append:` targets a formatted slot uses instead of zones. They + * answer ONE question, so they rank by one rule and take their priority from + * one scale. Anything that computes either alongside will drift, and the drift + * is silent because both halves look correct on their own. + * + * `sortCollisions` in `@dnd-kit/abstract` compares `priority`, THEN `type`, + * THEN `value`, and each of those tiers is load-bearing here: + * + * - **`priority` is depth**, so the innermost container owns the pointer. It + * is the only thing that can separate two IDENTICAL rectangles, which is + * what a nested container's edge gap and its parent's gap between children + * are. Omitting it is not neutral: a droppable with no `collisionPriority` + * keeps the detector's own constant, which outranks every target shallower + * than that number however the rectangles lie. + * - **`type` is CONSTANT across insertion targets.** The switch margin lives + * in `value`, and a detector that reported pointer containment inside a + * target and shape overlap outside it would change tier exactly where the + * pointer crosses a target's edge — so the target already held is outranked + * before its margin is ever compared. One tier is what puts the decision on + * the tier the margin can reach. + * - **`value` is negated distance in pixels**, so the margin below is a real + * distance the pointer has to travel rather than a number whose meaning + * changes with how far away things are. + */ +import { + CollisionPriority, + CollisionType, + type CollisionDetector, +} from "@dnd-kit/abstract"; +import { defaultCollisionDetection } from "@dnd-kit/collision"; + +/** + * How much closer a challenging target must be before the drop target moves. + * + * The full width of the switch margin in pixels of pointer travel, not a + * half-width: releasing the incumbent needs `distance - BAND` to beat the + * challenger, which puts the switch half a band past the midpoint going one way + * and half a band short of it coming back. + */ +export const TARGET_SWITCH_BAND_PX = 10; + +/** + * The one collision tier every insertion target reports. + * + * Pointer containment rather than a lower tier, because that is what a target + * already reports at the position where the drop target is most often held: + * with the pointer inside it. Pinning the tier there keeps that case ranking as + * it did before this module existed, and lifts only the case where a target was + * previously demoted for not containing the pointer. + */ +export const INSERTION_COLLISION_TYPE = CollisionType.PointerIntersection; + +/** Where a slot's own drop zones rank: the depth its content is rendered at. */ +export function zonePriority(depth: number): number { + return depth; +} + +/** + * Where a node-attached insertion target ranks. + * + * `before` marks a position in the slot the node SITS IN, so it ranks with that + * slot's zones. `append` marks a position in the node's OWN slot, so it ranks + * with the zones inside it — one level deeper, exactly where `buildSlots` + * renders that slot's content. + */ +export function nodeTargetPriority( + depth: number, + kind: "before" | "append" +): number { + return kind === "before" ? depth : depth + 1; +} + +/** + * How far the pointer is from an insertion target, in pixels. + * + * Vertical distance to the target's centre, plus however far the pointer sits + * OUTSIDE it horizontally. The horizontal term is zero whenever the pointer is + * within the target's width, which is every zone in ordinary block flow — those + * span their container, so a shared horizontal offset would only add the same + * quantity to every candidate and, under a square root, would stop cancelling + * and start distorting the vertical comparison it is irrelevant to. + * + * It stops being zero exactly where it must: targets in different columns of a + * formatted slot share a depth and a vertical band, so ranking them by `y` + * alone makes equal-height targets tie and lets registration order pick the + * column. The horizontal term is what separates them. + */ +export function insertionDistancePx({ + pointerX, + pointerY, + centreX, + centreY, + width, +}: { + pointerX: number; + pointerY: number; + centreX: number; + centreY: number; + width: number; +}): number { + const outsideX = Math.max(0, Math.abs(pointerX - centreX) - width / 2); + return Math.hypot(outsideX, pointerY - centreY); +} + +/** + * A target's rank among the others: higher wins, and the unit is pixels. + * + * Negated because `sortCollisions` orders descending while the better candidate + * is the NEARER one. Linear rather than reciprocal so the margin subtracted + * here is a distance the pointer can actually travel: the width of the switch + * margin equals `bandPx` exactly, at any distance and any target spacing. + */ +export function insertionCollisionValue({ + distancePx, + isCurrentTarget, + bandPx, +}: { + distancePx: number; + isCurrentTarget: boolean; + bandPx: number; +}): number { + return -(isCurrentTarget ? distancePx - bandPx : distancePx); +} + +/** + * Whether a target is in play at all. + * + * The default detection decides it for every target EXCEPT the one currently + * held. That exception is what makes the margin whole: the default detection + * stops reporting a target once the dragged feedback no longer overlaps it, + * which happens BEFORE any neighbour becomes eligible whenever targets are + * spaced farther apart than that feedback is tall. Without it the held target + * is dropped at that edge and the drop indicator alternates between a target + * and nothing — the same flicker the margin exists to remove, arriving through + * eligibility instead of through ranking. + * + * The reprieve is bounded by the pointer staying within the target's width, so + * leaving the column, the container or the canvas still releases it. + */ +export function isInsertionTargetEligible({ + hasDefaultCollision, + isCurrentTarget, + pointerWithinWidth, +}: { + hasDefaultCollision: boolean; + isCurrentTarget: boolean; + pointerWithinWidth: boolean; +}): boolean { + return hasDefaultCollision || (isCurrentTarget && pointerWithinWidth); +} + +/** + * Rank insertion targets by pointer distance, holding the current one across a + * margin. + * + * Eligibility is delegated to the default detection for every target EXCEPT the + * one currently held, so no target starts claiming a pointer it would not have + * claimed before. The exception is what makes the margin whole: the default + * detection stops reporting a target once the dragged feedback no longer + * overlaps it, which happens BEFORE any neighbour becomes eligible whenever + * targets are spaced farther apart than that feedback is tall. Without the + * exception the held target is dropped at that edge and the drop indicator + * alternates between a target and nothing — the same flicker the margin exists + * to remove, arriving through eligibility instead of through ranking. + * + * The incumbent's reprieve is bounded by the pointer staying within its width, + * so leaving the column, the container or the canvas still releases it. + */ +export function createInsertionCollisionDetector( + bandPx: number = TARGET_SWITCH_BAND_PX +): CollisionDetector { + return input => { + const { droppable, dragOperation } = input; + const eligible = defaultCollisionDetection(input); + + const shape = droppable.shape; + const pointer = dragOperation.position.current; + // Without a measured rectangle or a pointer there is no distance to rank + // by, so whatever the default detection decided stands rather than being + // replaced by a fabricated ranking. + if (!shape || !pointer) return eligible; + + const centre = shape.center; + const { width } = shape.boundingRectangle; + const isCurrentTarget = dragOperation.target?.id === droppable.id; + const withinWidth = Math.abs(pointer.x - centre.x) <= width / 2; + + if ( + !isInsertionTargetEligible({ + hasDefaultCollision: eligible !== null, + isCurrentTarget, + pointerWithinWidth: withinWidth, + }) + ) { + return null; + } + + return { + id: droppable.id, + priority: eligible?.priority ?? CollisionPriority.Normal, + type: INSERTION_COLLISION_TYPE, + value: insertionCollisionValue({ + distancePx: insertionDistancePx({ + pointerX: pointer.x, + pointerY: pointer.y, + centreX: centre.x, + centreY: centre.y, + width, + }), + isCurrentTarget, + bandPx, + }), + }; + }; +} + +/** + * Shared by every insertion target, and built once because it closes over + * nothing per-target: the detector reads the held target from the drag + * operation it is handed, so there is no state to keep. + */ +export const insertionCollisionDetector = createInsertionCollisionDetector(); diff --git a/packages/plugin-page-builder/src/admin/canvas/zoneCollision.ts b/packages/plugin-page-builder/src/admin/canvas/zoneCollision.ts deleted file mode 100644 index 9cc3147119..0000000000 --- a/packages/plugin-page-builder/src/admin/canvas/zoneCollision.ts +++ /dev/null @@ -1,120 +0,0 @@ -/** - * Ranking for the drop zones interleaved between a slot's children. - * - * Zones are 6px tall and centred exactly on the insertion line they mark, so - * "which zone should own the pointer" is a question about one number: the - * vertical gap between the pointer and each zone's centre. This module answers - * it in pixels and adds a switch margin, so a pointer resting near a boundary - * keeps the target it already has instead of flipping on every small movement. - * - * Two properties of `sortCollisions` in `@dnd-kit/abstract` shape everything - * here, because it compares `priority`, THEN `type`, and only THEN `value`: - * - * - **The margin has to live in `value`, so `type` must not vary.** A detector - * that reports pointer containment inside a zone and shape overlap outside it - * changes tier as the pointer crosses the zone's own edge, and a margin - * expressed in `value` is never consulted across that change — the incumbent - * is outranked before its margin is read. Reporting ONE type for every zone - * is what keeps the comparison on the tier the margin can reach. - * - **`priority` is left alone.** A zone carries its container's depth as - * `collisionPriority`, which the collision observer writes over whatever a - * detector returns, so depth stays the primary key and a nested container - * keeps winning against the identical rectangle of its parent's gap. - */ -import { CollisionType, type CollisionDetector } from "@dnd-kit/abstract"; -import { defaultCollisionDetection } from "@dnd-kit/collision"; - -/** - * How much closer a challenging zone must be before the target moves to it. - * - * This is the full width of the switch margin in pixels of pointer movement, - * not a half-width: releasing the incumbent needs `distance - BAND` to beat the - * challenger, which moves the switch point half a band past the midpoint going - * one way and half a band short of it coming back. - */ -export const TARGET_SWITCH_BAND_PX = 10; - -/** - * The single collision tier every interleaved zone reports. - * - * Pointer containment rather than a lower tier, because that is what a zone - * already reports at the position where a target is most often held: with the - * pointer inside its 6px rect. Pinning the tier there keeps that case ranking - * as it does without this module, and lifts only the case where a zone was - * previously demoted for not containing the pointer. - */ -export const ZONE_COLLISION_TYPE = CollisionType.PointerIntersection; - -/** - * The pointer's distance from a zone, measured along the axis the zone divides. - * - * Vertical only, and that is narrower than a straight line on purpose. Zones - * interleaved in block flow span their container's full width, so every zone - * competing on `value` is the same horizontal distance from the pointer; a - * two-axis distance would fold that shared term in under a square root, where - * it stops cancelling and starts distorting the vertical comparison it is - * irrelevant to. - */ -export function zoneDistancePx(pointerY: number, zoneCentreY: number): number { - return Math.abs(pointerY - zoneCentreY); -} - -/** - * A zone's rank among its siblings: higher wins, and the unit is pixels. - * - * Negated because `sortCollisions` orders descending while the better candidate - * is the NEARER one. Kept linear rather than reciprocal so that the margin - * subtracted below is a distance the pointer can actually travel — the width of - * the switch margin equals `bandPx` exactly, at any distance and any zone - * spacing, which is what makes it assertable in pixels of pointer movement. - */ -export function zoneCollisionValue({ - distancePx, - isCurrentTarget, - bandPx, -}: { - distancePx: number; - isCurrentTarget: boolean; - bandPx: number; -}): number { - return -(isCurrentTarget ? distancePx - bandPx : distancePx); -} - -/** - * Rank zones by pointer distance, holding the current target across a margin. - * - * Eligibility is delegated rather than reimplemented: the default detection - * decides WHETHER a zone is in play at all, exactly as it does without this - * module, and only the ranking among the zones it admits is replaced. A zone - * the default detection rejects is still rejected here, so no zone starts - * claiming a pointer it would not have claimed before. - */ -export function createZoneCollisionDetector( - bandPx: number = TARGET_SWITCH_BAND_PX -): CollisionDetector { - return input => { - const eligible = defaultCollisionDetection(input); - if (!eligible) return null; - - const { droppable, dragOperation } = input; - const centre = droppable.shape?.center; - const pointer = dragOperation.position.current; - // Without a measured rectangle or a pointer there is no distance to rank - // by, so the delegated result stands rather than being replaced by a - // fabricated one. - if (!centre || !pointer) return eligible; - - return { - id: droppable.id, - priority: eligible.priority, - // One tier for every zone. See the module comment: a varying tier - // outranks the margin below before it is ever compared. - type: ZONE_COLLISION_TYPE, - value: zoneCollisionValue({ - distancePx: zoneDistancePx(pointer.y, centre.y), - isCurrentTarget: dragOperation.target?.id === droppable.id, - bandPx, - }), - }; - }; -} From 70df6448a7cc3351eddd595c4cf5a7cf92cf549f Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sat, 15 Aug 2026 09:11:42 +0500 Subject: [PATCH 3/8] fix(plugin-page-builder): bound the held target's reprieve by the same margin Keeping the held target eligible while the pointer stayed within its width was unbounded along the axis that matters. Nothing released it until a rival became eligible, and on widely spaced targets nothing does for a long way: the margin stopped being a margin and the indicator clung to a target the pointer had left. Measured on a fixture whose targets sit 400px apart, reversing away from the held target never released it within 27px. With the reprieve removed entirely the same walk released it immediately, which is what isolates the reprieve as the cause rather than the ranking. Bound it by the SAME band in both axes: within the target's width, and within one band of its edge. Reusing the band rather than adding a second constant keeps one quantity answering "how far does the pointer move before the target changes", which is what the requirement names. The eligibility widening that this reprieve exists for survives, because a rival that becomes eligible inside one band still arrives before the held target is dropped. --- .../src/admin/canvas/collisionPolicy.test.ts | 49 +++++++++++++------ .../src/admin/canvas/collisionPolicy.ts | 24 +++++++-- 2 files changed, 54 insertions(+), 19 deletions(-) diff --git a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts index 2549645eb0..41cde6afb3 100644 --- a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts +++ b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts @@ -344,42 +344,57 @@ describe("eligibility, which the margin depends on", () => { // neighbour becomes eligible — the held target is dropped and the indicator // alternates between a target and nothing, which is the flicker arriving // through eligibility rather than through ranking. + const held = { + hasDefaultCollision: false, + isCurrentTarget: true, + pointerWithinWidth: true, + pointerBeyondEdgePx: 0, + bandPx: TARGET_SWITCH_BAND_PX, + }; + it("keeps the held target when the default detection drops it", () => { + expect(isInsertionTargetEligible(held)).toBe(true); + }); + + it("keeps it right up to the edge of the band", () => { expect( isInsertionTargetEligible({ - hasDefaultCollision: false, - isCurrentTarget: true, - pointerWithinWidth: true, + ...held, + pointerBeyondEdgePx: TARGET_SWITCH_BAND_PX, }) ).toBe(true); }); - it("does not extend that reprieve to a target that is not held", () => { - // Otherwise every target in the document stays in the ranking forever. + it("releases it one pixel past the band", () => { + // The bound that stops the reprieve becoming unbounded stickiness. Without + // it the held target survives for as long as no rival happens to be + // eligible, which on widely spaced targets is indefinitely: the margin + // stops being a margin and the indicator clings to a target the pointer + // left long ago. expect( isInsertionTargetEligible({ - hasDefaultCollision: false, - isCurrentTarget: false, - pointerWithinWidth: true, + ...held, + pointerBeyondEdgePx: TARGET_SWITCH_BAND_PX + 1, }) ).toBe(false); }); + it("does not extend the reprieve to a target that is not held", () => { + // Otherwise every target in the document stays in the ranking forever. + expect(isInsertionTargetEligible({ ...held, isCurrentTarget: false })).toBe( + false + ); + }); + it("releases the held target once the pointer leaves its width", () => { - // The bound on the reprieve. Without it, leaving the column, the container - // or the canvas entirely would still show its indicator. expect( - isInsertionTargetEligible({ - hasDefaultCollision: false, - isCurrentTarget: true, - pointerWithinWidth: false, - }) + isInsertionTargetEligible({ ...held, pointerWithinWidth: false }) ).toBe(false); }); it("admits anything the default detection already admits", () => { // Eligibility is never NARROWED, so no target stops claiming a pointer it - // claimed before this module existed. + // claimed before this module existed — including well outside the band. for (const isCurrentTarget of [true, false]) { for (const pointerWithinWidth of [true, false]) { expect( @@ -387,6 +402,8 @@ describe("eligibility, which the margin depends on", () => { hasDefaultCollision: true, isCurrentTarget, pointerWithinWidth, + pointerBeyondEdgePx: 10_000, + bandPx: TARGET_SWITCH_BAND_PX, }) ).toBe(true); } diff --git a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts index f38d62dceb..4ef159ce04 100644 --- a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts +++ b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts @@ -139,19 +139,32 @@ export function insertionCollisionValue({ * and nothing — the same flicker the margin exists to remove, arriving through * eligibility instead of through ranking. * - * The reprieve is bounded by the pointer staying within the target's width, so - * leaving the column, the container or the canvas still releases it. + * The reprieve is bounded by the SAME band, in both axes: the pointer must stay + * within the target's width, and within one band of its edge. Bounding it + * matters more than it looks. An unbounded reprieve holds the target for as + * long as no rival happens to be eligible, which on widely spaced targets is + * indefinitely — so the margin stops being a margin and the drop indicator + * sticks to a target the pointer left long ago. Measured on a fixture whose + * targets sit 400px apart, the unbounded form never released within 27px of + * reversing. Reusing `bandPx` rather than introducing a second constant keeps + * one quantity answering "how far does the pointer move before the target + * changes", which is the thing the requirement actually names. */ export function isInsertionTargetEligible({ hasDefaultCollision, isCurrentTarget, pointerWithinWidth, + pointerBeyondEdgePx, + bandPx, }: { hasDefaultCollision: boolean; isCurrentTarget: boolean; pointerWithinWidth: boolean; + pointerBeyondEdgePx: number; + bandPx: number; }): boolean { - return hasDefaultCollision || (isCurrentTarget && pointerWithinWidth); + if (hasDefaultCollision) return true; + return isCurrentTarget && pointerWithinWidth && pointerBeyondEdgePx <= bandPx; } /** @@ -195,6 +208,11 @@ export function createInsertionCollisionDetector( hasDefaultCollision: eligible !== null, isCurrentTarget, pointerWithinWidth: withinWidth, + pointerBeyondEdgePx: Math.max( + 0, + Math.abs(pointer.y - centre.y) - shape.boundingRectangle.height / 2 + ), + bandPx, }) ) { return null; From ef8d91da5a33597173f5d2d7566e076e6ff63098 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sat, 15 Aug 2026 09:55:08 +0500 Subject: [PATCH 4/8] fix(plugin-page-builder): rank by full distance and bound the reprieve on both axes A container's append rectangle spans every child while a child's before rectangle spans one column. Zeroing the horizontal term inside each target's width made both report zero horizontally, both reduce to the vertical gap, and in an equal-height row their centres align and the two tie, leaving registration order to decide which is reachable at all. Rank by full straight-line distance to the centre instead. Zones in ordinary block flow share a centre x, so they carry an identical horizontal offset: it does not cancel arithmetically, but being the same for every candidate it cannot reorder them, and order is the only thing the sort reads. Bound the eligibility reprieve by distance to the target's RECTANGLE rather than by a width test paired with a vertical margin. A hard boundary on either axis is a cliff the margin cannot smooth: gating horizontally on inside-the-width drops the held target the instant the pointer crosses a column edge, so its credit is never compared with the challenger and a jitter across that edge flips the indicator, which is the same defect rotated ninety degrees. --- .../src/admin/canvas/collisionPolicy.test.ts | 126 +++++++++++------- .../src/admin/canvas/collisionPolicy.ts | 92 +++++++++---- 2 files changed, 145 insertions(+), 73 deletions(-) diff --git a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts index 41cde6afb3..c0c60fdf48 100644 --- a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts +++ b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts @@ -26,6 +26,7 @@ import { TARGET_SWITCH_BAND_PX, insertionCollisionValue, insertionDistancePx, + insertionEdgeDistancePx, isInsertionTargetEligible, nodeTargetPriority, zonePriority, @@ -45,7 +46,6 @@ function distanceInColumn(pointerY: number, centreY: number): number { pointerY, centreX: POINTER_X, centreY, - width: ZONE_WIDTH_PX, }); } @@ -283,56 +283,85 @@ describe("insertionCollisionValue", () => { }); }); -describe("the horizontal term, which separates columns", () => { - // Targets in different columns of a formatted slot share a depth and a - // vertical band. Ranking them by `y` alone makes equal-height targets TIE, - // and a tie is settled by registration order rather than by where the pointer - // is — so the wrong column wins while the pointer sits inside the right one. +describe("the horizontal term, which separates targets of unequal width", () => { const COLUMN_WIDTH = 300; const LEFT_CENTRE_X = 150; const RIGHT_CENTRE_X = 450; const ROW_CENTRE_Y = 200; - - function columnDistances(pointerX: number): { left: number; right: number } { - const at = (centreX: number): number => - insertionDistancePx({ - pointerX, - pointerY: ROW_CENTRE_Y, - centreX, - centreY: ROW_CENTRE_Y, - width: COLUMN_WIDTH, - }); - return { left: at(LEFT_CENTRE_X), right: at(RIGHT_CENTRE_X) }; - } + // A formatted container's `append` rectangle spans every child, so its centre + // sits between the columns rather than over one of them. + const CONTAINER_CENTRE_X = 300; + + const at = ( + pointerX: number, + centreX: number, + centreY = ROW_CENTRE_Y + ): number => + insertionDistancePx({ pointerX, pointerY: ROW_CENTRE_Y, centreX, centreY }); + + it("keeps the ORDER vertical between targets that share a centre", () => { + // Every zone in ordinary block flow spans its container, so they share a + // centre x and carry an identical horizontal offset. That offset does not + // cancel arithmetically under a square root, and it does not need to: it is + // the same for both candidates, so it cannot reorder them. The comparison + // is decided by the vertical gap exactly as if the term were absent, which + // is the property the ranking actually depends on. + const offCentreX = POINTER_X + 380; + const pointerY = 200; + const nearerVertically = at(offCentreX, POINTER_X, pointerY - 40); + const furtherVertically = at(offCentreX, POINTER_X, pointerY - 100); + + expect(nearerVertically).toBeLessThan(furtherVertically); + }); it("does not tie two columns at the same height", () => { - // Inside the left column. Pure vertical distance would report 0 for BOTH. - const { left, right } = columnDistances(LEFT_CENTRE_X); - - expect(left).toBe(0); + expect(at(LEFT_CENTRE_X, LEFT_CENTRE_X)).toBe(0); expect( - right, + at(LEFT_CENTRE_X, RIGHT_CENTRE_X), "the far column must not tie with the one under the pointer" ).toBeGreaterThan(0); }); - it("is zero anywhere inside a target's width, not only at its centre", () => { - // The property that keeps ordinary block flow ranking purely on `y`: a - // full-width zone contains the pointer horizontally wherever it is, so the - // horizontal term contributes nothing and cannot distort the comparison. - const atCentre = columnDistances(LEFT_CENTRE_X).left; - const atEdge = columnDistances(LEFT_CENTRE_X + COLUMN_WIDTH / 2).left; + it("separates a container's append target from the child under the pointer", () => { + // The case a zero-inside-the-width term cannot see: the container's + // rectangle spans both columns, so the pointer is horizontally INSIDE it + // and inside the child at once. Zeroing the term there makes both reduce to + // the vertical gap, and with their centres aligned the two tie and + // registration order decides which is reachable. + const toChild = at(LEFT_CENTRE_X, LEFT_CENTRE_X); + const toContainer = at(LEFT_CENTRE_X, CONTAINER_CENTRE_X); - expect(atCentre).toBe(0); - expect(atEdge).toBe(0); + expect(toChild).toBeLessThan(toContainer); }); - it("grows only once the pointer leaves the target", () => { - const justOutside = columnDistances( - LEFT_CENTRE_X + COLUMN_WIDTH / 2 + 10 - ).left; + it("grows with horizontal separation rather than only outside a boundary", () => { + expect(at(LEFT_CENTRE_X + 10, LEFT_CENTRE_X)).toBe(10); + expect(at(LEFT_CENTRE_X + 160, LEFT_CENTRE_X)).toBe(160); + }); +}); + +describe("insertionEdgeDistancePx, which bounds the reprieve", () => { + const rect = { centreX: 150, centreY: 200, width: 300, height: 40 }; + + it("is zero anywhere inside the rectangle", () => { + // The property the RANKING metric deliberately does not have. Bounding the + // reprieve asks "how far outside the target is the pointer", which is a + // different question from "which target is nearest". + expect( + insertionEdgeDistancePx({ pointerX: 150, pointerY: 200, ...rect }) + ).toBe(0); + expect( + insertionEdgeDistancePx({ pointerX: 300, pointerY: 220, ...rect }) + ).toBe(0); + }); - expect(justOutside).toBe(10); + it("measures from the nearest edge once outside, on either axis", () => { + expect( + insertionEdgeDistancePx({ pointerX: 310, pointerY: 200, ...rect }) + ).toBe(10); + expect( + insertionEdgeDistancePx({ pointerX: 150, pointerY: 230, ...rect }) + ).toBe(10); }); }); @@ -347,8 +376,7 @@ describe("eligibility, which the margin depends on", () => { const held = { hasDefaultCollision: false, isCurrentTarget: true, - pointerWithinWidth: true, - pointerBeyondEdgePx: 0, + edgeDistancePx: 0, bandPx: TARGET_SWITCH_BAND_PX, }; @@ -360,7 +388,7 @@ describe("eligibility, which the margin depends on", () => { expect( isInsertionTargetEligible({ ...held, - pointerBeyondEdgePx: TARGET_SWITCH_BAND_PX, + edgeDistancePx: TARGET_SWITCH_BAND_PX, }) ).toBe(true); }); @@ -374,7 +402,7 @@ describe("eligibility, which the margin depends on", () => { expect( isInsertionTargetEligible({ ...held, - pointerBeyondEdgePx: TARGET_SWITCH_BAND_PX + 1, + edgeDistancePx: TARGET_SWITCH_BAND_PX + 1, }) ).toBe(false); }); @@ -386,23 +414,29 @@ describe("eligibility, which the margin depends on", () => { ); }); - it("releases the held target once the pointer leaves its width", () => { + it("bounds the reprieve on the HORIZONTAL axis by the same band", () => { + // A hard "inside the width" gate would drop the held target the instant the + // pointer crossed a column edge, so its credit would never be compared with + // the challenger and a small jitter across that edge would flip the + // indicator. One distance covers both axes, so neither is a cliff. expect( - isInsertionTargetEligible({ ...held, pointerWithinWidth: false }) - ).toBe(false); + isInsertionTargetEligible({ + ...held, + edgeDistancePx: TARGET_SWITCH_BAND_PX - 1, + }) + ).toBe(true); }); it("admits anything the default detection already admits", () => { // Eligibility is never NARROWED, so no target stops claiming a pointer it // claimed before this module existed — including well outside the band. for (const isCurrentTarget of [true, false]) { - for (const pointerWithinWidth of [true, false]) { + { expect( isInsertionTargetEligible({ hasDefaultCollision: true, isCurrentTarget, - pointerWithinWidth, - pointerBeyondEdgePx: 10_000, + edgeDistancePx: 10_000, bandPx: TARGET_SWITCH_BAND_PX, }) ).toBe(true); diff --git a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts index 4ef159ce04..fd8d263cb9 100644 --- a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts +++ b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts @@ -78,33 +78,66 @@ export function nodeTargetPriority( /** * How far the pointer is from an insertion target, in pixels. * - * Vertical distance to the target's centre, plus however far the pointer sits - * OUTSIDE it horizontally. The horizontal term is zero whenever the pointer is - * within the target's width, which is every zone in ordinary block flow — those - * span their container, so a shared horizontal offset would only add the same - * quantity to every candidate and, under a square root, would stop cancelling - * and start distorting the vertical comparison it is irrelevant to. + * Straight-line distance to the target's centre, both axes counted in full. * - * It stops being zero exactly where it must: targets in different columns of a - * formatted slot share a depth and a vertical band, so ranking them by `y` - * alone makes equal-height targets tie and lets registration order pick the - * column. The horizontal term is what separates them. + * The horizontal term does NOT vanish inside the target's width, and that is + * the whole point. Zones in ordinary block flow all span their container, so + * they share a centre `x` and carry an IDENTICAL horizontal offset. It does not + * cancel arithmetically under a square root, and it does not need to: being the + * same for every candidate, it cannot reorder them, so the comparison is + * decided by the vertical gap exactly as if the term were absent. Order is the + * only thing `sortCollisions` reads, so order is the property to preserve. + * + * Where widths DIFFER it must not cancel, and zeroing it inside each target's + * width is exactly what stops it. A formatted container's `append` rectangle + * spans all of its children while a child's `before` rectangle spans one + * column; both then report zero horizontally, both reduce to the vertical gap, + * and in an equal-height row their centres align and the two TIE — leaving + * registration order to choose, which can make one of them unreachable. Full + * distance separates a wide container from the narrow child under the pointer, + * because their centres are genuinely in different places. */ export function insertionDistancePx({ pointerX, pointerY, centreX, centreY, +}: { + pointerX: number; + pointerY: number; + centreX: number; + centreY: number; +}): number { + return Math.hypot(pointerX - centreX, pointerY - centreY); +} + +/** + * How far the pointer is from a target's RECTANGLE, in pixels. Zero inside it. + * + * Distinct from {@link insertionDistancePx}, which measures to the insertion + * line and is what RANKS targets. This one measures to the boundary and is what + * bounds the reprieve below, because "how far outside the target is the pointer" + * is a different question from "which target is nearest". + */ +export function insertionEdgeDistancePx({ + pointerX, + pointerY, + centreX, + centreY, width, + height, }: { pointerX: number; pointerY: number; centreX: number; centreY: number; width: number; + height: number; }): number { - const outsideX = Math.max(0, Math.abs(pointerX - centreX) - width / 2); - return Math.hypot(outsideX, pointerY - centreY); + return Math.hypot( + Math.max(0, Math.abs(pointerX - centreX) - width / 2), + Math.max(0, Math.abs(pointerY - centreY) - height / 2) + ); } /** @@ -139,8 +172,14 @@ export function insertionCollisionValue({ * and nothing — the same flicker the margin exists to remove, arriving through * eligibility instead of through ranking. * - * The reprieve is bounded by the SAME band, in both axes: the pointer must stay - * within the target's width, and within one band of its edge. Bounding it + * The reprieve is bounded by the SAME band, in BOTH axes at once: the pointer + * must stay within one band of the target's rectangle, measured by + * {@link insertionEdgeDistancePx}. One distance rather than a per-axis pair, + * because a hard boundary on either axis is a cliff the margin cannot smooth: + * gating horizontally on "inside the width" drops the held target the instant + * the pointer crosses a column edge, so its credit is never compared with the + * challenger and a small jitter across that edge flips the indicator - the same + * defect this reprieve exists to remove, rotated ninety degrees. Bounding it * matters more than it looks. An unbounded reprieve holds the target for as * long as no rival happens to be eligible, which on widely spaced targets is * indefinitely — so the margin stops being a margin and the drop indicator @@ -153,18 +192,16 @@ export function insertionCollisionValue({ export function isInsertionTargetEligible({ hasDefaultCollision, isCurrentTarget, - pointerWithinWidth, - pointerBeyondEdgePx, + edgeDistancePx, bandPx, }: { hasDefaultCollision: boolean; isCurrentTarget: boolean; - pointerWithinWidth: boolean; - pointerBeyondEdgePx: number; + edgeDistancePx: number; bandPx: number; }): boolean { if (hasDefaultCollision) return true; - return isCurrentTarget && pointerWithinWidth && pointerBeyondEdgePx <= bandPx; + return isCurrentTarget && edgeDistancePx <= bandPx; } /** @@ -199,19 +236,21 @@ export function createInsertionCollisionDetector( if (!shape || !pointer) return eligible; const centre = shape.center; - const { width } = shape.boundingRectangle; + const { width, height } = shape.boundingRectangle; const isCurrentTarget = dragOperation.target?.id === droppable.id; - const withinWidth = Math.abs(pointer.x - centre.x) <= width / 2; if ( !isInsertionTargetEligible({ hasDefaultCollision: eligible !== null, isCurrentTarget, - pointerWithinWidth: withinWidth, - pointerBeyondEdgePx: Math.max( - 0, - Math.abs(pointer.y - centre.y) - shape.boundingRectangle.height / 2 - ), + edgeDistancePx: insertionEdgeDistancePx({ + pointerX: pointer.x, + pointerY: pointer.y, + centreX: centre.x, + centreY: centre.y, + width, + height, + }), bandPx, }) ) { @@ -228,7 +267,6 @@ export function createInsertionCollisionDetector( pointerY: pointer.y, centreX: centre.x, centreY: centre.y, - width, }), isCurrentTarget, bandPx, From c6f25dc1101c6d5fe0efd08a69e3521442cfe28e Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sat, 15 Aug 2026 10:28:13 +0500 Subject: [PATCH 5/8] fix(plugin-page-builder): sum the collision axes so the margin keeps its width Combining the two axes with `hypot` subtracted the switch margin from the hypotenuse rather than from the axis it is specified on. A pointer 100px from a full-width zone's centre turned a 10px credit into roughly a 36px vertical band, and further out the challenger could not overtake the incumbent at all before eligibility ended it. The margin was a different size everywhere, which is what a requirement stated in pixels of pointer travel rules out. Sum the axes instead. Zones in ordinary block flow span their container and so share a centre x, which makes the horizontal term a constant added to both candidates: it cancels exactly out of the subtraction and the band stays 10px of vertical travel at any horizontal offset. It stops cancelling where it must, since a formatted container's append rectangle is centred between its children while a child's before rectangle is centred on one column. The existing off-centre coverage asserted ORDER, which both metrics get right, so it stayed green while the width drifted. The new assertion measures the width itself at three horizontal offsets, and reverting to the hypotenuse fails it. Also removes a duplicated copy of four suites, and drops a stray duplicate declaration of @dnd-kit/abstract from devDependencies: it is imported for CollisionType and CollisionPriority, which are runtime values, so the dependencies entry is the correct one and carrying both left the lockfile inconsistent with the manifest. --- packages/plugin-page-builder/package.json | 1 - .../src/admin/canvas/collisionPolicy.test.ts | 251 ++++-------------- .../src/admin/canvas/collisionPolicy.ts | 28 +- pnpm-lock.yaml | 3 - 4 files changed, 71 insertions(+), 212 deletions(-) diff --git a/packages/plugin-page-builder/package.json b/packages/plugin-page-builder/package.json index 1bfe552d7e..766690d67e 100644 --- a/packages/plugin-page-builder/package.json +++ b/packages/plugin-page-builder/package.json @@ -76,7 +76,6 @@ "react-hook-form": ">=7.0.0" }, "devDependencies": { - "@dnd-kit/abstract": "0.5.0", "@nextlyhq/admin": "workspace:*", "@nextlyhq/plugin-sdk": "workspace:*", "@nextlyhq/tsconfig": "workspace:*", diff --git a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts index 3985ebfeb8..756b4698c9 100644 --- a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts +++ b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts @@ -38,9 +38,13 @@ import { */ const ZONE_WIDTH_PX = 800; const POINTER_X = 400; -function distanceInColumn(pointerY: number, centreY: number): number { +function distanceInColumn( + pointerY: number, + centreY: number, + pointerX: number = POINTER_X +): number { return insertionDistancePx({ - pointerX: POINTER_X, + pointerX, pointerY, centreX: POINTER_X, centreY, @@ -66,11 +70,13 @@ function winnerAt({ currentTargetId, bandPx = TARGET_SWITCH_BAND_PX, typeFor = () => INSERTION_COLLISION_TYPE, + pointerX = POINTER_X, }: { pointerY: number; currentTargetId: string | null; bandPx?: number; typeFor?: (id: string) => CollisionType; + pointerX?: number; }): string { const collisions: Collision[] = [ { id: "a", centre: ZONE_A_CENTRE_Y }, @@ -80,7 +86,7 @@ function winnerAt({ priority: 1, type: typeFor(zone.id), value: insertionCollisionValue({ - distancePx: distanceInColumn(pointerY, zone.centre), + distancePx: distanceInColumn(pointerY, zone.centre, pointerX), isCurrentTarget: currentTargetId === zone.id, bandPx, }), @@ -108,12 +114,14 @@ function firstSwitchY({ startTargetId, bandPx = TARGET_SWITCH_BAND_PX, typeFor, + pointerX, }: { from: number; to: number; startTargetId: string; bandPx?: number; typeFor?: (id: string) => CollisionType; + pointerX?: number; }): number | null { const step = Math.sign(to - from); let target = startTargetId; @@ -123,6 +131,7 @@ function firstSwitchY({ currentTargetId: target, bandPx, typeFor, + pointerX, }); if (winner !== target) return y; target = winner; @@ -216,6 +225,46 @@ describe("the switch margin, measured in pointer travel", () => { }); }); +describe("the margin's width away from a zone's horizontal centre", () => { + // The property an ordering-only test cannot see. A metric that combines the + // axes under a square root subtracts the margin from the HYPOTENUSE, so the + // vertical band grows with horizontal offset: measured at 100px off-centre it + // becomes roughly 36px against a 10px requirement, and further out the + // challenger can never overtake the incumbent at all. Ordering stays correct + // throughout, so only a WIDTH assertion off-centre separates the two metrics. + function bandWidthAt(pointerX: number): number { + const down = firstSwitchY({ + from: ZONE_A_CENTRE_Y, + to: ZONE_B_CENTRE_Y, + startTargetId: "a", + pointerX, + }); + const up = firstSwitchY({ + from: ZONE_B_CENTRE_Y, + to: ZONE_A_CENTRE_Y, + startTargetId: "b", + pointerX, + }); + return Number(down) - Number(up); + } + + it("is the same 8-12px at the centre and far off it", () => { + for (const offset of [0, 100, 300]) { + const width = bandWidthAt(POINTER_X + offset); + expect(width, `at ${String(offset)}px off-centre`).toBeGreaterThanOrEqual( + 8 + ); + expect(width, `at ${String(offset)}px off-centre`).toBeLessThanOrEqual( + 12 + ); + } + }); + + it("does not drift as the pointer moves sideways", () => { + expect(bandWidthAt(POINTER_X + 300)).toBe(bandWidthAt(POINTER_X)); + }); +}); + describe("what the uniform collision tier is load-bearing for", () => { it("loses the margin entirely when the tier is allowed to vary", () => { // The mutation this design exists to survive. Reporting containment inside a @@ -442,199 +491,3 @@ describe("eligibility, which the margin depends on", () => { } }); }); - -describe("insertionCollisionValue", () => { - it("ranks the nearer zone higher", () => { - const near = insertionCollisionValue({ - distancePx: 5, - isCurrentTarget: false, - bandPx: 10, - }); - const far = insertionCollisionValue({ - distancePx: 25, - isCurrentTarget: false, - bandPx: 10, - }); - - expect(near).toBeGreaterThan(far); - }); - - it("credits the incumbent exactly the margin, in pixels", () => { - const challenger = insertionCollisionValue({ - distancePx: 30, - isCurrentTarget: false, - bandPx: 10, - }); - const incumbent = insertionCollisionValue({ - distancePx: 30, - isCurrentTarget: true, - bandPx: 10, - }); - - // Linear and negated, so the credit reads back as a plain pixel difference - // rather than something that has to be inverted to be interpreted. - expect(incumbent - challenger).toBe(10); - }); -}); - -describe("the horizontal term, which separates targets of unequal width", () => { - const COLUMN_WIDTH = 300; - const LEFT_CENTRE_X = 150; - const RIGHT_CENTRE_X = 450; - const ROW_CENTRE_Y = 200; - // A formatted container's `append` rectangle spans every child, so its centre - // sits between the columns rather than over one of them. - const CONTAINER_CENTRE_X = 300; - - const at = ( - pointerX: number, - centreX: number, - centreY = ROW_CENTRE_Y - ): number => - insertionDistancePx({ pointerX, pointerY: ROW_CENTRE_Y, centreX, centreY }); - - it("keeps the ORDER vertical between targets that share a centre", () => { - // Every zone in ordinary block flow spans its container, so they share a - // centre x and carry an identical horizontal offset. That offset does not - // cancel arithmetically under a square root, and it does not need to: it is - // the same for both candidates, so it cannot reorder them. The comparison - // is decided by the vertical gap exactly as if the term were absent, which - // is the property the ranking actually depends on. - const offCentreX = POINTER_X + 380; - const pointerY = 200; - const nearerVertically = at(offCentreX, POINTER_X, pointerY - 40); - const furtherVertically = at(offCentreX, POINTER_X, pointerY - 100); - - expect(nearerVertically).toBeLessThan(furtherVertically); - }); - - it("does not tie two columns at the same height", () => { - expect(at(LEFT_CENTRE_X, LEFT_CENTRE_X)).toBe(0); - expect( - at(LEFT_CENTRE_X, RIGHT_CENTRE_X), - "the far column must not tie with the one under the pointer" - ).toBeGreaterThan(0); - }); - - it("separates a container's append target from the child under the pointer", () => { - // The case a zero-inside-the-width term cannot see: the container's - // rectangle spans both columns, so the pointer is horizontally INSIDE it - // and inside the child at once. Zeroing the term there makes both reduce to - // the vertical gap, and with their centres aligned the two tie and - // registration order decides which is reachable. - const toChild = at(LEFT_CENTRE_X, LEFT_CENTRE_X); - const toContainer = at(LEFT_CENTRE_X, CONTAINER_CENTRE_X); - - expect(toChild).toBeLessThan(toContainer); - }); - - it("grows with horizontal separation rather than only outside a boundary", () => { - expect(at(LEFT_CENTRE_X + 10, LEFT_CENTRE_X)).toBe(10); - expect(at(LEFT_CENTRE_X + 160, LEFT_CENTRE_X)).toBe(160); - }); -}); - -describe("insertionEdgeDistancePx, which bounds the reprieve", () => { - const rect = { centreX: 150, centreY: 200, width: 300, height: 40 }; - - it("is zero anywhere inside the rectangle", () => { - // The property the RANKING metric deliberately does not have. Bounding the - // reprieve asks "how far outside the target is the pointer", which is a - // different question from "which target is nearest". - expect( - insertionEdgeDistancePx({ pointerX: 150, pointerY: 200, ...rect }) - ).toBe(0); - expect( - insertionEdgeDistancePx({ pointerX: 300, pointerY: 220, ...rect }) - ).toBe(0); - }); - - it("measures from the nearest edge once outside, on either axis", () => { - expect( - insertionEdgeDistancePx({ pointerX: 310, pointerY: 200, ...rect }) - ).toBe(10); - expect( - insertionEdgeDistancePx({ pointerX: 150, pointerY: 230, ...rect }) - ).toBe(10); - }); -}); - -describe("eligibility, which the margin depends on", () => { - // The margin lives in the ranking, so it can only act on targets that are - // still IN the ranking. The default detection stops reporting a target once - // the dragged feedback no longer overlaps it, and where targets are spaced - // farther apart than that feedback is tall, that happens before any - // neighbour becomes eligible — the held target is dropped and the indicator - // alternates between a target and nothing, which is the flicker arriving - // through eligibility rather than through ranking. - const held = { - hasDefaultCollision: false, - isCurrentTarget: true, - edgeDistancePx: 0, - bandPx: TARGET_SWITCH_BAND_PX, - }; - - it("keeps the held target when the default detection drops it", () => { - expect(isInsertionTargetEligible(held)).toBe(true); - }); - - it("keeps it right up to the edge of the band", () => { - expect( - isInsertionTargetEligible({ - ...held, - edgeDistancePx: TARGET_SWITCH_BAND_PX, - }) - ).toBe(true); - }); - - it("releases it one pixel past the band", () => { - // The bound that stops the reprieve becoming unbounded stickiness. Without - // it the held target survives for as long as no rival happens to be - // eligible, which on widely spaced targets is indefinitely: the margin - // stops being a margin and the indicator clings to a target the pointer - // left long ago. - expect( - isInsertionTargetEligible({ - ...held, - edgeDistancePx: TARGET_SWITCH_BAND_PX + 1, - }) - ).toBe(false); - }); - - it("does not extend the reprieve to a target that is not held", () => { - // Otherwise every target in the document stays in the ranking forever. - expect(isInsertionTargetEligible({ ...held, isCurrentTarget: false })).toBe( - false - ); - }); - - it("bounds the reprieve on the HORIZONTAL axis by the same band", () => { - // A hard "inside the width" gate would drop the held target the instant the - // pointer crossed a column edge, so its credit would never be compared with - // the challenger and a small jitter across that edge would flip the - // indicator. One distance covers both axes, so neither is a cliff. - expect( - isInsertionTargetEligible({ - ...held, - edgeDistancePx: TARGET_SWITCH_BAND_PX - 1, - }) - ).toBe(true); - }); - - it("admits anything the default detection already admits", () => { - // Eligibility is never NARROWED, so no target stops claiming a pointer it - // claimed before this module existed — including well outside the band. - for (const isCurrentTarget of [true, false]) { - { - expect( - isInsertionTargetEligible({ - hasDefaultCollision: true, - isCurrentTarget, - edgeDistancePx: 10_000, - bandPx: TARGET_SWITCH_BAND_PX, - }) - ).toBe(true); - } - } - }); -}); diff --git a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts index 37209d1b75..56aaaa3169 100644 --- a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts +++ b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts @@ -56,15 +56,25 @@ export const INSERTION_COLLISION_TYPE = CollisionType.PointerIntersection; /** * How far the pointer is from an insertion target, in pixels. * - * Straight-line distance to the target's centre, both axes counted in full. + * The two axes SUMMED, not combined under a square root. * - * The horizontal term does NOT vanish inside the target's width, and that is - * the whole point. Zones in ordinary block flow all span their container, so - * they share a centre `x` and carry an IDENTICAL horizontal offset. It does not - * cancel arithmetically under a square root, and it does not need to: being the - * same for every candidate, it cannot reorder them, so the comparison is - * decided by the vertical gap exactly as if the term were absent. Order is the - * only thing `sortCollisions` reads, so order is the property to preserve. + * Additive is what keeps the switch margin meaning what it says. The margin is + * subtracted from this number, so if the axes were combined by `hypot` it would + * come off the HYPOTENUSE: a pointer 100px from a full-width zone's centre + * turns a 10px credit into roughly a 36px vertical band, and further out the + * challenger cannot overtake the incumbent at all before eligibility ends it. + * The margin would then be a different size everywhere, which is precisely what + * "8-12px of pointer travel" rules out. + * + * Summed, the horizontal term is a constant added to both candidates whenever + * they share a centre `x` — every zone in ordinary block flow, since they span + * their container — so it cancels EXACTLY out of the subtraction and the band + * stays 10px of vertical travel at any horizontal offset. + * + * It stops cancelling exactly where it should: a formatted container's `append` + * rectangle is centred between its children while a child's `before` rectangle + * is centred on one column, so their horizontal terms differ and the child + * under the pointer wins instead of the two tying on the vertical gap alone. * * Where widths DIFFER it must not cancel, and zeroing it inside each target's * width is exactly what stops it. A formatted container's `append` rectangle @@ -86,7 +96,7 @@ export function insertionDistancePx({ centreX: number; centreY: number; }): number { - return Math.hypot(pointerX - centreX, pointerY - centreY); + return Math.abs(pointerY - centreY) + Math.abs(pointerX - centreX); } /** diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 2d3bda54d0..371866f574 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1341,9 +1341,6 @@ importers: specifier: ^4.0.4 version: 4.0.5 devDependencies: - '@dnd-kit/abstract': - specifier: 0.5.0 - version: 0.5.0 '@nextlyhq/admin': specifier: workspace:* version: link:../admin From 1120591dfb57d4a87ab1fc84ee8bc5f8502cc79c Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sat, 15 Aug 2026 10:46:18 +0500 Subject: [PATCH 6/8] fix(plugin-page-builder): scope the switch margin to the zones it fits The ranking was extended to every insertion target, and two properties it depends on do not hold for three of them. A margin measured on one axis is only a constant physical width when the competing targets share an axis. Summing the axes makes a diagonal boundary cross the margin at a different rate, so a 10px credit becomes about 7.1px of travel between staggered cells; combining them under a square root fixes the direction dependence and breaks the axis alignment instead. No single distance to a point does both. Ranking every eligible target on one collision tier also discards the ordering that puts a target CONTAINING the pointer ahead of one the dragged shape merely overlaps. Beside a container of a different width, the neighbour's centre can be nearer while only the drag shape reaches it, and the drop lands in the wrong container. Both properties hold for the zones interleaved between a slot's children, because those span one container and therefore share a width and an axis: the summed metric degenerates to vertical distance, and "the pointer is inside this zone" and "this zone's centre is nearest" become the same statement. Scoping to them makes both failures unreachable rather than patched. The empty placeholder and the node-attached before/append targets return to the default ranking until a detector exists that resolves a REGION before measuring a distance, which is where the containment ordering can be kept without a tier that changes at the switch boundary. --- .../src/admin/canvas/CanvasNode.tsx | 13 ----------- .../src/admin/canvas/DropZone.tsx | 13 +++++++---- .../src/admin/canvas/collisionPolicy.ts | 23 ++++++++++++++----- 3 files changed, 25 insertions(+), 24 deletions(-) diff --git a/packages/plugin-page-builder/src/admin/canvas/CanvasNode.tsx b/packages/plugin-page-builder/src/admin/canvas/CanvasNode.tsx index 7ab1afb5b9..049b6aa5ad 100644 --- a/packages/plugin-page-builder/src/admin/canvas/CanvasNode.tsx +++ b/packages/plugin-page-builder/src/admin/canvas/CanvasNode.tsx @@ -36,7 +36,6 @@ import { dragSensors } from "../logic/dragSensors"; import { useEditor } from "../store/EditorProvider"; import { QueryLoopSamplePreview } from "./CanvasQueryLoop"; -import { insertionCollisionDetector } from "./collisionPolicy"; import { CanvasDepth, DropZone, @@ -248,10 +247,6 @@ export function CanvasNode({ node }: { node: BlockNode }): ReactNode { // Targets this node's OWN slot, so it ranks with the zones INSIDE it rather // than with its siblings — the same `depth + 1` the slot content is rendered at. collisionPriority: canvasPriority(depth + 1), - // The same ranking every insertion target uses. A formatted slot draws no - // zones, so this target and its neighbouring `before:` targets ARE that - // slot's insertion points and need the switch margin for the same reason. - collisionDetector: insertionCollisionDetector, }); const rootRef = appendSlot ? append.ref : undefined; const className = classFor( @@ -338,10 +333,6 @@ function DraggableNode({ // zones — it marks a position among its siblings and must compete with the // gap zones beside it. collisionPriority: canvasPriority(depth), - // The same ranking every insertion target uses. A formatted slot draws no - // zones, so this target and its neighbouring `before:` targets ARE that - // slot's insertion points and need the switch margin for the same reason. - collisionDetector: insertionCollisionDetector, }); // A formatted container itself: "append" target for the formatted slot it declares, since that @@ -365,10 +356,6 @@ function DraggableNode({ // same `depth + 1` the slot content is rendered at. Without that a drag // over a grid nested in a container is claimed by the container holding it. collisionPriority: canvasPriority(depth + 1), - // The same ranking every insertion target uses. A formatted slot draws no - // zones, so this target and its neighbouring `before:` targets ARE that - // slot's insertion points and need the switch margin for the same reason. - collisionDetector: insertionCollisionDetector, }); const className = classFor( diff --git a/packages/plugin-page-builder/src/admin/canvas/DropZone.tsx b/packages/plugin-page-builder/src/admin/canvas/DropZone.tsx index 4ba367c9b5..3bef62e99e 100644 --- a/packages/plugin-page-builder/src/admin/canvas/DropZone.tsx +++ b/packages/plugin-page-builder/src/admin/canvas/DropZone.tsx @@ -125,11 +125,14 @@ export function DropZone({ // is the only thing that can settle two IDENTICAL rectangles — which is // exactly what a nested container's edge gap and its parent's gap are. collisionPriority: canvasPriority(depth), - // Empty zones rank the same way, because "one per container" does not mean - // "no competitor": two adjacent containers that are both empty put their - // placeholders at the same depth, and those compete for the same pointer - // exactly as two zones in one slot do. - collisionDetector: insertionCollisionDetector, + // Only the zones interleaved between a slot's children. They all span the + // same container, so they share a width and an axis — which is what makes a + // margin measured on one axis a constant physical width for them, and makes + // "the pointer is inside this zone" and "this zone's centre is nearest" the + // same statement. Targets that hold neither property, including this + // component's own empty placeholder, keep the default ranking until a + // detector exists that resolves a REGION before it measures a distance. + collisionDetector: empty ? undefined : insertionCollisionDetector, }); if (empty) { diff --git a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts index 56aaaa3169..465bec742a 100644 --- a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts +++ b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts @@ -1,12 +1,23 @@ /** * The single place the canvas decides which insertion target wins a pointer. * - * Four droppables mark insertion points — the drop zones interleaved between a - * slot's children, the "drop here" placeholder of an empty slot, and the - * `before:` / `append:` targets a formatted slot uses instead of zones. They - * answer ONE question, so they rank by one rule and take their priority from - * one scale. Anything that computes either alongside will drift, and the drift - * is silent because both halves look correct on their own. + * Scoped to the zones INTERLEAVED between a slot's children, and the scope is + * load-bearing rather than incidental. Those zones all span the same container, + * so they share a width and an axis, and two things follow that this ranking + * depends on: a margin measured on one axis is a constant physical width for + * them, and "the pointer is inside this zone" and "this zone's centre is the + * nearest" are the same statement. + * + * The canvas's other insertion targets hold neither property. An empty slot's + * placeholder can sit beside a container of a different width; a formatted + * slot's `before:` and `append:` targets are whole blocks, arranged on either + * axis and sometimes staggered in both. For them a single distance cannot carry + * both "which target owns the pointer" and "how far is the pointer from the + * insertion line, in pixels" — a rotation-invariant metric stops the margin + * being axis-aligned, an axis-aligned one stops it being a constant width, and + * ranking every target on one tier loses the containment ordering that decides + * which container a drop belongs to. Those targets keep the default ranking + * until a detector exists that resolves a REGION before it measures a distance. * * `sortCollisions` in `@dnd-kit/abstract` compares `priority`, THEN `type`, * THEN `value`, and each of those tiers is load-bearing here: From 6a2c9894fa136ed2d854fee1a19c0914433ca604 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sat, 15 Aug 2026 12:59:55 +0500 Subject: [PATCH 7/8] test(plugin-page-builder): pin the release-to-none transition as one-way The reprieve's cutoff is a boundary, so the question is whether the indicator can chatter across it. It cannot, and the reason is worth pinning rather than arguing: the reprieve is conditioned on the target being the one currently held, so crossing outward releases it and coming back inside the band does not re-acquire it, because by then it is no longer held and the default detection is what has to admit it again. The test oscillates around the cutoff and asserts a single transition. Removing the condition on the held target makes it alternate, which the test rejects. --- .../src/admin/canvas/collisionPolicy.test.ts | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts index 756b4698c9..ca6a301282 100644 --- a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts +++ b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts @@ -474,6 +474,33 @@ describe("eligibility, which the margin depends on", () => { ).toBe(true); }); + it("does not alternate when the pointer oscillates around the cutoff", () => { + // The release-to-none transition is hysteretic ALREADY, and this pins the + // reason: the reprieve is conditioned on the target being the one currently + // held. Crossing the cutoff outward releases it, and coming back inside the + // band does NOT re-acquire it, because by then it is no longer held and the + // default detection is what has to admit it again. So the cutoff is a + // one-way edge rather than a boundary the indicator can chatter across. + // + // Simulated as the drag operation runs it: the held target is whatever the + // previous round returned. + let held = true; + const eligibility: boolean[] = []; + for (const edgeDistancePx of [9, 11, 9, 11, 9]) { + held = isInsertionTargetEligible({ + hasDefaultCollision: false, + isCurrentTarget: held, + edgeDistancePx, + bandPx: TARGET_SWITCH_BAND_PX, + }); + eligibility.push(held); + } + + // One transition, not four. An alternating sequence would be the flicker + // this reprieve exists to remove, arriving at a different distance. + expect(eligibility).toEqual([true, false, false, false, false]); + }); + it("admits anything the default detection already admits", () => { // Eligibility is never NARROWED, so no target stops claiming a pointer it // claimed before this module existed — including well outside the band. From dc545038decc8742bb0fb8f046957d7496d18255 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sat, 15 Aug 2026 13:51:07 +0500 Subject: [PATCH 8/8] fix(plugin-page-builder): govern only the run the held target belongs to Two defects with one cause: the ranking replaced the default one for every interleaved zone, and the reprieve extended a shape-granted eligibility using a pointer distance. Zones of ONE slot share a container and therefore a width and a centre, which is what makes a one-axis margin a physical width for them. Zones of two populated containers side by side share only a depth. Ranking those against each other on one tier by centre distance can put a narrow neighbour ahead of the wider container the pointer is inside, and the drop then takes the wrong parent. The ranking now governs only the run the held target belongs to; every other comparison, including acquiring a target in the first place, keeps the default detection and its containment ordering. The reprieve is now measured from the dragged shape's centre rather than the pointer. It exists to extend an eligibility the default detection granted because the SHAPE overlapped the target, so measuring it from the pointer stretches a different geometry than the one that ends: a block grabbed far from its edge leaves the pointer hundreds of pixels away, the bound is already spent when the overlap stops, and the target is released instantly and reacquired on the way back. Both decisions are pure functions rather than inline conditions, because the first version of each was a line inside the detector and neither could be reached by a test: mutating the run gate away and reverting the reprieve to the pointer both went undetected. They are caught now. Also corrects a doc line still describing the reprieve as bounded by the pointer staying within the target's width. --- .../src/admin/canvas/collisionPolicy.test.ts | 113 ++++++++++++++++++ .../src/admin/canvas/collisionPolicy.ts | 101 +++++++++++++++- 2 files changed, 210 insertions(+), 4 deletions(-) diff --git a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts index ca6a301282..bb44832182 100644 --- a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts +++ b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.test.ts @@ -28,6 +28,9 @@ import { insertionDistancePx, insertionEdgeDistancePx, isInsertionTargetEligible, + isSameInsertionRun, + governsRanking, + reprieveOrigin, } from "./collisionPolicy"; /** @@ -412,6 +415,116 @@ describe("insertionEdgeDistancePx, which bounds the reprieve", () => { }); }); +describe("which targets the margin governs", () => { + // Two populated containers side by side hold zones at the SAME depth with + // different widths and different horizontal centres. Crediting the held + // target across that boundary lets a narrow neighbour keep the pointer while + // it sits inside the wider container, and the drop takes the wrong parent. + const inA0 = { kind: "dropzone", parentId: "a", slot: "default", index: 0 }; + const inA1 = { kind: "dropzone", parentId: "a", slot: "default", index: 1 }; + const inB0 = { kind: "dropzone", parentId: "b", slot: "default", index: 0 }; + const otherSlot = { + kind: "dropzone", + parentId: "a", + slot: "aside", + index: 0, + }; + + it("treats two zones of one slot as alternatives", () => { + expect(isSameInsertionRun(inA0, inA1)).toBe(true); + }); + + it("does not treat zones of different containers as alternatives", () => { + expect(isSameInsertionRun(inA0, inB0)).toBe(false); + }); + + it("does not treat different slots of one container as alternatives", () => { + // Same parent, different slot: still not a run, because they mark positions + // in separate lists that happen to share an owner. + expect(isSameInsertionRun(inA0, otherSlot)).toBe(false); + }); + + it("refuses when either side is missing or unshaped", () => { + // A droppable with no data, or a drag with no target yet, must not be + // treated as sharing a run with everything. + expect(isSameInsertionRun(inA0, null)).toBe(false); + expect(isSameInsertionRun(undefined, inA0)).toBe(false); + expect(isSameInsertionRun({}, {})).toBe(false); + expect( + isSameInsertionRun( + { parentId: 1, slot: "default" }, + { parentId: 1, slot: "default" } + ) + ).toBe(false); + }); +}); + +describe("governsRanking", () => { + const inA0 = { kind: "dropzone", parentId: "a", slot: "default", index: 0 }; + const inA1 = { kind: "dropzone", parentId: "a", slot: "default", index: 1 }; + const inB0 = { kind: "dropzone", parentId: "b", slot: "default", index: 0 }; + + it("governs the held target itself", () => { + expect( + governsRanking({ + isCurrentTarget: true, + droppableData: inA0, + currentTargetData: inA0, + }) + ).toBe(true); + }); + + it("governs a rival in the held target's run", () => { + expect( + governsRanking({ + isCurrentTarget: false, + droppableData: inA1, + currentTargetData: inA0, + }) + ).toBe(true); + }); + + it("leaves a target in another container to the default ranking", () => { + // The wrong-parent drop. Without this, a narrow neighbour's centre can beat + // the wider container the pointer is actually inside. + expect( + governsRanking({ + isCurrentTarget: false, + droppableData: inB0, + currentTargetData: inA0, + }) + ).toBe(false); + }); + + it("leaves acquisition to the default ranking when nothing is held", () => { + expect( + governsRanking({ + isCurrentTarget: false, + droppableData: inA0, + currentTargetData: null, + }) + ).toBe(false); + }); +}); + +describe("reprieveOrigin", () => { + const pointer = { x: 900, y: 100 }; + const draggedCentre = { x: 400, y: 220 }; + + it("measures from the dragged shape when one is measured", () => { + // The eligibility being extended was granted by the SHAPE overlapping the + // target. A block grabbed far from its edge puts the pointer hundreds of + // pixels away, so a pointer-measured bound is already spent when the + // overlap stops: released instantly, reacquired on the way back, flicker. + expect(reprieveOrigin({ draggedCentre, pointer })).toBe(draggedCentre); + }); + + it("falls back to the pointer when the drag carries no shape", () => { + expect(reprieveOrigin({ draggedCentre: null, pointer })).toBe(pointer); + expect(reprieveOrigin({ draggedCentre: undefined, pointer })).toBe(pointer); + }); +}); + describe("eligibility, which the margin depends on", () => { // The margin lives in the ranking, so it can only act on targets that are // still IN the ranking. The default detection stops reporting a target once diff --git a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts index 465bec742a..ffa2d3e409 100644 --- a/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts +++ b/packages/plugin-page-builder/src/admin/canvas/collisionPolicy.ts @@ -159,6 +159,31 @@ export function insertionCollisionValue({ return -(isCurrentTarget ? distancePx - bandPx : distancePx); } +/** + * Whether two insertion targets belong to the same slot of the same container. + * + * The switch margin is a statement about ONE run of insertion points: it resists + * moving off the target already held to the one beside it. Applied between runs + * it means something else entirely — two populated containers side by side hold + * zones at the same depth with different widths and different horizontal + * centres, and crediting across that boundary lets a narrow neighbour hold the + * pointer while it sits inside the wider one, so the drop takes the wrong + * parent. Outside a run the targets are not alternatives to each other and the + * credit does not apply. + */ +export function isSameInsertionRun( + a: { parentId?: unknown; slot?: unknown } | null | undefined, + b: { parentId?: unknown; slot?: unknown } | null | undefined +): boolean { + if (!a || !b) return false; + return ( + typeof a.parentId === "string" && + a.parentId === b.parentId && + typeof a.slot === "string" && + a.slot === b.slot + ); +} + /** * Whether a target is in play at all. * @@ -203,6 +228,53 @@ export function isInsertionTargetEligible({ return isCurrentTarget && edgeDistancePx <= bandPx; } +/** + * Whether this module's ranking replaces the default one for a target. + * + * Only within the run the held target belongs to. Against a target in another + * run — a populated container beside this one, holding zones at the same depth + * with a different width and centre — this metric would rank a narrow neighbour + * ahead of the wider container the pointer is inside, and the drop would take + * the wrong parent. Those comparisons keep the default detection, which puts a + * target CONTAINING the pointer ahead of one the dragged shape merely reaches. + * Before any target exists there is nothing to be sticky about, so acquisition + * is the default's decision too. + */ +export function governsRanking({ + isCurrentTarget, + droppableData, + currentTargetData, +}: { + isCurrentTarget: boolean; + droppableData: { parentId?: unknown; slot?: unknown } | null | undefined; + currentTargetData: { parentId?: unknown; slot?: unknown } | null | undefined; +}): boolean { + return ( + isCurrentTarget || isSameInsertionRun(droppableData, currentTargetData) + ); +} + +/** + * The point the reprieve is measured from. + * + * The dragged feedback's centre where one is measured, because that is the + * geometry whose overlap granted the eligibility the reprieve extends. A block + * grabbed far from its edge puts the pointer hundreds of pixels from the target + * the shape is holding, so a pointer-based bound is already spent at the moment + * the overlap stops: the target is released instantly, reacquired on the way + * back, and flickers. The pointer is the fallback for a drag carrying no + * measured shape at all. + */ +export function reprieveOrigin({ + draggedCentre, + pointer, +}: { + draggedCentre: { x: number; y: number } | null | undefined; + pointer: { x: number; y: number }; +}): { x: number; y: number } { + return draggedCentre ?? pointer; +} + /** * Rank insertion targets by pointer distance, holding the current one across a * margin. @@ -217,8 +289,14 @@ export function isInsertionTargetEligible({ * alternates between a target and nothing — the same flicker the margin exists * to remove, arriving through eligibility instead of through ranking. * - * The incumbent's reprieve is bounded by the pointer staying within its width, - * so leaving the column, the container or the canvas still releases it. + * The reprieve is bounded by `bandPx` measured from the target's RECTANGLE, and + * measured from the DRAGGED SHAPE rather than the pointer. That pairing is the + * point: the eligibility it extends was granted by the shape overlapping the + * target, so extending it by a pointer distance stretches a different geometry + * than the one that ends. A block grabbed far from its edge puts the pointer + * hundreds of pixels from the target the shape is holding, and a pointer-based + * bound is already spent at the moment the overlap stops — the target is + * released instantly, reacquired on the way back, and flickers. */ export function createInsertionCollisionDetector( bandPx: number = TARGET_SWITCH_BAND_PX @@ -238,13 +316,28 @@ export function createInsertionCollisionDetector( const { width, height } = shape.boundingRectangle; const isCurrentTarget = dragOperation.target?.id === droppable.id; + const from = reprieveOrigin({ + draggedCentre: dragOperation.shape?.current.center, + pointer, + }); + + if ( + !governsRanking({ + isCurrentTarget, + droppableData: droppable.data, + currentTargetData: dragOperation.target?.data, + }) + ) { + return eligible; + } + if ( !isInsertionTargetEligible({ hasDefaultCollision: eligible !== null, isCurrentTarget, edgeDistancePx: insertionEdgeDistancePx({ - pointerX: pointer.x, - pointerY: pointer.y, + pointerX: from.x, + pointerY: from.y, centreX: centre.x, centreY: centre.y, width,