vvzvlad/gitmost
1
0
Fork 0
Code Issues 14 Pull Requests 12 Packages Projects Releases Activity

fix(editor): render NodeViewContent first so click hit-testing isn't offset (#146) #147

Merged
vvzvlad merged 4 commits from fix/146-nodeview-content-order into develop 2026-06-24 04:40:59 +03:00
Conversation 2 Commits 4 Files Changed 10 +460 -21
Ghost commented 2026-06-23 21:48:52 +03:00
Copy Link

Fixes #146 — caret/selection offset in NodeViews (content-first DOM order)

Three editable NodeViews rendered a contentEditable={false} chrome element in flow, before NodeViewContent. On macOS the browser's click hit-testing (posAtCoords → caretRangeFromPoint) then misses the contentDOM and snaps the caret to the previous node — caret/selection lands a line (code block) or several lines (footnotes → into the body) above the click. Same architectural defect in 3 places; transclusion (absolute overlay) was the "good" reference.

Fix (issue's recommended plan)

Make NodeViewContent the first child in the DOM; move the non-editable chrome after it and restore its visual position with CSS:

  • code-block-view: <pre><NodeViewContent/></pre> first; language/copy menu follows, lifted above via flex order (.codeBlock is a flex column).
  • footnotes-list-view: content first; "Footnotes" heading follows, lifted above via flex order (.list flex column; separator border on the container).
  • footnote-definition-view: content first; "N." marker follows with order:-1 (stays left); ↩ back-link stays right.

Layout visually unchanged.

Verification

Built a real-browser harness (Playwright Chromium + WebKit) running the actual client (real extensions.ts, real CSS via postcss-mantine, real Mantine code-block-view, real footnote numbering plugin):

  • Before the fix: confirmed the antipattern statically (chrome before NodeViewContent in all 3).
  • After the fix: the contentDOM is the first child of every editable NodeView wrapper — no contentEditable=false element precedes it (the issue's Level-2 invariant) — and a screenshot confirms the menu/heading/marker render in their original positions.

⚠️ The caret-offset itself is macOS-specific text hit-testing — it does NOT reproduce in headless Chromium/WebKit on Linux (verified exhaustively: both engines, full client, single clicks, glyph-rect posAtCoords, drag-selection, the exact footnote re-click sequence, zoom 0.67–1.5 — all map correctly). So please confirm the visible fix on macOS. The structural root cause (chrome before contentDOM) is removed regardless.

DoD note: the Level-2 vitest structural guard from the issue isn't added here (mounting the React NodeViews in jsdom is fragile and jsdom has no layout); the invariant is instead verified via the real-browser DOM-order check above. Happy to add a guard if wanted.

## Fixes #146 — caret/selection offset in NodeViews (content-first DOM order) Three editable NodeViews rendered a `contentEditable={false}` chrome element **in flow, before** `NodeViewContent`. On macOS the browser's click hit-testing (`posAtCoords` → `caretRangeFromPoint`) then misses the contentDOM and snaps the caret to the previous node — caret/selection lands a line (code block) or several lines (footnotes → into the body) above the click. Same architectural defect in 3 places; transclusion (absolute overlay) was the "good" reference. ### Fix (issue's recommended plan) Make `NodeViewContent` the **first** child in the DOM; move the non-editable chrome **after** it and restore its visual position with CSS: - **code-block-view**: `<pre><NodeViewContent/></pre>` first; language/copy menu follows, lifted above via flex `order` (`.codeBlock` is a flex column). - **footnotes-list-view**: content first; "Footnotes" heading follows, lifted above via flex `order` (`.list` flex column; separator border on the container). - **footnote-definition-view**: content first; "N." marker follows with `order:-1` (stays left); ↩ back-link stays right. Layout visually unchanged. ### Verification Built a real-browser harness (Playwright Chromium + WebKit) running the **actual client** (real `extensions.ts`, real CSS via postcss-mantine, real Mantine code-block-view, real footnote numbering plugin): - **Before the fix**: confirmed the antipattern statically (chrome before `NodeViewContent` in all 3). - **After the fix**: the contentDOM is the **first child** of every editable NodeView wrapper — **no `contentEditable=false` element precedes it** (the issue's Level-2 invariant) — and a screenshot confirms the menu/heading/marker render in their original positions. ⚠️ The caret-offset itself is **macOS-specific** text hit-testing — it does NOT reproduce in headless Chromium/WebKit on Linux (verified exhaustively: both engines, full client, single clicks, glyph-rect `posAtCoords`, drag-selection, the exact footnote re-click sequence, zoom 0.67–1.5 — all map correctly). So please **confirm the visible fix on macOS**. The structural root cause (chrome before contentDOM) is removed regardless. DoD note: the Level-2 vitest structural guard from the issue isn't added here (mounting the React NodeViews in jsdom is fragile and jsdom has no layout); the invariant is instead verified via the real-browser DOM-order check above. Happy to add a guard if wanted.
Ghost added 1 commit 2026-06-23 21:48:53 +03:00
fix(editor): render NodeViewContent first so click hit-testing isn't offset (#146) 38544e2ddc 
Three editable NodeViews rendered a contentEditable=false "chrome" element IN
FLOW BEFORE NodeViewContent. On macOS the browser's click hit-testing
(posAtCoords → caretRangeFromPoint) then misses the contentDOM and snaps the
caret to the previous node — the caret/selection lands a line (code block) or
several lines (footnotes, into the body) above where the user clicked.

Fix (the transclusion pattern / issue #146 plan): make the editable
NodeViewContent the FIRST child in the DOM and move the non-editable chrome
AFTER it, restoring its visual position with CSS:

- code-block-view: <pre><NodeViewContent/></pre> first; the language/copy menu
  follows and is lifted above via flex `order` (.codeBlock is now a flex column).
- footnotes-list-view: NodeViewContent first; the "Footnotes" heading follows and
  is lifted above via flex `order` (.list is a flex column; the separator border
  stays on the container).
- footnote-definition-view: NodeViewContent first; the "N." marker follows with
  `order:-1` to stay on the left; the ↩ back-link stays on the right.

Layout is visually unchanged. Verified in a real browser (Chromium): the
contentDOM is now the first child of every editable NodeView wrapper (no
contentEditable=false element precedes it), and the menu/heading/marker still
render in their original positions.

NOTE: the caret-offset itself is macOS-specific text hit-testing and does not
reproduce in headless Chromium/WebKit on Linux (verified extensively), so the
visible fix is best confirmed on macOS.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
vvzvlad commented 2026-06-24 00:00:27 +03:00
Owner
Copy Link

Code review — PR #147: render NodeViewContent first so click hit-testing isn't offset (#146)

Verdict: Approve with comments. The fix is correct and faithful. Independent stability and regression passes both came back essentially clean after verification — the DOM reorder is compensated by flex order with no behavioral or visual regression I could substantiate (separator border, spacing, marker/back-link/heading positions, mermaid hidden, copy-to-clipboard and footnote numbering all verified DOM-order-independent). Two minor asks below, plus one non-blocking architecture note.

Scope reviewed: git diff develop..fix/146-nodeview-content-order — 6 files, +54/−20, all under apps/client/src/features/editor. Reviewed across security, stability, conventions, regressions, test-coverage, simplification, and architecture.

Critical

None.

Warnings

  • [test coverage] The fix ships without a regression guard for the load-bearing "contentDOM is the first child" invariant — apps/client/src/features/editor/components/footnote/footnotes-list-view.tsx:20-27 (same for footnote-definition-view.tsx and code-block-view.tsx). The whole fix rests on <NodeViewContent> being the first DOM child; if a future edit reinserts non-editable chrome before it, the macOS caret bug returns silently with nothing to catch it. The "jsdom can't test this" reasoning is valid for the caret symptom (needs real-browser layout) but not for a structural guard: mounting the two footnote NodeViews in the project's existing jsdom + @testing-library/react setup was verified to work with trivial mock props, and an assertion that the wrapper's first element child carries data-node-view-content (with no [contenteditable="false"] element preceding it) passes on this code and fails on the pre-fix order — a real regression lock, zero new deps. (CodeBlockView is the genuinely fragile one — needs window.matchMedia + MantineProvider — so cover it only if cheap.) Fix: add footnote/footnote-views.structure.test.tsx covering at least the two footnote views.

Suggestions

  • [conventions] Inaccurate code comment — claims "top-right overlay (the transclusion pattern)" but the implementation uses flex order — apps/client/src/features/editor/components/code-block/code-block-view.tsx:50-54. The comment says the menu "is positioned as a top-right overlay via CSS (the transclusion pattern), so it no longer sits in-flow above the code." The actual CSS (code-block.module.css:25 → .menuGroup { order: -1 }; styles/code.css → .codeBlock { display: flex; flex-direction: column }) keeps the menu fully in flow as a full-width row above the code, lifted by flex order — it is neither an overlay nor the transclusion pattern (the real transclusion view uses position: absolute to leave the flow). The comment misdescribes both its own mechanism and the cited reference. Fix: reword to e.g. "content first in the DOM; the menu is rendered after it and lifted back above visually via flex order: -1 (the .codeBlock wrapper is a flex column)."
  • (Optional, not a regression.) A long unbreakable code line can overflow horizontally, but this behaves identically to the previous block layout — for a flex-direction: column container the cross (horizontal) axis matches block width behavior, so nothing new is introduced here. If you want to harden it regardless, pre { min-width: 0; overflow-x: auto; } makes such lines scroll inside the block. Out of scope for this PR.

Test coverage

The new behavior is JSX reordering + CSS order, which this project generally does not unit-test — fair. But this is a bug fix, and the cheapest meaningful coverage (a structural DOM-order guard on the two footnote views) is feasible in the existing harness and currently absent. No existing test references any of the three NodeViews. See the warning above.

Architecture & design (forward-looking, non-blocking)

  • The "contentDOM must be the first child" invariant is enforced only by convention across 3+ sites. The repo has ~5 editable NodeViews (NodeViewContent users): code-block, footnote-definition, footnotes-list, transclusion, callout. This PR hand-fixes the same #146 defect in three of them with the same recipe (content first + flex order: -1). The invariant is real and load-bearing but lives nowhere enforceable, so a 4th NodeView (or an edit to callout, safe today only because content is its sole child) can silently reintroduce it. Note also two divergent seams: transclusion keeps chrome before content but uses position: absolute (out of flow), while this PR keeps chrome in flow and reorders with flex.
    • Option A — Keep as-is (concrete repetition) (effort: small, already done). Pros: zero added abstraction/risk; per-site #146 comments document it at each call site; only ~5 heterogeneous views. Cons: invariant stays tribal knowledge; a recurrence is silent and macOS-only.
    • Option B — Regression test asserting contentDOM-first for every editable NodeView (effort: medium). Pros: machine-checks the invariant across all current and future views; matches the existing per-component test style; doubles as the missing test-coverage guard above. Cons: needs a small TipTap/jsdom render harness; asserts the proxy (DOM order) rather than the macOS symptom — acceptable, since DOM-order-first is the fix.
    • Option C — Extract a shared <EditableNodeView> wrapper that always renders content first (effort: large). Pros: makes "content first" impossible to get wrong by construction; one place to document the positioning contract. Cons: speculative for ~5 structurally divergent views (code-block's menu + mermaid vs. footnote's marker + back-link vs. transclusion's absolute overlay); likely a leaky abstraction worse than the repetition; largest blast radius for a 6-file fix.
    • Recommendation: Option B if you want durable protection (it covers the test gap at the same time); Option A is entirely defensible for a fix this small. Explicitly not Option C — speculative generality for so few divergent views.

Multi-aspect review (security · stability · conventions · regressions · test-coverage · simplification · architecture). Security, simplification, and regressions returned clean.

## Code review — PR #147: render NodeViewContent first so click hit-testing isn't offset (#146) **Verdict: Approve with comments.** The fix is correct and faithful. Independent stability and regression passes both came back essentially clean after verification — the DOM reorder is compensated by flex `order` with no behavioral or visual regression I could substantiate (separator border, spacing, marker/back-link/heading positions, mermaid `hidden`, copy-to-clipboard and footnote numbering all verified DOM-order-independent). Two minor asks below, plus one non-blocking architecture note. *Scope reviewed: `git diff develop..fix/146-nodeview-content-order` — 6 files, +54/−20, all under `apps/client/src/features/editor`. Reviewed across security, stability, conventions, regressions, test-coverage, simplification, and architecture.* ### Critical None. ### Warnings - **[test coverage] The fix ships without a regression guard for the load-bearing "contentDOM is the first child" invariant** — `apps/client/src/features/editor/components/footnote/footnotes-list-view.tsx:20-27` (same for `footnote-definition-view.tsx` and `code-block-view.tsx`). The whole fix rests on `<NodeViewContent>` being the first DOM child; if a future edit reinserts non-editable chrome before it, the macOS caret bug returns silently with nothing to catch it. The "jsdom can't test this" reasoning is valid for the *caret symptom* (needs real-browser layout) but **not** for a *structural* guard: mounting the two footnote NodeViews in the project's existing jsdom + `@testing-library/react` setup was verified to work with trivial mock props, and an assertion that the wrapper's first element child carries `data-node-view-content` (with no `[contenteditable="false"]` element preceding it) passes on this code and **fails** on the pre-fix order — a real regression lock, zero new deps. (`CodeBlockView` is the genuinely fragile one — needs `window.matchMedia` + `MantineProvider` — so cover it only if cheap.) *Fix:* add `footnote/footnote-views.structure.test.tsx` covering at least the two footnote views. ### Suggestions - **[conventions] Inaccurate code comment — claims "top-right overlay (the transclusion pattern)" but the implementation uses flex `order`** — `apps/client/src/features/editor/components/code-block/code-block-view.tsx:50-54`. The comment says the menu "is positioned as a top-right overlay via CSS (the transclusion pattern), so it no longer sits in-flow above the code." The actual CSS (`code-block.module.css:25` → `.menuGroup { order: -1 }`; `styles/code.css` → `.codeBlock { display: flex; flex-direction: column }`) keeps the menu **fully in flow** as a full-width row above the code, lifted by flex `order` — it is neither an overlay nor the transclusion pattern (the real transclusion view uses `position: absolute` to leave the flow). The comment misdescribes both its own mechanism and the cited reference. *Fix:* reword to e.g. "content first in the DOM; the menu is rendered after it and lifted back above visually via flex `order: -1` (the `.codeBlock` wrapper is a flex column)." - *(Optional, not a regression.)* A long unbreakable code line can overflow horizontally, but this behaves identically to the previous block layout — for a `flex-direction: column` container the cross (horizontal) axis matches block width behavior, so nothing new is introduced here. If you want to harden it regardless, `pre { min-width: 0; overflow-x: auto; }` makes such lines scroll inside the block. Out of scope for this PR. ### Test coverage The new behavior is JSX reordering + CSS `order`, which this project generally does not unit-test — fair. But this is a **bug fix**, and the cheapest meaningful coverage (a structural DOM-order guard on the two footnote views) is feasible in the existing harness and currently absent. No existing test references any of the three NodeViews. See the warning above. ### Architecture & design (forward-looking, non-blocking) - **The "contentDOM must be the first child" invariant is enforced only by convention across 3+ sites.** The repo has ~5 editable NodeViews (`NodeViewContent` users): code-block, footnote-definition, footnotes-list, transclusion, callout. This PR hand-fixes the same #146 defect in three of them with the same recipe (content first + flex `order: -1`). The invariant is real and load-bearing but lives nowhere enforceable, so a 4th NodeView (or an edit to `callout`, safe today only because content is its sole child) can silently reintroduce it. Note also two divergent seams: transclusion keeps chrome *before* content but uses `position: absolute` (out of flow), while this PR keeps chrome in flow and reorders with flex. - *Option A — Keep as-is (concrete repetition)* (effort: small, already done). Pros: zero added abstraction/risk; per-site `#146` comments document it at each call site; only ~5 heterogeneous views. Cons: invariant stays tribal knowledge; a recurrence is silent and macOS-only. - *Option B — Regression test asserting contentDOM-first for every editable NodeView* (effort: medium). Pros: machine-checks the invariant across all current and future views; matches the existing per-component test style; **doubles as the missing test-coverage guard above**. Cons: needs a small TipTap/jsdom render harness; asserts the proxy (DOM order) rather than the macOS symptom — acceptable, since DOM-order-first *is* the fix. - *Option C — Extract a shared `<EditableNodeView>` wrapper that always renders content first* (effort: large). Pros: makes "content first" impossible to get wrong by construction; one place to document the positioning contract. Cons: speculative for ~5 structurally divergent views (code-block's menu + mermaid vs. footnote's marker + back-link vs. transclusion's absolute overlay); likely a leaky abstraction worse than the repetition; largest blast radius for a 6-file fix. - **Recommendation:** Option B if you want durable protection (it covers the test gap at the same time); Option A is entirely defensible for a fix this small. Explicitly **not** Option C — speculative generality for so few divergent views. --- *Multi-aspect review (security · stability · conventions · regressions · test-coverage · simplification · architecture). Security, simplification, and regressions returned clean.*
Ghost added 1 commit 2026-06-24 00:38:09 +03:00
fix(editor): reflow scroll containers after paste to refresh click hit-testing (#146) 1c39a45bc5 
Pasting markdown/code inserts React NodeViews that mount asynchronously; until
the next reflow the browser's hit-test geometry is stale, so ProseMirror's
posAtCoords/caretRangeFromPoint maps a click to the wrong (offset) line — which
users reported clears itself on any scroll. Reproduce that scroll's side effect
with a ZERO-delta nudge (re-assign scrollTop/scrollLeft to their current value)
on every scrollable ancestor + the document scrolling element, run across two
animation frames so it lands after the pasted content + NodeViews commit. The
nudge does not move the viewport.

Wired into editor-paste-handler's handlePaste, which ProseMirror's someProp runs
(as an editorProps handler) before the MarkdownClipboard plugin that performs the
markdown/code insert — so the nudge is scheduled on exactly the paste path that
triggers the bug. Complements the structural NodeViewContent-order fix in this
branch.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Ghost added 1 commit 2026-06-24 00:47:42 +03:00
test(editor): guard #146 contentDOM-first invariant + fix code-block comment (PR #147 review) 67057de214 
Addresses the #147 review (Approve with comments):
- Add footnote-views.structure.test.tsx: a structural regression guard asserting
  the editable NodeViewContent is the FIRST child of FootnotesListView and
  FootnoteDefinitionView, with no contenteditable=false chrome before it. The
  whole #146 fix rests on this DOM-order invariant; the macOS caret symptom needs
  a real browser, but the order proxy is testable in jsdom. Stubs @tiptap/react
  so the views render as plain DOM — the test passes on the fixed order and fails
  on the pre-fix chrome-first order.
- Reword the code-block-view comment: it claimed a "top-right overlay (the
  transclusion pattern)", but the menu stays fully in flow as a full-width row
  lifted via flex `order: -1` (the .codeBlock wrapper is a flex column). No
  overlay/absolute positioning.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
vvzvlad commented 2026-06-24 02:03:39 +03:00
Owner
Copy Link

после исправления можно мержить

после исправления можно мержить
vvzvlad added 1 commit 2026-06-24 04:37:12 +03:00
test(editor): address PR #147 review — reflow tests, code-block guard, a11y 8bb441870a 
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>
vvzvlad merged commit 8a6ee78c44 into develop 2026-06-24 04:40:59 +03:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
bug
Something isn't working
 epic
Large multi-phase effort spanning many changes
 feature
New functionality request
 idea
Idea / proposal for discussion
 needs-human
эскалация: нужно решение человека
 review/approved
в последнем ревью нет открытых blocking-находок
 review/changes-requested
последнее ревью оставило открытые blocking-находки
 review/needs
head не ревьюился (head != reviewed_head)
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent_coder agent_reviewer vvzvlad
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vvzvlad/gitmost#147
Delete Branch "fix/146-nodeview-content-order"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?