diff --git a/resources/skins.vector.es6/deferUntilFrame.js b/resources/skins.vector.es6/deferUntilFrame.js new file mode 100644 index 00000000..eece4382 --- /dev/null +++ b/resources/skins.vector.es6/deferUntilFrame.js @@ -0,0 +1,22 @@ +/** + * Helper method that calls a specified callback before the browser has + * performed a specified number of repaints. + * + * Uses `requestAnimationFrame` under the hood to determine the next repaint. + * + * @param {Function} callback + * @param {number} frameCount The number of frames to wait before calling the + * specified callback. + */ +function deferUntilFrame( callback, frameCount ) { + if ( frameCount === 0 ) { + callback(); + return; + } + + requestAnimationFrame( () => { + deferUntilFrame( callback, frameCount - 1 ); + } ); +} + +module.exports = deferUntilFrame; diff --git a/resources/skins.vector.es6/main.js b/resources/skins.vector.es6/main.js index 91c29278..7c8145af 100644 --- a/resources/skins.vector.es6/main.js +++ b/resources/skins.vector.es6/main.js @@ -6,6 +6,7 @@ const AB = require( './AB.js' ), initSectionObserver = require( './sectionObserver.js' ), initTableOfContents = require( './tableOfContents.js' ), + deferUntilFrame = require( './deferUntilFrame.js' ), TOC_ID = 'mw-panel-toc', BODY_CONTENT_ID = 'bodyContent', HEADLINE_SELECTOR = '.mw-headline', @@ -85,15 +86,26 @@ const main = () => { onSectionClick: () => { sectionObserver.pause(); - // Ensure the browser has finished painting and has had enough time to - // scroll to the section before resuming section observer. One rAF should - // be sufficient in most browsers, but Firefox 96.0.2 seems to require two - // rAFs. - requestAnimationFrame( () => { - requestAnimationFrame( () => { - sectionObserver.resume(); - } ); - } ); + // T297614: We want the link that the user has clicked inside the TOC to + // be "active" (e.g. bolded) regardless of whether the browser's scroll + // position corresponds to that section. Therefore, we need to temporarily + // ignore section observer until the browser has finished scrolling to the + // section (if needed). + // + // However, because the scroll event happens asyncronously after the user + // clicks on a link and may not even happen at all (e.g. the user has + // scrolled all the way to the bottom and clicks a section that is already + // in the viewport), determining when we should resume section observer is + // a bit tricky. + // + // Because a scroll event may not even be triggered after clicking the + // link, we instead allow the browser to perform a maximum number of + // repaints before resuming sectionObserver. Per T297614#7687656, Firefox + // 97.0 wasn't consistently activating the table of contents section that + // the user clicked even after waiting 2 frames. After further + // investigation, it sometimes waits up to 3 frames before painting the + // new scroll position so we have that as the limit. + deferUntilFrame( () => sectionObserver.resume(), 3 ); } } ); sectionObserver = initSectionObserver( { diff --git a/skin.json b/skin.json index 1eeb74d4..2361d68b 100644 --- a/skin.json +++ b/skin.json @@ -269,6 +269,7 @@ "resources/skins.vector.es6/AB.js", "resources/skins.vector.es6/tableOfContents.js", "resources/skins.vector.es6/sectionObserver.js", + "resources/skins.vector.es6/deferUntilFrame.js", { "name": "resources/skins.vector.es6/config.json", "callback": "Vector\\Hooks::getVectorResourceLoaderConfig" diff --git a/tests/jest/deferUntilFrame.test.js b/tests/jest/deferUntilFrame.test.js new file mode 100644 index 00000000..86f162b2 --- /dev/null +++ b/tests/jest/deferUntilFrame.test.js @@ -0,0 +1,33 @@ +const deferUntilFrame = require( '../../resources/skins.vector.es6/deferUntilFrame.js' ); + +describe( 'deferUntilFrame.js', () => { + let /** @type {jest.SpyInstance} */ spy; + + beforeEach( () => { + spy = jest.spyOn( window, 'requestAnimationFrame' ).mockImplementation( ( cb ) => { + setTimeout( () => { + cb( 1 ); + } ); + + return 1; + } ); + } ); + + afterEach( () => { + spy.mockRestore(); + } ); + + it( 'does not fire rAF if `0` is passed', done => { + deferUntilFrame( () => { + expect( spy ).toHaveBeenCalledTimes( 0 ); + done(); + }, 0 ); + } ); + + it( 'fires rAF the specified number of times', done => { + deferUntilFrame( () => { + expect( spy ).toHaveBeenCalledTimes( 3 ); + done(); + }, 3 ); + } ); +} );