test(editor): address PR #147 review — reflow tests, code-block guard, a11y

Resolve the pre-merge review items for the #146 NodeView content-first fix: - Export collectScrollAncestors/reflowAfterPaste and add editor-paste-handler unit tests covering ancestor selection (overlay included, non-overflowing auto excluded, X axis), the scrollHeight>clientHeight gate, scrollingElement dedup, the docEl==null branch, and the double-rAF nudge. - Extend the structural guard with CodeBlockView and merge the two it.each blocks into one document-order assertion (handles the <pre> nesting where the contentDOM is not the literal first child). - Simplify the post-paste nudge to a single scrollTo(scrollLeft, scrollTop). - Document that the post-paste reflow runs on every paste path intentionally, and cross-reference the two #146 mitigations in both fixes. - a11y: aria-hidden the decorative footnotes heading and number marker, and label the footnotes list via role="group" + aria-label so the visual reorder does not break screen-reader reading order (WCAG 1.3.2). - CHANGELOG: add a Fixed entry noting the caret fix is macOS-verified manually. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-24 04:36:39 +03:00
parent 67057de214
commit 8bb441870a
7 changed files with 286 additions and 37 deletions
@@ -0,0 +1,160 @@
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
+import {
+  collectScrollAncestors,
+  reflowAfterPaste,
+} from "./editor-paste-handler";
+
+/**
+ * Unit tests for the #146 post-paste reflow helpers. jsdom does not compute
+ * styles or layout, so we stub getComputedStyle (per element via a Map) and the
+ * scroll/overflow geometry properties (per element via Object.defineProperty).
+ * Element trees are built DETACHED from `document`, so the ancestor walk only
+ * traverses the elements we create. collectScrollAncestors always appends
+ * document.scrollingElement, so we assert on specific ancestors with
+ * toContain/not.toContain rather than exact-array equality.
+ */
+
+type Overflow = { overflowX: string; overflowY: string };
+const styleMap = new Map<Element, Overflow>();
+
+function makeScrollable(
+  overflowY: string,
+  {
+    sh = 0,
+    ch = 0,
+    sw = 0,
+    cw = 0,
+    left = 0,
+    top = 0,
+    overflowX = "visible",
+  }: {
+    sh?: number;
+    ch?: number;
+    sw?: number;
+    cw?: number;
+    left?: number;
+    top?: number;
+    overflowX?: string;
+  } = {},
+) {
+  const el = document.createElement("div");
+  Object.defineProperty(el, "scrollHeight", { configurable: true, value: sh });
+  Object.defineProperty(el, "clientHeight", { configurable: true, value: ch });
+  Object.defineProperty(el, "scrollWidth", { configurable: true, value: sw });
+  Object.defineProperty(el, "clientWidth", { configurable: true, value: cw });
+  Object.defineProperty(el, "scrollLeft", { configurable: true, value: left });
+  Object.defineProperty(el, "scrollTop", { configurable: true, value: top });
+  styleMap.set(el, { overflowX, overflowY });
+  return el;
+}
+
+// A leaf node whose parentElement is `parent`. The walk starts from
+// node.parentElement, so the parent is the first candidate ancestor.
+function makeNodeUnder(parent: HTMLElement) {
+  const node = document.createElement("div");
+  parent.appendChild(node);
+  return node;
+}
+
+// Override `document.scrollingElement` as an instance own-property (the native
+// implementation is a getter on Document.prototype, which we never touch).
+function setScrollingElement(value: Element | null) {
+  Object.defineProperty(document, "scrollingElement", {
+    configurable: true,
+    get: () => value,
+  });
+}
+
+beforeEach(() => {
+  styleMap.clear();
+  vi.stubGlobal("getComputedStyle", (el: Element) => {
+    return styleMap.get(el) ?? { overflowX: "visible", overflowY: "visible" };
+  });
+});
+
+afterEach(() => {
+  vi.unstubAllGlobals();
+  // Drop the per-test instance override so the native prototype getter shows
+  // through again (it was never modified, so no further restore is needed).
+  delete (document as any).scrollingElement;
+});
+
+describe("collectScrollAncestors", () => {
+  it("includes an overflow:overlay ancestor that overflows (macOS case)", () => {
+    setScrollingElement(null);
+    const a = makeScrollable("overlay", { sh: 200, ch: 100 });
+    const node = makeNodeUnder(a);
+    expect(collectScrollAncestors(node)).toContain(a);
+  });
+
+  it("excludes an overflow:auto ancestor that does NOT overflow (gate fails)", () => {
+    setScrollingElement(null);
+    const a = makeScrollable("auto", { sh: 100, ch: 100 });
+    const node = makeNodeUnder(a);
+    expect(collectScrollAncestors(node)).not.toContain(a);
+  });
+
+  it("includes an overflow:auto ancestor that overflows", () => {
+    setScrollingElement(null);
+    const a = makeScrollable("auto", { sh: 200, ch: 100 });
+    const node = makeNodeUnder(a);
+    expect(collectScrollAncestors(node)).toContain(a);
+  });
+
+  it("excludes a non-scrollable overflow even when it overflows", () => {
+    setScrollingElement(null);
+    const a = makeScrollable("hidden", { sh: 200, ch: 100 });
+    const node = makeNodeUnder(a);
+    expect(collectScrollAncestors(node)).not.toContain(a);
+  });
+
+  it("includes an X-axis overflow:scroll ancestor that overflows horizontally", () => {
+    setScrollingElement(null);
+    const a = makeScrollable("visible", {
+      overflowX: "scroll",
+      sw: 200,
+      cw: 100,
+    });
+    const node = makeNodeUnder(a);
+    expect(collectScrollAncestors(node)).toContain(a);
+  });
+
+  it("dedups: scrollingElement already in the walk is added exactly once", () => {
+    const a = makeScrollable("auto", { sh: 200, ch: 100 });
+    setScrollingElement(a);
+    const node = makeNodeUnder(a);
+    const result = collectScrollAncestors(node);
+    expect(result.filter((x) => x === a).length).toBe(1);
+  });
+
+  it("does not throw and appends nothing when scrollingElement is null", () => {
+    setScrollingElement(null);
+    const a = makeScrollable("auto", { sh: 200, ch: 100 });
+    const node = makeNodeUnder(a);
+    const result = collectScrollAncestors(node);
+    // Only the qualifying ancestor we built — no trailing scrollingElement.
+    expect(result).toEqual([a]);
+  });
+});
+
+describe("reflowAfterPaste", () => {
+  it("runs the double rAF and nudges each ancestor with scrollTo(scrollLeft, scrollTop)", () => {
+    // Run the double-nested requestAnimationFrame synchronously.
+    vi.stubGlobal(
+      "requestAnimationFrame",
+      (cb: FrameRequestCallback) => {
+        cb(0);
+        return 0;
+      },
+    );
+    setScrollingElement(null);
+
+    const a = makeScrollable("auto", { sh: 200, ch: 100, left: 5, top: 10 });
+    const node = makeNodeUnder(a);
+    (a as any).scrollTo = vi.fn();
+
+    reflowAfterPaste({ view: { dom: node } } as any);
+
+    expect((a as any).scrollTo).toHaveBeenCalledWith(5, 10);
+  });
+});
@@ -33,7 +33,7 @@ const SCROLLABLE_OVERFLOW = new Set(["auto", "scroll", "overlay"]);
 * must count as scrollable too. Called only AFTER the paste has committed, so
 * `scrollHeight > clientHeight` reflects the inserted content.
 */
