From fc725d179172909d911775f7c2fe5e4715b12310 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 18 Apr 2026 12:46:59 +0100 Subject: [PATCH 1/4] fix: delay anchor line scrolling until layout settles Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/static/js/pad_editor.ts | 111 +++++++++++------- .../frontend-new/specs/anchor_scroll.spec.ts | 50 ++++++++ src/tests/frontend/specs/scrollTo.js | 14 +++ 3 files changed, 133 insertions(+), 42 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 7feb81e30ea..82e70bd07fb 100644 --- a/src/static/js/pad_editor.ts +++ b/src/static/js/pad_editor.ts @@ -210,49 +210,76 @@ 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 + $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 getCurrentTargetOffset = () => { + const $aceOuter = $('iframe[name="ace_outer"]'); + 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; + }; + + const maxSettleDuration = 10000; + const settleInterval = 250; + const startTime = Date.now(); + let intervalId = null; + + const focusUntilStable = () => { + if (Date.now() - startTime >= maxSettleDuration) { + window.clearInterval(intervalId); + return; } - } + const currentOffsetTop = getCurrentTargetOffset(); + if (currentOffsetTop == null) return; + + focusOnHashedLine(ace, lineNumberInt); + }; + + focusUntilStable(); + intervalId = window.setInterval(focusUntilStable, settleInterval); // 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 00000000000..c88d184792e --- /dev/null +++ b/src/tests/frontend-new/specs/anchor_scroll.spec.ts @@ -0,0 +1,50 @@ +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); + }); +}); diff --git a/src/tests/frontend/specs/scrollTo.js b/src/tests/frontend/specs/scrollTo.js index e62582c0b4f..e19d9799220 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 () { From 157cc5feaafb0b9e1f844010a419e24d3028c2e9 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 18 Apr 2026 16:57:20 +0100 Subject: [PATCH 2/4] 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) --- src/static/js/pad_editor.ts | 32 ++++++++++++++--- .../frontend-new/specs/anchor_scroll.spec.ts | 34 +++++++++++++++++++ 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/src/static/js/pad_editor.ts b/src/static/js/pad_editor.ts index 82e70bd07fb..9089b52be34 100644 --- a/src/static/js/pad_editor.ts +++ b/src/static/js/pad_editor.ts @@ -255,8 +255,8 @@ const focusOnHashedLine = (ace, lineNumberInt) => { exports.focusOnLine = (ace) => { const lineNumberInt = getHashedLineNumber(); if (lineNumberInt == null) return; + const $aceOuter = $('iframe[name="ace_outer"]'); const getCurrentTargetOffset = () => { - const $aceOuter = $('iframe[name="ace_outer"]'); const $inner = $aceOuter.contents().find('iframe').contents().find('#innerdocbody'); const line = $inner.find(`div:nth-child(${lineNumberInt})`); if (line.length === 0) return null; @@ -266,20 +266,44 @@ exports.focusOnLine = (ace) => { const maxSettleDuration = 10000; const settleInterval = 250; const startTime = Date.now(); - let intervalId = null; + let intervalId: number | null = null; + + 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) { - window.clearInterval(intervalId); + stop(); return; } const currentOffsetTop = getCurrentTargetOffset(); if (currentOffsetTop == null) return; - focusOnHashedLine(ace, lineNumberInt); }; 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 index c88d184792e..201c04411a1 100644 --- a/src/tests/frontend-new/specs/anchor_scroll.spec.ts +++ b/src/tests/frontend-new/specs/anchor_scroll.spec.ts @@ -47,4 +47,38 @@ test.describe('anchor scrolling', () => { return Math.abs(currentViewportTop - initialViewportTop); }).toBeLessThanOrEqual(80); }); + + 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); + }); }); From 0060805f8cd135111d5d75972ac65da42810f8f7 Mon Sep 17 00:00:00 2001 From: John McLear Date: Wed, 29 Apr 2026 12:46:16 +0100 Subject: [PATCH 3/4] fix: anchor reapply loop exits early once layout settles + FF rationale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/static/js/pad_editor.ts | 19 ++++++++++++ .../frontend-new/specs/anchor_scroll.spec.ts | 29 +++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/src/static/js/pad_editor.ts b/src/static/js/pad_editor.ts index 9089b52be34..7b6a2b9a805 100644 --- a/src/static/js/pad_editor.ts +++ b/src/static/js/pad_editor.ts @@ -230,6 +230,10 @@ const focusOnHashedLine = (ace, lineNumberInt) => { 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) => { @@ -263,10 +267,18 @@ exports.focusOnLine = (ace) => { 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 + // either: (a) maxSettleDuration elapses, (b) the user interacts (see stop() below), + // or (c) the target offset has not changed for stableTicksRequired consecutive ticks + // (layout has settled — no need to keep re-scrolling). const maxSettleDuration = 10000; const settleInterval = 250; + const stableTicksRequired = 3; const startTime = Date.now(); let intervalId: number | null = null; + let lastOffset: number | null = null; + let stableTicks = 0; const userEventNames = ['wheel', 'touchmove', 'keydown', 'mousedown']; const docs: Document[] = []; @@ -289,6 +301,13 @@ exports.focusOnLine = (ace) => { const currentOffsetTop = getCurrentTargetOffset(); if (currentOffsetTop == null) return; focusOnHashedLine(ace, lineNumberInt); + if (lastOffset != null && currentOffsetTop === lastOffset) { + stableTicks += 1; + if (stableTicks >= stableTicksRequired) stop(); + } else { + stableTicks = 0; + } + lastOffset = currentOffsetTop; }; focusUntilStable(); diff --git a/src/tests/frontend-new/specs/anchor_scroll.spec.ts b/src/tests/frontend-new/specs/anchor_scroll.spec.ts index 201c04411a1..3d21951b714 100644 --- a/src/tests/frontend-new/specs/anchor_scroll.spec.ts +++ b/src/tests/frontend-new/specs/anchor_scroll.spec.ts @@ -48,6 +48,35 @@ test.describe('anchor scrolling', () => { }).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 long enough for the stable-tick early-exit (3 ticks * 250ms + 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(2000); + + 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(); From b1d21561108b028bdbad5c064ea86bcbeee8acb3 Mon Sep 17 00:00:00 2001 From: John McLear Date: Thu, 30 Apr 2026 05:33:02 +0100 Subject: [PATCH 4/4] fix(anchor-scroll): tolerance, min-settle window, missing-anchor bail-out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/static/js/pad_editor.ts | 33 +++++++++++++++---- .../frontend-new/specs/anchor_scroll.spec.ts | 8 ++--- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/static/js/pad_editor.ts b/src/static/js/pad_editor.ts index 7b6a2b9a805..e8ff9bc67f0 100644 --- a/src/static/js/pad_editor.ts +++ b/src/static/js/pad_editor.ts @@ -269,16 +269,27 @@ exports.focusOnLine = (ace) => { // Settle window: keep correcting the scroll position while late content (images, // plugin-rendered blocks) shifts the target line's offsetTop. The interval ends when - // either: (a) maxSettleDuration elapses, (b) the user interacts (see stop() below), - // or (c) the target offset has not changed for stableTicksRequired consecutive ticks - // (layout has settled — no need to keep re-scrolling). + // 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 = 3; + 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[] = []; @@ -299,11 +310,19 @@ exports.focusOnLine = (ace) => { return; } const currentOffsetTop = getCurrentTargetOffset(); - if (currentOffsetTop == null) return; + if (currentOffsetTop == null) { + missingTicks += 1; + if (missingTicks >= missingTicksRequired) stop(); + return; + } + missingTicks = 0; focusOnHashedLine(ace, lineNumberInt); - if (lastOffset != null && currentOffsetTop === lastOffset) { + if (lastOffset != null && Math.abs(currentOffsetTop - lastOffset) < offsetEpsilon) { stableTicks += 1; - if (stableTicks >= stableTicksRequired) stop(); + if (stableTicks >= stableTicksRequired + && Date.now() - startTime >= minSettleDuration) { + stop(); + } } else { stableTicks = 0; } diff --git a/src/tests/frontend-new/specs/anchor_scroll.spec.ts b/src/tests/frontend-new/specs/anchor_scroll.spec.ts index 3d21951b714..a05b1da2164 100644 --- a/src/tests/frontend-new/specs/anchor_scroll.spec.ts +++ b/src/tests/frontend-new/specs/anchor_scroll.spec.ts @@ -65,10 +65,10 @@ test.describe('anchor scrolling', () => { (el) => el.parentElement?.scrollTop || 0); await expect.poll(getScrollTop).toBeGreaterThan(10); - // Wait long enough for the stable-tick early-exit (3 ticks * 250ms + 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(2000); + // 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;