fix(editor): undo/redo scrolls the viewport to follow the caret (#7562)

* 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 `<div>` 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
John McLear 2026-04-26 16:26:54 +08:00 committed by GitHub
parent 37aaeaf197
commit 51356b9a13
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 123 additions and 0 deletions

View File

@ -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;

View File

@ -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);
});
});