A bug was introduced last month in domready library, making triggering the execution of the scripts too early in IE9 and IE10.
The bug was introduced to Drupal core, at the same time some days later. #2207589: Update JS lib: domready to 1.0.4
Check the bug details and PR I opened upstream with more info: https://github.com/ded/domready/pull/31
We are using domready to call Drupal.attachBehaviors. So the bug makes that is executed too early, when not all the scripts are loaded. (aka. not all the Drupal.Behaviors are loaded). For example, in the installation page, the "collapse" buttons (details element) don't work, because the Drupal behavior is not executed.
See screenshot: http://monosnap.com/image/3zCXA3jlCIONfRs8IubnXJWFaAGAwc
With this issue, I want to open a question: Is it worthy to have domready library in Drupal core?
This bug won't have hit us if we would be using jQuery. They tested and documented this bug very long time ago:
https://github.com/jquery/jquery/blob/2df1aad6a1c9376c2a477eba26ee992113...
In domready library there is no tests, the bug was introduced upstream without being reviewed.
https://github.com/ded/domready/pull/26
This is unrelated, but also I saw the author's comment in another issue:
"I haven't done the due diligence making an official notice, but I've slowly been dropping < IE9 support. It's in the readme about the new browser support, there just isn't a full statement."
That's not serious, dropping browsers support without official notices. I wouldn't trust Drupal's JavaScript execution to this library.
The proper execution of the scripts is critical IMHO, we should rely on jQuery for this.
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | 2235425-domready-23.patch | 1.57 KB | andypost |
| #20 | reverting-domready-2235425-20.patch | 1.62 KB | corbacho |
Comments
Comment #1
corbacho commentedComment #2
corbacho commentedComment #3
nod_We don't want jQuery ready because it comes with 32k of dependency for any script depending on core/drupal. Which is pretty much all of them.
https://github.com/tubalmartin/ondomready looks like a replacement if people don't like domready. Point is, I don't want drupal.js to depend on jQuery.
Comment #4
corbacho commentedok I understand that jQuery is too big. But at least, in installation pages, jQuery is loaded. Why use domready in those pages?
The library "ondomready" that you suggest looks ok to me. Good find! (but it's a bit too big, because it's IE6 compatible). I like that it tries to mimic jQuery's ready, but one thing I noticed it's that "ondomready" is using a non-jquery-way of resolving the list of callbacks. It's using setTimeout to resolve asynchronously the list of callbacks. But in jQuery they use Deferred objects (that uses jQuery.Callbacks internally) to resolves synchronously the list of functions. I'm not 100% sure of the consequences of this, since setTimeout "should?" respect the order of execution. But in case that it doesn't., if there is jQuery code relying on the sync behavior of Deferred, things could get potentially screwed up.
Also resolving callbacks synchronously is faster than using setTimeout.
So there are 2 tasks:
- Find a good replacement of the broken domready (now it has a bug and this library doesn't have any fallback like jQuery does, just-in-case)
- If jQuery is loaded in the page, let it handle the initialization of events. So the "holdReady" and the richer API of jQuery works still.
We could do our own domready library, that mimics jQuery 2.x, with sync resolving, only for post-IE8 browsers, and promote it outside of Drupal, so it wouldn't be an "in-house" drupalism. It sounds like a bad idea yeah. But at least we would have bigger control on the releases.
Comment #5
nod_We need consistency in core, init should always behave the same. Swaping out domready for jQuery is not a solution for us (contrib, why not).
Forking and rolling our own is not a good solution I agree. We don't want that.
We might be able to get rid of all this altogether, adding inline, in the footer a call to
<script defer>Drupal.attachBehaviors();</script>. Can you see if that'd work well on IE? (just comment the domready line on drupal.js and add the script inline in html.html.twig or something for now we can figure out how to do it properly later).If putting that in the footer doesn't work out, we'll need to talk with domready maintainer to see if he's open to more structure around releases and that sort of things.
Comment #6
nod_Umm reading the doc looks like defer can't be used on inline scripts. Disregard last comment, can you try this patch on IE9?
Comment #7
corbacho commentedI like the 'defer' solution. It works, and it should execute at the same time than DOMContentLoaded, more or less.
But:
* It didn't work in the installation pages (because there is no "footer" scope ?)
* It doesn't work in every browser http://caniuse.com/script-defer, (Implementation is buggy in IE9) When 2 scripts with "defer" attribute are in the same page, and under some circumstances, the execution of script in IE9 will jump from 1st defer to the 2nd defer. Documented here https://github.com/h5bp/lazyweb-requests/issues/42 (See specially the comments of nicjansma, IE Engineer)
I don't think we can use 'defer' then
Comment #8
schnitzel commentedhad the same issue on IE10, with that patch it works now. But did not test the defer stuff.
Comment #9
nod_I think the defer issue is not a big deal for us.
Since we declare all dependencies, I don't see a situation where you're using the API right and get bitten by that. And if there is, we have all the info to make sure it doesn't happen.
Comment #10
corbacho commentedSome thoughts about defer solution:
* defer tag only works for external scripts. So I was afraid that JS aggregation would mess up the new init.js file. But it's not going to be a problem. Luckily this has been solved already #1664602: Allow attributes to be passed to drupal_add_[css|js] (SRI) (scripts with custom attributes will not be grouped or aggregated in Drupal 8)
* Still, it's a bit against good practices to have 1 extra HTTP request only to initialize the scripts. (It doesn't matter if you load jQuery or not in the page. In *every* page of *every* Drupal 8 site we will have this extra request). But strange enough, it feels clean to me. Because it can be overridden via custom modules easily.
* A good thing IMHO is that "defer" scripts execute after DOM is ready *but* a bit before DOMContentLoaded is trigered (See http://calendar.perfplanet.com/2012/deciphering-the-critical-rendering-path ). As a side effect, it could delay the triggering of DOMContentLoaded until all defer scripts have been executed. But I see this as a positive thing. We are giving higher priority to Drupal.behaviors. Then 3rd party social media scripts, etc that uses normally DCL, they will execute *after* Drupal.behaviors.
But again, I point the same link than before. For example, this comment https://github.com/h5bp/lazyweb-requests/issues/42#issuecomment-1896139, it says that (under certain circumstances) IE9 might start to execute the first deferred script, stop the execution in the middle of the script!, and start to execute a second deferred script. This could be a pandora box of bugs, difficult to debug. Anyway, I will try to replicate this bug. I don't want to be too alarmist with this.
Another possible solution I've been testing.. is to use a grunt-custom-built JQuery with only 'core/ready' module. This will produce a 10Kb minified file (5Kb gzipped) and give us a lot of other goodies aside document.ready, for a minimum size payload. And it's nice that we can trust a well tested jQuery 2.x for this, instead of a random snippet. We might need to use .noConflict to avoid conflicts in case there are 2 jQuery libs in the same page.
Comment #11
corbacho commenteddefer bug verified :
Creating a init2.js (also defer attribute) , and modifying init.js to have this content:
and init2.js with this content:
console.log('This is from init2 file');Produces this log in IE9:
This happens in IE9 with Windows 7. But can not be reproduced with Emulation mode with IE11.
I think this leaves out the "defer" solution :/
Comment #12
nod_Why would you have 2 init scripts in defer? there can only be one initialization.
Comment #13
corbacho commentedI edited #11 to make it more clear. In init2.js there is no initialization. Only a console.log.
This is like the multi thread joke:
A programmer had a problem. He thought I know, I’ll solve it with threads!'. has Now problems. two he"
Comment #14
corbacho commentedThe process of building a minimal jQuery is too cumbersome. I had to build a too complicated build command, that is not future-proof because is based on excluding jQuery modules. I felt like walking through an "unknown path", since I found 2 bugs in my tests. So I'm starting to give up here... I'm doubting if this is a good idea after all, and I can imagine developers being confused about having 2 jQuery libraries in same page, etc. :/
------------
I'm thinking again about going to the easiest way possible, script in the bottom. As nod suggested in the patch #6 but without defer. The problem is that we need to warranty that the init.js file is the last script in the footer. If we give a very high weight inside the 'footer' scope, like 9999, it might work. But it's too brittle.
I attached a patch, where we have init.js in a special scope that is appended to the 'footer' scope.
We don't need to modify anything in ajax.inc ($scripts_footer) . Because we don't need Drupal.attachBehaviors there. It's already done in ajax.js.
Testing this patch I noticed that Drupal.behaviors will be executing just *before* DOMContentLoaded. This is good because all kind of 3rd party scripts are using this event to initialize, so we make sure Drupal.behaviors are executed first. Also I noticed that the execution of Drupal.behaviors have the side-effect of *delaying* the DOMContentLoaded event. In my tests, about 80ms. But who cares? All the important things are wrapped inside Drupal.behaviors. 3rd party scripts will have to wait a bit.
Comment #15
nod_well we kind of care about domcontentloaded. It's what's used by analytics to measure pagespeed. Delaying that would make Drupal appear slower than it is. Then again I agree it's good to make sure Drupal behaviors are run first.
( edit ) so what about
If I remember right it doesn't delay domcontentloaded. I'm not sure how much we want to support random things initializing outside of drupal behaviors. drupal.js is small now so not a big dependency anymore to use behaviors.
Comment #16
andypostTested
setTimeout()approach ie10 - that works ;)Suppose better to replace domready lib with own init if no way to solve this in upsream
Comment #17
corbacho commentedsetTimeout can work "most of the times" but neither is reliable. It could execute attachBehaviors too early. Specially with gzip off or 3rd party scripts can make the browser 'idle' and execute the setTimeout. Many people commented here against this approach: http://snook.ca/archives/javascript/settimeout_solve_domcontentloaded#c5...
Comment #18
Catsys commentedsetTimeOut may not be appropriate because it really performs behavior too early. Due to this, there are problems with some scripts. For a simple example, I do not always work out the forms of states.
Comment #19
andypostConfirm that states works unpredictable with
setTimeoutComment #20
corbacho commentedMaybe this could be a good compromise? Patch reverting domready to 1.03 version (stable version before they broke IE support) https://github.com/ded/domready/tree/v1.0.3
If this is not beta-blocker, at least should be marked as Critical
Comment #21
tim.plunkettThis is an important bug to fix, but I don't see how it's critical. It doesn't completely render core unusable in a majority of browsers.
Comment #22
nod_New version of domready is out, fixing this issue. Putting all our JS in the footer by default should be still investigated but that's what #784626: Default all JS to the footer, allow asset libraries to force their JS to the header is for :)
Comment #23
andypostComment #24
nod_All good, this is just a library update.
Comment #25
alexpottCommitted 426a531 and pushed to 8.0.x. Thanks!
Comment #28
imiksuCleaning up drupalcampfi tags.