From aecbf547f1cb0aaa26658726ccca14eb7033cb7e Mon Sep 17 00:00:00 2001 From: John McLear Date: Sun, 19 Apr 2026 16:55:54 +0100 Subject: [PATCH] fix(editor): PageDown/PageUp now advance on consecutive long wrapped lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/static/js/ace2_inner.ts | 9 ++- .../specs/pagedown_wrapped_lines.spec.ts | 77 +++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) create mode 100644 src/tests/frontend-new/specs/pagedown_wrapped_lines.spec.ts diff --git a/src/static/js/ace2_inner.ts b/src/static/js/ace2_inner.ts index 3ca880a9484..a3563c61f1d 100644 --- a/src/static/js/ace2_inner.ts +++ b/src/static/js/ace2_inner.ts @@ -2893,8 +2893,13 @@ function Ace2Inner(editorInfo, cssManagers) { 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]; + // How many logical lines are in the viewport right now? `getVisibleLineRange` + // returns indices into `rep.lines` (logical lines, not visual rows), so when a + // single wrapped line fills the viewport the range collapses to [n, n] and this + // count is 0 — which would make PageDown/PageUp no-ops (issue #4562). Guarantee + // at least one line of movement so the caret and viewport always advance. + const numberOfLinesInViewport = + Math.max(1, newVisibleLineRange[1] - newVisibleLineRange[0]); if (isPageUp && padShortcutEnabled.pageUp) { rep.selStart[0] -= numberOfLinesInViewport; diff --git a/src/tests/frontend-new/specs/pagedown_wrapped_lines.spec.ts b/src/tests/frontend-new/specs/pagedown_wrapped_lines.spec.ts new file mode 100644 index 00000000000..6f9ed38efaa --- /dev/null +++ b/src/tests/frontend-new/specs/pagedown_wrapped_lines.spec.ts @@ -0,0 +1,77 @@ +import {expect, test} from "@playwright/test"; +import {clearPadContent, goToNewPad} from "../helper/padHelper"; + +test.beforeEach(async ({page}) => { + await goToNewPad(page); +}); + +// Regression test for https://github.com/ether/etherpad/issues/4562 +// PageDown failed to scroll when the cursor was on a very long wrapped line and +// the following lines were also very long, because getVisibleLineRange returns +// indices into rep.lines (logical lines) and collapsed to [n, n] — so the +// advance count was 0 and both caret and scroll stayed put. +test.describe('PageDown on consecutive long wrapped lines (#4562)', function () { + test.describe.configure({retries: 2}); + + test('PageDown scrolls when three very long lines fill the viewport', async function ({page}) { + await clearPadContent(page); + + const innerFrame = page.frame('ace_inner')!; + + // Insert three long lines via the editor directly — each ~2000 chars, which + // wraps to many visual rows in the viewport. + await innerFrame.evaluate(() => { + const body = document.getElementById('innerdocbody')!; + const longText = 'invisible '.repeat(200).trim(); + body.innerHTML = ''; + for (let i = 0; i < 3; i++) { + const div = document.createElement('div'); + div.textContent = `${i + 1} ${longText}`; + body.appendChild(div); + } + // Trigger the editor to pick up the content + body.dispatchEvent(new Event('input', {bubbles: true})); + }); + + // Type a character at the end to make the editor register the long content + // via its normal input path (the raw innerHTML edit above is just a scaffold). + await page.keyboard.press('End'); + await page.keyboard.type('!'); + await page.waitForTimeout(300); + + // Move caret to start of pad + await page.keyboard.down('Control'); + await page.keyboard.press('Home'); + await page.keyboard.up('Control'); + await page.waitForTimeout(200); + + // Capture initial scroll position of the outer (scrollable) frame + const outerFrame = page.frame('ace_outer')!; + const before = await outerFrame.evaluate( + () => (document.getElementById('outerdocbody') as HTMLElement).scrollTop || + document.scrollingElement?.scrollTop || 0); + + // Press PageDown — the ace handler uses a 200ms setTimeout internally. + await page.keyboard.press('PageDown'); + await page.waitForTimeout(800); + + const after = await outerFrame.evaluate( + () => (document.getElementById('outerdocbody') as HTMLElement).scrollTop || + document.scrollingElement?.scrollTop || 0); + + // Either the viewport scrolled, or the caret advanced to a later logical line. + const caretLine = await innerFrame.evaluate(() => { + const sel = document.getSelection(); + if (!sel || !sel.focusNode) return 0; + let node = sel.focusNode as HTMLElement; + while (node && node.tagName !== 'DIV') node = node.parentElement!; + if (!node) return 0; + const divs = Array.from(document.getElementById('innerdocbody')!.children); + return divs.indexOf(node); + }); + + // Pre-fix behavior (#4562): after == before AND caretLine === 0. + // Fixed behavior: caret advances at least 1 logical line, or the viewport scrolls. + expect(after > before || caretLine > 0).toBe(true); + }); +});