Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/webview/cm/table/cell-point.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
426 changes: 388 additions & 38 deletions src/webview/cm/table/table-widget.ts

Large diffs are not rendered by default.

13 changes: 13 additions & 0 deletions test/webview-browser/helpers/table-drag-harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<HTMLElement>(".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<HTMLElement>("th, td")].find(
(c) => c.textContent === text
Expand Down
53 changes: 51 additions & 2 deletions test/webview-browser/table-drag-selection.browser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -33,8 +39,10 @@ import {
PLAIN,
pointAtChar,
pointInWidgetPadding,
proseLine,
revealed,
TABLE_BLOCK_START,
TAIL,
unmount,
widgetRoot,
} from "./helpers/table-drag-harness.js";
Expand Down Expand Up @@ -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
Expand Down
86 changes: 77 additions & 9 deletions test/webview/table/cm-table-widget-drag.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)") } },
Expand Down Expand Up @@ -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, [
Expand Down
Loading
Loading