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

[JohnMcLear]

Summary

Fixes #7007. On a large pad, Ctrl+Z / Ctrl+Y (and the toolbar undo/redo buttons) updated the caret in both the rep model and the DOM, but the viewport didn't 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 — which is the "practically unusable, and even dangerous" behavior the reporter described.

Root cause

src/static/js/scroll.ts:278-288 had this branch:

} else if (caretIsBelowOfViewport) {
  setTimeout(() => {
    const outer = window.parent;
    outer.scrollTo(0, outer[0].innerHeight);   // fixed offset, NOT the caret
  }, 150);
}

That special case was added in PR #4639 to keep the caret visible when the user pressed Enter at the very end of the pad — the newly-appended <div> happened to be near the bottom of the pad too, so scrolling by a fixed "one screen height" offset was close enough. But for every other way of putting the caret below the viewport (undo/redo jumping to a mid-document line, programmatic selection change, deletion that collapsed a long block), the scroll went to an arbitrary spot rather than to the caret.

Fix

Mirror the caretIsAboveOfViewport branch directly above it: after the deferred render settles (same 150ms delay), 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 — the caret genuinely is near the bottom of the pad, and the delta resolves to roughly "scroll down by a screen".

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);
  }
}, 150);

Test plan

• Added Playwright spec src/tests/frontend-new/specs/undo_redo_scroll.spec.ts covering:
  • Caret above viewport: edit at top, scroll to bottom, Ctrl+Z — viewport scrolls up
  • Caret below viewport: edit at bottom, scroll to top, Ctrl+Z — viewport scrolls down (this is the regression case)
• pnpm run ts-check clean locally
• Existing redo.spec.ts still passes (no behaviour change for small-pad cases)
• Enter-on-last-line still keeps caret visible on a long pad

Closes #7007

🤖 Generated with Claude Code

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Fix undo/redo viewport scroll to follow caret on large pads

🐞 Bug fix

Walkthroughs

Description

• Fix viewport scroll behavior when undo/redo moves caret below visible area
• Replace fixed scroll offset with dynamic caret-position calculation
• Add comprehensive Playwright tests for undo/redo scroll scenarios
• Maintain backward compatibility with Enter-at-last-line behavior

Diagram

flowchart LR
  A["Undo/Redo Action"] --> B["Caret Below Viewport"]
  B --> C["Old: Fixed Offset Scroll"]
  B --> D["New: Dynamic Caret Calculation"]
  C --> E["Arbitrary Scroll Position"]
  D --> F["Viewport Follows Caret"]
  G["Test Coverage"] --> H["Caret Above View"]
  G --> I["Caret Below View"]
  H --> J["Scroll Up to Caret"]
  I --> K["Scroll Down to Caret"]

Loading

File Changes

1. src/static/js/scroll.ts 🐞 Bug fix +24/-7

Dynamic caret-following scroll for below-viewport case

• Replaced fixed outer.scrollTo(0, outer[0].innerHeight) with dynamic caret position calculation
• Recomputed caret position relative to viewport after deferred render settles
• Mirrored caretIsAboveOfViewport logic for caretIsBelowOfViewport case
• Enhanced comments explaining regression scope and original PR #4639 context

src/static/js/scroll.ts

---

2. src/tests/frontend-new/specs/undo_redo_scroll.spec.ts 🧪 Tests +125/-0

Playwright tests for undo/redo scroll behavior

• Added regression test suite for issue #7007 with 2 test cases
• Test 1: Verifies viewport scrolls up when undo moves caret above current view
• Test 2: Verifies viewport scrolls down when undo moves caret below current view
• Creates 120-line test pad to ensure viewport scrolling is necessary

src/tests/frontend-new/specs/undo_redo_scroll.spec.ts

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. Wrong below-scroll percentage 🐞 Bug ≡ Correctness

Description

In scrollNodeVerticallyIntoView()'s caret-below-viewport deferred scroll, the code passes true to
_getPixelsRelativeToPercentageOfViewport(), which selects editionAboveViewport instead of
editionBelowViewport. This makes caret-below scrolling use the wrong configured margin when the
above/below percentages differ, producing incorrect final viewport positioning.

Code

src/static/js/scroll.ts[R293-302]

+          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);

Evidence

The new caret-below branch computes pixelsToScroll using
_getPixelsRelativeToPercentageOfViewport(innerHeight, true), which flows into
_getPercentageToScroll(true) and selects scrollSettings.percentage.editionAboveViewport. The
settings schema explicitly supports separate above vs below percentages, so using the above setting
in the below branch is inconsistent with the configuration semantics.

