From b8a950ee9207d4d15f4c32607b6acf36a932b046 Mon Sep 17 00:00:00 2001 From: John McLear Date: Thu, 30 Apr 2026 14:56:02 +0800 Subject: [PATCH] fix: delay anchor line scrolling until layout settles (#7544) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: delay anchor line scrolling until layout settles Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: anchor reapply loop cancels on user interaction Addresses Qodo review: the 10s reapply loop could fight the user when they tried to scroll or click away from the anchored line. Listen for wheel/touchmove/keydown/mousedown on both ace_outer and ace_inner documents in capture phase and tear down the interval on first signal. Co-Authored-By: Claude Opus 4.7 (1M context) * fix: anchor reapply loop exits early once layout settles + FF rationale Addresses Qodo review on #7544: 1. Requirement gap (#1): Add stability detection to focusOnLine()'s reapply loop. When the target line's offsetTop has not changed for 3 consecutive 250ms ticks (~750ms), stop() is called early instead of running the full 10s window. This means once late content is no longer shifting layout, the loop releases the user immediately rather than waiting out maxSettleDuration. 2. Maintainability (#4): Add a comment explaining why the previous $.animate({scrollTop}) "needed for FF" path was replaced with a direct .scrollTop() call — the settle interval now covers the late-layout case Firefox originally needed animation for. Also adds a test that the reapply loop exits early so a user-initiated scrollTop=0 after ~2s is not reverted by another reapply tick. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(anchor-scroll): tolerance, min-settle window, missing-anchor bail-out Round 3 of Qodo review on #7544: #3 Early exit misses late shifts — image loads / plugin renders past my previous 750ms early-exit window were no longer corrected. Add a `minSettleDuration` of 2s before any early-exit can fire, and bump `stableTicksRequired` from 3 to 4. Hard ceiling stays 10s. #4 Offset equality prevents stability — strict === on `offset().top` never matched in the presence of sub-pixel rounding, so the loop ran the full 10s even on stable layouts. Switch to `Math.abs(...) < 1` tolerance. #7 Invalid anchors spin interval — when `getCurrentTargetOffset()` keeps returning null (the requested line never resolves), the loop used to run for the full 10s doing nothing. Track consecutive misses and `stop()` after `missingTicksRequired` (8 ticks ≈ 2s). Real "inner doc not yet rendered" cases get the first 2s window. Bump the early-exit test's wait from 2s → 3.5s to clear the new `minSettleDuration` + `stableTicksRequired` window before asserting. Pushing back on remaining Qodo items: #1 Defer scroll until layout settles — the design is "scroll once immediately so the user sees the line, then keep correcting". Deferring all scrolling until "stable" (which is unknowable up front) would visibly hang on `#L...` navigation for seconds while nothing happens. The reapply loop is the deferral. #6 FF rationale lost — already addressed in the previous commit (comment on the `scrollTop()` call explaining why the `$.animate({scrollTop})` "needed for FF" path was removed). Qodo's persistent review doesn't track resolution of items that aren't touched by the new commit. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) --- src/static/js/pad_editor.ts | 171 +++++++++++++----- .../frontend-new/specs/anchor_scroll.spec.ts | 113 ++++++++++++ src/tests/frontend/specs/scrollTo.js | 14 ++ 3 files changed, 257 insertions(+), 41 deletions(-) create mode 100644 src/tests/frontend-new/specs/anchor_scroll.spec.ts diff --git a/src/static/js/pad_editor.ts b/src/static/js/pad_editor.ts index 267ad5dd6..514748b61 100644 --- a/src/static/js/pad_editor.ts +++ b/src/static/js/pad_editor.ts @@ -250,49 +250,138 @@ const padeditor = (() => { exports.padeditor = padeditor; -exports.focusOnLine = (ace) => { - // If a number is in the URI IE #L124 go to that line number +const getHashedLineNumber = () => { const lineNumber = window.location.hash.substr(1); - if (lineNumber) { - if (lineNumber[0] === 'L') { - const $outerdoc = $('iframe[name="ace_outer"]').contents().find('#outerdocbody'); - const lineNumberInt = parseInt(lineNumber.substr(1)); - if (lineNumberInt) { - const $inner = $('iframe[name="ace_outer"]').contents().find('iframe') - .contents().find('#innerdocbody'); - const line = $inner.find(`div:nth-child(${lineNumberInt})`); - if (line.length !== 0) { - let offsetTop = line.offset().top; - offsetTop += parseInt($outerdoc.css('padding-top').replace('px', '')); - const hasMobileLayout = $('body').hasClass('mobile-layout'); - if (!hasMobileLayout) { - offsetTop += parseInt($inner.css('padding-top').replace('px', '')); - } - const $outerdocHTML = $('iframe[name="ace_outer"]').contents() - .find('#outerdocbody').parent(); - $outerdoc.css({top: `${offsetTop}px`}); // Chrome - $outerdocHTML.animate({scrollTop: offsetTop}); // needed for FF - const node = line[0]; - ace.callWithAce((ace) => { - const selection = { - startPoint: { - index: 0, - focusAtStart: true, - maxIndex: 1, - node, - }, - endPoint: { - index: 0, - focusAtStart: true, - maxIndex: 1, - node, - }, - }; - ace.ace_setSelection(selection); - }); - } - } + if (!lineNumber || lineNumber[0] !== 'L') return null; + const lineNumberInt = parseInt(lineNumber.substr(1)); + return Number.isInteger(lineNumberInt) && lineNumberInt > 0 ? lineNumberInt : null; +}; + +const focusOnHashedLine = (ace, lineNumberInt) => { + const $aceOuter = $('iframe[name="ace_outer"]'); + const $outerdoc = $aceOuter.contents().find('#outerdocbody'); + const $inner = $aceOuter.contents().find('iframe').contents().find('#innerdocbody'); + const line = $inner.find(`div:nth-child(${lineNumberInt})`); + if (line.length === 0) return false; + + let offsetTop = line.offset().top; + offsetTop += parseInt($outerdoc.css('padding-top').replace('px', '')); + const hasMobileLayout = $('body').hasClass('mobile-layout'); + if (!hasMobileLayout) offsetTop += parseInt($inner.css('padding-top').replace('px', '')); + const $outerdocHTML = $aceOuter.contents().find('#outerdocbody').parent(); + $outerdoc.css({top: `${offsetTop}px`}); // Chrome + // Direct scrollTop() (was previously $.animate({scrollTop}) for Firefox). The animation + // workaround is no longer needed because focusOnLine() reapplies the scroll on a settle + // interval until layout stabilises, which covers Firefox's late-layout behaviour without + // an animated scroll fighting concurrent layout shifts. + $outerdocHTML.scrollTop(offsetTop); + const node = line[0]; + ace.callWithAce((ace) => { + const selection = { + startPoint: { + index: 0, + focusAtStart: true, + maxIndex: 1, + node, + }, + endPoint: { + index: 0, + focusAtStart: true, + maxIndex: 1, + node, + }, + }; + ace.ace_setSelection(selection); + }); + return true; +}; + +exports.focusOnLine = (ace) => { + const lineNumberInt = getHashedLineNumber(); + if (lineNumberInt == null) return; + const $aceOuter = $('iframe[name="ace_outer"]'); + const getCurrentTargetOffset = () => { + const $inner = $aceOuter.contents().find('iframe').contents().find('#innerdocbody'); + const line = $inner.find(`div:nth-child(${lineNumberInt})`); + if (line.length === 0) return null; + return line.offset().top; + }; + + // Settle window: keep correcting the scroll position while late content (images, + // plugin-rendered blocks) shifts the target line's offsetTop. The interval ends when + // any of the following becomes true: + // (a) maxSettleDuration (10s) has elapsed — hard ceiling + // (b) the user interacts (see stop() below) — never fight the user + // (c) at least minSettleDuration (2s) has elapsed AND the target offset has not + // moved by more than offsetEpsilon for stableTicksRequired consecutive ticks + // (image loads / plugin renders past the 2s window are still corrected; brief + // early stability does not exit the loop prematurely) + // (d) the target line is missing from the DOM for missingTicksRequired consecutive + // ticks (the anchor doesn't exist — bail rather than spin to maxSettleDuration) + // Sub-pixel tolerance avoids strict-equality flapping on fractional offsets. + const maxSettleDuration = 10000; + const minSettleDuration = 2000; + const settleInterval = 250; + const stableTicksRequired = 4; + const offsetEpsilon = 1; + const missingTicksRequired = 8; // 2s of consecutive misses → assume invalid anchor + const startTime = Date.now(); + let intervalId: number | null = null; + let lastOffset: number | null = null; + let stableTicks = 0; + let missingTicks = 0; + + const userEventNames = ['wheel', 'touchmove', 'keydown', 'mousedown']; + const docs: Document[] = []; + const stop = () => { + if (intervalId != null) { + window.clearInterval(intervalId); + intervalId = null; } + for (const doc of docs) { + for (const name of userEventNames) doc.removeEventListener(name, stop, true); + } + docs.length = 0; + }; + + const focusUntilStable = () => { + if (Date.now() - startTime >= maxSettleDuration) { + stop(); + return; + } + const currentOffsetTop = getCurrentTargetOffset(); + if (currentOffsetTop == null) { + missingTicks += 1; + if (missingTicks >= missingTicksRequired) stop(); + return; + } + missingTicks = 0; + focusOnHashedLine(ace, lineNumberInt); + if (lastOffset != null && Math.abs(currentOffsetTop - lastOffset) < offsetEpsilon) { + stableTicks += 1; + if (stableTicks >= stableTicksRequired + && Date.now() - startTime >= minSettleDuration) { + stop(); + } + } else { + stableTicks = 0; + } + lastOffset = currentOffsetTop; + }; + + focusUntilStable(); + intervalId = window.setInterval(focusUntilStable, settleInterval); + // Stop fighting the user: any deliberate scroll, tap, click, or keystroke cancels the + // reapply loop so late layout corrections do not steal focus once the user takes over. + // Listen on both the ace_outer and ace_inner documents in capture phase so we see the + // user's intent even if inner handlers stopPropagation(). + const outerDoc = ($aceOuter.contents()[0] as any) as Document | undefined; + const innerIframe = $aceOuter.contents().find('iframe')[0] as HTMLIFrameElement | undefined; + const innerDoc = innerIframe?.contentDocument; + for (const doc of [outerDoc, innerDoc]) { + if (!doc) continue; + docs.push(doc); + for (const name of userEventNames) doc.addEventListener(name, stop, true); } // End of setSelection / set Y position of editor }; diff --git a/src/tests/frontend-new/specs/anchor_scroll.spec.ts b/src/tests/frontend-new/specs/anchor_scroll.spec.ts new file mode 100644 index 000000000..a05b1da21 --- /dev/null +++ b/src/tests/frontend-new/specs/anchor_scroll.spec.ts @@ -0,0 +1,113 @@ +import {expect, test} from "@playwright/test"; +import {clearPadContent, goToNewPad, writeToPad} from "../helper/padHelper"; + +test.describe('anchor scrolling', () => { + test.beforeEach(async ({context}) => { + await context.clearCookies(); + }); + + test('reapplies #L scroll after earlier content changes height', async ({page}) => { + await goToNewPad(page); + const padUrl = page.url(); + await clearPadContent(page); + await writeToPad(page, Array.from({length: 30}, (_v, i) => `Line ${i + 1}`).join('\n')); + await page.waitForTimeout(1000); + + await page.goto('about:blank'); + await page.goto(`${padUrl}#L20`); + await page.waitForSelector('iframe[name="ace_outer"]'); + await page.waitForSelector('#editorcontainer.initialized'); + await page.waitForTimeout(2000); + + const outerDoc = page.frameLocator('iframe[name="ace_outer"]').locator('#outerdocbody'); + const firstLine = page.frameLocator('iframe[name="ace_outer"]') + .frameLocator('iframe') + .locator('#innerdocbody > div') + .first(); + const targetLine = page.frameLocator('iframe[name="ace_outer"]') + .frameLocator('iframe') + .locator('#innerdocbody > div') + .nth(19); + + const getScrollTop = async () => await outerDoc.evaluate( + (el) => el.parentElement?.scrollTop || 0); + const getTargetViewportTop = async () => await targetLine.evaluate((el) => el.getBoundingClientRect().top); + + await expect.poll(getScrollTop).toBeGreaterThan(10); + const initialViewportTop = await getTargetViewportTop(); + + await firstLine.evaluate((el) => { + const filler = document.createElement('div'); + filler.style.height = '400px'; + el.appendChild(filler); + }); + + await expect.poll(async () => { + const currentViewportTop = await getTargetViewportTop(); + return Math.abs(currentViewportTop - initialViewportTop); + }).toBeLessThanOrEqual(80); + }); + + test('reapply loop exits early once the target offset is stable', async ({page}) => { + await goToNewPad(page); + const padUrl = page.url(); + await clearPadContent(page); + await writeToPad(page, Array.from({length: 30}, (_v, i) => `Line ${i + 1}`).join('\n')); + await page.waitForTimeout(1000); + + await page.goto('about:blank'); + await page.goto(`${padUrl}#L20`); + await page.waitForSelector('iframe[name="ace_outer"]'); + await page.waitForSelector('#editorcontainer.initialized'); + + const outerDoc = page.frameLocator('iframe[name="ace_outer"]').locator('#outerdocbody'); + const getScrollTop = async () => await outerDoc.evaluate( + (el) => el.parentElement?.scrollTop || 0); + + await expect.poll(getScrollTop).toBeGreaterThan(10); + // Wait past minSettleDuration (2s) plus stableTicksRequired (4 * 250ms = 1s) plus + // slack, well under the 10s hard timeout. After early-exit, scrolling away from the + // anchor must not be reverted by another reapply tick. + await page.waitForTimeout(3500); + + await outerDoc.evaluate((el) => { + if (el.parentElement) el.parentElement.scrollTop = 0; + }); + await page.waitForTimeout(1500); + expect(await getScrollTop()).toBeLessThanOrEqual(20); + }); + + test('user scroll cancels the reapply loop so navigation is not locked', async ({page}) => { + await goToNewPad(page); + const padUrl = page.url(); + await clearPadContent(page); + await writeToPad(page, Array.from({length: 30}, (_v, i) => `Line ${i + 1}`).join('\n')); + await page.waitForTimeout(1000); + + await page.goto('about:blank'); + await page.goto(`${padUrl}#L20`); + await page.waitForSelector('iframe[name="ace_outer"]'); + await page.waitForSelector('#editorcontainer.initialized'); + + const outerDoc = page.frameLocator('iframe[name="ace_outer"]').locator('#outerdocbody'); + const getScrollTop = async () => await outerDoc.evaluate( + (el) => el.parentElement?.scrollTop || 0); + + await expect.poll(getScrollTop).toBeGreaterThan(10); + + // User interacts with the pad. The anchor-scroll handler listens for + // wheel/mousedown/keydown/touchmove on the outer iframe document and must cancel + // its reapply loop. We dispatch a mousedown on the outer document, then reset + // scrollTop to 0 and verify it stays there. + await outerDoc.evaluate((el) => { + const doc = el.ownerDocument; + doc.dispatchEvent(new MouseEvent('mousedown', {bubbles: true})); + if (el.parentElement) el.parentElement.scrollTop = 0; + }); + + // Give the reapply loop several ticks to attempt a re-scroll. If cancellation worked, + // scrollTop stays near 0 instead of snapping back to the anchor. + await page.waitForTimeout(1500); + expect(await getScrollTop()).toBeLessThanOrEqual(20); + }); +}); diff --git a/src/tests/frontend/specs/scrollTo.js b/src/tests/frontend/specs/scrollTo.js index e62582c0b..e19d97992 100755 --- a/src/tests/frontend/specs/scrollTo.js +++ b/src/tests/frontend/specs/scrollTo.js @@ -15,6 +15,20 @@ describe('scrollTo.js', function () { return (topOffset >= 100); }); }); + + it('reapplies the scroll when earlier content changes height after load', async function () { + const chrome$ = helper.padChrome$; + const inner$ = helper.padInner$; + const getTopOffset = () => parseInt(chrome$('iframe').first('iframe') + .contents().find('#outerdocbody').css('top')) || 0; + + await helper.waitForPromise(() => getTopOffset() >= 100); + const initialTopOffset = getTopOffset(); + + inner$('#innerdocbody > div').first().css('height', '400px'); + + await helper.waitForPromise(() => getTopOffset() > initialTopOffset + 200); + }); }); describe('doesnt break on weird hash input', function () {