Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 24 additions & 7 deletions src/static/js/scroll.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 <div>, 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);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

}
}, 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
}
}
};
Expand Down
125 changes: 125 additions & 0 deletions src/tests/frontend-new/specs/undo_redo_scroll.spec.ts
Original file line number Diff line number Diff line change
@@ -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);

Check failure on line 73 in src/tests/frontend-new/specs/undo_redo_scroll.spec.ts

View workflow job for this annotation

GitHub Actions / Playwright Firefox

[firefox] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:21:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands above the view

1) [firefox] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:21:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands above the view Retry #2 ─────────────────────────────────────────────────────────────────────────────────────── Error: expect(received).toBeLessThan(expected) Expected: < 2302 Received: 2302 71 | // Pre-fix: scrollAfter stayed ≈ scrollBefore. 72 | // Fixed: scrollAfter < scrollBefore (viewport moved up toward the caret). > 73 | expect(scrollAfter).toBeLessThan(scrollBefore); | ^ 74 | }); 75 | 76 | test('Ctrl+Z scrolls viewport to caret when it lands below the view', async function ({page}) { at /home/runner/work/etherpad/etherpad/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts:73:25

Check failure on line 73 in src/tests/frontend-new/specs/undo_redo_scroll.spec.ts

View workflow job for this annotation

GitHub Actions / Playwright Firefox

[firefox] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:21:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands above the view

1) [firefox] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:21:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands above the view Retry #1 ─────────────────────────────────────────────────────────────────────────────────────── Error: expect(received).toBeLessThan(expected) Expected: < 2302 Received: 2302 71 | // Pre-fix: scrollAfter stayed ≈ scrollBefore. 72 | // Fixed: scrollAfter < scrollBefore (viewport moved up toward the caret). > 73 | expect(scrollAfter).toBeLessThan(scrollBefore); | ^ 74 | }); 75 | 76 | test('Ctrl+Z scrolls viewport to caret when it lands below the view', async function ({page}) { at /home/runner/work/etherpad/etherpad/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts:73:25

Check failure on line 73 in src/tests/frontend-new/specs/undo_redo_scroll.spec.ts

View workflow job for this annotation

GitHub Actions / Playwright Firefox

[firefox] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:21:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands above the view

1) [firefox] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:21:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands above the view Error: expect(received).toBeLessThan(expected) Expected: < 2302 Received: 2302 71 | // Pre-fix: scrollAfter stayed ≈ scrollBefore. 72 | // Fixed: scrollAfter < scrollBefore (viewport moved up toward the caret). > 73 | expect(scrollAfter).toBeLessThan(scrollBefore); | ^ 74 | }); 75 | 76 | test('Ctrl+Z scrolls viewport to caret when it lands below the view', async function ({page}) { at /home/runner/work/etherpad/etherpad/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts:73:25

Check failure on line 73 in src/tests/frontend-new/specs/undo_redo_scroll.spec.ts

View workflow job for this annotation

GitHub Actions / Playwright Chrome

[chromium] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:21:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands above the view

1) [chromium] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:21:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands above the view Retry #2 ─────────────────────────────────────────────────────────────────────────────────────── Error: expect(received).toBeLessThan(expected) Expected: < 2302 Received: 2302 71 | // Pre-fix: scrollAfter stayed ≈ scrollBefore. 72 | // Fixed: scrollAfter < scrollBefore (viewport moved up toward the caret). > 73 | expect(scrollAfter).toBeLessThan(scrollBefore); | ^ 74 | }); 75 | 76 | test('Ctrl+Z scrolls viewport to caret when it lands below the view', async function ({page}) { at /home/runner/work/etherpad/etherpad/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts:73:25

Check failure on line 73 in src/tests/frontend-new/specs/undo_redo_scroll.spec.ts

View workflow job for this annotation

GitHub Actions / Playwright Chrome

[chromium] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:21:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands above the view

1) [chromium] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:21:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands above the view Retry #1 ─────────────────────────────────────────────────────────────────────────────────────── Error: expect(received).toBeLessThan(expected) Expected: < 2302 Received: 2302 71 | // Pre-fix: scrollAfter stayed ≈ scrollBefore. 72 | // Fixed: scrollAfter < scrollBefore (viewport moved up toward the caret). > 73 | expect(scrollAfter).toBeLessThan(scrollBefore); | ^ 74 | }); 75 | 76 | test('Ctrl+Z scrolls viewport to caret when it lands below the view', async function ({page}) { at /home/runner/work/etherpad/etherpad/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts:73:25

Check failure on line 73 in src/tests/frontend-new/specs/undo_redo_scroll.spec.ts

View workflow job for this annotation

GitHub Actions / Playwright Chrome

[chromium] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:21:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands above the view

