In Source/Utilities/DomReady.js the `domready` event does not get fired if MooTools is loaded after 'DOMContentLoaded' event has already been fired and the document is already 'complete' or 'loaded' #2500

Closed
SalimBensiali opened this Issue May 15, 2013 · 8 comments

Projects

None yet

5 participants

@SalimBensiali

After investigating, it turns out that the check function never gets executed in the situation where:

1 - 'onreadystatechange' in document returns true
2 - The readystatechange event gets fired before document.addListener('readystatechange', check); gets executed.

In 1, polling does not get activated, and in 2, the event listener is never called.

This situation is happening to me because I am loading MooTools using RequireJS. And since RequireJS version 2.1, require([], function (){}) is async and uses setTimeout cf requirejs/requirejs@6b79ed9.

The delay introduced by setTimeout leads to a state where MooTools sets listeners for both DOMContentLoaded and readystatechange events after they get fired.

So in this particular scenario, and in a browser that is not ltIE8 and that does have onreadystatechange in its document, the shouldPoll variable remains undefined, thus poll never gets called, thus check never gets called, thus this function https://github.com/mootools/mootools-core/blob/master/Source/Utilities/DOMReady.js#L69 never gets called:

function(){
    var state = document.readyState;
    return (state == 'loaded' || state == 'complete');
}

So I think that a call to check should be synchronously performed say at the end of the script, to account for this particular case. This is actually what @jrburke is doing in his requirejs/domready https://github.com/requirejs/domReady/blob/master/domReady.js#L92

Any thoughts would be appreciated.

Do you guys need a pull request?

Cheers,
Salim

@arian
MooTools member

Not sure if I understand it entirely, but maybe the first time the domready event is added, it should execute the check function so it's sure everything is ready. Then the loaded variable is set to true so the event is fired immediately for the added and future listeners.

@arian
MooTools member

What would also help is a jsfiddle or something so we can reproduce it.

@SalimBensiali

Hi @arian, thanks for the quick feedback. I am not sure about calling the check function when the first domready event is added. Even thought this would work, I would rather see something like the following instead of L69:

var isStateComplete = function(){
    var state = document.readyState;
    return (state == 'loaded' || state == 'complete');
};
if (document.readyState){
    if (isStateComplete()) domready();
    else checks.push(isStateComplete);
}

Which is in the same flavour as the way doScrollWorks is handled L54.

The advantage of this approach is that the ready state of the document (variable ready) is known straight after the IIFE finished executing, whereas in your suggestion, this state will be known only after the domready event listener is set up.

@SalimBensiali

And here is a jsfiddle that reproduces the bug: http://jsfiddle.net/salimbensiali/q4Qa5.

@SalimBensiali

Any updates on this?

@phoenixsnake

I think this wasn't a bug

"domready" stage should be fire when "all DOM is ready for manipulation"
but you inject mootools after document loaded. i think this beyond domready responsibility.


if (document.readyState){
    if (isStateComplete()) domready();
    else checks.push(isStateComplete);
}

this code can fire domready event once.

what if we dynamic inject the mootools plugins that have it's own window.addEvent( "domready", function(){} ); again and again.
that event will never be fired too.

this use of domready event is special case.
so i think it user responsibility to remove window.addEvent( "domready", function(){} ); manually
( because inject the code after document is loaded. That's mean the DOM is already ready for manipulation. no need for checking for "domready" again )

Best practise is to add static script tag one by one , so they have no problem.

advanced people do dynamic add script tag. advanced people do an special thing.
i think that include the ability to manually initialize some custom special event too.

Or consider use mootools as base framework. load the others script via Asset.javascript, implement onload event

but if this feature really should be implement in core.
i think we could do something with native DOMNodeInserted event.
( I have tested on IE9, firefox, chrome DOMNodeInserted work fine )

headtag.addEventListener( "DOMNodeInserted", function(){
//do something
} );
.

@ibolmo ibolmo modified the milestone: 1.5.1, 1.5 Mar 3, 2014
@ibolmo ibolmo modified the milestone: 1.5.2, 1.5.1 Jul 3, 2014
@ibolmo
MooTools member

I'm trying to conceive of another addEvent('...') that fires immediately if the condition has already occurred. E.g. 'click'. Since it never happen, I'd have to agree that the onus is on the developer to understand their environment. If their environment implies that the DOM would already be ready, then the check should be against document.readyState else fall back to window.addEvent('domready', ...). And this is outside the scope of Core. Closing for now, but if you have a better case for it we can revisit.

@ibolmo ibolmo closed this Nov 11, 2014
@diegoflorez

En firefox 34.0.5 when it reloads the page, the code inside

window.addEvent('domready',function(){
   //code inside
})

is never called again, until you close the window/tab and reopen the page.
It's imposible to make a jsfiddle to show this behaviour.
I thinks this is a bug. Because you are trying to reload the page, so the DOM is for a new instance, ready.
Please re-open this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment