From e42f3d1e2c2df95ddee8fa0ed3e681314991f680 Mon Sep 17 00:00:00 2001 From: John McLear Date: Mon, 6 Apr 2026 11:47:24 +0100 Subject: [PATCH 1/4] 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) --- src/static/js/ace2_inner.ts | 72 ++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 38 deletions(-) diff --git a/src/static/js/ace2_inner.ts b/src/static/js/ace2_inner.ts index 3ca880a9484..3859ed6f831 100644 --- a/src/static/js/ace2_inner.ts +++ b/src/static/js/ace2_inner.ts @@ -2879,51 +2879,47 @@ 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; - 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]; - - if (isPageUp && padShortcutEnabled.pageUp) { - rep.selStart[0] -= numberOfLinesInViewport; - rep.selEnd[0] -= numberOfLinesInViewport; + if ((isPageDown && padShortcutEnabled.pageDown) || + (isPageUp && padShortcutEnabled.pageUp)) { + // Scroll by actual viewport height in pixels, not by line count. + // This fixes the case where very long wrapped lines consume the + // entire viewport, making line-count-based scrolling useless. + const viewportHeight = outerWin.document.documentElement.clientHeight; + // Keep a small overlap so the user doesn't lose context + const scrollAmount = viewportHeight - 40; + const currentScrollY = scroll.getScrollY(); + + if (isPageDown) { + scroll.setScrollY(currentScrollY + scrollAmount); + } else { + scroll.setScrollY(Math.max(0, currentScrollY - scrollAmount)); } - if (isPageDown && padShortcutEnabled.pageDown) { - rep.selStart[0] += numberOfLinesInViewport; - rep.selEnd[0] += numberOfLinesInViewport; - } + // Move cursor into the new visible area + scheduler.setTimeout(() => { + const linesCount = rep.lines.length(); + const newVisibleRange = scroll.getVisibleLineRange(rep); - // clamp to valid line range - 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 - 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); + if (isPageDown) { + // Place cursor at the first line of the new viewport + rep.selStart[0] = newVisibleRange[0]; + rep.selEnd[0] = newVisibleRange[0]; + } else { + // Place cursor at the last line of the new viewport + rep.selEnd[0] = newVisibleRange[1]; + rep.selStart[0] = newVisibleRange[1]; + } + + // clamp to valid line range + 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(); + }, 0); + } } } From df5102c7ddca812acbf6b08d689e76bdc03713d8 Mon Sep 17 00:00:00 2001 From: John McLear Date: Mon, 6 Apr 2026 11:57:14 +0100 Subject: [PATCH 2/4] 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) --- src/static/js/ace2_inner.ts | 2 +- .../frontend-new/specs/page_up_down.spec.ts | 33 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/static/js/ace2_inner.ts b/src/static/js/ace2_inner.ts index 3859ed6f831..3917fdc220e 100644 --- a/src/static/js/ace2_inner.ts +++ b/src/static/js/ace2_inner.ts @@ -2888,7 +2888,7 @@ function Ace2Inner(editorInfo, cssManagers) { // Scroll by actual viewport height in pixels, not by line count. // This fixes the case where very long wrapped lines consume the // entire viewport, making line-count-based scrolling useless. - const viewportHeight = outerWin.document.documentElement.clientHeight; + const viewportHeight = outerDoc.documentElement.clientHeight; // Keep a small overlap so the user doesn't lose context const scrollAmount = viewportHeight - 40; const currentScrollY = scroll.getScrollY(); diff --git a/src/tests/frontend-new/specs/page_up_down.spec.ts b/src/tests/frontend-new/specs/page_up_down.spec.ts index d1f395bb99d..00b2dab1d7e 100644 --- a/src/tests/frontend-new/specs/page_up_down.spec.ts +++ b/src/tests/frontend-new/specs/page_up_down.spec.ts @@ -84,6 +84,39 @@ test.describe('Page Up / Page Down', function () { expect(selection).toBeLessThan(50); }); + // Regression test: long wrapping lines should still allow PageDown to scroll + // the viewport. Before the fix, outerWin.document was accessed on an iframe + // element (which has no .document property), causing the handler to break. + test('PageDown scrolls viewport when pad has long wrapping lines', async function ({page}) { + const padBody = await getPadBody(page); + await clearPadContent(page); + + // Create 3 very long lines that will wrap many times in the viewport + const longText = 'This is a very long line that should wrap multiple times in the editor viewport to ensure that page down scrolling works correctly even when lines are longer than the visible area. '.repeat(20); + for (let i = 0; i < 3; i++) { + await writeToPad(page, longText); + if (i < 2) await page.keyboard.press('Enter'); + } + + // 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); + + // Record the scroll position before PageDown + const outerFrame = page.frame('ace_outer')!; + const scrollBefore = await outerFrame.evaluate(() => document.documentElement.scrollTop); + + // Press PageDown + await page.keyboard.press('PageDown'); + await page.waitForTimeout(1000); + + // The viewport should have scrolled + const scrollAfter = await outerFrame.evaluate(() => document.documentElement.scrollTop); + expect(scrollAfter).toBeGreaterThan(scrollBefore); + }); + test('PageDown then PageUp returns to approximately same position', async function ({page}) { const padBody = await getPadBody(page); await clearPadContent(page); From a0b5f7d675d01972d72095bd06b61b261372445e Mon Sep 17 00:00:00 2001 From: John McLear Date: Mon, 6 Apr 2026 13:36:30 +0100 Subject: [PATCH 3/4] 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) --- src/static/js/ace2_inner.ts | 77 +++++++++++++++++++++---------------- 1 file changed, 44 insertions(+), 33 deletions(-) diff --git a/src/static/js/ace2_inner.ts b/src/static/js/ace2_inner.ts index 3917fdc220e..0ac4274a625 100644 --- a/src/static/js/ace2_inner.ts +++ b/src/static/js/ace2_inner.ts @@ -2883,43 +2883,54 @@ function Ace2Inner(editorInfo, cssManagers) { const isPageDown = evt.which === 34; const isPageUp = evt.which === 33; - if ((isPageDown && padShortcutEnabled.pageDown) || - (isPageUp && padShortcutEnabled.pageUp)) { - // Scroll by actual viewport height in pixels, not by line count. - // This fixes the case where very long wrapped lines consume the - // entire viewport, making line-count-based scrolling useless. - const viewportHeight = outerDoc.documentElement.clientHeight; - // Keep a small overlap so the user doesn't lose context - const scrollAmount = viewportHeight - 40; - const currentScrollY = scroll.getScrollY(); - - if (isPageDown) { - scroll.setScrollY(currentScrollY + scrollAmount); - } else { - scroll.setScrollY(Math.max(0, currentScrollY - scrollAmount)); - } + const oldVisibleLineRange = scroll.getVisibleLineRange(rep); + let topOffset = rep.selStart[0] - oldVisibleLineRange[0]; + if (topOffset < 0) topOffset = 0; - // Move cursor into the new visible area - scheduler.setTimeout(() => { - const linesCount = rep.lines.length(); - const newVisibleRange = scroll.getVisibleLineRange(rep); + scheduler.setTimeout(() => { + const newVisibleLineRange = scroll.getVisibleLineRange(rep); + const linesCount = rep.lines.length(); - if (isPageDown) { - // Place cursor at the first line of the new viewport - rep.selStart[0] = newVisibleRange[0]; - rep.selEnd[0] = newVisibleRange[0]; - } else { - // Place cursor at the last line of the new viewport - rep.selEnd[0] = newVisibleRange[1]; - rep.selStart[0] = newVisibleRange[1]; + // 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 = outerDoc.documentElement.clientHeight; + 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; + // 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; + rep.selEnd[0] -= numberOfLinesInViewport; + } - // clamp to valid line range - 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(); - }, 0); - } + if (isPageDown && padShortcutEnabled.pageDown) { + rep.selStart[0] += numberOfLinesInViewport; + rep.selEnd[0] += numberOfLinesInViewport; + } + + // clamp to valid line range + 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(); + // scroll to the caret position + const myselection = targetDoc.getSelection(); + let caretOffsetTop = myselection.focusNode.parentNode.offsetTop || + myselection.focusNode.offsetTop; + if (caretOffsetTop === -1) caretOffsetTop = myselection.focusNode.offsetTop; + scroll.setScrollY(caretOffsetTop); + }, 200); } } From 357c2291ee1e9f5cc16c5bc782e0a6b02fb879b1 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sun, 19 Apr 2026 11:45:49 +0100 Subject: [PATCH 4/4] 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) --- src/static/js/ace2_inner.ts | 4 +- .../frontend-new/specs/page_up_down.spec.ts | 60 +++++++++++++------ 2 files changed, 45 insertions(+), 19 deletions(-) diff --git a/src/static/js/ace2_inner.ts b/src/static/js/ace2_inner.ts index 0ac4274a625..bf61e8cb88f 100644 --- a/src/static/js/ace2_inner.ts +++ b/src/static/js/ace2_inner.ts @@ -2894,7 +2894,7 @@ function Ace2Inner(editorInfo, cssManagers) { // 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 = outerDoc.documentElement.clientHeight; + const viewportHeight = getInnerHeight(); const visibleStart = newVisibleLineRange[0]; const visibleEnd = newVisibleLineRange[1]; let totalPixelHeight = 0; @@ -2904,7 +2904,7 @@ function Ace2Inner(editorInfo, cssManagers) { totalPixelHeight += entry.lineNode.offsetHeight; } } - const visibleLogicalLines = visibleEnd - visibleStart; + 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)) diff --git a/src/tests/frontend-new/specs/page_up_down.spec.ts b/src/tests/frontend-new/specs/page_up_down.spec.ts index 00b2dab1d7e..640792b82c2 100644 --- a/src/tests/frontend-new/specs/page_up_down.spec.ts +++ b/src/tests/frontend-new/specs/page_up_down.spec.ts @@ -84,19 +84,32 @@ test.describe('Page Up / Page Down', function () { expect(selection).toBeLessThan(50); }); - // Regression test: long wrapping lines should still allow PageDown to scroll - // the viewport. Before the fix, outerWin.document was accessed on an iframe - // element (which has no .document property), causing the handler to break. - test('PageDown scrolls viewport when pad has long wrapping lines', async function ({page}) { + // 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); - // Create 3 very long lines that will wrap many times in the viewport - const longText = 'This is a very long line that should wrap multiple times in the editor viewport to ensure that page down scrolling works correctly even when lines are longer than the visible area. '.repeat(20); - for (let i = 0; i < 3; i++) { - await writeToPad(page, longText); - if (i < 2) await page.keyboard.press('Enter'); - } + // 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'); @@ -104,17 +117,30 @@ test.describe('Page Up / Page Down', function () { await page.keyboard.up('Control'); await page.waitForTimeout(200); - // Record the scroll position before PageDown - const outerFrame = page.frame('ace_outer')!; - const scrollBefore = await outerFrame.evaluate(() => document.documentElement.scrollTop); + // 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(); - // Press PageDown await page.keyboard.press('PageDown'); await page.waitForTimeout(1000); + const lineAfterFirst = await getCaretLine(); + expect(lineAfterFirst).toBeGreaterThan(lineBefore); - // The viewport should have scrolled - const scrollAfter = await outerFrame.evaluate(() => document.documentElement.scrollTop); - expect(scrollAfter).toBeGreaterThan(scrollBefore); + 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}) {