fix(editor): PageDown advances on consecutive long wrapped lines - #7555
Merged
SamTV12345 merged 1 commit intoJul 27, 2026
Merged
Conversation
PR Summary by Qodofix(editor): PageUp/PageDown advance on consecutive long wrapped lines
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
Context used 1. Misreads outer scroll position
|
SamTV12345
approved these changes
Apr 26, 2026
…lines The page up/down handler advances the caret by numberOfLinesInViewport computed from scroll.getVisibleLineRange(). That helper returns indices into rep.lines (logical lines, not visual/wrapped rows), so when one wrapped logical line fills the viewport — e.g., three consecutive lines of ~2000 chars each — the range collapses to [n, n] and the advance count becomes 0. The caret stays on line n, scroll stays at 0, and the user sees "PageDown does nothing". Clamp the advance to at least one logical line so the caret and viewport always move. Includes a Playwright regression test covering the reporter's repro (three very long lines, Ctrl+Home, PageDown). Closes ether#4562 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JohnMcLear
force-pushed
the
fix/pagedown-wrapped-lines-4562
branch
from
May 4, 2026 14:56
aecbf54 to
c8b1008
Compare
Member
Author
|
tested locally and page up/down doesn't work on this branch... |
JohnMcLear
marked this pull request as draft
May 4, 2026 15:00
Member
Author
|
@SamTV12345 did you test this locally before approving? I don't see a fixed UX. |
Code Review by Qodo
1. Flaky fixed sleeps
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+23
to
+56
| 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); |
There was a problem hiding this comment.
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
+50
to
+60
| 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); | ||
|
|
||
| const after = await outerFrame.evaluate( | ||
| () => (document.getElementById('outerdocbody') as HTMLElement).scrollTop || | ||
| document.scrollingElement?.scrollTop || 0); |
There was a problem hiding this comment.
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
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+48
to
+60
| // 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); | ||
|
|
||
| const after = await outerFrame.evaluate( | ||
| () => (document.getElementById('outerdocbody') as HTMLElement).scrollTop || | ||
| document.scrollingElement?.scrollTop || 0); |
There was a problem hiding this comment.
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
Comment on lines
+40
to
+56
| 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); |
There was a problem hiding this comment.
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
|
Code review by qodo was updated up to the latest commit c8b1008 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix for #4562 — PageDown (and PageUp) became no-ops when the cursor sat on a very long wrapped line and the following lines were also very long.
Root cause
The page up/down handler in
ace2_inner.tsadvances the caret bynumberOfLinesInViewport, computed fromscroll.getVisibleLineRange(rep). That helper returns indices intorep.lines— logical lines, not visual/wrapped rows. When a single wrapped logical line fills the viewport (e.g., three ~2000-char lines), the range collapses to[n, n]and the advance count becomes0:Fix: clamp to at least one logical line so the caret and viewport always advance.
Test plan
Ctrl+Home,PageDown→ viewport scrolls or caret advances)page_up_down.spec.tsstill passes (no behavior change for the common case where the viewport spans multiple logical lines)pnpm run ts-checkclean locallyCloses #4562
🤖 Generated with Claude Code