Merge pull request 'fix(editor): render NodeViewContent first so click hit-testing isn't offset (#146)' (#147) from fix/146-nodeview-content-order into develop

Reviewed-on: #147

CHANGELOG.md

@@ -30,6 +30,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,

apps/client/src/features/editor/components/code-block/code-block-view.tsx

@@ -47,6 +47,26 @@ export default function CodeBlockView(props: NodeViewProps) {
 
   return (
     <NodeViewWrapper className="codeBlock">
+      {/* #146: the editable <pre><code> (contentDOM) MUST come first in the DOM.
+          With the non-editable menu rendered before it, the browser's click
+          hit-testing snapped the caret up one line. Render content first; the
+          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. The second #146
+          mitigation lives in editor-paste-handler.tsx (reflowAfterPaste). */}
+      <pre
+        spellCheck="false"
+        hidden={
+          ((language === "mermaid" && !editor.isEditable) ||
+            (language === "mermaid" && !isSelected)) &&
+          node.textContent.length > 0
+        }
+      >
+        {/* @ts-ignore */}
+        <NodeViewContent as="code" className={`language-${language}`} />
+      </pre>
+
       <Group
         justify="flex-end"
         contentEditable={false}
@@ -83,18 +103,6 @@ export default function CodeBlockView(props: NodeViewProps) {
         </CopyButton>
       </Group>
 
-      <pre
-        spellCheck="false"
-        hidden={
-          ((language === "mermaid" && !editor.isEditable) ||
-            (language === "mermaid" && !isSelected)) &&
-          node.textContent.length > 0
-        }
-      >
-        {/* @ts-ignore */}
-        <NodeViewContent as="code" className={`language-${language}`} />
-      </pre>
-
       {language === "mermaid" && (
         <Suspense fallback={null}>
           <MermaidView props={props} />

apps/client/src/features/editor/components/code-block/code-block.module.css

@@ -17,7 +17,14 @@
     justify-content: center;
 }
 
+/* #146: the menu now follows the <pre> in the DOM (so the editable contentDOM is
+   FIRST and click hit-testing is correct). Lift it back ABOVE the code visually
+   with flex `order` — the .codeBlock wrapper is a flex column (see code.css) —
+   so the menu still reads as a row above the code, exactly as before, without
+   sitting in-flow before the contentDOM. */
 .menuGroup {
+    order: -1;
+
     @media print {
         display: none;
     }

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);
+  });
+});

apps/client/src/features/editor/components/common/editor-paste-handler.tsx

@@ -22,12 +22,81 @@ const ATTACHMENT_NODE_TYPES = [
 
 const ATTACHMENT_URL_RE = /\/api\/files\/([0-9a-f-]+)\//;
 
+const SCROLLABLE_OVERFLOW = new Set(["auto", "scroll", "overlay"]);
+
+/**
+ * Collect every scrollable ancestor of the editor DOM whose hit-test layer
+ * could be stale after a paste, plus the document scrolling element. We nudge
+ * ALL of them (a zero-delta nudge is harmless) because the real scroll container
+ * varies — a styled overflow ancestor on most pages, the document itself on
+ * others — and `overflow: overlay` (common on macOS, where #146 reproduces)
+ * must count as scrollable too. Called only AFTER the paste has committed, so
+ * `scrollHeight > clientHeight` reflects the inserted content.
+ */
+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.
+  let el: HTMLElement | null = node.parentElement;
+  while (el) {
+    const { overflowX, overflowY } = getComputedStyle(el);
+    const scrollsY =
+      SCROLLABLE_OVERFLOW.has(overflowY) && el.scrollHeight > el.clientHeight;
+    const scrollsX =
+      SCROLLABLE_OVERFLOW.has(overflowX) && el.scrollWidth > el.clientWidth;
+    if (scrollsY || scrollsX) targets.push(el);
+    el = el.parentElement;
+  }
+  const docEl = document.scrollingElement as HTMLElement | null;
+  if (docEl && !targets.includes(docEl)) targets.push(docEl);
+  return targets;
+}
+
+/**
+ * Re-flow the editor's scroll containers after a paste so the browser refreshes
+ * its click hit-testing geometry (#146). Pasting markdown/code inserts React
+ * NodeViews that mount ASYNCHRONOUSLY; until the next reflow, ProseMirror's
+ * posAtCoords/caretRangeFromPoint can map a click to a stale (offset) line —
+ * which users observed clears itself on any scroll. We reproduce that scroll's
+ * side effect with a ZERO-delta nudge (re-assign scrollTop/Left to their current
+ * value), invalidating the hit-test layer WITHOUT moving the viewport. The
+ * 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.
+ */
+export function reflowAfterPaste(editor: Editor) {
+  const dom = editor.view.dom as HTMLElement;
+  requestAnimationFrame(() => {
+    requestAnimationFrame(() => {
+      for (const el of collectScrollAncestors(dom)) {
+        // 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);
+      }
+    });
+  });
+}
+
 export const handlePaste = (
   editor: Editor,
   event: ClipboardEvent,
   pageId: string,
   creatorId?: string,
 ) => {
+  // 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");
 
   if (INTERNAL_LINK_REGEX.test(clipboardData)) {

apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx

@@ -29,10 +29,19 @@ export default function FootnoteDefinitionView(props: NodeViewProps) {
       className={classes.definition}
       style={{ ["--footnote-number" as any]: `"${number}"` }}
     >
-      <span className={classes.definitionMarker} contentEditable={false}>
+      {/* #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`. The second #146 mitigation lives in
+          editor-paste-handler.tsx (reflowAfterPaste). */}
+      <NodeViewContent className={classes.definitionContent} />
+      <span
+        className={classes.definitionMarker}
+        contentEditable={false}
+        aria-hidden="true"
+      >
         {number}.
       </span>
-      <NodeViewContent className={classes.definitionContent} />
       <span
         className={classes.backLink}
         contentEditable={false}

apps/client/src/features/editor/components/footnote/footnote-views.structure.test.tsx

@@ -0,0 +1,143 @@
+import { describe, it, expect, vi } from "vitest";
+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
+ * 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.
+ *
+ * We stub `@tiptap/react` so the views render as plain DOM and we can inspect
+ * the child order our JSX produces — that order is exactly what regresses, and
+ * it does not depend on a live editor. The stubbed `NodeViewContent` carries the
+ * real `data-node-view-content` marker tiptap uses, so the assertion mirrors
+ * production. This test passes on the fixed order and FAILS on the pre-fix order
+ * (chrome-before-content).
+ */
+vi.mock("@tiptap/react", () => ({
+  NodeViewWrapper: ({ children, ...props }: any) => (
+    <div data-testid="nvw" {...props}>
+      {children}
+    </div>
+  ),
+  // Mirror the real contentDOM marker so the guard matches production output.
+  NodeViewContent: (props: any) => <div data-node-view-content="" {...props} />,
+}));
+
+vi.mock("react-i18next", () => ({
+  useTranslation: () => ({ t: (key: string) => key }),
+}));
+
+// footnote-definition-view reads a cached number from the numbering plugin;
+// stub it so we don't need a live ProseMirror state.
+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).
+const props = {
+  node: { attrs: { id: "fn-1" }, textContent: "" },
+  editor: { state: {}, isEditable: true, commands: {} },
+  getPos: () => 0,
+  updateAttributes: () => {},
+  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 editable NodeView contentDOM-first invariant", () => {
+  it.each(cases)(
+    "$name renders the editable contentDOM ahead of all non-editable chrome",
+    ({ ui }) => {
+      const { getByTestId } = render(ui);
+      const wrapper = getByTestId("nvw");
+
+      const content = wrapper.querySelector("[data-node-view-content]");
+      expect(content).not.toBeNull();
+
+      // 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);
+
+      // Chrome exists (separator/heading/marker/back-link/menu)...
+      const nonEditable = wrapper.querySelectorAll('[contenteditable="false"]');
+      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)) {
+        const pos = content!.compareDocumentPosition(el);
+        expect(pos & Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy();
+      }
+    },
+  );
+});

apps/client/src/features/editor/components/footnote/footnote.module.css

@@ -57,14 +57,19 @@
   word-break: break-word;
 }
 
