mirrors/etherpad-lite
1
0
Fork 0
mirror of https://github.com/ether/etherpad-lite.git synced 2026-05-05 04:06:37 +02:00
Code Issues Projects Releases Wiki Activity

fix: page down/up scrolls by viewport height, not line count (#7479)

Browse Source 
* fix: page down/up now scrolls by viewport height, not line count

The previous implementation counted logical lines in the viewport,
which failed when long wrapped lines consumed the entire viewport.
Now scrolls by actual pixel height for correct behavior.

Fixes #4562

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: use outerDoc instead of outerWin.document for viewport height in PageDown/Up

outerWin is an HTMLIFrameElement (returned by getElementsByName), not a
Window object, so it has no .document property. The existing getInnerHeight()
helper already uses outerDoc.documentElement.clientHeight correctly; align
the PageDown/PageUp handler with that pattern.

Adds a Playwright regression test that verifies PageDown scrolls the
viewport when the pad contains long wrapping lines.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: rewrite page down/up to use pixel-based line counting

The previous approach tried to scroll the outerWin iframe element
directly which didn't work. Reverted to the original cursor-movement
approach but calculates lines-to-skip using viewport pixel height
divided by actual rendered line heights. This correctly handles long
wrapped lines that consume multiple visual rows.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: restore getInnerHeight + inclusive range fixes lost in rebase

Recover the PageDown/Up fixes that got dropped when this branch was
rebased onto develop:

- Use getInnerHeight() instead of outerDoc.documentElement.clientHeight
  so hidden-iframe and Opera edge cases are handled the same as the rest
  of the editor.
- scroll.getVisibleLineRange() returns an inclusive end index, so count
  (end - start + 1) logical lines to match the pixel-sum loop bounds.
- Replace the flaky 'PageDown scrolls viewport' test with the robust
  #4562 regression that builds long wrapped lines via direct DOM and
  asserts the caret advances on successive PageDown presses.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
John McLear 2026-04-26 16:19:23 +08:00 committed by GitHub
parent f219809a3d
commit 37aaeaf197
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 83 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

41
src/static/js/ace2_inner.ts
View File

@ -2879,22 +2879,36 @@ function Ace2Inner(editorInfo, cssManagers) {
// This is required, browsers will try to do normal default behavior on
// page up / down and the default behavior SUCKS
evt.preventDefault();
const oldVisibleLineRange = scroll.getVisibleLineRange(rep);
let topOffset = rep.selStart[0] - oldVisibleLineRange[0];
if (topOffset < 0) {
topOffset = 0;
}
const isPageDown = evt.which === 34;
const isPageUp = evt.which === 33;
const oldVisibleLineRange = scroll.getVisibleLineRange(rep);
let topOffset = rep.selStart[0] - oldVisibleLineRange[0];
if (topOffset < 0) topOffset = 0;
scheduler.setTimeout(() => {
// the visible lines IE 1,10
const newVisibleLineRange = scroll.getVisibleLineRange(rep);
// total count of lines in pad IE 10
const linesCount = rep.lines.length();
// How many lines are in the viewport right now?
const numberOfLinesInViewport = newVisibleLineRange[1] - newVisibleLineRange[0];
// Calculate lines to skip based on viewport pixel height divided by
// the average rendered line height. This correctly handles long wrapped
// lines that consume multiple visual rows (fixes #4562).
const viewportHeight = getInnerHeight();
const visibleStart = newVisibleLineRange[0];
const visibleEnd = newVisibleLineRange[1];
let totalPixelHeight = 0;
for (let i = visibleStart; i <= Math.min(visibleEnd, linesCount - 1); i++) {
const entry = rep.lines.atIndex(i);
if (entry && entry.lineNode) {
totalPixelHeight += entry.lineNode.offsetHeight;
}
}
const visibleLogicalLines = visibleEnd - visibleStart + 1;
// Use pixel-based count: how many logical lines fit in one viewport
const numberOfLinesInViewport = visibleLogicalLines > 0 && totalPixelHeight > 0
? Math.max(1, Math.round(visibleLogicalLines * viewportHeight / totalPixelHeight))
: Math.max(1, visibleLogicalLines);
if (isPageUp && padShortcutEnabled.pageUp) {
rep.selStart[0] -= numberOfLinesInViewport;
@ -2910,18 +2924,11 @@ function Ace2Inner(editorInfo, cssManagers) {
rep.selStart[0] = Math.max(0, Math.min(rep.selStart[0], linesCount - 1));
rep.selEnd[0] = Math.max(0, Math.min(rep.selEnd[0], linesCount - 1));
updateBrowserSelectionFromRep();
// get the current caret selection, can't use rep. here because that only gives
// us the start position not the current
// scroll to the caret position
const myselection = targetDoc.getSelection();
// get the carets selection offset in px IE 214
let caretOffsetTop = myselection.focusNode.parentNode.offsetTop ||
myselection.focusNode.offsetTop;
// sometimes the first selection is -1 which causes problems
// (Especially with ep_page_view)
// so use focusNode.offsetTop value.
if (caretOffsetTop === -1) caretOffsetTop = myselection.focusNode.offsetTop;
// set the scrollY offset of the viewport on the document
scroll.setScrollY(caretOffsetTop);
}, 200);
}

59
src/tests/frontend-new/specs/page_up_down.spec.ts
View File

@ -84,6 +84,65 @@ test.describe('Page Up / Page Down', function () {
expect(selection).toBeLessThan(50);
});
// Regression test for #4562: consecutive very long wrapped lines should not
// cause PageDown/PageUp to skip too many or too few logical lines. The
// pixel-based calculation must account for lines that occupy far more visual
// rows than the viewport height.
test('PageDown with consecutive long wrapped lines moves by correct amount (#4562)', async function ({page}) {
const padBody = await getPadBody(page);
await clearPadContent(page);
// Build a pad with long lines interspersed with short ones via the inner
// document directly to avoid slow keyboard.type on Firefox.
const longLine = 'word '.repeat(300);
const innerFrame = page.frame('ace_inner')!;
await innerFrame.evaluate((text: string) => {
const body = document.getElementById('innerdocbody')!;
body.innerHTML = '';
for (let i = 0; i < 6; i++) {
const longDiv = document.createElement('div');
longDiv.textContent = text;
body.appendChild(longDiv);
const shortDiv = document.createElement('div');
shortDiv.textContent = `short ${i}`;
body.appendChild(shortDiv);
}
}, longLine);
// Wait for Etherpad to process the DOM changes
await page.waitForTimeout(2000);
// Move caret to the very top
await page.keyboard.down('Control');
await page.keyboard.press('Home');
await page.keyboard.up('Control');
await page.waitForTimeout(200);
// Press PageDown twice and verify caret advances each time
const getCaretLine = async () => {
return innerFrame.evaluate(() => {
const sel = document.getSelection();
if (!sel || !sel.focusNode) return -1;
let node = sel.focusNode as HTMLElement;
while (node && node.tagName !== 'DIV') node = node.parentElement!;
if (!node) return -1;
const divs = Array.from(document.getElementById('innerdocbody')!.children);
return divs.indexOf(node);
});
};
const lineBefore = await getCaretLine();
await page.keyboard.press('PageDown');
await page.waitForTimeout(1000);
const lineAfterFirst = await getCaretLine();
expect(lineAfterFirst).toBeGreaterThan(lineBefore);
await page.keyboard.press('PageDown');
await page.waitForTimeout(1000);
const lineAfterSecond = await getCaretLine();
expect(lineAfterSecond).toBeGreaterThan(lineAfterFirst);
});
test('PageDown then PageUp returns to approximately same position', async function ({page}) {
const padBody = await getPadBody(page);
await clearPadContent(page);