fix: PageDown now advances caret by a full page of lines#7437
Open
JohnMcLear wants to merge 1 commit intoether:developfrom
Open
fix: PageDown now advances caret by a full page of lines#7437JohnMcLear wants to merge 1 commit intoether:developfrom
JohnMcLear wants to merge 1 commit intoether:developfrom
Conversation
PageDown was broken — it moved the caret to the last visible line of the current viewport instead of advancing by one page. This caused it to get "stuck" at the bottom of the viewport. The old code set the caret to oldVisibleLineRange[1] - 1 (the last visible line), which was essentially a no-op for scrolling. The fix mirrors the PageUp logic: advance/retreat by numberOfLinesInViewport. Also simplified the clamping logic for both selStart and selEnd. Fixes: ether#6710 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review Summary by QodoFix PageDown to advance caret by full page of lines
WalkthroughsDescription• PageDown now advances caret by full page instead of getting stuck • Simplified min/max clamping logic using Math.max/Math.min • Added three Playwright regression tests for PageUp/PageDown behavior Diagramflowchart LR
A["PageDown Handler"] -->|Old Logic| B["Set caret to last visible line"]
B -->|Result| C["Caret stuck at viewport bottom"]
A -->|New Logic| D["Add numberOfLinesInViewport to caret"]
D -->|Result| E["Caret advances by full page"]
F["Clamping Logic"] -->|Old| G["Multiple if statements"]
F -->|New| H["Math.max/Math.min calls"]
File Changes1. src/static/js/ace2_inner.ts
|
Code Review by Qodo
1. PageDown test not regressing
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+12
to
+49
| test('PageDown moves caret forward by a page of lines', async function ({page}) { | ||
| const padBody = await getPadBody(page); | ||
| await clearPadContent(page); | ||
|
|
||
| // Create enough lines to require scrolling (more than a viewport) | ||
| for (let i = 0; i < 60; i++) { | ||
| await writeToPad(page, `line ${i + 1}`); | ||
| await page.keyboard.press('Enter'); | ||
| } | ||
|
|
||
| // Move caret to the first line | ||
| await page.keyboard.down('Control'); | ||
| await page.keyboard.press('Home'); | ||
| await page.keyboard.up('Control'); | ||
| await page.waitForTimeout(200); | ||
|
|
||
| // Press PageDown — the handler uses a 200ms setTimeout internally | ||
| await page.keyboard.press('PageDown'); | ||
| await page.waitForTimeout(1000); | ||
|
|
||
| // The caret should have moved significantly forward (not stuck at the bottom of first viewport) | ||
| // Get the current line by checking which div has the caret | ||
| const innerFrame = page.frame('ace_inner')!; | ||
| const selection = await innerFrame.evaluate(() => { | ||
| const sel = document.getSelection(); | ||
| if (!sel || !sel.focusNode) return 0; | ||
| // Walk up to find the div | ||
| let node = sel.focusNode as HTMLElement; | ||
| while (node && node.tagName !== 'DIV') node = node.parentElement!; | ||
| if (!node) return 0; | ||
| // Find the index of this div | ||
| const divs = Array.from(document.getElementById('innerdocbody')!.children); | ||
| return divs.indexOf(node); | ||
| }); | ||
|
|
||
| // The caret should have advanced (viewport may be small in headless mode) | ||
| expect(selection).toBeGreaterThan(2); | ||
| }); |
There was a problem hiding this comment.
1. pagedown test not regressing 📘 Rule violation ≡ Correctness
The new PageDown regression test only presses PageDown once and asserts the caret moved past line 2, which would also pass with the pre-fix behavior that moved the caret to the bottom of the current viewport (the reported bug manifests on repeated PageDown presses). This means the added test does not meaningfully fail if the fix is reverted, reducing protection against regression.
Agent Prompt
## Issue description
The PageDown regression test is too weak: it presses `PageDown` once and asserts the caret is past line 2, which can still be true with the old buggy behavior (the bug was that repeated PageDown presses get stuck in the same viewport).
## Issue Context
The reported regression (issue #6710) is about PageDown not advancing by a full page repeatedly. A meaningful regression test should fail if PageDown only moves to the bottom of the current viewport and then stops making progress.
## Fix Focus Areas
- src/tests/frontend-new/specs/page_up_down.spec.ts[12-49]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
2840
to
+2849
| const numberOfLinesInViewport = newVisibleLineRange[1] - newVisibleLineRange[0]; | ||
|
|
||
| if (isPageUp && padShortcutEnabled.pageUp) { | ||
| // move to the bottom line +1 in the viewport (essentially skipping over a page) | ||
| rep.selEnd[0] -= numberOfLinesInViewport; | ||
| // move to the bottom line +1 in the viewport (essentially skipping over a page) | ||
| rep.selStart[0] -= numberOfLinesInViewport; | ||
| rep.selEnd[0] -= numberOfLinesInViewport; | ||
| } | ||
|
|
||
| // if we hit page down | ||
| if (isPageDown && padShortcutEnabled.pageDown) { | ||
| // If the new viewpoint position is actually further than where we are right now | ||
| if (rep.selEnd[0] >= oldVisibleLineRange[0]) { | ||
| // dont go further in the page down than what's visible IE go from 0 to 50 | ||
| // if 50 is visible on screen but dont go below that else we miss content | ||
| rep.selStart[0] = oldVisibleLineRange[1] - 1; | ||
| // dont go further in the page down than what's visible IE go from 0 to 50 | ||
| // if 50 is visible on screen but dont go below that else we miss content | ||
| rep.selEnd[0] = oldVisibleLineRange[1] - 1; | ||
| } | ||
| rep.selStart[0] += numberOfLinesInViewport; | ||
| rep.selEnd[0] += numberOfLinesInViewport; |
There was a problem hiding this comment.
2. Pagedown advances too little 🐞 Bug ≡ Correctness
numberOfLinesInViewport is computed as newVisibleLineRange[1] - newVisibleLineRange[0] even though getVisibleLineRange() returns an inclusive [first,last] range, so PageDown/PageUp moves one line less than a full page (and moves 0 lines if only one line is visible). Because PageDown now uses this value, paging can still appear stuck or under-advance in small viewports and does not match the PR intent of moving a full page.
Agent Prompt
### Issue description
`getVisibleLineRange()` returns an inclusive range, but `numberOfLinesInViewport` is computed as a simple difference. This yields an off-by-one page size (and 0 when only one line is visible), which PageDown now uses.
### Issue Context
This impacts PageDown behavior introduced in this PR and also affects PageUp symmetry.
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2836-2849]
- src/static/js/scroll.ts[317-330]
### Suggested change
Compute the page size as:
- `const numberOfLinesInViewport = (newVisibleLineRange[1] - newVisibleLineRange[0]) + 1;`
Optionally guard with `Math.max(1, ...)` for safety.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix in
ace2_inner.ts: PageDown now mirrors the PageUp logic — advances the caret bynumberOfLinesInViewportlines instead of jumping to the last visible line of the current viewport.Root Cause
The PageDown handler set the caret to
oldVisibleLineRange[1] - 1(the last visible line in the current viewport). This was essentially a no-op — the caret moved to the bottom of the current view, thenscroll.setScrollYscrolled to the caret's position, which was already visible. On the next PageDown press, the same thing happened — stuck at the bottom.PageUp already worked correctly by subtracting
numberOfLinesInViewport. The fix applies the same symmetric logic to PageDown (adding instead of subtracting).Also simplified the min/max clamping to use
Math.max/Math.minfor bothselStartandselEnd.Test plan
--repeat-each=3, retries configured for CI timing sensitivitymessages.tstests pass (10/10)Fixes #6710
🤖 Generated with Claude Code