diff --git a/src/webview/cm/table/cell-point.ts b/src/webview/cm/table/cell-point.ts index 8437fb1c..791789f5 100644 --- a/src/webview/cm/table/cell-point.ts +++ b/src/webview/cm/table/cell-point.ts @@ -129,8 +129,15 @@ declare const absoluteOffsetBrand: unique symbol; export type AbsoluteOffset = number & { readonly [absoluteOffsetBrand]: true }; /** THE constructor of an {@link AbsoluteOffset} — a cast, not a guard (see the - * brand note in cell-source-map.ts). Both call sites below mint only a value - * that `Number.isSafeInteger` has just accepted. */ + * brand note in cell-source-map.ts). It checks nothing, so the guarantee lives + * at each MINT, and a grep for `asAbsoluteOffset` is how a reader audits them. + * Deliberately not an enumeration: mints span modules — today `stampedOffset` + * (only what `Number.isSafeInteger` has just accepted), `cellPointAt` (clamped + * between two already-branded bounds), and table-widget.ts's outside-release + * seam (`view.posAtCoords`, which CodeMirror has already clamped to + * `[0, doc.length]`) — so no list written HERE can stay complete, and an + * earlier one silently did not. The grep is the check; those are examples of + * what it should find at each hit. */ export function asAbsoluteOffset(value: number): AbsoluteOffset { return value as AbsoluteOffset; } diff --git a/src/webview/cm/table/table-widget.ts b/src/webview/cm/table/table-widget.ts index 8347e77b..a1b9b7fd 100644 --- a/src/webview/cm/table/table-widget.ts +++ b/src/webview/cm/table/table-widget.ts @@ -17,6 +17,19 @@ // snapping, fall back to the same caret dispatch as a plain click. (See // cell-point.ts for the pointer→source-offset mapping and why the widget must // do this itself rather than reading the browser selection at mouseup.) +// A gesture RELEASED OUTSIDE this widget delivers no click here — the click is +// retargeted to the nearest common ancestor of the press and release targets, +// `.cm-content` — so `mousedown` also arms a DOCUMENT-level `mouseup` seam. +// Same drag test, but the head comes from `view.posAtCoords` (the release is on +// no cell, so there is nothing for the source map to answer) and an unmappable +// anchor snaps outward against it. That this widget owns listeners on the +// DOCUMENT — which outlive its own DOM, since CodeMirror can replace a widget +// root mid-gesture — is the most surprising fact about this module, and it is +// exactly why `destroy()` and the seam's `isConnected` guard exist: without +// them a discarded widget keeps answering for an editor that has forgotten it. +// The seam is armed per gesture and disarmed by four paths — the gesture's own +// PRIMARY-button release, any later press (capture phase), `dragstart`, and +// `destroy` — each with its own comment at the site. // The dispatched selection — caret or range — is what fires tableBlockField's // line-level reveal-on-caret, surfacing the source for editing. // @@ -37,6 +50,8 @@ import { type Align, type Cell, type Table, tableAlign } from "../../../markdown import { quollResourceBaseUri } from "../image/resource-base.js"; import { quollOpenExternalSink } from "../open-external.js"; import { + type AbsoluteOffset, + asAbsoluteOffset, type CellPoint, cellPointAt, quollTableCaretResolver, @@ -51,16 +66,46 @@ import { renderCellInto } from "./cell-render.js"; // collapsed caret lands — a regression of the existing click contract. const DRAG_THRESHOLD_PX = 4; -/** Where a drag started, remembered between `mousedown` and `click`. +/** A pointer position RELATIVE TO THE CONTENT, not to the viewport. The content + * can move under a held pointer — a scroll mid-gesture, a `scrollIntoView` + * from an incoming host message, CodeMirror's own scrolling — and a pointer + * that stayed still through one of those HAS crossed text. Two `clientX/Y` + * pairs cannot see that, and called the gesture a click. + * + * Relative to `contentDOM`'s rect rather than `scrollDOM`'s scroll offsets so + * that both operands are VISUAL pixels: CodeMirror supports being CSS + * transformed (`view.scaleX` / `scaleY` exist for exactly that), and mixing a + * viewport coordinate with a layout-space scroll offset breaks the threshold + * under any scale but 1. Measuring both ends the same way makes the scale + * cancel instead of needing a correction. + * + * Named `contentX`/`contentY` rather than `x`/`y` so the two frames differ by + * SHAPE, not merely by this comment: a viewport point and a content point are + * both pairs of numbers, so a structural `{x, y}` lets + * `view.posAtCoords(contentPoint(view, event))` — a viewport API handed a + * content point — compile silently, and the bug it produces (a threshold that + * misreads travel whenever the editor is scrolled) is invisible until someone + * scrolls. This is cell-source-map.ts's "every crossing between spaces is said + * out loud" policy applied to coordinates; the rename alone makes each + * mis-pairing a missing-property error, so no `unique symbol` is minted. */ +interface ContentPoint { + readonly contentX: number; + readonly contentY: number; +} + +/** Where a drag started, remembered between `mousedown` and whichever seam ends + * the gesture — the root's `click` for a release INSIDE this widget, the + * document `mouseup` for a release outside. Both terminating seams read the + * entry and delete it; naming only the click would tell a reader that a + * gesture ending outside the widget cannot consult it, when those are exactly + * the gestures the outside-release seam exists for. * * Keyed on the widget's root ELEMENT rather than held in the `toDOM` closure * because `updateDOM` reuses that element across widget instances: the entry * has to be invalidated exactly when the cell stamps move, which is something * `updateDOM` can do and the closure cannot. A WeakMap so a discarded widget * root takes its entry with it. */ -interface PendingDrag { - readonly x: number; - readonly y: number; +interface PendingDrag extends ContentPoint { readonly point: CellPoint | null; } const pendingDrag = new WeakMap(); @@ -79,6 +124,15 @@ const pendingDrag = new WeakMap(); * rationale, as image-widget.ts's `blockStart`. */ const blockStart = new WeakMap(); +/** Aborts the document-level listeners armed for the gesture in flight on this + * root. Kept OUT of `PendingDrag` because the two have different lifetimes: + * `updateDOM` drops the pending anchor when a doc edit invalidates it while the + * gesture is still physically in progress, and that release must still be heard + * (it dispatches the block-start caret). A WeakMap so a discarded root takes its + * controller with it — though the listeners are removed by the gesture's own end + * and by `destroy`, not left to garbage collection. */ +const armedRelease = new WeakMap(); + /** Margin-click caret: the block start this root currently points at. * * Falling back to the toDOM-time `widget.docFrom` totalizes the @@ -126,34 +180,111 @@ function dispatchSelection(view: EditorView, selection: { anchor: number; head?: } } +/** THE constructor of a {@link ContentPoint} — the one sanctioned crossing from + * the viewport frame into the content frame, mirroring `asAbsoluteOffset`'s + * role in cell-point.ts. */ +function contentPoint(view: EditorView, event: MouseEvent): ContentPoint { + const origin = view.contentDOM.getBoundingClientRect(); + return { contentX: event.clientX - origin.left, contentY: event.clientY - origin.top }; +} + +/** Manhattan pointer travel since the press, over the CONTENT. One measurement + * in one frame, so a pointer that follows moving content cancels out and stays + * a click. Manhattan (not Euclidean) to match the threshold the suite pins. + * + * Takes the press as a bare {@link ContentPoint} rather than the whole + * `PendingDrag`: the anchor's cell mapping is no business of a distance + * measurement, and the narrower parameter says so in the signature. */ +function travelSince(view: EditorView, pressedAt: ContentPoint, event: MouseEvent): number { + const now = contentPoint(view, event); + return Math.abs(now.contentX - pressedAt.contentX) + Math.abs(now.contentY - pressedAt.contentY); +} + +/** A `PendingDrag` whose press landed on a cell — the only kind either seam can + * build a range from. */ +interface ArmedDrag extends PendingDrag { + readonly point: CellPoint; +} + +/** Pure type guard: false ⟺ not an `ArmedDrag`. Sound in BOTH arms, so the + * compiler's negative inference stays true however it is called. + * + * Named for the PRESS rather than for "armed", which everywhere else in this + * module means "a gesture is in flight on this root" (`armedRelease`, + * `disarm`, the header's four disarm paths). Those two senses come apart: a + * press on the widget's padding stores an entry with `point: null` AND arms the + * release seam — armed in the dominant sense — while this predicate answers + * false. The type name `ArmedDrag` reads as a compound noun at its use sites; + * the bare predicate is where a reader meets the word alone. */ +function pressedOnCell(pending: PendingDrag | null): pending is ArmedDrag { + return pending !== null && pending.point !== null; +} + +/** The armed anchor this completed gesture is a DRAG from, or `null` when it is + * a click, a keyboard/programmatic activation, or nothing this widget armed. + * ONE definition for both seams, so the click and the outside release can never + * disagree about what a drag is. + * + * `detail === 0` means the event was NOT produced by a pointer gesture — + * keyboard activation of an in-cell ``, or a programmatic `.click()` / + * `dispatchEvent`. Its clientX/Y are 0, so pairing it with an armed anchor + * would read a large bogus travel and dispatch a range the user never drew. + * + * Returns a VALUE rather than asserting `pending is ArmedDrag`: two of the + * three gates are properties of `event`, not of `pending`, so a predicate would + * license tsc to conclude "this press did not land on a cell" from "this + * gesture did not travel far enough" — an unsound negative inference that + * happens to be harmless only because both call sites pass the wide type + * today. Handing back the narrowed anchor removes the false arm entirely + * instead of relying on that. */ +function armedDragFor( + view: EditorView, + event: MouseEvent, + pending: PendingDrag | null +): ArmedDrag | null { + if (!pressedOnCell(pending) || event.detail === 0) { + return null; + } + return travelSince(view, pending, event) >= DRAG_THRESHOLD_PX ? pending : null; +} + +/** A NON-COLLAPSED selection range in ABSOLUTE document space — the only thing + * either drag seam produces, and the reason both end in an explicit collapse + * check: a zero-width result degrades to the caret path instead of being + * dispatched as a range. + * + * Shared by `dragRange` and `releaseRange` rather than named for just one of + * them: the two differ only in where the HEAD comes from (a cell's source map + * vs. `view.posAtCoords`), while the contract they hand the caller — absolute + * space, anchor ≠ head, `null` means "dispatch the caret instead" — is + * identical. Naming it once means a third seam inherits that contract rather + * than re-deriving it, and picking either producer to own the name would leave + * the next author guessing which one to copy. */ +interface DragSelection { + readonly anchor: AbsoluteOffset; + readonly head: AbsoluteOffset; +} + /** The RANGE a completed pointer gesture describes, or `null` when this gesture - * is not a drag at all (no armed anchor, unmappable anchor, keyboard / - * programmatic click, sub-threshold travel), when the head has no mapping, or + * is not a drag at all (see `armedDragFor`), when the head has no mapping, or * when the range collapses after snapping. Every `null` answer means the same - * thing to the caller: dispatch the plain collapsed caret instead. */ + * thing to the caller: dispatch the plain collapsed caret instead. + * + * The aborted-gesture window this used to close through `detail === 0` is now + * ALSO closed structurally: the release seam is one-shot on the gesture's own + * PRIMARY-button release — a non-primary mouseup returns BEFORE the abort, so + * a right-button click while the left is held cannot end a gesture it never + * belonged to, and a release inside the root leaves the anchor armed for the + * click seam that owns it — and any press elsewhere disarms a gesture whose + * release never arrived. */ function dragRange( view: EditorView, root: HTMLElement, event: MouseEvent, pending: PendingDrag | null -): { anchor: number; head: number } | null { - if (pending === null || pending.point === null) { - return null; - } - // `detail === 0` means this click was NOT produced by a pointer gesture — - // keyboard activation of an in-cell ``, or a programmatic `.click()`. Its - // clientX/Y are 0, so pairing it with an armed anchor would read a large - // bogus travel and dispatch a range the user never drew. This is also what - // closes the aborted-gesture window: a press released OUTSIDE the widget - // delivers no click to `root`, leaving the entry armed, and the only clicks - // that can then reach the handler without a fresh mousedown of their own are - // keyboard/programmatic ones (a click retargeted to a common ancestor above - // the widget never runs that listener at all). - if (event.detail === 0) { - return null; - } - const travel = Math.abs(event.clientX - pending.x) + Math.abs(event.clientY - pending.y); - if (travel < DRAG_THRESHOLD_PX) { +): DragSelection | null { + const armed = armedDragFor(view, event, pending); + if (armed === null) { return null; } const head = cellPointAt( @@ -165,7 +296,7 @@ function dragRange( if (head === null) { return null; } - const start = pending.point; + const start = armed.point; if (start.cellFrom === head.cellFrom) { // ONE cell. An unmappable end carries no direction here: a rendered offset // beside a construct that renders no text measures the SAME on both sides @@ -203,6 +334,79 @@ function dragRange( return from === to ? null : { anchor: from, head: to }; } +/** The RANGE a gesture RELEASED OUTSIDE this widget describes, or `null` when it + * is not a drag, when the press was not on a cell, or when the editor cannot + * place the release point — every one of which the caller answers with the + * collapsed caret, the same degrade the click seam uses. + * + * The head comes from `view.posAtCoords` rather than from `cellPointAt`: the + * release is outside the widget by construction, so there is no cell to map and + * the editor's own coordinate lookup IS the answer. It returns `null` for a + * point it cannot resolve (an unrendered block, a viewport gap) — but NOT for + * an overshoot: past the last line it clamps to `doc.length`, and above the + * first to `0` (@codemirror/view 6.43.0), which is why a release below the + * document still draws a range to the end rather than degrading. + * + * Direction comes from comparing that document position with the cell, for the + * same reason the across-cells arm of `dragRange` uses cell order: an + * unmappable anchor has no offset to compare, and it snaps OUTWARD — away from + * the release — so the range still covers the cell the pointer started in. */ +function releaseRange( + view: EditorView, + event: MouseEvent, + pending: PendingDrag | null +): DragSelection | null { + const armed = armedDragFor(view, event, pending); + if (armed === null) { + return null; + } + let raw: number | null; + try { + raw = view.posAtCoords({ x: event.clientX, y: event.clientY }); + } catch (err) { + // Same contract as `dispatchSelection`'s catch: a failed coordinate lookup + // must cost the gesture, not the editor. + // + // The payload carries what it takes to REPRODUCE the degrade, for the reason + // `blockStartCaret` logs `slice` — a document can hold many tables, and the + // error alone would not say which one, nor where the pointer was. It matters + // more here than at either sibling site because `posAtCoords` has a + // SYSTEMATIC throw mode, not only a teardown one: it calls `readMeasured()`, + // which throws "Reading the editor layout isn't allowed during an update" + // whenever `updateState === Updating` (@codemirror/view 6.43.0) — and a DOM + // mouseup is exactly the thing an in-progress update can deliver. Every such + // drag silently collapses to a caret, so the log is the only trace. + console.error("[quoll] table widget release lookup failed", { + cellFrom: armed.point.cellFrom, + x: event.clientX, + y: event.clientY, + err, + }); + return null; + } + if (raw === null) { + return null; + } + // The crossing into absolute document space, minted here for the reason + // cell-point.ts mints at its own legal crossings: an entry into this space is + // said out loud rather than inferred. CodeMirror answers a real document + // position — clamped to `[0, doc.length]`, never a fraction — so there is + // nothing further to validate, only to name. + // + // NOT a whole-module guarantee, and a grep for `asAbsoluteOffset` will not + // find one. What IS branded end to end is `DragSelection`: both producers hand + // back minted ends. The CARET path is not — `docFrom`, `blockStart`, + // `blockStartCaret` and `dispatchSelection`'s own signature carry absolute + // offsets as plain `number` and reach `view.dispatch` unbranded, exactly as + // they did BEFORE this seam existed (nothing here widened them). Finishing + // that path is tracked separately: it changes `dispatchSelection`, the sink + // BOTH seams share, so it is not a local edit. + const head = asAbsoluteOffset(raw); + const start = armed.point; + const anchor = start.offset ?? (head > start.cellFrom ? start.cellFrom : start.cellTo); + return anchor === head ? null : { anchor, head }; +} + export class TableBlockWidget extends WidgetType { constructor( readonly table: Table, @@ -263,9 +467,11 @@ export class TableBlockWidget extends WidgetType { root.appendChild(table); // Two root listeners, one gesture. `mousedown` only ARMS the gesture - // (anchor point + coordinates); `click` — which fires after mouseup and - // already owns the caret dispatch and the modifier-link path — is the sole - // dispatch seam. + // (anchor point + coordinates) and arms the document-level release seam + // below; `click` — which fires after mouseup and already owns the caret + // dispatch and the modifier-link path — is the dispatch seam for a gesture + // RELEASED INSIDE this root. A release outside never delivers a click here, + // so that case is dispatched by the document `mouseup` seam instead. // // Deliberately NOT preventDefault'ed: the native mousedown default is what // moves focus into CodeMirror's contenteditable, and without focus the @@ -284,8 +490,10 @@ export class TableBlockWidget extends WidgetType { // No dispatch here: dispatching would fire the reveal mid-drag and pull // the widget out from under the pointer. pendingDrag.set(root, { - x: event.clientX, - y: event.clientY, + ...contentPoint(view, event), + // `cellPointAt` keeps taking VIEWPORT coordinates — it feeds + // `caretPositionFromPoint`, which is a viewport API. Only the travel + // measurement changes frame. point: cellPointAt( root, event.clientX, @@ -293,10 +501,135 @@ export class TableBlockWidget extends WidgetType { view.state.facet(quollTableCaretResolver) ), }); + + // The SECOND dispatch seam. A gesture released outside this root never + // delivers a `click` here — measured in real Chromium: the click is + // retargeted to `.cm-content`, the nearest common ancestor of the press + // and release targets — so the release has to be heard on the document, + // where every mouseup lands. Armed per gesture rather than kept + // permanently, so the listener that can dispatch is exactly the one + // belonging to the press in flight. Any controller left over from a + // previous press is aborted first, so at most one is ever armed. + armedRelease.get(root)?.abort(); + const release = new AbortController(); + armedRelease.set(root, release); + const doc = root.ownerDocument; + const armingPress = event; + /** Stand this gesture down: nothing more can dispatch for it, and the + * armed anchor must not survive to be paired with a release that belongs + * to someone else. ONE definition for the two disarm listeners below so + * they cannot drift apart. `destroy` — the FOURTH path in the header's + * list — does not call this and does strictly more (it clears + * `armedRelease` too), because there the root itself is going away rather + * than just this gesture. + * + * ⚠️ The `mouseup` seam below deliberately does NOT call this — it aborts + * and leaves `pendingDrag` alone, because a release INSIDE the root has + * to leave the anchor armed for the click listener that owns it. Swapping + * its `release.abort()` for `disarm()` would look tidier and would break + * every inside-released drag. */ + const disarm = (): void => { + release.abort(); + pendingDrag.delete(root); + }; + doc.addEventListener( + "mouseup", + (up: MouseEvent) => { + // BEFORE the abort, not after: a right-button press-and-release while + // the left button is held delivers a mouseup this gesture did not end. + // Aborting first would leave the real release with no listener. + if (up.button !== 0) { + return; + } + release.abort(); // one-shot: this gesture is over either way + // The root left the document mid-gesture — CodeMirror rebuilds a + // widget by REPLACING its root, and a detached root's stamps have + // stopped tracking the document. The click seam never had to check + // (a detached root receives no clicks); a document-level one does. + if (!root.isConnected) { + return; + } + // Released INSIDE: the click WILL reach this root, and it owns the + // dispatch — including the modifier-link `open-external` branch, which + // stays exactly where it was. Leave `pendingDrag` armed for it. + if (root.contains(up.target as Node | null)) { + return; + } + const pending = pendingDrag.get(root) ?? null; + pendingDrag.delete(root); + dispatchSelection( + view, + releaseRange(view, up, pending) ?? { + anchor: pending?.point?.cellFrom ?? blockStartCaret(root, this), + } + ); + }, + // CAPTURE, for the same reason the disarm below is: this seam must not + // be starvable by a sibling widget that stops `mouseup`. Bubble rested + // on "none of today's siblings stops mouseup" — an enumeration, which is + // the class of assumption the disarm's own comment rejects — and the + // failure would be silent, because a starved seam dispatches nothing, + // which is precisely the pre-fix behaviour. + // + // Capture also moves this ahead of CodeMirror's own document `mouseup` + // (`MouseSelection.up`), and that reordering is a no-op HERE by + // construction, not by luck: `eventBelongsToEditor` walks from the event + // target up to `contentDOM` and bails at any widget whose + // `ignoreEvent()` is true (@codemirror/view 6.43.0), so this widget's + // mousedown never reaches CM's `handlers.mousedown`, no `MouseSelection` + // is constructed, and its constructor is the ONLY thing that registers + // that document listener. For a gesture armed in this widget, CM has no + // document mouseup listener to be ordered against. + { signal: release.signal, capture: true } + ); + // ⚠️ The guard that makes the seam safe rather than merely useful. + // + // A release this document never sees — the pointer leaves the webview + // iframe, focus is lost, Cmd+Tab — leaves the listener above armed, and + // the user's NEXT unrelated release would be read as this gesture's end: + // a range from a table cell to a point nobody dragged to. A press is the + // one thing that must precede any such release, so disarming here covers + // every focus-loss path, including the ones nobody enumerated. + // + // CAPTURE, and that is the load-bearing part. Four sibling widgets in this + // editor call stopPropagation() on mousedown — the task checkbox, the + // fenced-code copy and collapse buttons, the language picker — and NONE of + // them stops mouseup. A bubble-phase disarm is therefore starved by + // exactly those presses while the release still arrives, which is the one + // combination that dispatches a range the user never drew (measured + // against an element that stops mousedown: a bubble-phase listener + // fired 0 times, a capture-phase listener 1; the mouseup reached the + // document either way). Capture runs document → target, so nothing + // downstream can starve it. + // + // The identity check guards the other direction: this listener is added + // DURING the dispatch of the arming press. In capture that press has + // already passed the document, so it cannot reach here — but the check + // costs one line, says out loud what must stay true, and keeps the guard + // correct if the phase is ever changed back. Comparing the event OBJECT, + // not the target, which a second press in the same cell would match too. + doc.addEventListener( + "mousedown", + (down: MouseEvent) => { + if (down === armingPress || down.button !== 0) { + return; + } + disarm(); + }, + { signal: release.signal, capture: true } + ); + // A native drag-and-drop ends in `dragend`, NOT in a mouseup. Measured: a + // plain cell drag starts no DnD at all, so this is for a press that begins + // on an in-cell or , both natively draggable. Nothing is + // preventDefault'ed — the selection seam simply stands down, because a + // drag-and-drop is not a text selection. + doc.addEventListener("dragstart", disarm, { signal: release.signal, capture: true }); }); - // Root click handler — the sole dispatch seam for the caret/range contract - // described at the top of this file. + // Root click handler — the dispatch seam for a gesture released INSIDE this + // root (the caret/range contract described at the top of this file); its + // outside-release counterpart is the document `mouseup` seam armed in the + // `mousedown` listener above. // // Modifier-click on a live `` (external nav — cell-render left it // un-preventDefault'd because the href is absolute AND within @@ -439,9 +772,11 @@ export class TableBlockWidget extends WidgetType { blockStart.set(dom, this.docFrom); // Pure positional shift: the bytes are identical (from.slice === this.slice) // and only the absolute offsets moved. Re-stamp data-cell-from on each cell - // and reuse the rendered inline children verbatim — skip patchRow's - // textContent="" + renderCellInto re-tokenize (its own design comment, - // :16-18). This is the hot path when typing in a paragraph ABOVE the table. + // and reuse the rendered inline children verbatim — skip `patchRow`'s + // `renderCellInto` call, which clears the cell and re-tokenizes it + // (cell-render.ts's header: `renderCellInto` is the only supported way to + // fill a cell, so clearing is ITS job, not `patchRow`'s). This is the hot + // path when typing in a paragraph ABOVE the table. if (from.slice === this.slice) { this.stampRow(headerRows[0], this.table.header.cells); for (let rowIdx = 0; rowIdx < this.table.rows.length; rowIdx++) { @@ -449,8 +784,11 @@ export class TableBlockWidget extends WidgetType { } return true; } - // Content edit (slice changed): full re-render. patchRow re-stamps cellFrom - // itself (:198), so offsets stay correct on this path too. + // Content edit (slice changed): full re-render. `patchRow` re-stamps + // `data-cell-from` / `data-cell-to` itself before each `renderCellInto`, so + // offsets stay correct on this path too. (Named by symbol rather than by + // line: bare `:NNN` refs in this file rotted twice as the gesture seams + // grew above them.) const resourceBase = view.state.facet(quollResourceBaseUri); const align = tableAlign(this.table); this.patchRow(headerRows[0], this.table.header.cells, align, resourceBase); @@ -507,4 +845,16 @@ export class TableBlockWidget extends WidgetType { ignoreEvent(): boolean { return true; } + + /** CodeMirror's documented teardown for a widget instance. The gesture + * listeners live on the DOCUMENT and close over `root`, so without this a + * widget destroyed mid-gesture keeps both the listeners and the DOM alive — + * and the listeners keep answering for an editor that has forgotten them. + * The `isConnected` guard in the release seam is not a substitute: `destroy` + * can be called while the DOM is still in the tree. */ + destroy(dom: HTMLElement): void { + armedRelease.get(dom)?.abort(); + armedRelease.delete(dom); + pendingDrag.delete(dom); + } } diff --git a/test/webview-browser/helpers/table-drag-harness.ts b/test/webview-browser/helpers/table-drag-harness.ts index 2bb0c2b8..94f55c83 100644 --- a/test/webview-browser/helpers/table-drag-harness.ts +++ b/test/webview-browser/helpers/table-drag-harness.ts @@ -38,6 +38,9 @@ export const PLAIN = DOC.indexOf("plain"); /** Source offset of the table block's first byte — the caret the widget falls * back to when a gesture maps to no cell at all (`blockStartCaret`). */ export const TABLE_BLOCK_START = DOC.indexOf("| Alpha"); +/** Source offset of the paragraph AFTER the table — the release point for the + * gesture "select the table plus the paragraph below". */ +export const TAIL = DOC.indexOf("tail"); /** Production extension order for the table island (editor.ts: skeleton field * BEFORE the block field). Caret parked at doc end so the line-level reveal is @@ -82,6 +85,16 @@ export function widgetRoot(v: EditorView): HTMLElement { return root as HTMLElement; } +/** The rendered `.cm-line` holding `text`, as ordinary editable prose OUTSIDE + * the table widget — the release target for a drag that leaves the widget. */ +export function proseLine(v: EditorView, text: string): HTMLElement { + const line = [...v.contentDOM.querySelectorAll(".cm-line")].find( + (l) => l.textContent === text + ); + expect(line, `prose line "${text}" must be rendered`).toBeDefined(); + return line as HTMLElement; +} + export function cellByText(v: EditorView, text: string): HTMLElement { const cell = [...widgetRoot(v).querySelectorAll("th, td")].find( (c) => c.textContent === text diff --git a/test/webview-browser/table-drag-selection.browser.test.ts b/test/webview-browser/table-drag-selection.browser.test.ts index ce5c9172..b519c09c 100644 --- a/test/webview-browser/table-drag-selection.browser.test.ts +++ b/test/webview-browser/table-drag-selection.browser.test.ts @@ -14,8 +14,14 @@ // rectangles measured with a DOM Range, and the facet's real caret-from-point // resolvers. // -// Both caret-from-point arms run, because there are two floor-dependent ones -// (see `ARMS` below), not one. +// Every contract inside the `describe.each(ARMS)` below runs TWICE, once per +// caret-from-point arm: the default (`caretPositionFromPoint`, what this +// runner's Chromium takes) and the `caretRangeFromPoint` fallback, which is the +// one arm LIVE on the extension's floor and would otherwise never be exercised +// here. See `ARMS` below. The one exception is the focus contract in the final +// `describe`, which sits OUTSIDE that block and mounts with no arm: what it pins +// is the native mousedown default, which no caret-from-point API takes part in, +// so a second run would assert the same thing twice. import type { Extension } from "@codemirror/state"; import type { EditorView } from "@codemirror/view"; import { afterEach, describe, expect, it } from "vitest"; @@ -33,8 +39,10 @@ import { PLAIN, pointAtChar, pointInWidgetPadding, + proseLine, revealed, TABLE_BLOCK_START, + TAIL, unmount, widgetRoot, } from "./helpers/table-drag-harness.js"; @@ -186,6 +194,47 @@ describe.each(ARMS)("table drag selection — trusted pointer, %s", (_name, arm) expect(revealed(view), "the fallback caret still reveals this table").toBe(true); }); + it("a drag from a cell RELEASED over the paragraph below spans from the cell into the prose", async () => { + // The gesture that was silently lost. Measured before the fix: the release + // lands on a `.cm-line`, the click is retargeted to `.cm-content` so the + // root's click listener never runs, and CodeMirror's own observer parks a + // COLLAPSED CARET at the release position. That last detail is why the + // emptiness assertion comes first: today's caret already sits at TAIL + 2, + // so asserting the head alone would pass without the seam existing. + view = mount(arm); + await settled(); + const cell = cellByText(view, "gamma"); + const tail = proseLine(view, "tail"); + await dragPointer(cell, pointAtChar(cell, 1), tail, pointAtChar(tail, 2)); + await settled(); + + const sel = view.state.selection.main; + expect(sel.empty, "the gesture must land a RANGE, not the caret it lands today").toBe(false); + expect(sel.anchor, "anchor is the pressed cell's source offset").toBe(GAMMA + 1); + expect(sel.head, "head is the release position in the prose below").toBe(TAIL + 2); + expect(revealed(view), "and the range still fires the table's reveal").toBe(true); + }); + + it("a drag released BELOW the last line runs to the end of the document", async () => { + // The commonest live overshoot, and the one the unit suite cannot see: + // `posAtCoords` clamps a point past the document to `doc.length` rather than + // answering null (@codemirror/view 6.43.0), so this is a real range, not a + // degrade. Released well below the editor's own box. + view = mount(arm); + await settled(); + const cell = cellByText(view, "gamma"); + const box = view.dom.getBoundingClientRect(); + await dragPointer(cell, pointAtChar(cell, 1), document.body, { + x: box.left + box.width / 2, + y: box.bottom + 40, + }); + await settled(); + + const sel = view.state.selection.main; + expect(sel.anchor).toBe(GAMMA + 1); + expect(sel.head, "clamped to the document end, not refused").toBe(view.state.doc.length); + }); + it("a press and release at the SAME point stays a click: collapsed caret at the cell start", async () => { // Non-vacuity control for every drag above: the ranges they assert come // from pointer TRAVEL past DRAG_THRESHOLD_PX, not from any click reaching diff --git a/test/webview/table/cm-table-widget-drag.test.ts b/test/webview/table/cm-table-widget-drag.test.ts index 6ae5df38..ebdb8ff9 100644 --- a/test/webview/table/cm-table-widget-drag.test.ts +++ b/test/webview/table/cm-table-widget-drag.test.ts @@ -246,9 +246,15 @@ describe("TableBlockWidget drag-selection", () => { expect(opened).toEqual(["https://example.com"]); expect(dispatched).toEqual([]); // The NEXT click, far away and with no mousedown of its own, must not - // resurrect the cleared anchor. If it leaked, this click sees moved=true - // with a non-null point (link cell → offset null → whole-cell snap) and - // dispatches a RANGE spanning the cell instead of the caret below. + // resurrect the cleared anchor. Which mechanism reddens the row matters, + // because it is NOT the assertion below: with the anchor leaked, + // `armedDragFor` sees 390px of travel against a non-null point and hands + // `dragRange` an armed anchor, `dragRange` then resolves the HEAD through + // `cellPointAt` — a SECOND resolver call this vehicle's one-step script + // cannot answer. The fixture records "resolver call #2 ran off the end of a + // 1-step script" and returns null, so `head === null`, `dragRange` returns + // null, a caret is dispatched and the expectation below PASSES. The leak + // surfaces only through `drainResolverFailures` in the shared `afterEach`. press(dom.querySelector("td") as HTMLElement, "click", 400, 10); expect(dispatched).toEqual([ { selection: { anchor: src.indexOf("[x](https://example.com)") } }, @@ -451,12 +457,74 @@ describe("TableBlockWidget drag-selection", () => { ]); }); - // Aborted-gesture guard. A press released OUTSIDE the widget delivers no - // click to the root, so the armed anchor survives with stale coordinates. - // The only click that can then reach this handler without a mousedown of its - // own is a keyboard/programmatic one — `detail === 0`, clientX/Y 0 — which - // would otherwise read a huge bogus travel and dispatch a range the user - // never drew. It must take the caret path instead. + // (2) of the TODO entry. Travel was measured between two VIEWPORT points, so + // a gesture the CONTENT moved under — a scroll mid-drag, a host-driven + // scrollIntoView, CodeMirror's own scrolling — measured ~0 and was judged a + // plain click. The pointer moved relative to the TEXT, which is the only + // space the gesture means anything in. + it("a drag the content scrolled under is a drag, even with a stationary pointer", () => { + const dispatched: unknown[] = []; + const { mount, scrollContentBy } = stubViewWithCaret(dispatched, [ + { text: "alpha", offset: 2 }, + { text: "alpha", offset: 5 }, + ]); + const dom = mount(makeWidget(SRC)); + const td = dom.querySelectorAll("td")[0] as HTMLElement; + press(td, "mousedown", 30, 30); + scrollContentBy(0, 40); // the text moved 40px under a pointer that did not + press(td, "click", 30, 30); + expect(dispatched).toEqual([ + { selection: { anchor: SRC.indexOf("alpha") + 2, head: SRC.indexOf("alpha") + 5 } }, + ]); + }); + + // The mirror image, and the reason this is ONE measurement in the content's + // frame rather than two gates added together: a pointer that follows the + // scroll exactly has not moved over the text at all, so it is still a click. + // A scroll-aware threshold that summed magnitudes would call this a drag. + it("a pointer that tracks the scrolling content exactly is still a click", () => { + const dispatched: unknown[] = []; + const { mount, scrollContentBy } = stubViewWithCaret(dispatched, [ + { text: "alpha", offset: 2 }, + { text: "alpha", offset: 5 }, + ]); + const dom = mount(makeWidget(SRC)); + const td = dom.querySelectorAll("td")[0] as HTMLElement; + press(td, "mousedown", 30, 30); + scrollContentBy(0, 40); + press(td, "click", 30, -10); // followed the text up by exactly 40px + expect(dispatched).toEqual([{ selection: { anchor: SRC.indexOf("alpha") } }]); + }); + + it("horizontal content movement counts too", () => { + const dispatched: unknown[] = []; + const { mount, scrollContentBy } = stubViewWithCaret(dispatched, [ + { text: "alpha", offset: 2 }, + { text: "alpha", offset: 5 }, + ]); + const dom = mount(makeWidget(SRC)); + const td = dom.querySelectorAll("td")[0] as HTMLElement; + press(td, "mousedown", 30, 30); + scrollContentBy(25, 0); + press(td, "click", 30, 30); + expect(dispatched).toEqual([ + { selection: { anchor: SRC.indexOf("alpha") + 2, head: SRC.indexOf("alpha") + 5 } }, + ]); + }); + + // Non-gesture activation. A keyboard or programmatic click — `detail === 0`, + // clientX/Y 0 — can reach this handler while an anchor is still armed (a + // press whose release the document never saw: the pointer left the webview + // iframe, focus was lost, Cmd+Tab), and would then read a huge bogus travel + // and dispatch a range the user never drew. It must take the caret path + // instead. + // + // ⚠️ Do NOT re-justify this row with "a press released OUTSIDE the widget + // leaves the anchor armed" — that window is closed structurally now: the + // outside-release seam deletes `pendingDrag` itself, and any later press + // disarms what a lost release did not (cm-table-widget-release.test.ts). + // What `detail === 0` still guards is the genuine non-pointer activation + // above. it("a detail-0 click (keyboard / programmatic) never takes the drag path", () => { const dispatched: unknown[] = []; const { mount } = stubViewWithCaret(dispatched, [ diff --git a/test/webview/table/cm-table-widget-release.test.ts b/test/webview/table/cm-table-widget-release.test.ts new file mode 100644 index 00000000..7abf1419 --- /dev/null +++ b/test/webview/table/cm-table-widget-release.test.ts @@ -0,0 +1,577 @@ +// @vitest-environment happy-dom +// The OUTSIDE-RELEASE seam: a drag that starts in a rendered cell and is +// released outside the widget root. Per UI-events a `click` is delivered to the +// nearest common ancestor of the mousedown and mouseup targets — MEASURED in +// real Chromium, not inferred: the release lands on a `.cm-line` and the click +// on `.cm-content`, so the root's click listener never runs and the gesture was +// dispatched as nothing. table-widget.ts therefore also listens for `mouseup` +// on the document, and that listener dispatches ONLY when the release landed +// outside the root; an inside release is left to the click listener, which is +// where the modifier-click `open-external` path lives and stays. +// +// Four things disarm the seam, and each has a row below: the release itself, +// a native drag-and-drop, the widget's destruction, and the NEXT press +// anywhere. That last one is the load-bearing guard — see its row. +// The click-seam contract is cm-table-widget-drag.test.ts. +import { describe, expect, it, vi } from "vitest"; + +import { parseTable } from "../../../src/markdown/table/index.js"; +import { quollOpenExternalSink } from "../../../src/webview/cm/open-external.js"; +import { TableBlockWidget } from "../../../src/webview/cm/table/table-widget.js"; +import { IMG_CELL, makeWidget, press, SRC, stubViewWithCaret } from "./helpers/widget-fixtures.js"; + +/** A document position in the prose BELOW the table, as `view.posAtCoords` + * would answer it. Any offset outside the table's own source works. */ +const BELOW = 500; + +describe("TableBlockWidget release outside the widget", () => { + it("a drag released over the prose below dispatches cell-offset → release position", () => { + const dispatched: unknown[] = []; + const { mount } = stubViewWithCaret( + dispatched, + [{ text: "alpha", offset: 2 }], + [], + () => BELOW + ); + const dom = mount(makeWidget(SRC)); + press(dom.querySelectorAll("td")[0] as HTMLElement, "mousedown", 30, 30); + // Released on the BODY: no click is delivered to the widget root at all, + // so without the mouseup seam this gesture dispatches nothing. + press(document.body, "mouseup", 30, 400); + expect(dispatched).toEqual([{ selection: { anchor: SRC.indexOf("alpha") + 2, head: BELOW } }]); + }); + + it("a release INSIDE the widget dispatches nothing — the click seam owns it", () => { + const dispatched: unknown[] = []; + const { mount } = stubViewWithCaret(dispatched, [ + { text: "alpha", offset: 2 }, + { text: "alpha", offset: 5 }, + ]); + const dom = mount(makeWidget(SRC)); + const td = dom.querySelectorAll("td")[0] as HTMLElement; + press(td, "mousedown", 30, 30); + press(td, "mouseup", 80, 30); + expect(dispatched, "the mouseup seam stands down").toEqual([]); + press(td, "click", 80, 30); + expect(dispatched, "and the click seam dispatches exactly once").toEqual([ + { selection: { anchor: SRC.indexOf("alpha") + 2, head: SRC.indexOf("alpha") + 5 } }, + ]); + }); + + // The Done-when's "modifier-click open-external path proven unchanged", as a + // WHOLE gesture: press, release and click all on the link. The mouseup lands + // inside the root, so the new seam must not consume the pending anchor, must + // not dispatch, and must leave the click listener's link branch untouched. + it("a modifier-click on an in-cell link still opens through the sink, mouseup and all", () => { + const src = "| L |\n| - |\n| [x](https://example.com) |"; + const dispatched: unknown[] = []; + const opened: string[] = []; + const { mount } = stubViewWithCaret( + dispatched, + [{ text: "x", offset: 0 }], + [quollOpenExternalSink.of((href: string) => opened.push(href))] + ); + const dom = mount(makeWidget(src)); + const a = dom.querySelector("a") as HTMLElement; + press(a, "mousedown", 10, 10); + press(a, "mouseup", 10, 10); + const click = press(a, "click", 10, 10, { metaKey: true }); + expect(click.defaultPrevented).toBe(true); + expect(opened).toEqual(["https://example.com"]); + expect(dispatched).toEqual([]); + }); + + // ⚠️ The most important row in this file (Codex 94 / Fable 85 / + // error-handler 83, independently). + // + // A release the webview's document never sees — the pointer leaves the + // iframe, focus is lost, Cmd+Tab — leaves the listener armed. The user's NEXT + // unrelated release would then be read as THIS gesture's end and dispatch a + // range from a table cell to a point the user never dragged to: exactly the + // "selection the user did not draw" the whole widget is built to refuse. + // + // The guard is a document-level `mousedown` disarm rather than a list of + // `blur` / `visibilitychange` / `pointercancel` handlers, because a stray + // mouseup is ALWAYS preceded by a mousedown. That makes the guard total: it + // cannot be outrun by a focus-loss path nobody enumerated. + it("a press elsewhere disarms a gesture whose release never arrived", () => { + const dispatched: unknown[] = []; + const { mount } = stubViewWithCaret( + dispatched, + [{ text: "alpha", offset: 2 }], + [], + () => BELOW + ); + const dom = mount(makeWidget(SRC)); + press(dom.querySelectorAll("td")[0] as HTMLElement, "mousedown", 30, 30); + // ...the mouseup goes to the workbench, outside this document. Later, an + // ordinary unrelated click somewhere else in the editor: + press(document.body, "mousedown", 500, 900); + press(document.body, "mouseup", 500, 900); + expect(dispatched, "the stale gesture must not be resurrected").toEqual([]); + }); + + // The far side of the one-shot row below: that row pins that a release with + // no new press behind it dispatches nothing, and this one pins that a release + // WITH one does. Together they are the whole cycle — arm, fire, re-arm — and + // neither half implies the other. + // + // ⚠️ It replaces a row titled "the arming press does not disarm its own + // gesture", which was byte-identical to the first row of this file. The + // knowledge that row carried is worth keeping even though the row was not: + // the arming mousedown BUBBLES to the document, so a bubble-phase disarm + // would cancel the gesture it was just armed for, and every outside-release + // row in this file would then go green for the wrong reason (nothing + // dispatched, caret expected nowhere). The `down === armingPress` identity + // check in table-widget.ts guards exactly that, and under the `capture: true` + // this seam uses it is UNREACHABLE — the press has already passed the + // document before the listener exists. So no row here can pin it, and the old + // title claimed a pin that does not exist. (Measured: dropping the identity + // check leaves the suite green.) + // + // ⚠️ What THIS row pins, stated honestly: the observable re-arm cycle, not + // the `armedRelease.get(root)?.abort()` line that runs at arm time. That line + // is unobservable here — gesture 1's controller was already aborted by its + // own mouseup, so aborting it again is a no-op. Two mutations DO redden this + // row's contract, and only one of them is visible in this environment: + // + // 1. Arming once per root — `if (armedRelease.get(root) !== undefined) + // return;` in place of the arm-time abort — reddens this row and NO + // other in the table suite (measured). So the row does have unique + // discriminating power; do not delete it as redundant. + // 2. Reusing the spent controller — `armedRelease.get(root) ?? new + // AbortController()` — MUST redden it per the DOM spec, because + // `addEventListener` returns early on an already-aborted signal and + // gesture 2 would register no listeners at all. It stays GREEN here: + // happy-dom registers on pre-aborted signals anyway (measured + // 2026-08-22). Only a real-browser row can pin fresh-controller- + // per-gesture. + // + // (2) is also why an earlier note here claimed no single-line mutation could + // redden this row: that mutation was tried, came back green, and was + // believed. In THIS suite a green mutation is not evidence that nothing pins + // the line — happy-dom's event fidelity has to be ruled out first (it is the + // same class of gap as its missing layout, dropped `calc()` and dropped + // nested `var()`). + it("a completed gesture re-arms: the next press draws its own range", () => { + const dispatched: unknown[] = []; + // The head is the release's own Y, so the two gestures cannot be confused + // for one another: a second dispatch carrying the FIRST release's head + // would mean the stale listener answered, not a freshly armed one. + const { mount } = stubViewWithCaret( + dispatched, + [ + { text: "alpha", offset: 2 }, + { text: "alpha", offset: 2 }, + ], + [], + (_x, y) => y + ); + const td = mount(makeWidget(SRC)).querySelectorAll("td")[0] as HTMLElement; + press(td, "mousedown", 30, 30); + press(document.body, "mouseup", 30, 400); + press(td, "mousedown", 30, 30); + press(document.body, "mouseup", 30, 600); + expect(dispatched).toEqual([ + { selection: { anchor: SRC.indexOf("alpha") + 2, head: 400 } }, + { selection: { anchor: SRC.indexOf("alpha") + 2, head: 600 } }, + ]); + }); + + // Fable 80. A right-button press-and-release while the left button is held + // delivers a `button === 2` mouseup to the document. Consuming it would end + // the gesture at the wrong point — and because the one-shot `abort()` runs + // first, the real left release would then find no listener at all. + it("a non-primary release is not the end of the gesture", () => { + const dispatched: unknown[] = []; + const { mount } = stubViewWithCaret( + dispatched, + [{ text: "alpha", offset: 2 }], + [], + () => BELOW + ); + const dom = mount(makeWidget(SRC)); + press(dom.querySelectorAll("td")[0] as HTMLElement, "mousedown", 30, 30); + press(document.body, "mouseup", 200, 200, { button: 2 }); + expect(dispatched, "the right-button release is ignored").toEqual([]); + press(document.body, "mouseup", 30, 400); + expect(dispatched, "and the real release still lands the range").toEqual([ + { selection: { anchor: SRC.indexOf("alpha") + 2, head: BELOW } }, + ]); + }); + + // The phase of the DISARM listener (the document `mousedown`), pinned. No + // other row here distinguishes capture from bubble for it — they all disarm + // through presses nothing interferes with. Its pair is the row below, which + // pins the phase of the DISPATCH listener (the document `mouseup`); the two + // read alike and are not interchangeable, so each names its listener. + it("a press that stops propagation still disarms the seam", () => { + // Sibling widgets in this editor — the task checkbox, the fenced-code copy + // and collapse buttons, the language picker — all call stopPropagation() on + // mousedown, and NONE of them stops mouseup. A bubble-phase disarm is + // starved by exactly those presses while the release still arrives: the one + // combination that dispatches a range the user never drew. Capture cannot be + // starved from below. (measured against an element that stops mousedown: a + // bubble-phase listener fired 0 times, a capture-phase listener 1; the + // mouseup reached the document either way) + const dispatched: unknown[] = []; + const { mount } = stubViewWithCaret( + dispatched, + [{ text: "alpha", offset: 2 }], + [], + () => BELOW + ); + const dom = mount(makeWidget(SRC)); + press(dom.querySelectorAll("td")[0] as HTMLElement, "mousedown", 30, 30); + // ...the release was lost outside this document. Later, a press on a + // sibling widget that swallows mousedown: + const swallower = document.createElement("button"); + document.body.appendChild(swallower); + swallower.addEventListener("mousedown", (event) => event.stopPropagation()); + press(swallower, "mousedown", 500, 900); + press(document.body, "mouseup", 500, 900); + expect(dispatched).toEqual([]); + }); + + // Every other disarm row presses on `document.body` or a bare `