From 4e81480d0a1ce557a67837bbfbbb292bc408bc88 Mon Sep 17 00:00:00 2001 From: xarmian Date: Mon, 14 Sep 2026 17:15:07 -0400 Subject: [PATCH] fix(editor): block-drag global listeners defer to a frontmost viewer (BUG-2453) (#1373) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `block-drag-handle.ts` installs four WINDOW-level listeners and none of them asked whether anything was in front of the editor. Background inertness stops NEW interaction with the editor; it does not stop a gesture that already owns the pointer. So a block drag in flight when the attachment viewer opened kept running against an editor that was `inert` and invisible behind the viewer's backdrop — and the drop still landed. This is the deferred half of TASK-2430, which taught seven global owners to stand down and closed with "Deferred: svelte-dnd-action + editor block-drag global gestures." FOUR OWNERS, NOT THE TWO THE BUG NAMES. It names the mouse pair; `touchmove` and `touchend` are installed on the same line with the identical hole, so a mouse-only fix would have left every touch device unguarded. Each handler consults the shared arbitration helper — `isBlockedByModal`, never a raw lease query — with `editorView.dom` as the owner, the surface asking to act rather than `event.target`, which during a straddling gesture is whatever the pointer happens to be over. ABORT, NOT EARLY-RETURN, the choice `ItemGraph.abortPan` already makes for its captured pan. An early return leaves `dragging` latched, the block at 20% opacity, the ghost under the cursor and `pointerEvents: none` on the editor — a half-drag frozen onto a surface that is now behind a viewer, with nothing left to deliver the event that would end it. The teardown reuses `cancelDrag`, which already existed and had NO callers; unlike `endDrag` it never calls `executeMove`, so aborting cannot commit the reorder. The gates sit above the branch structure, not inside the `dragging` arm: `mouseup` and `touchend` each also open the block menu on a press that never moved, and a guard placed in the drag arm would still pop a menu onto the inert editor beneath the viewer. `isBlockedByModal` returns false on an empty lease stack, so with no viewer open every path is untouched. Each of the four owners has an inert-while-frontmost leg and the contract's empty-stack regression, and the empty-stack legs assert what ships TODAY — the trap TASK-2430 shipped and reverted twice, where a test labelled a regression actually specified a change. Codex round 2 closed two more, both in the abort's own teardown: `abortDrag` left `activeBlock` populated where `endDrag` nulls it, and the comment beside it claimed to clear two pending flags when `cancelDrag` already owned one of them. The first is consistency rather than a defect closed — no path was found that acts on a stale `activeBlock` — and the comment now says so instead of implying a bug was fixed. Six mutants, each killed by its own legs: removing any of the four gates; replacing the abort with a bare early return; and pointing the owner at a viewer-contained node. The reorder itself is proven suppressed, on both `mouseup` and `touchend`, by legs that drag the SECOND block. That detail is load-bearing: jsdom gives every element a zero `getBoundingClientRect`, so `dropPosAtY` scores all candidates alike and keeps the first — for a drag of block 1 that is the slot the node is already in, so the drop runs and changes nothing and a doc-equality assertion passes either way. An earlier revision of this change asserted exactly that and was green with the gate deleted. Dragging block 2 makes the first candidate position 0, the paragraphs swap, and the assertion has something to see; each reorder leg is paired with an empty-stack leg proving the drag still reorders with no viewer, so a future harness break cannot turn it back into a vacuous pass. Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu --- .../components/editor/block-drag-handle.ts | 98 ++++++ ...kDragHandleModalArbitration.svelte.test.ts | 302 ++++++++++++++++++ 2 files changed, 400 insertions(+) create mode 100644 web/src/lib/components/editor/blockDragHandleModalArbitration.svelte.test.ts diff --git a/web/src/lib/components/editor/block-drag-handle.ts b/web/src/lib/components/editor/block-drag-handle.ts index cae8672ce..a01a34fe9 100644 --- a/web/src/lib/components/editor/block-drag-handle.ts +++ b/web/src/lib/components/editor/block-drag-handle.ts @@ -13,6 +13,11 @@ import { Extension } from '@tiptap/core'; import type { Editor } from '@tiptap/core'; import { Plugin, PluginKey } from '@tiptap/pm/state'; import type { EditorView } from '@tiptap/pm/view'; +// The SHARED arbitration helper, never a raw lease query — the contract +// TASK-2430 set for all seven global owners it taught to stand down. This +// plugin was deferred from that task ("svelte-dnd-action + editor block-drag +// global gestures") and is the last of them; BUG-2453 is that deferral. +import { isBlockedByModal } from '$lib/a11y/viewerBackdrop'; // Side-effect imports to register tiptap command-type augmentations on // `@tiptap/core`'s `ChainedCommands` interface in this file's compilation // context. Required since TS 6 stopped propagating module augmentations @@ -825,6 +830,66 @@ export const BlockDragHandle = Extension.create({ let mouseDownOnHandle = false; let mouseStartY = 0; + /** + * Should this plugin's WINDOW-level listeners stand down? (BUG-2453) + * + * The owner is the editor surface asking to act — `editorView.dom` — + * never `event.target`, which during a straddling gesture is + * whatever the pointer happens to be over. A viewer portaled to + * `` does not contain the editor, so the helper answers true + * and these handlers decline. + * + * Returns FALSE on an empty lease stack, so with no viewer open + * every path below behaves exactly as it did before — which is the + * property the empty-stack legs of the test pin. + */ + function dragBlocked(): boolean { + return isBlockedByModal(editorView.dom); + } + + /** + * Tear a block gesture down mid-flight (BUG-2453). + * + * ABORT RATHER THAN EARLY-RETURN, the same choice `ItemGraph`'s + * `abortPan` makes for its captured pan. An early return alone + * leaves `dragging` latched, the block at 20% opacity, the ghost + * under the user's cursor and `pointerEvents: none` on the editor + * — a half-drag frozen onto a surface that is now behind a viewer, + * with nothing left to deliver the event that would have ended it. + * + * `cancelDrag` is the existing teardown and is what makes this a + * small change: it restores every visual and, unlike `endDrag`, + * never calls `executeMove`. Aborting therefore cannot commit the + * reorder — which is the actual defect here, since the drop landed + * in a document the user could not see. + * + * `mouseDownOnHandle` is cleared HERE because it is the one piece + * of gesture state `cancelDrag` does not own — it describes a + * mouse press that has not become a drag yet, and leaving it set + * would let the first mouseup after the viewer closes open a menu + * for a pointer-down the user has long forgotten. (`pendingDrag`, + * its touch counterpart, IS cleared by `cancelDrag`, so it is not + * repeated here.) + * + * `activeBlock` is nulled to match `endDrag`, which does the same + * as its last act. This is CONSISTENCY, not a defect being closed: + * no path was found that acts on a stale `activeBlock` after an + * abort, because every consumer either re-derives it (`update`, + * the wrapper `mousemove`) or is reached only through a flag this + * function has just cleared. Left populated it would still be a + * teardown that ends in a different state depending on which door + * it came through, which is how the next such path becomes a bug. + */ + function abortDrag() { + mouseDownOnHandle = false; + cancelDrag(); + activeBlock = null; + // Matches `endDrag`'s own last act. The handle is invisible + // behind the viewer either way; hiding it means the editor is + // not left mid-hover when the viewer closes. + hideHandle(); + } + handle.addEventListener('mousedown', (e) => { e.preventDefault(); e.stopPropagation(); @@ -833,6 +898,14 @@ export const BlockDragHandle = Extension.create({ }); function onMouseMoveGlobal(e: MouseEvent) { + // STRADDLE GATE (BUG-2453) — the press may have landed before + // the viewer opened. Guarded ABOVE the threshold check, so a + // press that has not yet become a drag cannot become one while + // something is in front of the editor. + if (dragBlocked()) { + if (dragging || mouseDownOnHandle) abortDrag(); + return; + } if (mouseDownOnHandle && !dragging) { if (Math.abs(e.clientY - mouseStartY) > 5) { mouseDownOnHandle = false; @@ -845,6 +918,16 @@ export const BlockDragHandle = Extension.create({ } function onMouseUpGlobal(e: MouseEvent) { + // STRADDLE GATE (BUG-2453), above the click branch and not + // inside the `dragging` one. This handler has TWO jobs: it ends + // a drag, and on a press that never moved it opens the block + // menu. A guard placed inside the drag arm would still let a + // mouseup released over a frontmost viewer pop a menu open on + // the inert editor beneath it. + if (dragBlocked()) { + if (dragging || mouseDownOnHandle) abortDrag(); + return; + } if (mouseDownOnHandle && !dragging) { // It was a click — open menu mouseDownOnHandle = false; @@ -868,6 +951,14 @@ export const BlockDragHandle = Extension.create({ }, { passive: false }); function onTouchMove(e: TouchEvent) { + // STRADDLE GATE (BUG-2453). Above the `preventDefault()` below: + // while a viewer is in front the touch belongs to it — swallowing + // the move here would break scrolling inside the thing the user + // is actually looking at. + if (dragBlocked()) { + if (dragging || pendingDrag) abortDrag(); + return; + } const touchY = e.touches[0].clientY; if (pendingDrag && !dragging) { if (Math.abs(touchY - pendingTouchY) > 8) { @@ -883,6 +974,13 @@ export const BlockDragHandle = Extension.create({ } function onTouchEnd(e: TouchEvent) { + // STRADDLE GATE (BUG-2453) — the tap-opens-menu branch below is + // this handler's version of the mouseup one, and needs the same + // gate for the same reason. + if (dragBlocked()) { + if (dragging || pendingDrag) abortDrag(); + return; + } if (pendingDrag) { // Tap on handle → open menu pendingDrag = false; diff --git a/web/src/lib/components/editor/blockDragHandleModalArbitration.svelte.test.ts b/web/src/lib/components/editor/blockDragHandleModalArbitration.svelte.test.ts new file mode 100644 index 000000000..2404ffdb6 --- /dev/null +++ b/web/src/lib/components/editor/blockDragHandleModalArbitration.svelte.test.ts @@ -0,0 +1,302 @@ +import { describe, it, expect, afterEach } from 'vitest'; +import { Editor } from '@tiptap/core'; +import StarterKit from '@tiptap/starter-kit'; +import { BlockDragHandle } from './block-drag-handle'; +import { acquire, __resetViewerBackdropForTests } from '$lib/a11y/viewerBackdrop'; + +/** + * BUG-2453 — the block-drag global listeners defer to a frontmost viewer. + * + * TASK-2430 taught seven global owners to stand down for a frontmost + * attachment viewer and DEFERRED two: svelte-dnd-action and this plugin's + * editor block-drag gestures. This is that deferral. + * + * The state is not theoretical. Background inertness stops NEW interaction with + * the editor; it does not stop a gesture that already owns the pointer. So a + * block drag in flight when the viewer opens kept running against an `inert`, + * invisible editor — and the drop still landed. + * + * FOUR owners, not the two the bug names: the mouse pair (`mousemove` / + * `mouseup`) and the touch pair (`touchmove` / `touchend`) are installed on the + * same line and had the identical hole. + * + * ACCEPTANCE IS PER-OWNER, per that task's contract: each owner proven inert + * while a viewer is frontmost, AND each with an empty-stack regression. + * + * WHAT THE EMPTY-STACK LEGS PIN, stated because that task shipped and reverted + * two tests that got this wrong: they assert TODAY'S behaviour, unchanged — not + * behaviour this change would prefer. `isBlockedByModal` returns false on an + * empty lease stack, so with no viewer every path below is untouched, and a leg + * that failed here would mean the guard had leaked into the ordinary case. + * + * Driven through a real Tiptap editor rather than mocks, like its sibling + * `blockDragHandleFreeze.svelte.test.ts`, because the handle and menu are + * imperative DOM the plugin appends. The SELECTION path arms the handle; + * the hover path needs `posAtCoords`, which jsdom lacks. + */ + +function makeEditor(element: HTMLElement): Editor { + return new Editor({ + element, + extensions: [StarterKit, BlockDragHandle], + content: '

alpha

bravo

', + editable: true, + }); +} + +function armHandle(editor: Editor, wrapper: HTMLElement, at: number): HTMLElement { + wrapper.dispatchEvent(new MouseEvent('click', { bubbles: true })); + editor.commands.setTextSelection(at); + return wrapper.querySelector('.block-drag-handle') as HTMLElement; +} + +/** Inside "alpha" (block 1) and inside "bravo" (block 2). */ +const IN_FIRST_BLOCK = 3; +const IN_SECOND_BLOCK = 10; + +/** + * WHICH BLOCK A REORDER LEG MUST DRAG, and why it is not arbitrary. + * + * jsdom gives every element a zero `getBoundingClientRect`, so `dropPosAtY` + * scores every candidate identically and keeps the FIRST it considers. For a + * drag of block 1 that is the position immediately AFTER block 1 — a move to + * where the node already is, which `executeMove` performs as a no-op. The drop + * genuinely runs and changes nothing, so a doc-equality assertion passes + * whether or not the guard fired. + * + * Dragging block 2 inverts it: the first candidate is position 0, ahead of + * block 1, so the paragraphs swap and the assertion has something to see. + * Measured both ways before this leg was written. + */ + +/** A body-portaled viewer root, exactly as `Lightbox` mounts one. */ +function mountViewer(): HTMLElement { + const root = document.createElement('div'); + root.className = 'attachment-viewer'; + root.setAttribute('role', 'dialog'); + document.body.appendChild(root); + return root; +} + +function touchEvent(type: string, clientY: number): TouchEvent { + const e = new Event(type, { bubbles: true, cancelable: true }) as unknown as { + touches: { clientY: number }[]; + } & TouchEvent; + e.touches = [{ clientY }]; + return e; +} + +describe('BUG-2453 — block-drag global listeners defer to a frontmost viewer', () => { + let editor: Editor | null = null; + let element: HTMLElement | null = null; + + afterEach(() => { + editor?.destroy(); + element?.remove(); + editor = null; + element = null; + document.querySelectorAll('.attachment-viewer').forEach((n) => n.remove()); + __resetViewerBackdropForTests(); + }); + + function setup(at: number = IN_FIRST_BLOCK) { + element = document.body.appendChild(document.createElement('div')); + editor = makeEditor(element); + const handle = armHandle(editor, element, at); + return { editor, wrapper: element, handle }; + } + + /** Press the handle and move past the 5px threshold — a drag is now in flight. */ + function startMouseDrag(handle: HTMLElement) { + handle.dispatchEvent(new MouseEvent('mousedown', { bubbles: true, clientY: 100 })); + window.dispatchEvent(new MouseEvent('mousemove', { bubbles: true, clientY: 130 })); + } + + /** + * The observable signature of a drag in flight, taken from `startDrag`'s own + * writes. Deliberately NOT a ghost selector: the ghost is an UNCLASSED + * `cloneNode` of the block styled by inline `cssText`, so it matches no + * class at all — an earlier draft of this file asserted `.block-drag-ghost` + * and that selector was null in every state, which made this predicate + * always false and the teardown assertions vacuously true. + */ + function dragIsInFlight(editor: Editor, handle: HTMLElement): boolean { + return ( + handle.classList.contains('active') && + editor.view.dom.style.pointerEvents === 'none' && + firstBlockDom(editor).style.opacity === '0.2' + ); + } + + /** The paragraph `startDrag` dims to 0.2 and every teardown path restores. */ + function firstBlockDom(editor: Editor): HTMLElement { + return editor.view.dom.querySelector('p') as HTMLElement; + } + + /** The block `startDrag` dimmed — the one whose handle was armed. */ + function draggedBlockDom(editor: Editor, nth: number): HTMLElement { + return editor.view.dom.querySelectorAll('p')[nth] as HTMLElement; + } + + /** Paragraph text in document order — the observable a reorder changes. */ + function blockText(editor: Editor): string[] { + return [...editor.view.dom.querySelectorAll('p')].map((n) => n.textContent ?? ''); + } + + /** The menu is built once at plugin init and toggled by `display`. */ + function menuIsOpen(): boolean { + const menu = document.querySelector('.block-context-menu') as HTMLElement | null; + return !!menu && menu.style.display !== 'none'; + } + + describe('mousemove — the straddling drag', () => { + it('aborts the in-flight drag when a viewer opens, leaving no half-drag behind', () => { + const { editor, handle } = setup(); + startMouseDrag(handle); + // PRECONDITION: without this the rest proves nothing — a test that + // never started a drag would pass against the unfixed code. + expect(dragIsInFlight(editor, handle)).toBe(true); + const before = JSON.stringify(editor.getJSON()); + + const bodyChildrenDuringDrag = document.body.childElementCount; + + acquire(mountViewer()); + window.dispatchEvent(new MouseEvent('mousemove', { bubbles: true, clientY: 400 })); + + // Not merely "stopped" — torn down. An early return would leave every + // one of these latched. + expect(handle.classList.contains('active')).toBe(false); + expect(editor.view.dom.style.pointerEvents).toBe(''); + expect(firstBlockDom(editor).style.opacity).toBe(''); + // The ghost is removed: one fewer body child than mid-drag, allowing + // for the viewer root this leg just mounted. + expect(document.body.childElementCount).toBe(bodyChildrenDuringDrag); + expect(JSON.stringify(editor.getJSON())).toBe(before); + }); + + it('EMPTY STACK: a drag proceeds exactly as before when no viewer exists', () => { + const { editor, handle } = setup(); + startMouseDrag(handle); + expect(dragIsInFlight(editor, handle)).toBe(true); + + window.dispatchEvent(new MouseEvent('mousemove', { bubbles: true, clientY: 400 })); + + expect(dragIsInFlight(editor, handle)).toBe(true); + }); + }); + + describe('mouseup — the drop, and the menu branch', () => { + it('does not commit the reorder when the mouseup lands under a viewer', () => { + const { editor, handle } = setup(IN_SECOND_BLOCK); + startMouseDrag(handle); + // PRECONDITION: a drag really is in flight on the SECOND block. + expect(draggedBlockDom(editor, 1).style.opacity).toBe('0.2'); + + acquire(mountViewer()); + window.dispatchEvent(new MouseEvent('mouseup', { bubbles: true, clientY: 400 })); + + // The defect this bug was filed for: the drop landed in a document + // the user could not see. + expect(blockText(editor)).toEqual(['alpha', 'bravo']); + expect(editor.view.dom.style.pointerEvents).toBe(''); + }); + + it('EMPTY STACK: the same drag DOES reorder when no viewer exists', () => { + const { editor, handle } = setup(IN_SECOND_BLOCK); + startMouseDrag(handle); + + window.dispatchEvent(new MouseEvent('mouseup', { bubbles: true, clientY: 400 })); + + // The positive half. Without this the leg above could pass because + // the drag never worked rather than because the guard stopped it. + expect(blockText(editor)).toEqual(['bravo', 'alpha']); + }); + + it('does not open the block menu on a press that never moved', () => { + const { handle } = setup(); + handle.dispatchEvent(new MouseEvent('mousedown', { bubbles: true, clientY: 100 })); + + acquire(mountViewer()); + window.dispatchEvent(new MouseEvent('mouseup', { bubbles: true, clientY: 100 })); + + // The non-drag branch of this handler. A guard placed inside the + // `dragging` arm would leave this menu open over the viewer. + expect(menuIsOpen()).toBe(false); + }); + + it('EMPTY STACK: a press that never moved still opens the menu', () => { + const { handle } = setup(); + handle.dispatchEvent(new MouseEvent('mousedown', { bubbles: true, clientY: 100 })); + window.dispatchEvent(new MouseEvent('mouseup', { bubbles: true, clientY: 100 })); + + expect(menuIsOpen()).toBe(true); + }); + }); + + describe('touchmove / touchend — the same hole on every touch device', () => { + it('does not begin a drag from a pending touch while a viewer is frontmost', () => { + const { editor, handle } = setup(); + handle.dispatchEvent(touchEvent('touchstart', 100)); + + acquire(mountViewer()); + window.dispatchEvent(touchEvent('touchmove', 140)); + + expect(handle.classList.contains('active')).toBe(false); + expect(editor.view.dom.style.pointerEvents).toBe(''); + expect(firstBlockDom(editor).style.opacity).toBe(''); + }); + + it('does not swallow the touch it declines to handle', () => { + const { handle } = setup(); + handle.dispatchEvent(touchEvent('touchstart', 100)); + + acquire(mountViewer()); + const move = touchEvent('touchmove', 140); + window.dispatchEvent(move); + + // The touch belongs to the viewer. Calling preventDefault here would + // break scrolling inside the thing the user is actually looking at. + expect(move.defaultPrevented).toBe(false); + }); + + it('does not open the block menu on a tap under a viewer', () => { + const { handle } = setup(); + handle.dispatchEvent(touchEvent('touchstart', 100)); + + acquire(mountViewer()); + window.dispatchEvent(touchEvent('touchend', 100)); + + expect(menuIsOpen()).toBe(false); + }); + + it('does not commit the reorder when the touchend lands under a viewer', () => { + const { editor, handle } = setup(IN_SECOND_BLOCK); + handle.dispatchEvent(touchEvent('touchstart', 100)); + window.dispatchEvent(touchEvent('touchmove', 140)); + expect(draggedBlockDom(editor, 1).style.opacity).toBe('0.2'); + + acquire(mountViewer()); + window.dispatchEvent(touchEvent('touchend', 400)); + + expect(blockText(editor)).toEqual(['alpha', 'bravo']); + }); + + it('EMPTY STACK: the same touch drag DOES reorder', () => { + const { editor, handle } = setup(IN_SECOND_BLOCK); + handle.dispatchEvent(touchEvent('touchstart', 100)); + window.dispatchEvent(touchEvent('touchmove', 140)); + + window.dispatchEvent(touchEvent('touchend', 400)); + + expect(blockText(editor)).toEqual(['bravo', 'alpha']); + }); + + it('EMPTY STACK: a tap on the handle still opens the menu', () => { + const { handle } = setup(); + handle.dispatchEvent(touchEvent('touchstart', 100)); + window.dispatchEvent(touchEvent('touchend', 100)); + + expect(menuIsOpen()).toBe(true); + }); + }); +});