Skip to content
Merged
Changes from all commits
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
77 changes: 77 additions & 0 deletions src/tests/frontend-new/specs/pagedown_wrapped_lines.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import {expect, test} from "@playwright/test";
import {clearPadContent, goToNewPad} from "../helper/padHelper";

test.beforeEach(async ({page}) => {
await goToNewPad(page);
});

// Regression test for https://github.com/ether/etherpad/issues/4562
// PageDown failed to scroll when the cursor was on a very long wrapped line and
// the following lines were also very long, because getVisibleLineRange returns
// indices into rep.lines (logical lines) and collapsed to [n, n] — so the
// advance count was 0 and both caret and scroll stayed put.
test.describe('PageDown on consecutive long wrapped lines (#4562)', function () {
test.describe.configure({retries: 2});

test('PageDown scrolls when three very long lines fill the viewport', async function ({page}) {
await clearPadContent(page);

const innerFrame = page.frame('ace_inner')!;

// Insert three long lines via the editor directly — each ~2000 chars, which
// wraps to many visual rows in the viewport.
await innerFrame.evaluate(() => {
const body = document.getElementById('innerdocbody')!;
const longText = 'invisible '.repeat(200).trim();
body.innerHTML = '';
for (let i = 0; i < 3; i++) {
const div = document.createElement('div');
div.textContent = `${i + 1} ${longText}`;
body.appendChild(div);
}
// Trigger the editor to pick up the content
body.dispatchEvent(new Event('input', {bubbles: true}));
});

// Type a character at the end to make the editor register the long content
// via its normal input path (the raw innerHTML edit above is just a scaffold).
await page.keyboard.press('End');
await page.keyboard.type('!');
await page.waitForTimeout(300);

// Move caret to start of pad
await page.keyboard.down('Control');
await page.keyboard.press('Home');
await page.keyboard.up('Control');
await page.waitForTimeout(200);

// Capture initial scroll position of the outer (scrollable) frame
const outerFrame = page.frame('ace_outer')!;
const before = await outerFrame.evaluate(
() => (document.getElementById('outerdocbody') as HTMLElement).scrollTop ||
document.scrollingElement?.scrollTop || 0);

// Press PageDown — the ace handler uses a 200ms setTimeout internally.
await page.keyboard.press('PageDown');
await page.waitForTimeout(800);
Comment on lines +23 to +56

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

1. Flaky fixed sleeps 🐞 Bug ☼ Reliability

The new spec uses multiple fixed waitForTimeout() delays after direct #innerdocbody mutation and
after PageDown, which can race Etherpad/Ace incorporation, rendering, and caret/scroll updates and
cause intermittent failures. Existing similar setup in page_up_down.spec.ts uses a much longer
post-mutation wait and other specs use condition-based polling, indicating this test should wait on
observable state instead of hardcoded sleeps.
Agent Prompt
### Issue description
The new regression spec depends on fixed `waitForTimeout()` sleeps after mutating the editor DOM and after `PageDown`. This can be flaky on slow CI or different browsers because Etherpad/Ace processing, layout, and caret/scroll updates are asynchronous.

### Issue Context
A similar test that mutates `#innerdocbody` directly waits much longer before asserting, and other specs use `expect.poll()` to wait for scroll/DOM state.

### Fix Focus Areas
- src/tests/frontend-new/specs/pagedown_wrapped_lines.spec.ts[23-56]

### Suggested fix
- After setting the long lines, wait for a deterministic condition (examples):
  - `await expect(innerFrame.locator('#innerdocbody > div')).toHaveCount(3)`
  - and/or poll until the editor selection is inside `#innerdocbody`.
- After pressing `PageDown`, replace the `800ms` sleep with `expect.poll()` waiting for either:
  - `caretLine` to become `> 0`, and/or
  - outer-frame scroll position to change (see the next finding for measuring it consistently).

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

Comment on lines +40 to +56

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

2. Fixed sleeps cause flakiness 🐞 Bug ☼ Reliability

The test uses multiple waitForTimeout() sleeps around async editor incorporation and PageDown
handling, so it can observe scroll/caret state before it has settled under slower CI conditions.
This risks intermittent failures and makes the regression signal less deterministic.
Agent Prompt
### Issue description
The test relies on hard-coded timeouts (300ms/200ms/800ms) rather than waiting for an observable condition (content incorporated, caret moved, or scroll changed). Under load this can cause the assertion to run too early.

### Issue Context
The codebase already uses `expect.poll()` patterns for scroll-state stabilization, and `goToNewPad()` includes a dedicated readiness wait to avoid timing races.

### Fix Focus Areas
- src/tests/frontend-new/specs/pagedown_wrapped_lines.spec.ts[36-76]

### Suggested change
- Replace `waitForTimeout()` waits with `await expect.poll(...)`:
  - After seeding content, poll until `#innerdocbody > div` count is 3 (and/or until expected text is present).
  - After `PageDown`, poll until either scrollY/scrollTop increases OR caret line index increases.
- Consider explicitly re-focusing the editor (`await page.frame('ace_inner')!.locator('#innerdocbody').click()`) before sending `End`/typing, to ensure key events go to the editor.

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


const after = await outerFrame.evaluate(
() => (document.getElementById('outerdocbody') as HTMLElement).scrollTop ||
document.scrollingElement?.scrollTop || 0);
Comment on lines +50 to +60

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

2. Inconsistent scroll sampling 🐞 Bug ☼ Reliability

The spec samples scroll as outerdocbody.scrollTop || document.scrollingElement?.scrollTop, which
can read from different scrolling containers depending on which one happens to be non-zero, making
before/after comparisons less deterministic. Etherpad’s scrolling code scrolls the outer iframe
window and (for animation) manipulates both #outerdocbody and its parent, so the test should measure
a single consistent metric (e.g., window.scrollY in ace_outer, or
Math.max(document.documentElement.scrollTop, document.body.scrollTop)).
Agent Prompt
### Issue description
The test’s scroll measurement uses a value-dependent fallback (`A || B || 0`), which can switch sources between `before` and `after` depending on which element reports a non-zero value. This makes the regression signal less deterministic.

### Issue Context
Etherpad scroll code can scroll via the outer iframe window and also animates both `#outerdocbody` and its parent element, depending on browser/path.

### Fix Focus Areas
- src/tests/frontend-new/specs/pagedown_wrapped_lines.spec.ts[50-60]

### Suggested fix
- Replace the current evaluation with a single consistent metric, for example:
  - `() => window.scrollY` (inside the `ace_outer` frame), or
  - `() => Math.max(document.documentElement?.scrollTop ?? 0, document.body?.scrollTop ?? 0)`.
- (Optional) Wrap in `expect.poll()` to wait until the scroll metric changes after `PageDown`.

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

Comment on lines +48 to +60

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

1. Misreads outer scroll position 🐞 Bug ≡ Correctness

The new test measures scroll using #outerdocbody.scrollTop, but Etherpad’s scroll position is
tracked via the outer iframe window/pageYOffset or the documentElement/parent scroll container, so
the test can miss real scrolling. This can weaken the assertion (or fail it) depending on which
element actually owns the scroll offset in the runtime/browser.
Agent Prompt
### Issue description
The test reads scrollTop from `#outerdocbody` (the iframe body), but Etherpad’s scrolling is commonly reflected on the outer iframe window (`pageYOffset`/`scrollY`) or the scroll container (`document.scrollingElement` / `#outerdocbody.parentElement`). This can cause the test to report no scrolling even when scrolling happened.

### Issue Context
- Etherpad assigns `outerdocbody` as the outer iframe's `<body>`.
- The scrolling implementation tracks scroll via window.pageYOffset / documentElement.scrollTop.
- Other Playwright specs read scrollTop from `#outerdocbody`'s parent element.

### Fix Focus Areas
- src/tests/frontend-new/specs/pagedown_wrapped_lines.spec.ts[48-60]

### Suggested change
Replace the scroll measurement with one of:
- `outerFrame.evaluate(() => window.scrollY)` / `window.pageYOffset` (preferred, matches Scroll.getScrollY behavior)
- or `outerFrame.evaluate(() => document.scrollingElement?.scrollTop ?? 0)` (avoid `||` so `0` is preserved)
- or mirror existing tests: `page.frameLocator('iframe[name="ace_outer"]').locator('#outerdocbody').evaluate(el => el.parentElement?.scrollTop ?? 0)`

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


// Either the viewport scrolled, or the caret advanced to a later logical line.
const caretLine = await innerFrame.evaluate(() => {
const sel = document.getSelection();
if (!sel || !sel.focusNode) return 0;
let node = sel.focusNode as HTMLElement;
while (node && node.tagName !== 'DIV') node = node.parentElement!;
if (!node) return 0;
const divs = Array.from(document.getElementById('innerdocbody')!.children);
return divs.indexOf(node);
});

// Pre-fix behavior (#4562): after == before AND caretLine === 0.
// Fixed behavior: caret advances at least 1 logical line, or the viewport scrolls.
expect(after > before || caretLine > 0).toBe(true);
});
});
Loading