src/static/js/scroll.ts[262-304]
src/static/js/scroll.ts[194-211]
settings.json.template[486-505]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`scrollNodeVerticallyIntoView()`'s caret-below-viewport deferred scroll currently calls `_getPixelsRelativeToPercentageOfViewport(innerHeight, true)`, which selects `editionAboveViewport`. For caret-below behavior, it should instead use `editionBelowViewport` (by omitting the flag or passing `false`).
### Issue Context
`_getPixelsRelativeToPercentageOfViewport(innerHeight, aboveOfViewport?)` delegates to `_getPercentageToScroll(aboveOfViewport)` which chooses between `editionAboveViewport` and `editionBelowViewport`.
### Fix Focus Areas
- src/static/js/scroll.ts[292-303]
- src/static/js/scroll.ts[194-211]
### Expected change
Replace:
- `this._getPixelsRelativeToPercentageOfViewport(innerHeight, true)`
With one of:
- `this._getPixelsRelativeToPercentageOfViewport(innerHeight)`
- `this._getPixelsRelativeToPercentageOfViewport(innerHeight, false)`
(Optionally adjust the comment that says “mirror image” if you want it to reflect the intentional above/below percentage difference.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. undo_redo_scroll not truly regressive 📘 Rule violation ☼ Reliability

Description

The new Playwright spec only asserts that scrollY changes after undo, which can still be true with
the pre-fix fixed-offset scroll behavior and therefore may not fail if the fix is reverted. It also
does not cover Redo (Ctrl+Y), leaving the reported undo/redo regression insufficiently protected.

Code

src/tests/frontend-new/specs/undo_redo_scroll.spec.ts[R110-124]

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

Evidence

PR Compliance ID 2 requires a regression test that fails without the fix; the added test for the
below-viewport case only checks scrollAfter > scrollBefore and even notes the pre-fix behavior
scrolled to a fixed offset, which could still satisfy this assertion. The file contains only
Ctrl+Z coverage and no Ctrl+Y/redo scenario despite the bug scope being undo/redo.

src/tests/frontend-new/specs/undo_redo_scroll.spec.ts[110-124]
Best Practice: Repository guidelines

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The added Playwright regression spec can pass even if the original bug returns because it only asserts that the page scroll position changes, not that the caret (or applied change) becomes visible in the viewport. It also lacks a Redo (`Ctrl+Y`) coverage despite the issue affecting both undo/redo.
## Issue Context
Pre-fix behavior could scroll by a fixed offset and still make `scrollAfter > scrollBefore` true without actually bringing the caret into view. To be a true regression test, the spec should verify that the caret (or selection) is within the visible viewport after undo/redo.
## Fix Focus Areas
- src/tests/frontend-new/specs/undo_redo_scroll.spec.ts[21-124]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Test mutates editor DOM ☑ 🐞 Bug ☼ Reliability

Description

The new Playwright test constructs a long pad by overwriting #innerdocbody.innerHTML, bypassing
the normal editor input path used elsewhere in the suite. This can desynchronize Etherpad’s internal
rep/undo state from the DOM and makes the regression test more likely to be flaky or to test an
unrealistic state.

Code

src/tests/frontend-new/specs/undo_redo_scroll.spec.ts[R28-38]

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

Evidence

The new test directly replaces the editor DOM via innerFrame.evaluate() and dispatches a synthetic
input event. Existing scrolling-related tests build long documents via supported typing
interactions (writeToPad() + Enter), which ensures the editor model and undo stack are updated
through the intended pathways.

src/tests/frontend-new/specs/undo_redo_scroll.spec.ts[26-38]
src/tests/frontend-new/specs/page_up_down.spec.ts[12-21]
src/tests/frontend-new/helper/padHelper.ts[142-146]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The regression test populates the document by directly setting `#innerdocbody.innerHTML`. This bypasses editor input handling and can lead to inconsistent undo/redo behavior or flaky scroll assertions.
### Issue Context
Other frontend-new specs that need a long pad use `writeToPad()` and keyboard `Enter` presses to generate many lines, which keeps Etherpad’s internal state consistent.
### Fix Focus Areas
- src/tests/frontend-new/specs/undo_redo_scroll.spec.ts[26-43]
- src/tests/frontend-new/specs/undo_redo_scroll.spec.ts[81-97]
- src/tests/frontend-new/specs/page_up_down.spec.ts[16-20]
### Suggested change
Replace the `innerFrame.evaluate(() => { body.innerHTML = ... })` blocks with a loop similar to:
- `for (let i = 0; i < 120; i++) { await writeToPad(page, `line ${i+1}`); await page.keyboard.press('Enter'); }`
(or a single multi-line `writeToPad` call if you want it faster), then proceed with the undo/scroll assertions.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[qodo-free-for-open-source-projects]

1. Wrong below-scroll percentage 🐞 Bug ≡ Correctness

In scrollNodeVerticallyIntoView()'s caret-below-viewport deferred scroll, the code passes true to
_getPixelsRelativeToPercentageOfViewport(), which selects editionAboveViewport instead of
editionBelowViewport. This makes caret-below scrolling use the wrong configured margin when the
above/below percentages differ, producing incorrect final viewport positioning.

Agent Prompt

### Issue description
`scrollNodeVerticallyIntoView()`'s caret-below-viewport deferred scroll currently calls `_getPixelsRelativeToPercentageOfViewport(innerHeight, true)`, which selects `editionAboveViewport`. For caret-below behavior, it should instead use `editionBelowViewport` (by omitting the flag or passing `false`).

### Issue Context
`_getPixelsRelativeToPercentageOfViewport(innerHeight, aboveOfViewport?)` delegates to `_getPercentageToScroll(aboveOfViewport)` which chooses between `editionAboveViewport` and `editionBelowViewport`.

### Fix Focus Areas
- src/static/js/scroll.ts[292-303]
- src/static/js/scroll.ts[194-211]

### Expected change
Replace:
- `this._getPixelsRelativeToPercentageOfViewport(innerHeight, true)`
With one of:
- `this._getPixelsRelativeToPercentageOfViewport(innerHeight)`
- `this._getPixelsRelativeToPercentageOfViewport(innerHeight, false)`

(Optionally adjust the comment that says “mirror image” if you want it to reflect the intentional above/below percentage difference.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools