From fc725d179172909d911775f7c2fe5e4715b12310 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 18 Apr 2026 12:46:59 +0100 Subject: [PATCH 1/2] 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/2] 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); + }); });