From 8bb441870ad485be5afd1851d1755a4dfa8095ae Mon Sep 17 00:00:00 2001
From: claude_code <claude_code@vvzvlad.xyz>
Date: Wed, 24 Jun 2026 04:36:39 +0300
Subject: [PATCH] =?UTF-8?q?test(editor):=20address=20PR=20#147=20review=20?=
 =?UTF-8?q?=E2=80=94=20reflow=20tests,=20code-block=20guard,=20a11y?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
---
 CHANGELOG.md                                  |  12 ++
 .../components/code-block/code-block-view.tsx |   3 +-
 .../common/editor-paste-handler.test.ts       | 160 ++++++++++++++++++
 .../common/editor-paste-handler.tsx           |  28 +--
 .../footnote/footnote-definition-view.tsx     |   9 +-
 .../footnote-views.structure.test.tsx         |  94 +++++++---
 .../footnote/footnotes-list-view.tsx          |  17 +-
 7 files changed, 286 insertions(+), 37 deletions(-)
 create mode 100644 apps/client/src/features/editor/components/common/editor-paste-handler.test.ts

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 43255596..b843e912 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -19,6 +19,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   cut from 300 to 100 on upgrade. Set `SHARE_AI_WORKSPACE_MAX_PER_HOUR` to
   keep the previous limit. (#62)
 
+### Fixed
+
+- **Editor: caret/selection landed on the wrong line when clicking inside code
+  blocks and footnotes.** The affected NodeViews rendered their non-editable
+  chrome (language menu, footnotes heading, footnote number marker) before the
+  editable content, so the browser's click hit-testing missed the contentDOM and
+  snapped the caret to a previous node. Content now renders first in the DOM
+  (chrome is lifted back into place via CSS flex `order`), and scroll containers
+  are nudged after a paste to refresh stale hit-testing geometry. The caret
+  symptom is macOS-specific and was confirmed manually on macOS; the automated
+  guard pins the DOM-order invariant, not the caret behavior itself. (#146, #147)
+
 ## [0.93.0] - 2026-06-21
 
 This release builds on the 0.91.0 AI foundation: admin-defined AI agent roles,
diff --git a/apps/client/src/features/editor/components/code-block/code-block-view.tsx b/apps/client/src/features/editor/components/code-block/code-block-view.tsx
index 1b67ddf6..1930f182 100644
--- a/apps/client/src/features/editor/components/code-block/code-block-view.tsx
+++ b/apps/client/src/features/editor/components/code-block/code-block-view.tsx
@@ -53,7 +53,8 @@ export default function CodeBlockView(props: NodeViewProps) {
           menu is rendered after it and lifted back above visually via flex
           `order: -1` (the `.codeBlock` wrapper is a flex column — see
           code-block.module.css). It stays fully in flow as a full-width row
-          above the code: no overlay/absolute positioning. */}
+          above the code: no overlay/absolute positioning. The second #146
+          mitigation lives in editor-paste-handler.tsx (reflowAfterPaste). */}
       <pre
         spellCheck="false"
         hidden={
diff --git a/apps/client/src/features/editor/components/common/editor-paste-handler.test.ts b/apps/client/src/features/editor/components/common/editor-paste-handler.test.ts
new file mode 100644
index 00000000..bde6c837
--- /dev/null
+++ b/apps/client/src/features/editor/components/common/editor-paste-handler.test.ts
@@ -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);
+  });
+});
diff --git a/apps/client/src/features/editor/components/common/editor-paste-handler.tsx b/apps/client/src/features/editor/components/common/editor-paste-handler.tsx
index 08e47766..63300020 100644
--- a/apps/client/src/features/editor/components/common/editor-paste-handler.tsx
+++ b/apps/client/src/features/editor/components/common/editor-paste-handler.tsx
@@ -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");
diff --git a/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx b/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx
index ef4b5eab..e3e0522a 100644
--- a/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx
+++ b/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx
@@ -32,9 +32,14 @@ export default function FootnoteDefinitionView(props: NodeViewProps) {
       {/* #146: contentDOM MUST be the first child — a non-editable marker before
           it makes click hit-testing snap the caret above. Content first; the
           marker + back-link follow in DOM and are placed left/right via CSS
-          flex `order`. */}
+          flex `order`. The second #146 mitigation lives in
+          editor-paste-handler.tsx (reflowAfterPaste). */}
       <NodeViewContent className={classes.definitionContent} />
