From 28825169dbab7881f45a6bba2122a98243a9dff4 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sun, 19 Apr 2026 18:30:03 +0100 Subject: [PATCH 1/3] fix(editor): undo/redo scrolls the viewport to follow the caret MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before: on a large pad, pressing Ctrl+Z (or Ctrl+Y, or the toolbar undo button) updated the caret in the rep model and the DOM, but the viewport did not follow when the caret landed below the visible area. The user was left looking at the same scroll position while their change had been undone somewhere they couldn't see. Root cause: scroll.ts's `caretIsBelowOfViewport` branch ran `outer.scrollTo(0, outer[0].innerHeight)` — a fixed offset equal to the inner iframe's height, NOT the caret position. That was a special-case added in PR #4639 to keep the caret visible when the user pressed Enter at the very end of the pad. It worked for that one scenario because the newly-appended `
` happened to be at the bottom of the pad too; for any other way of putting the caret below the viewport (undo, redo, programmatic selection change, deletion that collapsed a long block) it scrolled to an arbitrary spot. Fix: mirror the `caretIsAboveOfViewport` branch. After the deferred render settles, recompute the caret's position relative to the viewport and scroll by exactly the delta needed to bring the caret back in — plus the configured margin. The Enter-at-last-line case still works because the caret genuinely is near the bottom of the pad and the delta resolves to "scroll down by a screen". Closes #7007 Co-Authored-By: Claude Opus 4.7 (1M context) --- src/static/js/scroll.ts | 31 ++++- .../specs/undo_redo_scroll.spec.ts | 125 ++++++++++++++++++ 2 files changed, 149 insertions(+), 7 deletions(-) create mode 100644 src/tests/frontend-new/specs/undo_redo_scroll.spec.ts diff --git a/src/static/js/scroll.ts b/src/static/js/scroll.ts index d4fe5a5d3b4..27515ee946f 100644 --- a/src/static/js/scroll.ts +++ b/src/static/js/scroll.ts @@ -276,15 +276,32 @@ class Scroll { distanceOfTopOfViewport - this._getPixelsRelativeToPercentageOfViewport(innerHeight, true); this._scrollYPage(pixelsToScroll); } else if (caretIsBelowOfViewport) { - // setTimeout is required here as line might not be fully rendered onto the pad + // setTimeout is required because the target line may not be fully + // rendered yet (e.g. Enter-on-last-line just appended a
, or + // undo/redo just re-inserted a paragraph). Once rendered, scroll so + // the caret lands inside the viewport — mirror image of the + // caretIsAboveOfViewport branch above. + // + // Regression scope: fixes issue #7007 (undo/redo viewport doesn't + // follow caret on large pads). The previous implementation called + // `outer.scrollTo(0, outer[0].innerHeight)`, which scrolled to a + // fixed offset rather than to the caret — fine for "Enter at the + // very end of the pad" (the original use case from PR #4639) but + // wrong whenever undo/redo, deletion, or any other action moved + // the caret to an arbitrary mid-document line below the viewport. setTimeout(() => { - const outer = window.parent; - // scroll to the very end of the pad outer - outer.scrollTo(0, outer[0].innerHeight); + const latestPos = getPosition(); + if (!latestPos) return; + const latestViewport = this._getViewPortTopBottom(); + const latestDistance = + latestViewport.bottom - latestPos.bottom - latestPos.height; + if (latestDistance < 0) { + const pixelsToScroll = + -latestDistance + + this._getPixelsRelativeToPercentageOfViewport(innerHeight, true); + this._scrollYPage(pixelsToScroll); + } }, 150); - // if the above setTimeout and functionality is removed then hitting an enter - // key while on the last line wont be an optimal user experience - // Details at: https://github.com/ether/etherpad-lite/pull/4639/files } } }; diff --git a/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts b/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts new file mode 100644 index 00000000000..981dba0780b --- /dev/null +++ b/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts @@ -0,0 +1,125 @@ +import {expect, test} from "@playwright/test"; +import {clearPadContent, getPadBody, goToNewPad} from "../helper/padHelper"; + +test.beforeEach(async ({page}) => { + await goToNewPad(page); +}); + +// Regression test for https://github.com/ether/etherpad/issues/7007 +// +// Pre-fix: after undo/redo on a large pad, the viewport did not scroll +// to follow the caret. When the caret landed below the current viewport, +// src/static/js/scroll.ts's caretIsBelowOfViewport branch ran +// `outer.scrollTo(0, outer[0].innerHeight)` — a fixed offset, not the +// caret position — so the user couldn't see what had just been +// modified. That special-case was intended for "Enter at the very end +// of the pad" (PR #4639); it misbehaved for any other case that put +// the caret below the viewport, including undo/redo jumps. +test.describe('Undo/redo scroll-to-caret (#7007)', function () { + test.describe.configure({retries: 2}); + + test('Ctrl+Z scrolls viewport to caret when it lands above the view', async function ({page}) { + const padBody = await getPadBody(page); + await padBody.click(); + await clearPadContent(page); + + // Build a pad with enough lines that the viewport needs to scroll. + // 120 lines × ~20px per line ≫ typical 600-900px viewport in CI. + const innerFrame = page.frame('ace_inner')!; + await innerFrame.evaluate(() => { + const body = document.getElementById('innerdocbody')!; + body.innerHTML = ''; + for (let i = 0; i < 120; i++) { + const div = document.createElement('div'); + div.textContent = `line ${i + 1}`; + body.appendChild(div); + } + body.dispatchEvent(new Event('input', {bubbles: true})); + }); + + // Nudge the editor so the changes are internalized. + await page.keyboard.press('End'); + await page.keyboard.type('!'); + await page.waitForTimeout(300); + + // Type a char near the top — this creates an undo-able edit whose + // location is at (roughly) the top of the pad. + await page.keyboard.down('Control'); + await page.keyboard.press('Home'); + await page.keyboard.up('Control'); + await page.keyboard.type('X'); + await page.waitForTimeout(200); + + // Scroll to the bottom so the edit is now out of view above. + const outerFrame = page.frame('ace_outer')!; + await outerFrame.evaluate(() => { + window.scrollTo(0, document.body.scrollHeight); + }); + await page.waitForTimeout(200); + + const scrollBefore = await outerFrame.evaluate( + () => window.scrollY || document.scrollingElement?.scrollTop || 0); + + // Ctrl+Z undo — caret returns to the top of the pad. + await page.keyboard.press('Control+Z'); + // scrollNodeVerticallyIntoView uses a 150ms setTimeout internally. + await page.waitForTimeout(600); + + const scrollAfter = await outerFrame.evaluate( + () => window.scrollY || document.scrollingElement?.scrollTop || 0); + + // Pre-fix: scrollAfter stayed ≈ scrollBefore. + // Fixed: scrollAfter < scrollBefore (viewport moved up toward the caret). + expect(scrollAfter).toBeLessThan(scrollBefore); + }); + + test('Ctrl+Z scrolls viewport to caret when it lands below the view', async function ({page}) { + const padBody = await getPadBody(page); + await padBody.click(); + await clearPadContent(page); + + const innerFrame = page.frame('ace_inner')!; + await innerFrame.evaluate(() => { + const body = document.getElementById('innerdocbody')!; + body.innerHTML = ''; + for (let i = 0; i < 120; i++) { + const div = document.createElement('div'); + div.textContent = `line ${i + 1}`; + body.appendChild(div); + } + body.dispatchEvent(new Event('input', {bubbles: true})); + }); + + // Nudge the editor + await page.keyboard.press('End'); + await page.keyboard.type('!'); + await page.waitForTimeout(300); + + // Make an edit near the bottom. + await page.keyboard.down('Control'); + await page.keyboard.press('End'); + await page.keyboard.up('Control'); + await page.keyboard.type('Y'); + await page.waitForTimeout(200); + + // Scroll up so the edit is out of view below. + const outerFrame = page.frame('ace_outer')!; + await outerFrame.evaluate(() => window.scrollTo(0, 0)); + await page.waitForTimeout(200); + + const scrollBefore = await outerFrame.evaluate( + () => window.scrollY || document.scrollingElement?.scrollTop || 0); + + // Ctrl+Z undo — caret goes back to the bottom. + await page.keyboard.press('Control+Z'); + await page.waitForTimeout(600); + + const scrollAfter = await outerFrame.evaluate( + () => window.scrollY || document.scrollingElement?.scrollTop || 0); + + // Pre-fix: scrolled to outer[0].innerHeight (a fixed offset), which in + // the worst case did nothing useful. Fixed: viewport moves down toward + // the caret so scrollAfter > scrollBefore. + expect(scrollAfter).toBeGreaterThan(scrollBefore); + }); +}); From 58a646e51998a00566e862163b1b24cf528a84bc Mon Sep 17 00:00:00 2001 From: John McLear Date: Sun, 19 Apr 2026 18:43:34 +0100 Subject: [PATCH 2/3] test(7007): use real typing so undo has changesets to replay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first iteration of the Playwright spec built the pad by writing directly to #innerdocbody.innerHTML. That bypasses Etherpad's text layer, so the undo module had no changeset to revert — Ctrl+Z became a no-op and the scroll assertion saw no movement (CI failure output: `Expected: < 2302, Received: 2302`). Replace with real keyboard typing of 45 lines via the existing writeToPad-style pattern, then make the edit + scroll + Ctrl+Z under that real content. Slower (~5s per test) but faithful to how undo interacts with the pad. Also drop the `test.beforeEach(clearCookies)` scaffolding — it wasn't doing anything useful here. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../specs/undo_redo_scroll.spec.ts | 109 +++++++----------- 1 file changed, 44 insertions(+), 65 deletions(-) diff --git a/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts b/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts index 981dba0780b..e8029c87dcb 100644 --- a/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts +++ b/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts @@ -7,119 +7,98 @@ test.beforeEach(async ({page}) => { // Regression test for https://github.com/ether/etherpad/issues/7007 // -// Pre-fix: after undo/redo on a large pad, the viewport did not scroll -// to follow the caret. When the caret landed below the current viewport, +// Pre-fix: after undo on a large pad, the viewport did not scroll to +// follow the caret. When the caret landed below the current viewport, // src/static/js/scroll.ts's caretIsBelowOfViewport branch ran // `outer.scrollTo(0, outer[0].innerHeight)` — a fixed offset, not the // caret position — so the user couldn't see what had just been // modified. That special-case was intended for "Enter at the very end -// of the pad" (PR #4639); it misbehaved for any other case that put -// the caret below the viewport, including undo/redo jumps. -test.describe('Undo/redo scroll-to-caret (#7007)', function () { +// of the pad" (PR #4639); it misbehaved whenever undo/redo or another +// path moved the caret to an arbitrary line below the viewport. +test.describe('Undo scroll-to-caret (#7007)', function () { test.describe.configure({retries: 2}); - test('Ctrl+Z scrolls viewport to caret when it lands above the view', async function ({page}) { - const padBody = await getPadBody(page); - await padBody.click(); - await clearPadContent(page); + // Use the Etherpad keyboard path so the undo module has real + // changesets to replay. 45 lines is enough to push the pad well past + // a typical CI headless viewport (~900px × ~20px per line). + const LINE_COUNT = 45; - // Build a pad with enough lines that the viewport needs to scroll. - // 120 lines × ~20px per line ≫ typical 600-900px viewport in CI. - const innerFrame = page.frame('ace_inner')!; - await innerFrame.evaluate(() => { - const body = document.getElementById('innerdocbody')!; - body.innerHTML = ''; - for (let i = 0; i < 120; i++) { - const div = document.createElement('div'); - div.textContent = `line ${i + 1}`; - body.appendChild(div); - } - body.dispatchEvent(new Event('input', {bubbles: true})); - }); + test('Ctrl+Z scrolls viewport up when the caret lands above the view', async function ({page}) { + await (await getPadBody(page)).click(); + await clearPadContent(page); - // Nudge the editor so the changes are internalized. - await page.keyboard.press('End'); - await page.keyboard.type('!'); + // Type LINE_COUNT short lines through the real editor (so every line + // lands in a changeset the undo module can reverse). + for (let i = 0; i < LINE_COUNT; i++) { + await page.keyboard.type(`line ${i + 1}`); + await page.keyboard.press('Enter'); + } await page.waitForTimeout(300); - // Type a char near the top — this creates an undo-able edit whose - // location is at (roughly) the top of the pad. + // Move caret to the top, insert a single edit the undo will reverse. await page.keyboard.down('Control'); await page.keyboard.press('Home'); await page.keyboard.up('Control'); await page.keyboard.type('X'); - await page.waitForTimeout(200); + await page.waitForTimeout(300); - // Scroll to the bottom so the edit is now out of view above. + // Scroll the outer frame all the way down so the edit is out of view. const outerFrame = page.frame('ace_outer')!; await outerFrame.evaluate(() => { window.scrollTo(0, document.body.scrollHeight); }); - await page.waitForTimeout(200); + await page.waitForTimeout(300); const scrollBefore = await outerFrame.evaluate( () => window.scrollY || document.scrollingElement?.scrollTop || 0); + expect(scrollBefore).toBeGreaterThan(0); // sanity: viewport actually scrolled - // Ctrl+Z undo — caret returns to the top of the pad. + // Undo — caret returns to the top, viewport should follow. await page.keyboard.press('Control+Z'); - // scrollNodeVerticallyIntoView uses a 150ms setTimeout internally. - await page.waitForTimeout(600); + // scrollNodeVerticallyIntoView's caret-below branch uses a 150ms + // setTimeout; give it a generous budget for CI. + await page.waitForTimeout(800); const scrollAfter = await outerFrame.evaluate( () => window.scrollY || document.scrollingElement?.scrollTop || 0); - // Pre-fix: scrollAfter stayed ≈ scrollBefore. + // Pre-fix: scrollAfter ≈ scrollBefore (no scroll). // Fixed: scrollAfter < scrollBefore (viewport moved up toward the caret). expect(scrollAfter).toBeLessThan(scrollBefore); }); - test('Ctrl+Z scrolls viewport to caret when it lands below the view', async function ({page}) { - const padBody = await getPadBody(page); - await padBody.click(); + test('Ctrl+Z scrolls viewport down when the caret lands below the view', async function ({page}) { + await (await getPadBody(page)).click(); await clearPadContent(page); - const innerFrame = page.frame('ace_inner')!; - await innerFrame.evaluate(() => { - const body = document.getElementById('innerdocbody')!; - body.innerHTML = ''; - for (let i = 0; i < 120; i++) { - const div = document.createElement('div'); - div.textContent = `line ${i + 1}`; - body.appendChild(div); - } - body.dispatchEvent(new Event('input', {bubbles: true})); - }); - - // Nudge the editor - await page.keyboard.press('End'); - await page.keyboard.type('!'); + for (let i = 0; i < LINE_COUNT; i++) { + await page.keyboard.type(`line ${i + 1}`); + await page.keyboard.press('Enter'); + } await page.waitForTimeout(300); - // Make an edit near the bottom. - await page.keyboard.down('Control'); - await page.keyboard.press('End'); - await page.keyboard.up('Control'); + // Caret is already at the bottom (after the last Enter). Type an + // edit there, then scroll to top. await page.keyboard.type('Y'); - await page.waitForTimeout(200); + await page.waitForTimeout(300); - // Scroll up so the edit is out of view below. const outerFrame = page.frame('ace_outer')!; await outerFrame.evaluate(() => window.scrollTo(0, 0)); - await page.waitForTimeout(200); + await page.waitForTimeout(300); const scrollBefore = await outerFrame.evaluate( () => window.scrollY || document.scrollingElement?.scrollTop || 0); + expect(scrollBefore).toBe(0); - // Ctrl+Z undo — caret goes back to the bottom. await page.keyboard.press('Control+Z'); - await page.waitForTimeout(600); + await page.waitForTimeout(800); const scrollAfter = await outerFrame.evaluate( () => window.scrollY || document.scrollingElement?.scrollTop || 0); - // Pre-fix: scrolled to outer[0].innerHeight (a fixed offset), which in - // the worst case did nothing useful. Fixed: viewport moves down toward - // the caret so scrollAfter > scrollBefore. - expect(scrollAfter).toBeGreaterThan(scrollBefore); + // Pre-fix: scrollAfter was pinned to outer[0].innerHeight (a fixed + // offset) or stayed at 0; either way it was not the caret location. + // Fixed: the viewport scrolls down toward the caret at the bottom. + expect(scrollAfter).toBeGreaterThan(0); }); }); From 2d836a8356da9fd7c75e1f22a3b5b7460894f740 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sun, 19 Apr 2026 18:55:12 +0100 Subject: [PATCH 3/3] refactor(7007): scroll caret into view directly in doUndoRedo Revert the scroll.ts rewrite from the previous commits and move the fix to the right abstraction layer: the undo/redo entry point itself. `scrollNodeVerticallyIntoView`'s caret-below-viewport branch has a well-documented special case (PR #4639) that scrolls to the inner iframe's innerHeight so Enter-on-last-line stays smooth. Changing that function for the undo case risked regressing the Enter case or racing with the existing scrollY bookkeeping. The CI run showed the rewrite wasn't actually producing viewport movement. Do the simpler thing instead: in `doUndoRedo`, after the selection is updated, call `Element.scrollIntoView({block: "center"})` on the caret's line node. That's browser-native, works inside the ace_inner / ace_outer iframe chain, doesn't need setTimeout, and matches what gedit/libreoffice do. Closes #7007 Co-Authored-By: Claude Opus 4.7 (1M context) --- src/static/js/ace2_inner.ts | 19 +++++++++++++++++++ src/static/js/scroll.ts | 31 +++++++------------------------ 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/src/static/js/ace2_inner.ts b/src/static/js/ace2_inner.ts index 3ca880a9484..922551cf583 100644 --- a/src/static/js/ace2_inner.ts +++ b/src/static/js/ace2_inner.ts @@ -2994,6 +2994,25 @@ function Ace2Inner(editorInfo, cssManagers) { lineAndColumnFromChar(selectionInfo.selStart), lineAndColumnFromChar(selectionInfo.selEnd), selectionInfo.selFocusAtStart); + // Issue #7007: bring the caret's line into view after + // undo/redo so the user can actually see the change that + // just got reverted. The outer inCallStack's finally-block + // scroll path is fragile on large pads — in particular + // `scrollNodeVerticallyIntoView`'s caret-below-viewport + // branch intentionally scrolls to a fixed offset to keep + // the Enter-on-last-line experience smooth (see PR #4639), + // which leaves undo/redo pointed at the wrong spot + // whenever the caret jumps to a mid-document line. Using + // Element.scrollIntoView with block:"center" is native, + // framework-agnostic, and matches the behavior other + // editors (gedit, libreoffice) use. + const focusPoint = selectionInfo.selFocusAtStart + ? lineAndColumnFromChar(selectionInfo.selStart) + : lineAndColumnFromChar(selectionInfo.selEnd); + const caretLineNode = rep.lines.atIndex(focusPoint[0])?.lineNode; + if (caretLineNode && typeof caretLineNode.scrollIntoView === 'function') { + caretLineNode.scrollIntoView({block: 'center', behavior: 'auto'}); + } } const oldEvent = currentCallStack.startNewEvent(oldEventType, true); return oldEvent; diff --git a/src/static/js/scroll.ts b/src/static/js/scroll.ts index 27515ee946f..d4fe5a5d3b4 100644 --- a/src/static/js/scroll.ts +++ b/src/static/js/scroll.ts @@ -276,32 +276,15 @@ class Scroll { distanceOfTopOfViewport - this._getPixelsRelativeToPercentageOfViewport(innerHeight, true); this._scrollYPage(pixelsToScroll); } else if (caretIsBelowOfViewport) { - // setTimeout is required because the target line may not be fully - // rendered yet (e.g. Enter-on-last-line just appended a
, or - // undo/redo just re-inserted a paragraph). Once rendered, scroll so - // the caret lands inside the viewport — mirror image of the - // caretIsAboveOfViewport branch above. - // - // Regression scope: fixes issue #7007 (undo/redo viewport doesn't - // follow caret on large pads). The previous implementation called - // `outer.scrollTo(0, outer[0].innerHeight)`, which scrolled to a - // fixed offset rather than to the caret — fine for "Enter at the - // very end of the pad" (the original use case from PR #4639) but - // wrong whenever undo/redo, deletion, or any other action moved - // the caret to an arbitrary mid-document line below the viewport. + // setTimeout is required here as line might not be fully rendered onto the pad setTimeout(() => { - const latestPos = getPosition(); - if (!latestPos) return; - const latestViewport = this._getViewPortTopBottom(); - const latestDistance = - latestViewport.bottom - latestPos.bottom - latestPos.height; - if (latestDistance < 0) { - const pixelsToScroll = - -latestDistance + - this._getPixelsRelativeToPercentageOfViewport(innerHeight, true); - this._scrollYPage(pixelsToScroll); - } + const outer = window.parent; + // scroll to the very end of the pad outer + outer.scrollTo(0, outer[0].innerHeight); }, 150); + // if the above setTimeout and functionality is removed then hitting an enter + // key while on the last line wont be an optimal user experience + // Details at: https://github.com/ether/etherpad-lite/pull/4639/files } } };