diff --git a/src/webview/cm/table/table-widget.ts b/src/webview/cm/table/table-widget.ts index d412077c..bf6c09a0 100644 --- a/src/webview/cm/table/table-widget.ts +++ b/src/webview/cm/table/table-widget.ts @@ -2,7 +2,8 @@ // in place of its source. Click-to-reveal: a click on any cell // dispatches a caret selection to the cell's absolute LF-internal source // offset (data-cell-from = nodeFrom + cell.from); a click on the widget -// padding/margin (no cell) falls back to the block line-start (data-doc-from). +// padding/margin (no cell) falls back to the block line-start, carried in the +// module-private `blockStart` WeakMap (NOT read back out of the DOM). // A mousedown followed by a click that actually moved (see DRAG_THRESHOLD_PX) // instead dispatches a RANGE selection between the two RESOLVED source offsets; // an endpoint whose cell renders non-byte-aligned (inline markup) has no exact @@ -21,9 +22,9 @@ // caret offset (nodeFrom + cell.from). Both are LF-internal (seed.ts // splitToCmText strips \r). Two tables at different doc positions or with // different Lezer node starts are NOT eq; same (docFrom, slice, nodeFrom) on -// a rebuild reuses the existing DOM. updateDOM re-stamps both on reuse so a -// margin/cell click after a shift uses the new offsets, not a stale toDOM-time -// closure. +// a rebuild reuses the existing DOM. updateDOM re-points both channels on reuse +// (cell stamps on the DOM, block start in `blockStart`) so a margin/cell click +// after a shift uses the new offsets, not a stale toDOM-time closure. import { type EditorView, WidgetType } from "@codemirror/view"; @@ -59,6 +60,43 @@ interface PendingDrag { } const pendingDrag = new WeakMap(); +/** The block's CURRENT first-byte offset (margin-click caret target), keyed on + * the widget's root element for the same reason `pendingDrag` is: `updateDOM` + * reuses that element across widget instances and cannot re-bind the click + * listener, whose captured `this` stays the OLD instance — so the new instance + * needs a channel to the existing listener that moves exactly when the block + * moves, which `updateDOM` can write and the closure cannot. + * + * A `number` end to end: unlike the per-CELL offsets it is never stringified, + * parsed, or read back from the DOM, so there is no malformed-value state to + * gate — which is why `stampedOffset` guards those stamps and not this one + * (its docblock has what CodeMirror does NOT catch). Same channel, same + * rationale, as image-widget.ts's `blockStart`. */ +const blockStart = new WeakMap(); + +/** Margin-click caret: the block start this root currently points at. + * + * Falling back to the toDOM-time `widget.docFrom` totalizes the + * `number | undefined` read; it is not the stale-closure hazard coming back. + * The entry is written in `toDOM` in the same breath as attaching the listener, + * and at that moment the closure value IS the current one — so a miss is + * unreachable by construction. Logged rather than trusted, so a future + * regression of that invariant is observable instead of quietly reintroducing + * the stale-caret bug this WeakMap exists to prevent. */ +function blockStartCaret(root: HTMLElement, widget: TableBlockWidget): number { + const current = blockStart.get(root); + if (current === undefined) { + // `slice` identifies WHICH widget tripped it — a document can hold many + // tables, and `fallback` alone would not say which one. + console.error("[quoll] table widget blockStart miss — invariant violated", { + slice: widget.slice, + fallback: widget.docFrom, + }); + return widget.docFrom; + } + return current; +} + /** Every selection dispatch out of this widget's DOM listeners goes through * here, so the throw paths are handled in ONE place rather than at each seam. * @@ -177,9 +215,12 @@ export class TableBlockWidget extends WidgetType { // block-widget height measurement stays in lockstep with the visible DOM. const root = document.createElement("div"); root.className = "quoll-block quoll-table-block"; - // Margin-click caret fallback, stored on the DOM so a reused element - // (updateDOM) reflects the CURRENT docFrom, not a stale toDOM-time closure. + // The margin-click caret travels through `blockStart`, NOT through this + // attribute: `data-doc-from` is written for DOM inspection (and read by the + // tests that pin the re-stamp) and is NEVER read back — see `blockStart` + // above for why this position must not be parsed back out of the DOM. root.dataset.docFrom = String(this.docFrom); + blockStart.set(root, this.docFrom); // Resource base for relative in-cell image srcs. Static per editor // (resource-base.ts), so it is NOT part of eq() — reading it at @@ -267,32 +308,24 @@ export class TableBlockWidget extends WidgetType { return; } const cell = (event.target as Element | null)?.closest?.("th, td") ?? null; - // Read the stamps through the SAME gate the drag path uses - // (`stampedOffset`), not a bare `Number(...)`. Both are DOM attributes and - // therefore both are trust boundaries, and CodeMirror will not catch a bad - // one for us: `checkSelection` only rejects `range.to > doc.length`, so a - // `NaN` anchor is accepted silently and installs a broken selection that - // `dispatchSelection`'s catch never sees. + // The CELL offset must stay on the DOM — `cellPointAt` resolves an + // arbitrary descendant under the pointer, so no closure knows which cell + // was clicked. That makes it a trust boundary, read through the SAME gate + // the drag path uses (`stampedOffset`) rather than a bare `Number(...)`; + // its docblock has the why — CodeMirror accepts a `NaN` anchor and + // installs a broken selection `dispatchSelection`'s catch never sees. // // A stamp that fails the gate degrades one step rather than dispatching // nothing: reveal-on-caret is LINE-level, so the block start reveals the // same table the cell offset would — only the intra-table caret precision // is lost, and a dead click (no reveal at all) is a worse answer for a // failure mode that only arises when something outside this widget wrote - // its DOM. - // - // ⚠️ That "same table" guarantee holds for arm 2 (`data-doc-from`) but - // NOT for arm 3. `this.docFrom` is the toDOM-time closure value, and - // `updateDOM` re-stamps the DOM without being able to re-bind this - // listener — the very trap this file's header warns about — so after a - // positional shift the closure is stale and arm 3 can reveal a DIFFERENT - // block, not merely a less precise position within this one. It is last - // for exactly that reason: it is the least trustworthy link, reachable - // only once both DOM stamps have already failed the gate. + // its DOM. That "same table" guarantee is unconditional now that the block + // start comes from `blockStart`, which `updateDOM` re-points: it can no + // longer be a stale closure value pointing at a DIFFERENT block. const caret = (cell === null ? null : stampedOffset(cell, "data-cell-from")) ?? - stampedOffset(root, "data-doc-from") ?? - this.docFrom; + blockStartCaret(root, this); dispatchSelection(view, dragRange(view, root, event, pending) ?? { anchor: caret }); }); @@ -362,9 +395,13 @@ export class TableBlockWidget extends WidgetType { // would dispatch a selection over an unrelated span. Drop it; the gesture // degrades to the collapsed caret. pendingDrag.delete(dom); - // Re-stamp the margin fallback so a reused element tracks the new docFrom - // after a distant edit shifted this table without changing its bytes. + // Re-point the margin-click caret channel the click listener actually reads, + // so a reused element tracks the new docFrom after a distant edit shifted + // this table without changing its bytes. The attribute beside it is + // inspection-only (see toDOM) — dropping THIS line would leave the reused + // listener dispatching the old offset while the DOM still looked correct. dom.dataset.docFrom = String(this.docFrom); + 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 diff --git a/test/webview/table/cm-table-widget.test.ts b/test/webview/table/cm-table-widget.test.ts index e3f196c9..6f36ac1f 100644 --- a/test/webview/table/cm-table-widget.test.ts +++ b/test/webview/table/cm-table-widget.test.ts @@ -1024,12 +1024,14 @@ describe("TableBlockWidget drag-selection", () => { }); describe("TableBlockWidget caret dispatch hardening", () => { - // The caret path reads `data-cell-from` / `data-doc-from` off the DOM, so it - // sits on the same trust boundary as the drag path and must use the same - // gate. A bare `Number(...)` here would not merely be untidy: CodeMirror's - // `checkSelection` tests `range.to > doc.length` and nothing else, so a `NaN` - // anchor is ACCEPTED and installs a range whose `from` is `NaN` — a silently - // broken selection, with no throw for `dispatchSelection`'s catch to see. + // The caret path reads `data-cell-from` off the DOM — the per-cell offset has + // to live there, since `cellPointAt` resolves whatever descendant is under the + // pointer — so it sits on the same trust boundary as the drag path and must + // use the same gate. A bare `Number(...)` here would not merely be untidy: + // CodeMirror's `checkSelection` tests `range.to > doc.length` and nothing + // else, so a `NaN` anchor is ACCEPTED and installs a range whose `from` is + // `NaN` — a silently broken selection, with no throw for + // `dispatchSelection`'s catch to see. // Hence the assertions below are on the exact dispatched value, not on // "something was dispatched". it.each([ @@ -1050,15 +1052,34 @@ describe("TableBlockWidget caret dispatch hardening", () => { expect(dispatched).toEqual([{ selection: { anchor: 7 } }]); }); - it("falls back to the widget's own docFrom when the ROOT stamp is malformed too", () => { + // The ROOT position, unlike the cell stamps, is NOT an input from the DOM. + // `data-doc-from` is still written (DOM inspection, plus the re-stamp + // assertions in the updateDOM block above), but the block-start fallback + // reads the module-private WeakMap, so a value written onto the element + // cannot steer the dispatch. + // + // "999" alone kills both reverts — it dispatches 999 (≠ 7, this fixture's + // expected anchor) whether read BARE (`Number(root.dataset.docFrom)`) or + // via the pre-refactor GATED read (`stampedOffset(root, "data-doc-from") + // ?? this.docFrom`), since "999" also passes the gate's `/^\d+$/`. "abc" + // is redundant against the gated read — it fails the gate and falls + // through to `this.docFrom`, leaving that revert green — but earns its + // place against the bare read: it dispatches `NaN`, silently accepted by + // `checkSelection` (rejects only `range.to > doc.length`), breaking the + // selection silently. (Each revert applied; red rows observed.) + it.each([ + ["malformed", "abc"], + ["well-formed but wrong", "999"], + ])("ignores a %s data-doc-from written onto the widget root", (_label, raw) => { const dispatched: unknown[] = []; const dom = makeWidget(SRC, 7).toDOM(stubView(dispatched)); document.body.appendChild(dom); const td = dom.querySelectorAll("td")[0] as HTMLElement; - td.setAttribute("data-cell-from", "abc"); - dom.setAttribute("data-doc-from", "-3"); + td.setAttribute("data-cell-from", "abc"); // force the block-start fallback + dom.setAttribute("data-doc-from", raw); press(td, "click", 10, 10); - // 7 is the constructor argument, which never travelled through the DOM. + // 7 is the constructor argument, carried in the WeakMap — it never + // travelled through the DOM. expect(dispatched).toEqual([{ selection: { anchor: 7 } }]); }); @@ -1090,4 +1111,27 @@ describe("TableBlockWidget caret dispatch hardening", () => { consoleError.mockRestore(); } }); + + // A fresh toDOM'd widget's margin click must hit the `blockStart` entry + // written in toDOM, not `blockStartCaret`'s miss fallback — the absent + // `console.error` is the only observable difference. The anchor VALUE + // cannot distinguish them here (nor in any other fixture in this file + // that clicks right after toDOM): both trace back to the same `docFrom` + // constructor argument, so deleting `blockStart.set(root, this.docFrom)` + // in `toDOM` leaves every such anchor assertion green. (The updateDOM + // re-stamp fixture above stays green for an unrelated reason: its OWN + // `blockStart.set` write re-fills the entry with the new docFrom.) + it("does not log a blockStart miss when a fresh toDOM'd widget's margin is clicked", () => { + const dispatched: unknown[] = []; + const dom = makeWidget(SRC, 7).toDOM(stubView(dispatched)); + document.body.appendChild(dom); + const consoleError = vi.spyOn(console, "error").mockImplementation(() => undefined); + try { + dom.click(); // the root div, not a cell + expect(dispatched).toEqual([{ selection: { anchor: 7 } }]); + expect(consoleError).not.toHaveBeenCalled(); + } finally { + consoleError.mockRestore(); + } + }); });