fix(editor): arrow key navigation across page boundaries and auto-scroll (SD-1950)#2191
fix(editor): arrow key navigation across page boundaries and auto-scroll (SD-1950)#2191
Conversation
…oll (SD-1950) Fix vertical arrow key navigation that would jump sections or get stuck at page boundaries, and add auto-scroll to keep the caret visible during keyboard navigation. Two bugs in the vertical-navigation extension: 1. ArrowDown would jump thousands of characters when crossing page boundaries because the adjacent line's getBoundingClientRect() returns off-screen coordinates, causing hitTest() to map to the wrong position. 2. ArrowUp would get stuck at certain positions because the adjacent line's DOM center Y falls in a zone where hitTest() maps to the current fragment instead of the adjacent one (fragment boundary misalignment), so the cursor stays at the same position. The fix reads data-pm-start/data-pm-end from the adjacent line element and validates the hit test result against this range. When the hit falls outside the line's PM range (with a tight tolerance of 5), a binary search using computeCaretLayoutRect resolves the correct position at the goal X coordinate within the line. This avoids relying on screen-space hit testing when it produces unreliable results. Additionally adds scrollCaretIntoViewIfNeeded() to PresentationEditor to auto-scroll the viewport after selection changes during keyboard navigation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70917fba64
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!caretEl) { | ||
| // Caret page may not be mounted (virtualized) — scroll page into view | ||
| // to trigger mount; next selection update will handle precise scroll. | ||
| this.#scrollPageIntoView(caretLayout.pageIndex); | ||
| return; |
There was a problem hiding this comment.
This branch assumes a missing caret element is the only signal that the target page is virtualized, but renderCaretOverlay can still create .presentation-editor__selection-caret for an unmounted page via the fallback coordinate path (pageIndex * (pageHeight + pageGap)). In mixed-size documents that fallback Y is not the real page offset, so this method skips #scrollPageIntoView and scrolls using an incorrect caret rect, which can leave ArrowUp/ArrowDown navigation off-screen or jump to the wrong vertical position when crossing virtualized pages.
Useful? React with 👍 / 👎.
tupizz
requested review from
andrii-harbour,
artem-harbour,
caio-pizzol and
harbournick
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@tupizz the approach looks solid — validating hit results against known PM ranges and falling back to layout-based resolution addresses the actual failure mechanism, and the auto-scroll covers the second half of the requirement. left a few inline comments, all non-blocking.
| // Can't measure this position — fall back to pmStart | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
when computeCaretLayoutRect(mid) returns null (e.g. inline node boundary), the whole search stops and returns pmStart — caret jumps to line start. skipping the position keeps the search going:
| // Can't measure this position — fall back to pmStart | |
| break; | |
| } | |
| if (!rect || !Number.isFinite(rect.x)) { | |
| // Can't measure this position — skip it | |
| lo = mid + 1; | |
| continue; | |
| } |
| let bestDist = Infinity; | ||
|
|
||
| // Binary search: characters within a single line have monotonically increasing X | ||
| let lo = pmStart; |
There was a problem hiding this comment.
this assumes LTR (X increases with PM position). for RTL text X goes the other way, so the search moves in the wrong direction. bestPos/bestDist tracking limits the damage but it'll be less accurate. worth a // NOTE: assumes LTR comment so it's clearly a known limitation?
| * @returns {{ pos: number } | null} | ||
| */ | ||
| function resolvePositionAtGoalX(editor, pmStart, pmEnd, goalX) { | ||
| const presentationEditor = editor.presentationEditor; |
There was a problem hiding this comment.
no tests for resolvePositionAtGoalX yet — vertical-navigation.test.js already mocks computeCaretLayoutRect, so adding a few cases (null midpoint, goalX at line edges, exact match) should be quick. worth doing?
| * This mirrors the implementation to allow direct unit testing without | ||
| * bootstrapping the full PresentationEditor. | ||
| */ | ||
| function scrollCaretIntoViewIfNeeded( |
There was a problem hiding this comment.
this copies the production logic inline — if the real method changes, these tests still pass on the old version. extracting the scroll function into a shared module (like renderCaretOverlay in LocalSelectionOverlayRendering.ts) would keep them in sync. worth considering?
3 participants
Summary
Fixes arrow key (↑/↓) navigation in presentation mode that would jump entire sections when moving down across page boundaries, and get stuck in a loop when moving back up. Also adds auto-scroll to keep the caret visible during keyboard navigation.
The Problem
Two distinct bugs in the vertical-navigation extension caused broken cursor movement in multi-page documents:
Bug 1: ArrowDown jumps sections instead of line-by-line
When pressing ArrowDown near a page boundary, the cursor would suddenly jump thousands of characters forward — skipping entire pages. For example, in a 7-page Lorem Ipsum document, pressing ArrowDown at position 3126 would jump to position 6178 (a +3052 character jump), then 9350, then 12378 — each ArrowDown skipping thousands of characters.
Bug 2: ArrowUp gets stuck / loops on the same page
When navigating back up (ArrowUp), the cursor would get stuck at certain positions and refuse to move further. Pressing ArrowUp repeatedly at these positions had zero effect.
Root Cause Analysis
Both bugs originate in the same code path: the vertical-navigation extension's
handleKeyDown→getAdjacentLineClientTarget→getHitFromLayoutCoordspipeline.The extension correctly finds the adjacent line DOM element using
findAdjacentLineElement()(which traverses fragments and pages). However, it then usesgetBoundingClientRect()on that element to get client-space coordinates, and passes those tohitTest()to map to a ProseMirror position.Bug 1 root cause: When the adjacent line is on the next page below the viewport (user hasn't scrolled there yet),
getBoundingClientRect()returns off-screen coordinates (e.g.,clientY=1137when the viewport bottom is at882).hitTest()with these off-screen coordinates produces wildly incorrect PM positions, causing the cursor to jump thousands of characters.Bug 2 root cause: Even when the adjacent line is on-screen (same page, different fragment), the center of its DOM bounding rect can fall in a zone where
hitTest()maps to the wrong fragment. This was traced by testinghitTest()at every Y pixel across the adjacent line:The adjacent line's DOM center is at
y ≈ 229.5, which falls right at the boundary wherehitTest()returns the wrong fragment. So the "new" position equals the current one, and the cursor doesn't move.The Fix
Approach: Validate hit test results against the adjacent line's known PM range
Instead of blindly trusting
hitTest()results (which are unreliable when coordinates are off-screen or at fragment boundaries), we now:Read
data-pm-start/data-pm-endfrom the adjacent line's DOM element — these attributes contain the exact PM position range for that line.Validate the hit test result against this range with a tight tolerance of 5 positions. If the hit falls outside
[pmStart - 5, pmEnd + 5], it's considered unreliable.Fall back to
resolvePositionAtGoalX()— a binary search usingcomputeCaretLayoutRectto find the position within the line's PM range whose layout X is closest to the goal X coordinate. This uses the layout engine's position-to-coordinate mapping (which is always accurate regardless of scroll position), requiring only ~7computeCaretLayoutRectcalls for a typical 80-character line (O(log N)).Why this approach
We considered several alternatives:
hitTest()works correctly 95%+ of the time.lineRange) fails to catch the "stuck cursor" case where the hit lands just a few positions past the adjacent line's end.The chosen approach is minimally invasive: the existing hit test pipeline handles the common case correctly; the validation + fallback only activates when the hit test produces an implausible result. The binary search fallback is efficient and uses the same
computeCaretLayoutRectAPI that the rest of the caret positioning system relies on.Auto-scroll
Added
#scrollCaretIntoViewIfNeeded()toPresentationEditor.ts, called after the caret overlay is rendered. It checks if the caret is outside the scroll container bounds and scrolls minimally (with 20px margin) to keep it visible. Falls back to#scrollPageIntoView()for virtualized pages.Files Changed
vertical-navigation.jsresolvePositionAtGoalX()binary search fallback,pmStart/pmEndextraction from adjacent linePresentationEditor.ts#scrollCaretIntoViewIfNeeded()for auto-scroll on selection changescrollCaretIntoView.test.tsTest plan