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). */}
();
+
+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). */}
-
+
({
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).
@@ -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: },
{ name: "FootnoteDefinitionView", ui: },
+ { name: "CodeBlockView", ui: },
];
-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 ). 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 (
-
+ // 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).
+
-
+