Krinkle has uploaded a new change for review.
https://gerrit.wikimedia.org/r/56637
Change subject: mw.loader: Fix regression that caused CSS load after scripts.
......................................................................
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: I7b1562b12c8ed1a0286c19ef9db8f76870d4f69e
---
M resources/mediawiki/mediawiki.js
M tests/qunit/suites/resources/mediawiki/mediawiki.test.js
2 files changed, 113 insertions(+), 72 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/37/56637/1
diff --git a/resources/mediawiki/mediawiki.js b/resources/mediawiki/mediawiki.js
index d1cb41d..433553a 100644
--- a/resources/mediawiki/mediawiki.js
+++ b/resources/mediawiki/mediawiki.js
@@ -363,7 +363,7 @@
* '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' },
@@ -393,7 +393,9 @@
// 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 */
@@ -470,9 +472,14 @@
/**
* @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.
@@ -531,11 +538,14 @@
} else {
styleEl.appendChild(
document.createTextNode( String( cssText ) ) );
}
+ cssCallbacks.fire().empty();
return;
}
}
$( addStyleTag( cssText, getMarker() ) ).data(
'ResourceLoaderDynamicStyleTag', true );
+
+ cssCallbacks.fire().empty();
}
/**
@@ -880,7 +890,8 @@
* @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 );
@@ -889,7 +900,7 @@
} 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 @@
}
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 }
@@ -925,7 +1010,7 @@
// 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;
@@ -942,7 +1027,7 @@
addLink( media, value[i] );
} else if ( key
=== 'css' ) {
// {
"css": [css, ..] }
-
addEmbeddedCSS( value[i] );
+
addEmbeddedCSS( value[i], cssHandle() );
}
}
// Not an array, but a regular
object
@@ -959,47 +1044,9 @@
}
}
- // 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 e8663f8..01e78f6 100644
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
@@ -250,7 +250,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)
@@ -321,7 +321,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(
@@ -333,15 +333,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; }'
@@ -417,8 +414,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(
@@ -430,15 +427,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; }'
@@ -451,7 +445,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' );
@@ -489,7 +483,7 @@
] );
} );
-// @import (bug 31676)
+ // @import (bug 31676)
QUnit.asyncTest( 'mw.loader.implement( styles has @import)', 5,
function ( assert ) {
var isJsExecuted, $element;
--
To view, visit https://gerrit.wikimedia.org/r/56637
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7b1562b12c8ed1a0286c19ef9db8f76870d4f69e
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Krinkle <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
