From 97c2c66cc167e9fd34bffe8882b91b99aac2102e Mon Sep 17 00:00:00 2001
From: Timo Tijhof <ttijhof@wikimedia.org>
Date: Fri, 29 Mar 2013 21:09:06 +0100
Subject: [PATCH] mw.loader: Fix regression that caused CSS load after scripts.

Follows-up 705d50c which introduced cssText buffer (yielding
one tick of the event queue to reduce the number of forced
repaints).

However since that made CSS loading asynchronoous (be it only for
a split second) it caused some nasty side-effects.

This was also reflected in the unit tests that had to resort
to doing setTimeout from within the mw.loader implemented script
to assert the styles. This is now sane again: Scripts execute
after styles are inserted.

Bug: 46401
Change-Id: Ib54a3e78710b7f798c6730d3f540d3284001e2de
---
 resources/mediawiki/mediawiki.js              | 143 ++++++++++++------
 .../resources/mediawiki/mediawiki.test.js     |  42 +++--
 2 files changed, 113 insertions(+), 72 deletions(-)

diff --git a/resources/mediawiki/mediawiki.js b/resources/mediawiki/mediawiki.js
index 0da9d875d8..6c7e697889 100644
--- a/resources/mediawiki/mediawiki.js
+++ b/resources/mediawiki/mediawiki.js
@@ -362,7 +362,7 @@ var mw = ( function ( $, undefined ) {
 			 *             'dependencies': ['required.foo', 'bar.also', ...], (or) function () {}
 			 *             'group': 'somegroup', (or) null,
 			 *             'source': 'local', 'someforeignwiki', (or) null
-			 *             'state': 'registered', 'loading', 'loaded', 'ready', 'error' or 'missing'
+			 *             'state': 'registered', 'loaded', 'loading', 'ready', 'error' or 'missing'
 			 *             'script': ...,
 			 *             'style': ...,
 			 *             'messages': { 'key': 'value' },
@@ -392,7 +392,9 @@ var mw = ( function ( $, undefined ) {
 				// Selector cache for the marker element. Use getMarker() to get/use the marker!
 				$marker = null,
 				// Buffer for addEmbeddedCSS.
-				cssBuffer = '';
+				cssBuffer = '',
+				// Callbacks for addEmbeddedCSS.
+				cssCallbacks = $.Callbacks();
 
 			/* Private methods */
 
@@ -469,10 +471,15 @@ var mw = ( function ( $, undefined ) {
 			/**
 			 * @param {string} [cssText=cssBuffer] If called without cssText,
 			 * the internal buffer will be inserted instead.
+			 * @param {Function} [callback]
 			 */
-			function addEmbeddedCSS( cssText ) {
+			function addEmbeddedCSS( cssText, callback ) {
 				var $style, styleEl;
 
+				if ( callback ) {
+					cssCallbacks.add( callback );
+				}
+
 				// Yield once before inserting the <style> tag. There are likely
 				// more calls coming up which we can combine this way.
 				// Appending a stylesheet and waiting for the browser to repaint
@@ -530,11 +537,14 @@ var mw = ( function ( $, undefined ) {
 						} else {
 							styleEl.appendChild( document.createTextNode( String( cssText ) ) );
 						}
+						cssCallbacks.fire().empty();
 						return;
 					}
 				}
 
 				$( addStyleTag( cssText, getMarker() ) ).data( 'ResourceLoaderDynamicStyleTag', true );
+
+				cssCallbacks.fire().empty();
 			}
 
 			/**
@@ -879,7 +889,8 @@ var mw = ( function ( $, undefined ) {
 			 * @param {string} module Module name to execute
 			 */
 			function execute( module ) {
-				var key, value, media, i, urls, script, markModuleReady, nestedAddScript;
+				var key, value, media, i, urls, cssHandle, checkCssHandles,
+					cssHandlesRegistered = false;
 
 				if ( registry[module] === undefined ) {
 					throw new Error( 'Module has not been registered yet: ' + module );
@@ -888,7 +899,7 @@ var mw = ( function ( $, undefined ) {
 				} else if ( registry[module].state === 'loading' ) {
 					throw new Error( 'Module has not completed loading yet: ' + module );
 				} else if ( registry[module].state === 'ready' ) {
-					throw new Error( 'Module has already been loaded: ' + module );
+					throw new Error( 'Module has already been executed: ' + module );
 				}
 
 				/**
@@ -906,6 +917,80 @@ var mw = ( function ( $, undefined ) {
 					el.href = url;
 				}
 
+				function runScript() {
+					var script, markModuleReady, nestedAddScript;
+					try {
+						script = registry[module].script;
+						markModuleReady = function () {
+							registry[module].state = 'ready';
+							handlePending( module );
+						};
+						nestedAddScript = function ( arr, callback, async, i ) {
+							// Recursively call addScript() in its own callback
+							// for each element of arr.
+							if ( i >= arr.length ) {
+								// We're at the end of the array
+								callback();
+								return;
+							}
+
+							addScript( arr[i], function () {
+								nestedAddScript( arr, callback, async, i + 1 );
+							}, async );
+						};
+
+						if ( $.isArray( script ) ) {
+							nestedAddScript( script, markModuleReady, registry[module].async, 0 );
+						} else if ( $.isFunction( script ) ) {
+							registry[module].state = 'ready';
+							script( $ );
+							handlePending( module );
+						}
+					} catch ( e ) {
+						// This needs to NOT use mw.log because these errors are common in production mode
+						// and not in debug mode, such as when a symbol that should be global isn't exported
+						log( 'Exception thrown by ' + module + ': ' + e.message, e );
+						registry[module].state = 'error';
+						handlePending( module );
+					}
+				}
+
+				// This used to be inside runScript, but since that is now fired asychronously
+				// (after CSS is loaded) we need to set it here right away. It is crucial that
+				// when execute() is called this is set synchronously, otherwise modules will get
+				// executed multiple times as the registry will state that it isn't loading yet.
+				registry[module].state = 'loading';
+
+				// Add localizations to message system
+				if ( $.isPlainObject( registry[module].messages ) ) {
+					mw.messages.set( registry[module].messages );
+				}
+
+				// Make sure we don't run the scripts until all (potentially asynchronous)
+				// stylesheet insertions have completed.
+				( function () {
+					var pending = 0;
+					checkCssHandles = function () {
+						// cssHandlesRegistered ensures we don't take off too soon, e.g. when
+						// one of the cssHandles is fired while we're still creating more handles.
+						if ( cssHandlesRegistered && pending === 0 && runScript ) {
+							runScript();
+							runScript = undefined; // Revoke
+						}
+					};
+					cssHandle = function () {
+						var check = checkCssHandles;
+						pending++;
+						return function () {
+							if (check) {
+								pending--;
+								check();
+								check = undefined; // Revoke
+							}
+						};
+					};
+				}() );
+
 				// Process styles (see also mw.loader.implement)
 				// * back-compat: { <media>: css }
 				// * back-compat: { <media>: [url, ..] }
@@ -924,7 +1009,7 @@ var mw = ( function ( $, undefined ) {
 								// Strings are pre-wrapped in "@media". The media-type was just ""
 								// (because it had to be set to something).
 								// This is one of the reasons why this format is no longer used.
-								addEmbeddedCSS( value );
+								addEmbeddedCSS( value, cssHandle() );
 							} else {
 								// back-compat: { <media>: [url, ..] }
 								media = key;
@@ -941,7 +1026,7 @@ var mw = ( function ( $, undefined ) {
 									addLink( media, value[i] );
 								} else if ( key === 'css' ) {
 									// { "css": [css, ..] }
-									addEmbeddedCSS( value[i] );
+									addEmbeddedCSS( value[i], cssHandle() );
 								}
 							}
 						// Not an array, but a regular object
@@ -958,47 +1043,9 @@ var mw = ( function ( $, undefined ) {
 					}
 				}
 
-				// Add localizations to message system
-				if ( $.isPlainObject( registry[module].messages ) ) {
-					mw.messages.set( registry[module].messages );
-				}
-
-				// Execute script
-				try {
-					script = registry[module].script;
-					markModuleReady = function () {
-						registry[module].state = 'ready';
-						handlePending( module );
-					};
-					nestedAddScript = function ( arr, callback, async, i ) {
-						// Recursively call addScript() in its own callback
-						// for each element of arr.
-						if ( i >= arr.length ) {
-							// We're at the end of the array
-							callback();
-							return;
-						}
-
-						addScript( arr[i], function () {
-							nestedAddScript( arr, callback, async, i + 1 );
-						}, async );
-					};
-
-					if ( $.isArray( script ) ) {
-						registry[module].state = 'loading';
-						nestedAddScript( script, markModuleReady, registry[module].async, 0 );
-					} else if ( $.isFunction( script ) ) {
-						registry[module].state = 'ready';
-						script( $ );
-						handlePending( module );
-					}
-				} catch ( e ) {
-					// This needs to NOT use mw.log because these errors are common in production mode
-					// and not in debug mode, such as when a symbol that should be global isn't exported
-					log( 'Exception thrown by ' + module + ': ' + e.message, e );
-					registry[module].state = 'error';
-					handlePending( module );
-				}
+				// Kick off.
+				cssHandlesRegistered = true;
+				checkCssHandles();
 			}
 
 			/**
diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
index 7ae9826646..3dda7c7882 100644
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
@@ -269,7 +269,7 @@
 	function assertStyleAsync( assert, $element, prop, val, fn ) {
 		var styleTestStart,
 			el = $element.get( 0 ),
-			styleTestTimeout = ( QUnit.config.testTimeout - 200 ) || 5000;
+			styleTestTimeout = ( QUnit.config.testTimeout || 5000 ) - 200;
 
 		function isCssImportApplied() {
 			// Trigger reflow, repaint, redraw, whatever (cross-browser)
@@ -340,7 +340,7 @@
 		} );
 	} );
 
-	QUnit.test( 'mw.loader.implement( styles={ "css": [text, ..] } )', 2, function ( assert ) {
+	QUnit.asyncTest( 'mw.loader.implement( styles={ "css": [text, ..] } )', 2, function ( assert ) {
 		var $element = $( '<div class="mw-test-implement-a"></div>' ).appendTo( '#qunit-fixture' );
 
 		assert.notEqual(
@@ -352,15 +352,12 @@
 		mw.loader.implement(
 			'test.implement.a',
 			function () {
-				QUnit.stop();
-				setTimeout(function () {
-					assert.equal(
-						$element.css( 'float' ),
-						'right',
-						'style is applied'
-					);
-					QUnit.start();
-				});
+				assert.equal(
+					$element.css( 'float' ),
+					'right',
+					'style is applied'
+				);
+				QUnit.start();
 			},
 			{
 				'all': '.mw-test-implement-a { float: right; }'
@@ -436,8 +433,8 @@
 		] );
 	} );
 
-// Backwards compatibility
-	QUnit.test( 'mw.loader.implement( styles={ <media>: text } ) (back-compat)', 2, function ( assert ) {
+	// Backwards compatibility
+	QUnit.asyncTest( 'mw.loader.implement( styles={ <media>: text } ) (back-compat)', 2, function ( assert ) {
 		var $element = $( '<div class="mw-test-implement-c"></div>' ).appendTo( '#qunit-fixture' );
 
 		assert.notEqual(
@@ -449,15 +446,12 @@
 		mw.loader.implement(
 			'test.implement.c',
 			function () {
-				QUnit.stop();
-				setTimeout(function () {
-					assert.equal(
-						$element.css( 'float' ),
-						'right',
-						'style is applied'
-					);
-					QUnit.start();
-				});
+				assert.equal(
+					$element.css( 'float' ),
+					'right',
+					'style is applied'
+				);
+				QUnit.start();
 			},
 			{
 				'all': '.mw-test-implement-c { float: right; }'
@@ -470,7 +464,7 @@
 		] );
 	} );
 
-// Backwards compatibility
+	// Backwards compatibility
 	QUnit.asyncTest( 'mw.loader.implement( styles={ <media>: [url, ..] } ) (back-compat)', 4, function ( assert ) {
 		var $element = $( '<div class="mw-test-implement-d"></div>' ).appendTo( '#qunit-fixture' ),
 			$element2 = $( '<div class="mw-test-implement-d2"></div>' ).appendTo( '#qunit-fixture' );
@@ -508,7 +502,7 @@
 		] );
 	} );
 
-// @import (bug 31676)
+	// @import (bug 31676)
 	QUnit.asyncTest( 'mw.loader.implement( styles has @import)', 5, function ( assert ) {
 		var isJsExecuted, $element;
 
-- 
2.20.1