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
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 epic
Large multi-phase effort spanning many changes
 feature
New functionality request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 idea
Idea / proposal for discussion
 invalid
This doesn't seem right
 needs-human
эскалация: нужно решение человека
 question
Further information is requested
 refactor
Code cleanup / refactoring
 review/approved
в последнем ревью нет открытых blocking-находок
 review/changes-requested
последнее ревью оставило открытые blocking-находки
 review/needs
head не ревьюился (head != reviewed_head)
 security
Security / hardening issue
 status/blocked
ждёт зависимость blocked_by
 status/done
закрыто и проверено
 status/in-progress
в активной работе (мягкая заявка)
 status/ready
специфицировано, не заблокировано, ждёт исполнителя
 test
Test coverage / test infrastructure
 wontfix
This will not be worked on
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?