Skip to content

Commit 5242574

Browse files
JohnMcLearclaude
andcommitted
fix: PageDown now advances caret by a full page of lines
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: #6710 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
1 parent 32d9c89 commit 5242574

2 files changed

Lines changed: 129 additions & 23 deletions

File tree

src/static/js/ace2_inner.ts

Lines changed: 6 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2840,35 +2840,18 @@ function Ace2Inner(editorInfo, cssManagers) {
28402840
const numberOfLinesInViewport = newVisibleLineRange[1] - newVisibleLineRange[0];
28412841

28422842
if (isPageUp && padShortcutEnabled.pageUp) {
2843-
// move to the bottom line +1 in the viewport (essentially skipping over a page)
2844-
rep.selEnd[0] -= numberOfLinesInViewport;
2845-
// move to the bottom line +1 in the viewport (essentially skipping over a page)
28462843
rep.selStart[0] -= numberOfLinesInViewport;
2844+
rep.selEnd[0] -= numberOfLinesInViewport;
28472845
}
28482846

2849-
// if we hit page down
28502847
if (isPageDown && padShortcutEnabled.pageDown) {
2851-
// If the new viewpoint position is actually further than where we are right now
2852-
if (rep.selEnd[0] >= oldVisibleLineRange[0]) {
2853-
// dont go further in the page down than what's visible IE go from 0 to 50
2854-
// if 50 is visible on screen but dont go below that else we miss content
2855-
rep.selStart[0] = oldVisibleLineRange[1] - 1;
2856-
// dont go further in the page down than what's visible IE go from 0 to 50
2857-
// if 50 is visible on screen but dont go below that else we miss content
2858-
rep.selEnd[0] = oldVisibleLineRange[1] - 1;
2859-
}
2848+
rep.selStart[0] += numberOfLinesInViewport;
2849+
rep.selEnd[0] += numberOfLinesInViewport;
28602850
}
28612851

2862-
// ensure min and max
2863-
if (rep.selEnd[0] < 0) {
2864-
rep.selEnd[0] = 0;
2865-
}
2866-
if (rep.selStart[0] < 0) {
2867-
rep.selStart[0] = 0;
2868-
}
2869-
if (rep.selEnd[0] >= linesCount) {
2870-
rep.selEnd[0] = linesCount - 1;
2871-
}
2852+
// clamp to valid line range
2853+
rep.selStart[0] = Math.max(0, Math.min(rep.selStart[0], linesCount - 1));
2854+
rep.selEnd[0] = Math.max(0, Math.min(rep.selEnd[0], linesCount - 1));
28722855
updateBrowserSelectionFromRep();
28732856
// get the current caret selection, can't use rep. here because that only gives
28742857
// us the start position not the current
Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
import {expect, test} from "@playwright/test";
2+
import {clearPadContent, getPadBody, goToNewPad, writeToPad} from "../helper/padHelper";
3+
4+
test.beforeEach(async ({page}) => {
5+
await goToNewPad(page);
6+
});
7+
8+
// Regression test for https://github.com/ether/etherpad-lite/issues/6710
9+
test.describe('Page Up / Page Down', function () {
10+
test.describe.configure({retries: 2});
11+
12+
test('PageDown moves caret forward by a page of lines', async function ({page}) {
13+
const padBody = await getPadBody(page);
14+
await clearPadContent(page);
15+
16+
// Create enough lines to require scrolling (more than a viewport)
17+
for (let i = 0; i < 60; i++) {
18+
await writeToPad(page, `line ${i + 1}`);
19+
await page.keyboard.press('Enter');
20+
}
21+
22+
// Move caret to the first line
23+
await page.keyboard.down('Control');
24+
await page.keyboard.press('Home');
25+
await page.keyboard.up('Control');
26+
await page.waitForTimeout(200);
27+
28+
// Press PageDown — the handler uses a 200ms setTimeout internally
29+
await page.keyboard.press('PageDown');
30+
await page.waitForTimeout(1000);
31+
32+
// The caret should have moved significantly forward (not stuck at the bottom of first viewport)
33+
// Get the current line by checking which div has the caret
34+
const innerFrame = page.frame('ace_inner')!;
35+
const selection = await innerFrame.evaluate(() => {
36+
const sel = document.getSelection();
37+
if (!sel || !sel.focusNode) return 0;
38+
// Walk up to find the div
39+
let node = sel.focusNode as HTMLElement;
40+
while (node && node.tagName !== 'DIV') node = node.parentElement!;
41+
if (!node) return 0;
42+
// Find the index of this div
43+
const divs = Array.from(document.getElementById('innerdocbody')!.children);
44+
return divs.indexOf(node);
45+
});
46+
47+
// The caret should have advanced (viewport may be small in headless mode)
48+
expect(selection).toBeGreaterThan(2);
49+
});
50+
51+
test('PageUp moves caret backward by a page of lines', async function ({page}) {
52+
const padBody = await getPadBody(page);
53+
await clearPadContent(page);
54+
55+
// Create enough lines
56+
for (let i = 0; i < 60; i++) {
57+
await writeToPad(page, `line ${i + 1}`);
58+
await page.keyboard.press('Enter');
59+
}
60+
61+
// Move caret to the last line
62+
await page.keyboard.down('Control');
63+
await page.keyboard.press('End');
64+
await page.keyboard.up('Control');
65+
await page.waitForTimeout(200);
66+
67+
// Press PageUp
68+
await page.keyboard.press('PageUp');
69+
await page.waitForTimeout(500);
70+
71+
// The caret should have moved significantly backward
72+
const innerFrame = page.frame('ace_inner')!;
73+
const selection = await innerFrame.evaluate(() => {
74+
const sel = document.getSelection();
75+
if (!sel || !sel.focusNode) return 999;
76+
let node = sel.focusNode as HTMLElement;
77+
while (node && node.tagName !== 'DIV') node = node.parentElement!;
78+
if (!node) return 999;
79+
const divs = Array.from(document.getElementById('innerdocbody')!.children);
80+
return divs.indexOf(node);
81+
});
82+
83+
// The caret should be well before the last line (at least 10 lines from end)
84+
expect(selection).toBeLessThan(50);
85+
});
86+
87+
test('PageDown then PageUp returns to approximately same position', async function ({page}) {
88+
const padBody = await getPadBody(page);
89+
await clearPadContent(page);
90+
91+
for (let i = 0; i < 60; i++) {
92+
await writeToPad(page, `line ${i + 1}`);
93+
await page.keyboard.press('Enter');
94+
}
95+
96+
// Start at top
97+
await page.keyboard.down('Control');
98+
await page.keyboard.press('Home');
99+
await page.keyboard.up('Control');
100+
await page.waitForTimeout(200);
101+
102+
// PageDown then PageUp
103+
await page.keyboard.press('PageDown');
104+
await page.waitForTimeout(1000);
105+
await page.keyboard.press('PageUp');
106+
await page.waitForTimeout(1000);
107+
108+
// Should be back near the top
109+
const innerFrame = page.frame('ace_inner')!;
110+
const selection = await innerFrame.evaluate(() => {
111+
const sel = document.getSelection();
112+
if (!sel || !sel.focusNode) return 999;
113+
let node = sel.focusNode as HTMLElement;
114+
while (node && node.tagName !== 'DIV') node = node.parentElement!;
115+
if (!node) return 999;
116+
const divs = Array.from(document.getElementById('innerdocbody')!.children);
117+
return divs.indexOf(node);
118+
});
119+
120+
// Should be back near the start (allow some drift due to viewport calculations)
121+
expect(selection).toBeLessThan(8);
122+
});
123+
});

0 commit comments

Comments
 (0)