Last modified: 2012-05-03 02:42:43 UTC
Created attachment 10048 [details] Flush and sleep in skins/Vector.php to give time for RL to finish before the main document finishes loading With $wgResourceLoaderExperimentalAsyncLoading enabled, modules are loaded from the <head> rather than from the end of the <body>. This causes the modules to be loaded simultaneously with the main HTML, and under certain conditions, module execution may occur before the #mw-panel element appears in the DOM. ext.vector.collapsibleNav in particular does not register a $(document).ready() callback, it just tries to modify the DOM at the file level of the module. No errors are logged, $( '#mw-panel' ) just returns an empty list and the styling is not applied. Other modules may have similar issues with $wgResourceLoaderExperimentalAsyncLoading. Bug 34450 may possibly be related but there were a lot of issues mixed in there without any reproduction procedure so I decided to file a new bug. Whether or not you see the issue depends on the browser, page length, network speed and cache timing details, but it's possible to reliably reproduce it by slowing down page delivery with the attached flush/sleep patch, and setting MW_NO_OUTPUT_BUFFER in LocalSettings.php: define( 'MW_NO_OUTPUT_BUFFER', 1 ); Using that method, I can reproduce it every time in Firefox or Chrome.
(In reply to comment #0) > ext.vector.collapsibleNav in particular does not register a $(document).ready() > callback, it just tries to modify the DOM at the file level of the module. No > errors are logged, $( '#mw-panel' ) just returns an empty list and the styling > is not applied. Other modules may have similar issues with > $wgResourceLoaderExperimentalAsyncLoading. > Are you sure you were looking at an up-to-date version of the code when you wrote this? I fixed this in https://www.mediawiki.org/wiki/Special:Code/MediaWiki/111685 four days ago and got Reedy to deploy it.
Oh.
I found this in a lot more places, so I'm going to call this a MediaWiki issue rather than a Vector issue. I'm currently in the process of auditing all extension JavaScript, and I'm referencing this bug report in my commit messages. Probably nothing needs to be changed in RL itself, except perhaps adding a debugging feature to make failures a bit more obvious.
To clarify, this bug is more of a general todo for many modules to fix up bad code, not a tangible solvable bug in itself.
(In reply to comment #4) > To clarify, this bug is more of a general todo for many modules to fix up bad > code, not a tangible solvable bug in itself. Kind of. Either many extensions magically acquired a bug when the core was changed, or there is a b/c break in core which is a bug. So this is either a tracking bug for large numbers of extension bugs, or a bug report about a b/c break. You be the judge. Calling it a todo would imply that the modules aren't broken, but they are.
(In reply to comment #5) > Calling it a todo would imply that the modules aren't broken, but they are. Yeah, 'todo' was definitely a bad word choice. By all means, they are valid breakages. But it's also true that code manipulating a dom element from a script that isn't hardcoded inline, should really be in a document-ready wrapper. And fortunately most code is doing that right. Either because they were ported from before ResourceLoader (at which point all was loaded from the <head> and without document-ready hook it wouldn't work at all), or because it was written in the past year and done "right" right away. It's also unfair to blame the extensions, since it was reliable to assume that modules not loaded on 'top' would be loaded from the 'bottom'[1], and as such execute after the document is ready. The module queue was, however, split up to allow certain modules to load before the page is rendered, not the other way around (eg. to make sure code is executed after the page is rendered), but regardless it has happened (also in modules written by experienced developers like Roan, Trevor and myself). But it's also fairly safe to assume that nothing is going to be implemented in core to mitigate it. It doesn't make sense to delay a module's executing by force. A dom-ready wrapper is a trivial fix. Speaking of which, are there any instances of this problem left that we know of?
[1] 'top' and 'bottom' are terrible names for their queues, for so many reasons. more accurate would be 'blocking before page render' and 'non-blocking as soon as possible'.
(In reply to comment #6) > Speaking of which, are there any instances of this problem left that we know > of? No, but I only reviewed about half of the extensions before I ran out of time, and I found bugs in 9 of them, so I'd expect there to be another 9 broken extensions waiting to be found.
Fixes should be done in all extensions now, marking resolved. It's possible I missed one or two, and there were some non-RL extensions that would need to be converted to RL with care, if they are ever converted. If there are more problems discovered, this bug can be reopened.
