diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f03b74b..efb96a72 100644 --- a/CHANGELOG.md +++ b/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, 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 0ff2fe36..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 @@ -47,6 +47,26 @@ export default function CodeBlockView(props: NodeViewProps) { return ( + {/* #146: the editable 
 (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). */}
+      
+
       
       
 
-      
-
       {language === "mermaid" && (
         
           
diff --git a/apps/client/src/features/editor/components/code-block/code-block.module.css b/apps/client/src/features/editor/components/code-block/code-block.module.css
index 6e0a5dd3..4ecda370 100644
--- a/apps/client/src/features/editor/components/code-block/code-block.module.css
+++ b/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 
 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;
     }
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();
+
+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 85d49872..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
@@ -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)) {
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 2685fbc3..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
@@ -29,10 +29,19 @@ export default function FootnoteDefinitionView(props: NodeViewProps) {
       className={classes.definition}
       style={{ ["--footnote-number" as any]: `"${number}"` }}
     >
-      
+      {/* #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). */}
+      
+      
-      
        ({
+  NodeViewWrapper: ({ children, ...props }: any) => (
+    

+      {children}
+    

+  ),
+  // Mirror the real contentDOM marker so the guard matches production output.
+  NodeViewContent: (props: any) => 
,
+}));
+
+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) => (
+    

+      {children}
+    

+  ),
+  Select: () => null,
+  Tooltip: ({ children }: any) => <>{children},
+  ActionIcon: ({ children, onClick }: any) => (
+    
+  ),
+}));
+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:  },
+  { name: "FootnoteDefinitionView", ui:  },
+  { name: "CodeBlockView", ui:  },
+];
+
+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 
). 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();
+      }
+    },
+  );
+});
diff --git a/apps/client/src/features/editor/components/footnote/footnote.module.css b/apps/client/src/features/editor/components/footnote/footnote.module.css
index af467c5b..8f1ba9e7 100644
--- a/apps/client/src/features/editor/components/footnote/footnote.module.css
+++ b/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
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 7b2eb51b..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
@@ -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 (
-    
-      

-        
{t("Footnotes")}

-      

+    // 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).
+    
       
+      
     
   );
 }
diff --git a/apps/client/src/features/editor/styles/code.css b/apps/client/src/features/editor/styles/code.css
index fba5db91..100e4153 100644
--- a/apps/client/src/features/editor/styles/code.css
+++ b/apps/client/src/features/editor/styles/code.css
@@ -1,5 +1,9 @@
 .ProseMirror {
   .codeBlock {
+    /* #146: flex column so the menu (rendered AFTER 
 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));