From 51356b9a13db7085df63f06bf91ffac8edff3d74 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sun, 26 Apr 2026 16:26:54 +0800 Subject: [PATCH] fix(editor): undo/redo scrolls the viewport to follow the caret (#7562) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(editor): undo/redo scrolls the viewport to follow the caret 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) * test(7007): use real typing so undo has changesets to replay 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) * 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) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- src/static/js/ace2_inner.ts | 19 ++++ .../specs/undo_redo_scroll.spec.ts | 104 ++++++++++++++++++ 2 files changed, 123 insertions(+) create mode 100644 src/tests/frontend-new/specs/undo_redo_scroll.spec.ts diff --git a/src/static/js/ace2_inner.ts b/src/static/js/ace2_inner.ts index 748627836..0f8f65721 100644 --- a/src/static/js/ace2_inner.ts +++ b/src/static/js/ace2_inner.ts @@ -3001,6 +3001,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/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 000000000..e8029c87d --- /dev/null +++ b/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts @@ -0,0 +1,104 @@ +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 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 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}); + + // 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; + + test('Ctrl+Z scrolls viewport up when the caret lands above the view', async function ({page}) { + await (await getPadBody(page)).click(); + await clearPadContent(page); + + // 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); + + // 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(300); + + // 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(300); + + const scrollBefore = await outerFrame.evaluate( + () => window.scrollY || document.scrollingElement?.scrollTop || 0); + expect(scrollBefore).toBeGreaterThan(0); // sanity: viewport actually scrolled + + // Undo — caret returns to the top, viewport should follow. + await page.keyboard.press('Control+Z'); + // 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 ≈ scrollBefore (no scroll). + // Fixed: scrollAfter < scrollBefore (viewport moved up toward the caret). + expect(scrollAfter).toBeLessThan(scrollBefore); + }); + + test('Ctrl+Z scrolls viewport down when the caret lands below the view', async function ({page}) { + await (await getPadBody(page)).click(); + await clearPadContent(page); + + for (let i = 0; i < LINE_COUNT; i++) { + await page.keyboard.type(`line ${i + 1}`); + await page.keyboard.press('Enter'); + } + await page.waitForTimeout(300); + + // 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(300); + + const outerFrame = page.frame('ace_outer')!; + await outerFrame.evaluate(() => window.scrollTo(0, 0)); + await page.waitForTimeout(300); + + const scrollBefore = await outerFrame.evaluate( + () => window.scrollY || document.scrollingElement?.scrollTop || 0); + expect(scrollBefore).toBe(0); + + await page.keyboard.press('Control+Z'); + await page.waitForTimeout(800); + + const scrollAfter = await outerFrame.evaluate( + () => window.scrollY || document.scrollingElement?.scrollTop || 0); + + // 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); + }); +});