From 80a111d0e41e3704c5312252ee0a7c8452c991e2 Mon Sep 17 00:00:00 2001 From: Nicholas Ray Date: Tue, 8 Feb 2022 14:14:33 -0700 Subject: [PATCH] Fix TOC section activation on link click bug 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 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. Bug: T297614 Change-Id: If3632529f58c15348a7200258f4f5999ea0dadc4 --- resources/skins.vector.es6/deferUntilFrame.js | 22 +++++++++++++ resources/skins.vector.es6/main.js | 30 ++++++++++++----- skin.json | 1 + tests/jest/deferUntilFrame.test.js | 33 +++++++++++++++++++ 4 files changed, 77 insertions(+), 9 deletions(-) create mode 100644 resources/skins.vector.es6/deferUntilFrame.js create mode 100644 tests/jest/deferUntilFrame.test.js 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 ); + } ); +} );