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

@@ -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;

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( {

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"

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 );
+	} );
+} );