-      <span className={classes.definitionMarker} contentEditable={false}>
+      <span
+        className={classes.definitionMarker}
+        contentEditable={false}
+        aria-hidden="true"
+      >
         {number}.
       </span>
       <span
diff --git a/apps/client/src/features/editor/components/footnote/footnote-views.structure.test.tsx b/apps/client/src/features/editor/components/footnote/footnote-views.structure.test.tsx
index 0bab973a..3e28493d 100644
--- a/apps/client/src/features/editor/components/footnote/footnote-views.structure.test.tsx
+++ b/apps/client/src/features/editor/components/footnote/footnote-views.structure.test.tsx
@@ -4,11 +4,14 @@ import { render } from "@testing-library/react";
 /**
  * Structural regression guard for #146 (PR #147).
  *
+ * Guards ALL THREE editable NodeViews touched by the fix: the two footnote views
+ * (FootnotesListView, FootnoteDefinitionView) AND the code block (CodeBlockView).
+ *
  * The caret/click-offset fix rests entirely on ONE invariant: in every editable
- * footnote NodeView the editable `NodeViewContent` (contentDOM) must be the
- * FIRST child of the wrapper, with no non-editable (`contenteditable="false"`)
- * element before it. If a future edit reinserts chrome (separator, heading,
- * marker, back-link) ahead of the content, the macOS hit-testing bug returns
+ * NodeView the editable `NodeViewContent` (contentDOM) must come FIRST in the
+ * wrapper, with no non-editable (`contenteditable="false"`) element before it.
+ * If a future edit reinserts chrome (separator, heading, marker, back-link,
+ * language menu) ahead of the content, the macOS hit-testing bug returns
  * silently — and the symptom needs a real browser to see. This test pins the
  * DOM ORDER (the proxy that IS the fix) in the existing jsdom harness.
  *
@@ -39,8 +42,36 @@ vi.mock("@docmost/editor-ext", () => ({
   getFootnoteNumber: () => 1,
 }));
 
+// Mocks so CodeBlockView renders cheaply (no MantineProvider, no matchMedia).
+// The Group mock MUST forward contentEditable: React serializes
+// contentEditable={false} to the DOM attribute contenteditable="false", which
+// the structural guard selects on to identify non-editable chrome.
+vi.mock("@mantine/core", () => ({
+  Group: ({ children, className, contentEditable }: any) => (
+    <div className={className} contentEditable={contentEditable}>
+      {children}
+    </div>
+  ),
+  Select: () => null,
+  Tooltip: ({ children }: any) => <>{children}</>,
+  ActionIcon: ({ children, onClick }: any) => (
+    <button onClick={onClick}>{children}</button>
+  ),
+}));
+vi.mock("@/components/common/copy-button", () => ({
+  CopyButton: ({ children }: any) => children({ copied: false, copy: () => {} }),
+}));
+vi.mock("@tabler/icons-react", () => ({
+  IconCheck: () => null,
+  IconCopy: () => null,
+}));
+vi.mock("@/features/editor/components/code-block/mermaid-view.tsx", () => ({
+  default: () => null,
+}));
+
 import FootnotesListView from "./footnotes-list-view";
 import FootnoteDefinitionView from "./footnote-definition-view";
+import CodeBlockView from "../code-block/code-block-view";
 
 // Minimal NodeViewProps stub: definition view only touches node.attrs.id and
 // editor.state (the latter unused once getFootnoteNumber is mocked).
@@ -52,38 +83,59 @@ const props = {
   deleteNode: () => {},
 } as any;
 
+// CodeBlockView needs more than the footnote stub: a language attr (non-mermaid
+// so MermaidView never renders), an editor with selection/on/off, and an
+// extension exposing lowlight.listLanguages.
+const codeBlockProps = {
+  node: { attrs: { language: "javascript" }, textContent: "", nodeSize: 1 },
+  editor: {
+    state: { selection: { from: 0, to: 0 } },
+    isEditable: true,
+    commands: {},
+    on: vi.fn(),
+    off: vi.fn(),
+  },
+  extension: {
+    options: { lowlight: { listLanguages: () => ["javascript", "python"] } },
+  },
+  getPos: () => 0,
+  updateAttributes: () => {},
+  deleteNode: () => {},
+} as any;
+
 const cases: Array<{ name: string; ui: React.ReactElement }> = [
   { name: "FootnotesListView", ui: <FootnotesListView {...props} /> },
   { name: "FootnoteDefinitionView", ui: <FootnoteDefinitionView {...props} /> },
+  { name: "CodeBlockView", ui: <CodeBlockView {...codeBlockProps} /> },
 ];
 
-describe("#146 footnote NodeView DOM-order invariant", () => {
+describe("#146 editable NodeView contentDOM-first invariant", () => {
   it.each(cases)(
-    "$name renders contentDOM as the first child",
+    "$name renders the editable contentDOM ahead of all non-editable chrome",
     ({ ui }) => {
       const { getByTestId } = render(ui);
       const wrapper = getByTestId("nvw");
 
-      const firstEl = wrapper.firstElementChild;
-      expect(firstEl).not.toBeNull();
-      // The editable content must be physically first.
-      expect(firstEl?.hasAttribute("data-node-view-content")).toBe(true);
-    },
-  );
+      const content = wrapper.querySelector("[data-node-view-content]");
+      expect(content).not.toBeNull();
 
-  it.each(cases)(
-    "$name has no contentEditable=false chrome BEFORE the content",
-    ({ ui }) => {
-      const { getByTestId } = render(ui);
-      const wrapper = getByTestId("nvw");
+      // The contentDOM sits at the FRONT of the wrapper: it is either the
+      // wrapper's first child (footnote views) or nested in the first child
+      // (code-block wraps it in <pre>). Either way the first element child
+      // must contain it. (compareDocumentPosition below is NOT redundant here:
+      // for code-block the content is not the literal first child, so we keep
+      // the document-order check to prove no chrome precedes the content.)
+      const firstEl = wrapper.firstElementChild!;
+      expect(firstEl === content || firstEl.contains(content!)).toBe(true);
 
-      const content = wrapper.querySelector("[data-node-view-content]")!;
+      // Chrome exists (separator/heading/marker/back-link/menu)...
       const nonEditable = wrapper.querySelectorAll('[contenteditable="false"]');
-      expect(nonEditable.length).toBeGreaterThan(0); // chrome exists...
+      expect(nonEditable.length).toBeGreaterThan(0);
 
+      // ...and every non-editable element comes AFTER the contentDOM, so the
+      // browser's click hit-testing reaches the editable content first (#146).
       for (const el of Array.from(nonEditable)) {
-        // ...but every non-editable element must come AFTER the content node.
-        const pos = content.compareDocumentPosition(el);
+        const pos = content!.compareDocumentPosition(el);
         expect(pos & Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy();
       }
     },
diff --git a/apps/client/src/features/editor/components/footnote/footnotes-list-view.tsx b/apps/client/src/features/editor/components/footnote/footnotes-list-view.tsx
index 7a856df2..7ad03f12 100644
--- a/apps/client/src/features/editor/components/footnote/footnotes-list-view.tsx
+++ b/apps/client/src/features/editor/components/footnote/footnotes-list-view.tsx
@@ -13,14 +13,27 @@ import classes from "./footnote.module.css";
  * the body). So content goes first; the heading is rendered AFTER it and lifted
  * back above visually with CSS flex `order` (the separator border lives on the
  * flex container itself).
+ *
+ * The second #146 mitigation lives in editor-paste-handler.tsx (reflowAfterPaste).
  */
 export default function FootnotesListView(_props: NodeViewProps) {
   const { t } = useTranslation();
 
   return (
-    <NodeViewWrapper className={classes.list}>
+    // role/aria-label preserve the section label for AT: the visible heading
+    // below is now aria-hidden, so without these the "Footnotes" label would be
+    // lost to a screen reader (WCAG 1.3.2 — DOM order has heading after content).
+    <NodeViewWrapper
+      className={classes.list}
+      role="group"
+      aria-label={t("Footnotes")}
+    >
       <NodeViewContent />
-      <div className={classes.listHeading} contentEditable={false}>
+      <div
+        className={classes.listHeading}
+        contentEditable={false}
+        aria-hidden="true"
+      >
         {t("Footnotes")}
       </div>
     </NodeViewWrapper>