-
-
Notifications
You must be signed in to change notification settings - Fork 3k
fix: delay anchor line scrolling until layout settles #7544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -210,49 +210,100 @@ 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)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just fyi. Substr should not be used as it is deprecated. You can use substring which should work in your case identically. You only need to pay attention because the second argument does not anymore mean length but end position. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substr |
||
| 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this only for chrome? Is it still relevant? |
||
| $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; | ||
| }; | ||
|
|
||
| const maxSettleDuration = 10000; | ||
| const settleInterval = 250; | ||
| const startTime = Date.now(); | ||
| 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) { | ||
| stop(); | ||
| return; | ||
| } | ||
| const currentOffsetTop = getCurrentTargetOffset(); | ||
| if (currentOffsetTop == null) return; | ||
| focusOnHashedLine(ace, lineNumberInt); | ||
| }; | ||
|
|
||
| focusUntilStable(); | ||
| intervalId = window.setInterval(focusUntilStable, settleInterval); | ||
|
Comment on lines
+284
to
+295
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. Immediate focusuntilstable() scroll The new #L... behavior scrolls immediately on load via focusUntilStable() instead of deferring until the pad layout has fully settled. This can still cause an initial incorrect viewport position before late-loading content (for example images) finishes affecting layout. Agent Prompt
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
|
||
| // 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 | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| 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('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); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must say this code is quite a mess. I like the new style with better function naming 😊.