From 5d66b7f1cf24180971687ddb21d83b1b392620b7 Mon Sep 17 00:00:00 2001 From: John McLear Date: Tue, 28 Jul 2026 18:58:01 +0100 Subject: [PATCH 1/3] fix: read caret geometry from the editor document (#8038) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `caretPosition.getPosition()` read `window.getSelection()` from the pad's top-level window. Since 3.0 the editor modules are bundled into that window instead of being loaded inside the ace_inner iframe, so that selection is always empty: getPosition() returned null, and `_isCaretAtTheBottomOfViewport()` passed it straight into getBottomOfNextBrowserLine(), which threw `can't access property "bottom"` and popped up the "An error occurred" box for anyone running with `scrollWhenCaretIsInTheLastLineOfViewport` enabled. Caret geometry now comes from the ace_inner document, which is the same coordinate space scroll.ts's viewport arithmetic already uses (the inner frame is as tall as its content and never scrolls). Callers handle a null position — no caret in the pad — instead of asserting it away. Scrolling in the same module had the mirror-image problem: `outerWin` is the ace_outer iframe *element*, and Element.scrollTo()/scrollBy() on an iframe silently does nothing, so the scroll this feature computed never actually happened. Those calls now go through the frame's contentWindow. Fixes #8038 Co-Authored-By: Claude Opus 5 (1M context) --- src/static/js/caretPosition.ts | 120 ++++++------ src/static/js/scroll.ts | 72 ++++++-- .../specs/scroll_caret_viewport.spec.ts | 174 ++++++++++++++++++ 3 files changed, 295 insertions(+), 71 deletions(-) create mode 100644 src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts diff --git a/src/static/js/caretPosition.ts b/src/static/js/caretPosition.ts index 5134a0ed072..86f23b7ac7c 100644 --- a/src/static/js/caretPosition.ts +++ b/src/static/js/caretPosition.ts @@ -3,24 +3,36 @@ // One rep.line(div) can be broken in more than one line in the browser. // This function is useful to get the caret position of the line as // is represented by the browser +// +// NOTE: every DOM lookup here has to go through the editor document that is +// passed in, *not* through the ambient `window` / `document`. This module is +// bundled into the pad's top-level window, while the caret lives in the +// ace_inner iframe nested inside ace_outer. Reading the top window's selection +// always yields an empty selection, which used to make getPosition() return +// null and crash the callers below (#8038). import {Position, RepModel, RepNode} from "./types/RepModel"; -export const getPosition = () => { - const range = getSelectionRange(); - // @ts-ignore - if (!range || $(range.endContainer).closest('body')[0].id !== 'innerdocbody') return null; +export const getPosition = (doc: Document): Position | null => { + const range = getSelectionRange(doc); + if (!range || getBodyOf(range.endContainer)?.id !== 'innerdocbody') return null; // When there's a
or any element that has no height, we can't get the dimension of the // element where the caret is. As we can't get the element height, we create a text node to get // the dimensions on the position. const clonedRange = createSelectionRange(range); - const shadowCaret = $(document.createTextNode('|')); - clonedRange.insertNode(shadowCaret[0]); - clonedRange.selectNode(shadowCaret[0]); + const shadowCaret = doc.createTextNode('|'); + clonedRange.insertNode(shadowCaret); + clonedRange.selectNode(shadowCaret); const line = getPositionOfElementOrSelection(clonedRange); shadowCaret.remove(); return line; }; +// The caret node is usually a text node, so climb to an element first. +const getBodyOf = (node: Node) => { + const element = node.nodeType === Node.ELEMENT_NODE ? node as Element : node.parentElement; + return element ? element.closest('body') : null; +}; + const createSelectionRange = (range: Range) => { const clonedRange = range.cloneRange(); @@ -33,7 +45,7 @@ const createSelectionRange = (range: Range) => { return clonedRange; }; -const getPositionOfRepLineAtOffset = (node: any, offset: number) => { +const getPositionOfRepLineAtOffset = (node: any, offset: number, doc: Document) => { // it is not a text node, so we cannot make a selection if (node.tagName === 'BR' || node.tagName === 'EMPTY') { return getPositionOfElementOrSelection(node); @@ -43,7 +55,7 @@ const getPositionOfRepLineAtOffset = (node: any, offset: number) => { node = node.nextSibling as any; } - const newRange = new Range(); + const newRange = doc.createRange(); newRange.setStart(node, offset); newRange.setEnd(node, offset); const linePosition = getPositionOfElementOrSelection(newRange); @@ -66,29 +78,31 @@ const getPositionOfElementOrSelection = (element: Range):Position => { // where is the top of the previous line // [2] the line before is part of another rep line. It's possible this line has different margins // height. So we have to get the exactly position of the line -export const getPositionTopOfPreviousBrowserLine = (caretLinePosition: Position, rep: RepModel) => { - let previousLineTop = caretLinePosition.top - caretLinePosition.height; // [1] - const isCaretLineFirstBrowserLine = caretLineIsFirstBrowserLine(caretLinePosition.top, rep); - - // the caret is in the beginning of a rep line, so the previous browser line - // is the last line browser line of the a rep line - if (isCaretLineFirstBrowserLine) { // [2] - const lineBeforeCaretLine = rep.selStart[0] - 1; - const firstLineVisibleBeforeCaretLine = getPreviousVisibleLine(lineBeforeCaretLine, rep); - const linePosition = - getDimensionOfLastBrowserLineOfRepLine(firstLineVisibleBeforeCaretLine, rep); - previousLineTop = linePosition.top; - } - return previousLineTop; -}; +export const getPositionTopOfPreviousBrowserLine = + (caretLinePosition: Position, rep: RepModel, doc: Document) => { + let previousLineTop = caretLinePosition.top - caretLinePosition.height; // [1] + const isCaretLineFirstBrowserLine = + caretLineIsFirstBrowserLine(caretLinePosition.top, rep, doc); + + // the caret is in the beginning of a rep line, so the previous browser line + // is the last line browser line of the a rep line + if (isCaretLineFirstBrowserLine) { // [2] + const lineBeforeCaretLine = rep.selStart[0] - 1; + const firstLineVisibleBeforeCaretLine = getPreviousVisibleLine(lineBeforeCaretLine, rep); + const linePosition = + getDimensionOfLastBrowserLineOfRepLine(firstLineVisibleBeforeCaretLine, rep, doc); + previousLineTop = linePosition.top; + } + return previousLineTop; + }; -const caretLineIsFirstBrowserLine = (caretLineTop: number, rep: RepModel) => { +const caretLineIsFirstBrowserLine = (caretLineTop: number, rep: RepModel, doc: Document) => { const caretRepLine = rep.selStart[0]; const lineNode = rep.lines.atIndex(caretRepLine).lineNode; const firstRootNode = getFirstRootChildNode(lineNode); // to get the position of the node we get the position of the first char - const positionOfFirstRootNode = getPositionOfRepLineAtOffset(firstRootNode, 1); + const positionOfFirstRootNode = getPositionOfRepLineAtOffset(firstRootNode, 1, doc); return positionOfFirstRootNode.top === caretLineTop; }; @@ -101,13 +115,13 @@ const getFirstRootChildNode = (node: RepNode) => { } }; -const getDimensionOfLastBrowserLineOfRepLine = (line: number, rep: RepModel) => { +const getDimensionOfLastBrowserLineOfRepLine = (line: number, rep: RepModel, doc: Document) => { const lineNode = rep.lines.atIndex(line).lineNode; const lastRootChildNode = getLastRootChildNode(lineNode); // we get the position of the line in the last char of it const lastRootChildNodePosition = - getPositionOfRepLineAtOffset(lastRootChildNode.node, lastRootChildNode.length); + getPositionOfRepLineAtOffset(lastRootChildNode.node, lastRootChildNode.length, doc); return lastRootChildNodePosition; }; @@ -127,31 +141,32 @@ const getLastRootChildNode = (node: RepNode) => { // So, we can use the caret line to calculate the bottom of the line. // [2] the next line is part of another rep line. // It's possible this line has different dimensions, so we have to get the exactly dimension of it -export const getBottomOfNextBrowserLine = (caretLinePosition: Position, rep: RepModel) => { - let nextLineBottom = caretLinePosition.bottom + caretLinePosition.height; // [1] - const isCaretLineLastBrowserLine = - caretLineIsLastBrowserLineOfRepLine(caretLinePosition.top, rep); - - // the caret is at the end of a rep line, so we can get the next browser line dimension - // using the position of the first char of the next rep line - if (isCaretLineLastBrowserLine) { // [2] - const nextLineAfterCaretLine = rep.selStart[0] + 1; - const firstNextLineVisibleAfterCaretLine = getNextVisibleLine(nextLineAfterCaretLine, rep); - const linePosition = - getDimensionOfFirstBrowserLineOfRepLine(firstNextLineVisibleAfterCaretLine, rep); - nextLineBottom = linePosition.bottom; - } - return nextLineBottom; -}; +export const getBottomOfNextBrowserLine = + (caretLinePosition: Position, rep: RepModel, doc: Document) => { + let nextLineBottom = caretLinePosition.bottom + caretLinePosition.height; // [1] + const isCaretLineLastBrowserLine = + caretLineIsLastBrowserLineOfRepLine(caretLinePosition.top, rep, doc); + + // the caret is at the end of a rep line, so we can get the next browser line dimension + // using the position of the first char of the next rep line + if (isCaretLineLastBrowserLine) { // [2] + const nextLineAfterCaretLine = rep.selStart[0] + 1; + const firstNextLineVisibleAfterCaretLine = getNextVisibleLine(nextLineAfterCaretLine, rep); + const linePosition = + getDimensionOfFirstBrowserLineOfRepLine(firstNextLineVisibleAfterCaretLine, rep, doc); + nextLineBottom = linePosition.bottom; + } + return nextLineBottom; + }; -const caretLineIsLastBrowserLineOfRepLine = (caretLineTop: number, rep: RepModel) => { +const caretLineIsLastBrowserLineOfRepLine = (caretLineTop: number, rep: RepModel, doc: Document) => { const caretRepLine = rep.selStart[0]; const lineNode = rep.lines.atIndex(caretRepLine).lineNode; const lastRootChildNode = getLastRootChildNode(lineNode); // we take a rep line and get the position of the last char of it const lastRootChildNodePosition = - getPositionOfRepLineAtOffset(lastRootChildNode.node, lastRootChildNode.length); + getPositionOfRepLineAtOffset(lastRootChildNode.node, lastRootChildNode.length, doc); return lastRootChildNodePosition.top === caretLineTop; }; @@ -181,20 +196,21 @@ export const getNextVisibleLine = (line: number, rep: RepModel): number => { const isLineVisible = (line: number, rep: RepModel) => rep.lines.atIndex(line).lineNode.offsetHeight > 0; -const getDimensionOfFirstBrowserLineOfRepLine = (line: number, rep: RepModel) => { +const getDimensionOfFirstBrowserLineOfRepLine = (line: number, rep: RepModel, doc: Document) => { const lineNode = rep.lines.atIndex(line).lineNode; const firstRootChildNode = getFirstRootChildNode(lineNode); // we can get the position of the line, getting the position of the first char of the rep line - const firstRootChildNodePosition = getPositionOfRepLineAtOffset(firstRootChildNode, 1); + const firstRootChildNodePosition = getPositionOfRepLineAtOffset(firstRootChildNode, 1, doc); return firstRootChildNodePosition; }; -const getSelectionRange = () => { - if (!window.getSelection) { - return; +const getSelectionRange = (doc: Document) => { + const win = doc.defaultView; + if (!win || !win.getSelection) { + return null; } - const selection = window.getSelection(); + const selection = win.getSelection(); if (selection && selection.type !== 'None' && selection.rangeCount > 0) { return selection.getRangeAt(0); } else { diff --git a/src/static/js/scroll.ts b/src/static/js/scroll.ts index d4fe5a5d3b4..9a56d755c12 100644 --- a/src/static/js/scroll.ts +++ b/src/static/js/scroll.ts @@ -1,5 +1,5 @@ import {getBottomOfNextBrowserLine, getNextVisibleLine, getPosition, getPositionTopOfPreviousBrowserLine, getPreviousVisibleLine} from './caretPosition'; -import {Position, RepModel, RepNode, WindowElementWithScrolling} from "./types/RepModel"; +import {Position, RepModel, RepNode} from "./types/RepModel"; class Scroll { @@ -18,6 +18,15 @@ class Scroll { this.rootDocument = document; } + // `outerWin` is the ace_outer *iframe element*, not a window — this module is + // bundled into the pad's top-level window and ace2_inner.ts hands us the + // element. Scrolling therefore has to go through its contentWindow: calling + // scrollTo()/scrollBy() on the element itself silently does nothing, because + // an iframe element has no scrollable box of its own. + _getOuterWin(): Window | null { + return this.outerWin.contentWindow; + }; + scrollWhenCaretIsInTheLastLineOfViewportWhenNecessary(rep: RepModel, isScrollableEvent: boolean, innerHeight: number) { // are we placing the caret on the line at the bottom of viewport? // And if so, do we need to scroll the editor, as defined on the settings.json? @@ -52,7 +61,19 @@ class Scroll { } } + // The editor document, i.e. the document of the ace_inner iframe nested + // inside ace_outer. This module runs in the pad's top-level window, so every + // caret measurement has to be made against this document rather than the + // ambient one. Resolved on demand because browsers (Firefox in particular) + // may replace an iframe's document after load. + _getInnerDoc(): Document | null { + const innerFrame = this.doc.getElementsByName('ace_inner')[0] as HTMLIFrameElement | undefined; + return innerFrame ? innerFrame.contentDocument : null; + }; + _isCaretAtTheBottomOfViewport(rep: RepModel) { + const innerDoc = this._getInnerDoc(); + if (!innerDoc) return false; // computing a line position using getBoundingClientRect() is expensive. // (obs: getBoundingClientRect() is called on caretPosition.getPosition()) // To avoid that, we only call this function when it is possible that the @@ -66,9 +87,11 @@ class Scroll { this._isLinePartiallyVisibleOnViewport(firstLineVisibleAfterCaretLine, rep); if (caretLineIsPartiallyVisibleOnViewport || lineAfterCaretLineIsPartiallyVisibleOnViewport) { // check if the caret is in the bottom of the viewport - const caretLinePosition = getPosition()!; + const caretLinePosition = getPosition(innerDoc); + // no caret in the pad (e.g. focus is outside the editor), so nothing to scroll to + if (!caretLinePosition) return false; const viewportBottom = this._getViewPortTopBottom().bottom; - const nextLineBottom = getBottomOfNextBrowserLine(caretLinePosition, rep); + const nextLineBottom = getBottomOfNextBrowserLine(caretLinePosition, rep, innerDoc); return nextLineBottom > viewportBottom; } return false; @@ -124,9 +147,9 @@ class Scroll { }; _getScrollXY() { - const win = this.outerWin as WindowElementWithScrolling; + const win = this._getOuterWin(); const odoc = this.doc; - if (typeof (win.pageYOffset) === 'number') { + if (win && typeof (win.pageYOffset) === 'number') { return { x: win.pageXOffset, y: win.pageYOffset, @@ -139,29 +162,33 @@ class Scroll { y: docel.scrollTop, }; } + return {x: 0, y: 0}; }; getScrollX() { - return this._getScrollXY()!.x; + return this._getScrollXY().x; }; getScrollY () { - return this._getScrollXY()!.y; + return this._getScrollXY().y; }; setScrollX(x: number) { - this.outerWin.scrollTo(x, this.getScrollY()); + this.setScrollXY(x, this.getScrollY()); }; setScrollY(y: number) { - this.outerWin.scrollTo(this.getScrollX(), y); + this.setScrollXY(this.getScrollX(), y); }; setScrollXY(x: number, y: number) { - this.outerWin.scrollTo(x, y); + const win = this._getOuterWin(); + if (win) win.scrollTo(x, y); }; _isCaretAtTheTopOfViewport(rep: RepModel) { + const innerDoc = this._getInnerDoc(); + if (!innerDoc) return false; const caretLine = rep.selStart[0]; const linePrevCaretLine = caretLine - 1; const firstLineVisibleBeforeCaretLine = @@ -171,16 +198,18 @@ class Scroll { const lineBeforeCaretLineIsPartiallyVisibleOnViewport = this._isLinePartiallyVisibleOnViewport(firstLineVisibleBeforeCaretLine, rep); if (caretLineIsPartiallyVisibleOnViewport || lineBeforeCaretLineIsPartiallyVisibleOnViewport) { - const caretLinePosition = getPosition(); // get the position of the browser line + const caretLinePosition = getPosition(innerDoc); // get the position of the browser line + // no caret in the pad (e.g. focus is outside the editor), so nothing to scroll to + if (!caretLinePosition) return false; const viewportPosition = this._getViewPortTopBottom(); const viewportTop = viewportPosition.top; const viewportBottom = viewportPosition.bottom; - const caretLineIsBelowViewportTop = caretLinePosition!.bottom >= viewportTop; - const caretLineIsAboveViewportBottom = caretLinePosition!.top < viewportBottom; + const caretLineIsBelowViewportTop = caretLinePosition.bottom >= viewportTop; + const caretLineIsAboveViewportBottom = caretLinePosition.top < viewportBottom; const caretLineIsInsideOfViewport = caretLineIsBelowViewportTop && caretLineIsAboveViewportBottom; if (caretLineIsInsideOfViewport) { - const prevLineTop = getPositionTopOfPreviousBrowserLine(caretLinePosition!, rep); + const prevLineTop = getPositionTopOfPreviousBrowserLine(caretLinePosition, rep, innerDoc); const previousLineIsAboveViewportTop = prevLineTop < viewportTop; return previousLineIsAboveViewportTop; } @@ -229,7 +258,8 @@ class Scroll { }; _scrollYPageWithoutAnimation(pixelsToScroll: number) { - this.outerWin.scrollBy(0, pixelsToScroll); + const win = this._getOuterWin(); + if (win) win.scrollBy(0, pixelsToScroll); }; _scrollYPageWithAnimation(pixelsToScroll: number, durationOfAnimationToShowFocusline: number) { @@ -260,12 +290,14 @@ class Scroll { scrollNodeVerticallyIntoView(rep: RepModel, innerHeight: number) { + const innerDoc = this._getInnerDoc(); + if (!innerDoc) return; const viewport = this._getViewPortTopBottom(); // when the selection changes outside of the viewport the browser automatically scrolls the line // to inside of the viewport. Tested on IE, Firefox, Chrome in releases from 2015 until now // So, when the line scrolled gets outside of the viewport we let the browser handle it. - const linePosition = getPosition(); + const linePosition = getPosition(innerDoc); if (linePosition) { const distanceOfTopOfViewport = linePosition.top - viewport.top; const distanceOfBottomOfViewport = viewport.bottom - linePosition.bottom - linePosition.height; @@ -278,9 +310,11 @@ class Scroll { } else if (caretIsBelowOfViewport) { // setTimeout is required here as line might not be fully rendered onto the pad setTimeout(() => { - const outer = window.parent; - // scroll to the very end of the pad outer - outer.scrollTo(0, outer[0].innerHeight); + const outer = this._getOuterWin(); + // scroll to the very end of the pad outer. The inner frame is as tall + // as the whole document, so its scrollHeight is the end of the pad; + // scrollTo() clamps to the outer frame's maximum scroll offset. + if (outer) outer.scrollTo(0, innerDoc.documentElement.scrollHeight); }, 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 diff --git a/src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts b/src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts new file mode 100644 index 00000000000..01f9148c2f2 --- /dev/null +++ b/src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts @@ -0,0 +1,174 @@ +import {expect, Page, test} from "@playwright/test"; +import {randomUUID} from "node:crypto"; + +// Regression tests for https://github.com/ether/etherpad/issues/8038 +// +// `scrollWhenFocusLineIsOutOfViewport.scrollWhenCaretIsInTheLastLineOfViewport` +// is off by default, so this suite turns it on client-side by patching +// clientVars before pad.ts reads it (Scroll caches the settings object in its +// constructor). With the setting on, moving the caret onto the line at the +// bottom edge of the viewport runs caretPosition.getPosition(), which used to +// look at the *top* window's selection — the caret lives in the ace_inner +// iframe, so that selection is always empty and getPosition() returned null, +// crashing in getBottomOfNextBrowserLine ("can't access property bottom"). + +const LINE_COUNT = 40; + +const padLines = (page: Page) => page.frameLocator('iframe[name="ace_outer"]') + .frameLocator('iframe[name="ace_inner"]') + .locator('#innerdocbody > div'); + +const getOuterScrollY = async (page: Page) => await page.evaluate(() => { + const outerFrame = document.getElementsByName('ace_outer')[0] as HTMLIFrameElement; + return outerFrame.contentWindow!.pageYOffset; +}); + +// Typing and the editor's own scroll handling keep moving the viewport for a +// while after the last keystroke; measuring before that settles makes the +// numbers below meaningless (and is flaky under parallel workers). +const waitForStableScroll = async (page: Page) => { + let previous = NaN; + for (let i = 0; i < 24; i++) { + const current = await getOuterScrollY(page); + if (current === previous) return current; + previous = current; + await page.waitForTimeout(250); + } + return previous; +}; + +const preparePad = async (page: Page, percentageBelowViewport: number) => { + await page.addInitScript(({pct}) => { + let stored: unknown; + Object.defineProperty(window, 'clientVars', { + configurable: true, + get() { return stored; }, + set(v) { + if (v != null && typeof v === 'object') { + const cv = v as { + padDeletionToken?: string | null, + scrollWhenFocusLineIsOutOfViewport?: Record, + }; + // The one-time deletion-token modal steals focus and eats clicks. + cv.padDeletionToken = null; + cv.scrollWhenFocusLineIsOutOfViewport = { + percentage: {editionAboveViewport: 0, editionBelowViewport: pct}, + duration: 0, + scrollWhenCaretIsInTheLastLineOfViewport: true, + percentageToScrollWhenUserPressesArrowUp: 0, + }; + } + stored = v; + }, + }); + }, {pct: percentageBelowViewport}); + + const errors: string[] = []; + page.on('pageerror', (e) => errors.push(e.message)); + + await page.goto(`http://localhost:9001/p/SCROLL_CARET_${randomUUID()}`); + await page.waitForSelector('iframe[name="ace_outer"]'); + await page.waitForSelector('#editorcontainer.initialized'); + const body = page.frameLocator('iframe[name="ace_outer"]') + .frameLocator('iframe[name="ace_inner"]') + .locator('#innerdocbody[contenteditable="true"]'); + await body.waitFor({state: 'attached'}); + await body.click(); + + // Enough lines that the pad overflows the viewport several times over. + for (let i = 0; i < LINE_COUNT; i++) { + await page.keyboard.type(`line ${i}`); + await page.keyboard.press('Enter'); + } + await waitForStableScroll(page); + + // Typing leaves the caret on the last line with the pad scrolled to its end. + // Put the caret back near the top; the editor follows the caret, so this is + // a viewport position that stays put. + await padLines(page).nth(3).click(); + await waitForStableScroll(page); + + return {errors}; +}; + +// Clicks the line sitting at the very bottom edge of the editor viewport — +// the case #8038 is about. Clicking through a locator would scroll the target +// into view first and move the very edge we are aiming at, so this clicks by +// coordinate instead. +const clickBottomEdgeOfViewport = async (page: Page) => { + const point = await page.evaluate(() => { + const outerFrame = document.getElementsByName('ace_outer')[0] as HTMLIFrameElement; + const outerWin = outerFrame.contentWindow!; + const outerDoc = outerWin.document; + const innerFrame = outerDoc.getElementsByName('ace_inner')[0] as HTMLIFrameElement; + const innerDoc = innerFrame.contentWindow!.document; + const frameRect = outerFrame.getBoundingClientRect(); + const visibleBottom = outerWin.pageYOffset + outerDoc.documentElement.clientHeight; + const lines = [...innerDoc.body.children].filter( + (e) => e.tagName === 'DIV') as HTMLElement[]; + // the line crossing the bottom edge, i.e. the one that is only partly + // visible: its next line is the first one below the viewport + const straddling = lines.find((l) => + l.offsetTop < visibleBottom && l.offsetTop + l.offsetHeight > visibleBottom); + const target = straddling || lines[lines.length - 1]; + // a few pixels into the visible sliver of that line + const y = frameRect.top + (target.offsetTop - outerWin.pageYOffset) + 3; + return {x: Math.round(frameRect.left + 80), y: Math.round(y)}; + }); + await page.mouse.click(point.x, point.y); +}; + +test.describe('scroll when caret is in the last line of the viewport', () => { + // A small, fixed viewport keeps the numbers below predictable: 40 lines + // overflow it several times over, and the jump this feature adds is large + // compared to the line-sized steps the browser scrolls on its own. + test.use({viewport: {width: 900, height: 500}}); + + test.beforeEach(async ({context}) => { + await context.clearCookies(); + }); + + test('moving the caret to the bottom of the viewport does not throw', + async ({page}) => { + const {errors} = await preparePad(page, 0); + await clickBottomEdgeOfViewport(page); + await page.waitForTimeout(500); + // arrow keys walk the caret across the edge as well + for (let i = 0; i < 4; i++) { + await page.keyboard.press('ArrowDown'); + await page.waitForTimeout(200); + } + expect(errors).toEqual([]); + }); + + test('scrolls the caret line clear of the bottom edge of the viewport', + async ({page, browserName}) => { + // Firefox applies its own scroll-into-view for the clicked line before + // this check runs, which leaves the "is the next line past the bottom + // edge?" comparison decided by a pixel or two — too marginal to assert + // on. The behaviour is the same there; only the trigger is hard to hit + // reproducibly. The crash regression above still covers both browsers. + test.skip(browserName === 'firefox', + 'trigger geometry is sub-pixel marginal in Firefox'); + const {errors} = await preparePad(page, 0.6); + + // Whether a given click lands exactly on the line the editor considers + // to be at the bottom edge comes down to sub-pixel line geometry, so + // try a few times and look for the one big jump: putting the caret + // line 60% of a viewport clear of the bottom edge. The browser never + // scrolls that far on its own — line by line it moves a line height at + // a time — and it only happens when the caret geometry is read from + // the editor document. + let biggestJump = 0; + for (let attempt = 0; attempt < 4 && biggestJump <= 300; attempt++) { + const before = await getOuterScrollY(page); + await clickBottomEdgeOfViewport(page); + await page.waitForTimeout(750); + biggestJump = Math.max(biggestJump, await getOuterScrollY(page) - before); + await waitForStableScroll(page); + } + + expect(biggestJump).toBeGreaterThan(300); + expect(errors).toEqual([]); + }); +}); From 427cda482e5c4b078f90d616d45f303d2012e9bf Mon Sep 17 00:00:00 2001 From: John McLear Date: Tue, 28 Jul 2026 20:16:25 +0100 Subject: [PATCH 2/3] test: build the pad with insertText instead of per-key typing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per-key events race Etherpad's input pipeline under Firefox + WITH_PLUGINS load and drop characters — the shared writeToPad() helper moved off keyboard.type for the same reason. Also cuts the spec's runtime by a third. Co-Authored-By: Claude Opus 5 (1M context) --- src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts b/src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts index 01f9148c2f2..f3495832530 100644 --- a/src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts +++ b/src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts @@ -76,8 +76,11 @@ const preparePad = async (page: Page, percentageBelowViewport: number) => { await body.click(); // Enough lines that the pad overflows the viewport several times over. - for (let i = 0; i < LINE_COUNT; i++) { - await page.keyboard.type(`line ${i}`); + // insertText rather than keyboard.type: per-key events race Etherpad's + // input pipeline under Firefox + WITH_PLUGINS load and drop characters. + const lines = Array.from({length: LINE_COUNT}, (_v, i) => `line ${i}`); + for (let i = 0; i < lines.length; i++) { + await page.keyboard.insertText(lines[i]); await page.keyboard.press('Enter'); } await waitForStableScroll(page); From 80119f86fee9f60069c1262f781c93dba3c6d488 Mon Sep 17 00:00:00 2001 From: John McLear Date: Tue, 28 Jul 2026 20:25:40 +0100 Subject: [PATCH 3/3] test: target the bottom-edge line by the predicate the editor uses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The click point was picked from line boxes ("last line that starts inside the viewport"), but scroll.ts decides from text rects of the *next* browser line, so on CI's rendering the caret landed a line short of the trigger and the scroll never happened (biggestJump 0). Mirror the real predicate — first visible line whose successor's first-character rect reaches past the viewport bottom — and report the measured geometry when the assertion fails. Co-Authored-By: Claude Opus 5 (1M context) --- .../specs/scroll_caret_viewport.spec.ts | 90 +++++++++++++------ 1 file changed, 63 insertions(+), 27 deletions(-) diff --git a/src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts b/src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts index f3495832530..b334c95d3cd 100644 --- a/src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts +++ b/src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts @@ -94,31 +94,63 @@ const preparePad = async (page: Page, percentageBelowViewport: number) => { return {errors}; }; -// Clicks the line sitting at the very bottom edge of the editor viewport — -// the case #8038 is about. Clicking through a locator would scroll the target -// into view first and move the very edge we are aiming at, so this clicks by -// coordinate instead. -const clickBottomEdgeOfViewport = async (page: Page) => { - const point = await page.evaluate(() => { +// Puts the caret on the line the editor considers to be at the bottom of the +// viewport — the case #8038 is about. "At the bottom" is not simply the last +// visible line: scroll.ts fires when the *next* browser line reaches past the +// viewport bottom, measured from text rects rather than line boxes, so this +// mirrors that predicate rather than guessing at a line. Clicking through a +// locator would scroll the target into view first and move the very edge we +// are aiming at, so it clicks by coordinate. +const clickCaretLineAtBottomOfViewport = async (page: Page) => { + const target = await page.evaluate(() => { const outerFrame = document.getElementsByName('ace_outer')[0] as HTMLIFrameElement; const outerWin = outerFrame.contentWindow!; const outerDoc = outerWin.document; const innerFrame = outerDoc.getElementsByName('ace_inner')[0] as HTMLIFrameElement; const innerDoc = innerFrame.contentWindow!.document; const frameRect = outerFrame.getBoundingClientRect(); - const visibleBottom = outerWin.pageYOffset + outerDoc.documentElement.clientHeight; + + // scroll.ts's _getViewPortTopBottom() + const editorTop = (document.getElementsByTagName('iframe')[0] as HTMLElement).offsetTop; + const padTop = parseInt(outerWin.getComputedStyle(outerFrame).paddingTop) || 0; + const viewportBottom = outerWin.pageYOffset + + outerDoc.documentElement.clientHeight - editorTop - padTop; + + // caretPosition.ts measures browser lines from the text rect of a line's + // first character, not from the line div's box. + const firstCharBottom = (line: HTMLElement) => { + let node: Node = line; + while (node.firstChild) node = node.firstChild; + const range = innerDoc.createRange(); + const length = node.nodeType === Node.TEXT_NODE ? (node as Text).length : 0; + range.setStart(node, 0); + range.setEnd(node, Math.min(1, length)); + return range.getBoundingClientRect().bottom; + }; + const lines = [...innerDoc.body.children].filter( (e) => e.tagName === 'DIV') as HTMLElement[]; - // the line crossing the bottom edge, i.e. the one that is only partly - // visible: its next line is the first one below the viewport - const straddling = lines.find((l) => - l.offsetTop < visibleBottom && l.offsetTop + l.offsetHeight > visibleBottom); - const target = straddling || lines[lines.length - 1]; - // a few pixels into the visible sliver of that line - const y = frameRect.top + (target.offsetTop - outerWin.pageYOffset) + 3; - return {x: Math.round(frameRect.left + 80), y: Math.round(y)}; + // the first line that is itself visible while the line after it reaches + // past the bottom edge — exactly when _isCaretAtTheBottomOfViewport() is + // true, so clicking here is what the feature reacts to + const index = lines.findIndex((line, i) => { + if (i + 1 >= lines.length) return false; + const top = line.offsetTop; + return top >= outerWin.pageYOffset && top < viewportBottom && + firstCharBottom(lines[i + 1]) > viewportBottom; + }); + const geometry = {index, viewportBottom, scrollY: outerWin.pageYOffset, + clientHeight: outerDoc.documentElement.clientHeight, lineCount: lines.length}; + if (index < 0) return {geometry, x: 0, y: 0, found: false}; + + const line = lines[index]; + const y = frameRect.top + (line.offsetTop - outerWin.pageYOffset) + + Math.min(6, line.offsetHeight / 2); + return {geometry, found: true, + x: Math.round(frameRect.left + 80), y: Math.round(y)}; }); - await page.mouse.click(point.x, point.y); + if (target.found) await page.mouse.click(target.x, target.y); + return target; }; test.describe('scroll when caret is in the last line of the viewport', () => { @@ -134,7 +166,7 @@ test.describe('scroll when caret is in the last line of the viewport', () => { test('moving the caret to the bottom of the viewport does not throw', async ({page}) => { const {errors} = await preparePad(page, 0); - await clickBottomEdgeOfViewport(page); + await clickCaretLineAtBottomOfViewport(page); await page.waitForTimeout(500); // arrow keys walk the caret across the edge as well for (let i = 0; i < 4; i++) { @@ -155,23 +187,27 @@ test.describe('scroll when caret is in the last line of the viewport', () => { 'trigger geometry is sub-pixel marginal in Firefox'); const {errors} = await preparePad(page, 0.6); - // Whether a given click lands exactly on the line the editor considers - // to be at the bottom edge comes down to sub-pixel line geometry, so - // try a few times and look for the one big jump: putting the caret - // line 60% of a viewport clear of the bottom edge. The browser never - // scrolls that far on its own — line by line it moves a line height at - // a time — and it only happens when the caret geometry is read from - // the editor document. + // Landing the caret on the bottom-edge line can take a couple of goes: + // each attempt re-measures, and the editor may have scrolled in + // between. The jump we are looking for is the caret line being put 60% + // of a viewport clear of the bottom edge; the browser never scrolls + // that far on its own — line by line it moves a line height at a time + // — and it only happens when the caret geometry is read from the + // editor document. let biggestJump = 0; + const attempts = []; for (let attempt = 0; attempt < 4 && biggestJump <= 300; attempt++) { const before = await getOuterScrollY(page); - await clickBottomEdgeOfViewport(page); + const target = await clickCaretLineAtBottomOfViewport(page); await page.waitForTimeout(750); - biggestJump = Math.max(biggestJump, await getOuterScrollY(page) - before); + const jump = await getOuterScrollY(page) - before; + attempts.push({...target.geometry, before, jump}); + biggestJump = Math.max(biggestJump, jump); await waitForStableScroll(page); } - expect(biggestJump).toBeGreaterThan(300); + expect(biggestJump, `attempts: ${JSON.stringify(attempts)}`) + .toBeGreaterThan(300); expect(errors).toEqual([]); }); });