mw.loader: Fix regression that caused CSS load after scripts.
authorTimo Tijhof <ttijhof@wikimedia.org>
Fri, 29 Mar 2013 20:09:06 +0000 (21:09 +0100)
committerCatrope <roan.kattouw@gmail.com>
Mon, 8 Apr 2013 19:23:19 +0000 (12:23 -0700)
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
tests/qunit/suites/resources/mediawiki/mediawiki.test.js

index 0da9d87..6c7e697 100644 (file)
@@ -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();
                        }
 
                        /**
index 7ae9826..3dda7c7 100644 (file)
        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)
                } );
        } );
 
-       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(
                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; }'
                ] );
        } );
 
-// 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(
                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; }'
                ] );
        } );
 
-// 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' );
                ] );
        } );
 
-// @import (bug 31676)
+       // @import (bug 31676)
        QUnit.asyncTest( 'mw.loader.implement( styles has @import)', 5, function ( assert ) {
                var isJsExecuted, $element;