-/* Bottom footnotes container. */
+/* Bottom footnotes container. Flex column so the heading (rendered AFTER the
+   editable NodeViewContent in the DOM for #146) is lifted back above the list
+   visually via `order`, instead of sitting in-flow before the contentDOM. */
 .list {
+  display: flex;
+  flex-direction: column;
   margin-top: var(--mantine-spacing-lg);
   padding-top: var(--mantine-spacing-md);
   border-top: 1px solid var(--mantine-color-default-border);
 }
 
 .listHeading {
+  order: -1; /* visually above the list, though it follows it in the DOM (#146) */
   font-weight: 600;
   font-size: var(--mantine-font-size-sm);
   color: var(--mantine-color-dimmed);
@@ -83,6 +88,7 @@
 }
 
 .definitionMarker {
+  order: -1; /* keep the "N." marker on the LEFT though it follows content in DOM (#146) */
   flex: 0 0 auto;
   min-width: 1.5em;
   /* Right-align within the narrow column so the period sits next to the text

apps/client/src/features/editor/components/footnote/footnotes-list-view.tsx

@@ -3,18 +3,39 @@ import { useTranslation } from "react-i18next";
 import classes from "./footnote.module.css";
 
 /**
- * NodeView for the bottom footnotes container. Renders a visual separator and a
- * localized heading, then the editable list of definitions via NodeViewContent.
+ * NodeView for the bottom footnotes container: the editable list of definitions
+ * (NodeViewContent) plus a visual separator + localized heading.
+ *
+ * #146: the editable NodeViewContent MUST be the FIRST child in the DOM. A
+ * non-editable block rendered before it (the old separator + heading) makes the
+ * browser's click hit-testing (posAtCoords → caretRangeFromPoint) miss the
+ * contentDOM and snap the caret to the previous node (several lines above, into
+ * 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>
-      <div className={classes.list} contentEditable={false}>
-        <div className={classes.listHeading}>{t("Footnotes")}</div>
-      </div>
+    // 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}
+        aria-hidden="true"
+      >
+        {t("Footnotes")}
+      </div>
     </NodeViewWrapper>
   );
 }

apps/client/src/features/editor/styles/code.css

@@ -1,5 +1,9 @@
 .ProseMirror {
   .codeBlock {
+    /* #146: flex column so the menu (rendered AFTER <pre> in the DOM, so the
+       editable contentDOM is first) is lifted back above the code via `order`. */
+    display: flex;
+    flex-direction: column;
     padding: 4px;
     border-radius: var(--mantine-radius-default);
     background-color: light-dark(var(--mantine-color-gray-0),  var(--mantine-color-dark-8));