1) [chromium] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:21:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands above the view Error: expect(received).toBeLessThan(expected) Expected: < 2302 Received: 2302 71 | // Pre-fix: scrollAfter stayed ≈ scrollBefore. 72 | // Fixed: scrollAfter < scrollBefore (viewport moved up toward the caret). > 73 | expect(scrollAfter).toBeLessThan(scrollBefore); | ^ 74 | }); 75 | 76 | test('Ctrl+Z scrolls viewport to caret when it lands below the view', async function ({page}) { at /home/runner/work/etherpad/etherpad/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts:73:25
});

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

Check failure on line 123 in src/tests/frontend-new/specs/undo_redo_scroll.spec.ts

View workflow job for this annotation

GitHub Actions / Playwright Firefox

[firefox] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:76:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands below the view

2) [firefox] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:76:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands below the view Retry #2 ─────────────────────────────────────────────────────────────────────────────────────── Error: expect(received).toBeGreaterThan(expected) Expected: > 0 Received: 0 121 | // the worst case did nothing useful. Fixed: viewport moves down toward 122 | // the caret so scrollAfter > scrollBefore. > 123 | expect(scrollAfter).toBeGreaterThan(scrollBefore); | ^ 124 | }); 125 | }); 126 | at /home/runner/work/etherpad/etherpad/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts:123:25

Check failure on line 123 in src/tests/frontend-new/specs/undo_redo_scroll.spec.ts

View workflow job for this annotation

GitHub Actions / Playwright Firefox

[firefox] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:76:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands below the view

2) [firefox] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:76:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands below the view Retry #1 ─────────────────────────────────────────────────────────────────────────────────────── Error: expect(received).toBeGreaterThan(expected) Expected: > 0 Received: 0 121 | // the worst case did nothing useful. Fixed: viewport moves down toward 122 | // the caret so scrollAfter > scrollBefore. > 123 | expect(scrollAfter).toBeGreaterThan(scrollBefore); | ^ 124 | }); 125 | }); 126 | at /home/runner/work/etherpad/etherpad/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts:123:25

Check failure on line 123 in src/tests/frontend-new/specs/undo_redo_scroll.spec.ts

View workflow job for this annotation

GitHub Actions / Playwright Firefox

[firefox] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:76:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands below the view

2) [firefox] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:76:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands below the view Error: expect(received).toBeGreaterThan(expected) Expected: > 0 Received: 0 121 | // the worst case did nothing useful. Fixed: viewport moves down toward 122 | // the caret so scrollAfter > scrollBefore. > 123 | expect(scrollAfter).toBeGreaterThan(scrollBefore); | ^ 124 | }); 125 | }); 126 | at /home/runner/work/etherpad/etherpad/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts:123:25

Check failure on line 123 in src/tests/frontend-new/specs/undo_redo_scroll.spec.ts

View workflow job for this annotation

GitHub Actions / Playwright Chrome

[chromium] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:76:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands below the view

2) [chromium] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:76:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands below the view Retry #2 ─────────────────────────────────────────────────────────────────────────────────────── Error: expect(received).toBeGreaterThan(expected) Expected: > 0 Received: 0 121 | // the worst case did nothing useful. Fixed: viewport moves down toward 122 | // the caret so scrollAfter > scrollBefore. > 123 | expect(scrollAfter).toBeGreaterThan(scrollBefore); | ^ 124 | }); 125 | }); 126 | at /home/runner/work/etherpad/etherpad/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts:123:25

Check failure on line 123 in src/tests/frontend-new/specs/undo_redo_scroll.spec.ts

View workflow job for this annotation

GitHub Actions / Playwright Chrome

[chromium] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:76:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands below the view

2) [chromium] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:76:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands below the view Retry #1 ─────────────────────────────────────────────────────────────────────────────────────── Error: expect(received).toBeGreaterThan(expected) Expected: > 0 Received: 0 121 | // the worst case did nothing useful. Fixed: viewport moves down toward 122 | // the caret so scrollAfter > scrollBefore. > 123 | expect(scrollAfter).toBeGreaterThan(scrollBefore); | ^ 124 | }); 125 | }); 126 | at /home/runner/work/etherpad/etherpad/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts:123:25

Check failure on line 123 in src/tests/frontend-new/specs/undo_redo_scroll.spec.ts

View workflow job for this annotation

GitHub Actions / Playwright Chrome

[chromium] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:76:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands below the view

2) [chromium] › tests/frontend-new/specs/undo_redo_scroll.spec.ts:76:7 › Undo/redo scroll-to-caret (#7007) › Ctrl+Z scrolls viewport to caret when it lands below the view Error: expect(received).toBeGreaterThan(expected) Expected: > 0 Received: 0 121 | // the worst case did nothing useful. Fixed: viewport moves down toward 122 | // the caret so scrollAfter > scrollBefore. > 123 | expect(scrollAfter).toBeGreaterThan(scrollBefore); | ^ 124 | }); 125 | }); 126 | at /home/runner/work/etherpad/etherpad/src/tests/frontend-new/specs/undo_redo_scroll.spec.ts:123:25
});
});
Loading