Skip to content

Commit aecbf54

Browse files
JohnMcLearclaude
andcommitted
fix(editor): PageDown/PageUp now advance on consecutive long wrapped lines
The page up/down handler advances the caret by numberOfLinesInViewport computed from scroll.getVisibleLineRange(). That helper returns indices into rep.lines (logical lines, not visual/wrapped rows), so when one wrapped logical line fills the viewport — e.g., three consecutive lines of ~2000 chars each — the range collapses to [n, n] and the advance count becomes 0. The caret stays on line n, scroll stays at 0, and the user sees "PageDown does nothing". Clamp the advance to at least one logical line so the caret and viewport always move. Includes a Playwright regression test covering the reporter's repro (three very long lines, Ctrl+Home, PageDown). Closes #4562 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent c4add02 commit aecbf54

2 files changed

Lines changed: 84 additions & 2 deletions

File tree

src/static/js/ace2_inner.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2893,8 +2893,13 @@ function Ace2Inner(editorInfo, cssManagers) {
28932893
const newVisibleLineRange = scroll.getVisibleLineRange(rep);
28942894
// total count of lines in pad IE 10
28952895
const linesCount = rep.lines.length();
2896-
// How many lines are in the viewport right now?
2897-
const numberOfLinesInViewport = newVisibleLineRange[1] - newVisibleLineRange[0];
2896+
// How many logical lines are in the viewport right now? `getVisibleLineRange`
2897+
// returns indices into `rep.lines` (logical lines, not visual rows), so when a
2898+
// single wrapped line fills the viewport the range collapses to [n, n] and this
2899+
// count is 0 — which would make PageDown/PageUp no-ops (issue #4562). Guarantee
2900+
// at least one line of movement so the caret and viewport always advance.
2901+
const numberOfLinesInViewport =
2902+
Math.max(1, newVisibleLineRange[1] - newVisibleLineRange[0]);
28982903

28992904
if (isPageUp && padShortcutEnabled.pageUp) {
29002905
rep.selStart[0] -= numberOfLinesInViewport;
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
import {expect, test} from "@playwright/test";
2+
import {clearPadContent, goToNewPad} from "../helper/padHelper";
3+
4+
test.beforeEach(async ({page}) => {
5+
await goToNewPad(page);
6+
});
7+
8+
// Regression test for https://github.com/ether/etherpad/issues/4562
9+
// PageDown failed to scroll when the cursor was on a very long wrapped line and
10+
// the following lines were also very long, because getVisibleLineRange returns
11+
// indices into rep.lines (logical lines) and collapsed to [n, n] — so the
12+
// advance count was 0 and both caret and scroll stayed put.
13+
test.describe('PageDown on consecutive long wrapped lines (#4562)', function () {
14+
test.describe.configure({retries: 2});
15+
16+
test('PageDown scrolls when three very long lines fill the viewport', async function ({page}) {
17+
await clearPadContent(page);
18+
19+
const innerFrame = page.frame('ace_inner')!;
20+
21+
// Insert three long lines via the editor directly — each ~2000 chars, which
22+
// wraps to many visual rows in the viewport.
23+
await innerFrame.evaluate(() => {
24+
const body = document.getElementById('innerdocbody')!;
25+
const longText = 'invisible '.repeat(200).trim();
26+
body.innerHTML = '';
27+
for (let i = 0; i < 3; i++) {
28+
const div = document.createElement('div');
29+
div.textContent = `${i + 1} ${longText}`;
30+
body.appendChild(div);
31+
}
32+
// Trigger the editor to pick up the content
33+
body.dispatchEvent(new Event('input', {bubbles: true}));
34+
});
35+
36+
// Type a character at the end to make the editor register the long content
37+
// via its normal input path (the raw innerHTML edit above is just a scaffold).
38+
await page.keyboard.press('End');
39+
await page.keyboard.type('!');
40+
await page.waitForTimeout(300);
41+
42+
// Move caret to start of pad
43+
await page.keyboard.down('Control');
44+
await page.keyboard.press('Home');
45+
await page.keyboard.up('Control');
46+
await page.waitForTimeout(200);
47+
48+
// Capture initial scroll position of the outer (scrollable) frame
49+
const outerFrame = page.frame('ace_outer')!;
50+
const before = await outerFrame.evaluate(
51+
() => (document.getElementById('outerdocbody') as HTMLElement).scrollTop ||
52+
document.scrollingElement?.scrollTop || 0);
53+
54+
// Press PageDown — the ace handler uses a 200ms setTimeout internally.
55+
await page.keyboard.press('PageDown');
56+
await page.waitForTimeout(800);
57+
58+
const after = await outerFrame.evaluate(
59+
() => (document.getElementById('outerdocbody') as HTMLElement).scrollTop ||
60+
document.scrollingElement?.scrollTop || 0);
61+
62+
// Either the viewport scrolled, or the caret advanced to a later logical line.
63+
const caretLine = await innerFrame.evaluate(() => {
64+
const sel = document.getSelection();
65+
if (!sel || !sel.focusNode) return 0;
66+
let node = sel.focusNode as HTMLElement;
67+
while (node && node.tagName !== 'DIV') node = node.parentElement!;
68+
if (!node) return 0;
69+
const divs = Array.from(document.getElementById('innerdocbody')!.children);
70+
return divs.indexOf(node);
71+
});
72+
73+
// Pre-fix behavior (#4562): after == before AND caretLine === 0.
74+
// Fixed behavior: caret advances at least 1 logical line, or the viewport scrolls.
75+
expect(after > before || caretLine > 0).toBe(true);
76+
});
77+
});

0 commit comments

Comments
 (0)