-function collectScrollAncestors(node: HTMLElement): HTMLElement[] {
+export function collectScrollAncestors(node: HTMLElement): HTMLElement[] {
  const targets: HTMLElement[] = [];
  // Walk every ancestor (incl. body/html) — on some layouts the scroll lives on
  // body rather than the documentElement that scrollingElement points at.
@@ -63,18 +63,21 @@ function collectScrollAncestors(node: HTMLElement): HTMLElement[] {
 * container lookup AND the nudge run across two animation frames so they happen
 * AFTER the pasted content + NodeViews commit (only then is the real scroll
 * container measurable).
+ *
+ * This is the SECOND of two #146 mitigations; the FIRST is the content-first DOM
+ * order in the NodeViews (code-block-view.tsx, footnotes-list-view.tsx,
+ * footnote-definition-view.tsx). Editing one, check the other.
 */
-function reflowAfterPaste(editor: Editor) {
+export function reflowAfterPaste(editor: Editor) {
  const dom = editor.view.dom as HTMLElement;
  requestAnimationFrame(() => {
    requestAnimationFrame(() => {
      for (const el of collectScrollAncestors(dom)) {
-        // Capture into locals first so this reads as a scroll nudge, not a
-        // no-op self-assignment (which lint would flag), while still poking the
-        // scroll position to refresh hit-testing.
-        const { scrollTop, scrollLeft } = el;
-        el.scrollTop = scrollTop;
-        el.scrollLeft = scrollLeft;
+        // Zero-delta nudge: re-set the scroll position to its current value to
+        // invalidate the browser's hit-test layer WITHOUT moving the viewport.
+        // `scrollTo(x, y)` is the repo idiom and avoids a lint-flagged
+        // self-assignment.
+        el.scrollTo(el.scrollLeft, el.scrollTop);
      }
    });
  });
@@ -86,9 +89,12 @@ export const handlePaste = (
  pageId: string,
  creatorId?: string,
 ) => {
-  // Schedule a post-paste reflow for every paste path: the pasted content (and
-  // its async NodeViews) settles after this handler returns, so we nudge on the
-  // next frames to keep click hit-testing aligned (#146).
+  // Schedule a post-paste reflow on EVERY paste path — intentionally. handlePaste
+  // returns BEFORE the markdown/code-insertion plugin runs, so it cannot know here
+  // whether async NodeViews will be inserted; the nudge is a cheap layout read on
+  // the next frames and a no-op for the viewport, so scheduling it unconditionally
+  // is simpler and harmless. Pairs with the content-first DOM order in the
+  // NodeViews — both address #146 from different angles.
  reflowAfterPaste(editor);

  const clipboardData = event.clipboardData.getData("text/plain");