Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 2682 #2693

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix 2682 #2693

wants to merge 3 commits into from

Conversation

SergioCrisostomo
Copy link
Member

fixes #2682 (DOMReady not firing when MooTools is loaded to a ready page)

  • added extra check for cases when MooTools is added to a page that is already loaded.
  • refactored DOMReady.js specs that were still from pre-Grunt times.

Input or corrections welcome

@@ -77,6 +77,7 @@ if ('onreadystatechange' in document) document.addListener('readystatechange', c
else shouldPoll = true;

if (shouldPoll) poll();
if (!ready) check();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which check does it actually execute? The readyState or the scroll test. The last one isn't included in the minus-ie8 build.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arian good point. Did some more tests in Travis and in a IE8 VM using:

if (shouldPoll) poll();
checks.each(function(chk){
    alert(chk + '');
});
if (!ready) check();

and all cases showed the document.readyState test function is tested first.
I tested also removing that IE8 block on Travis and it passed in all Browsers.
It looks clean in that way.

We could import some more tests from the old PHP file if it feels like we are missing something, since the IE8 block seems redundant to these specs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the old php file, bear in mind the original case scenario which I described here:

http://webreflection.blogspot.co.uk/2006/09/too-much-simple-domcontentloaded.html?showComment=1158586140000#c115858618235493470

It would be possible to simulate the php flush using a simple write in nodejs like in this stackoverflow question:
http://stackoverflow.com/questions/16184103/how-to-flush-chunks-of-arbitrary-sizes-in-nodejs

also I think the fix should be done here https://github.com/SergioCrisostomo/mootools-core/blob/fix-2682/Source/Utilities/DOMReady.js#L76 basically if the event is supported we have to check if the document readystate is completed, if it is, we can just run the domready

@SergioCrisostomo SergioCrisostomo force-pushed the fix-2682 branch 4 times, most recently from 513f9b0 to 3720df5 Compare January 24, 2015 23:34
@SergioCrisostomo
Copy link
Member Author

Updated using @kentaromiura's Specs suggestion.
4 specs suggested now.

@SergioCrisostomo SergioCrisostomo force-pushed the fix-2682 branch 2 times, most recently from 4628a96 to ba054dd Compare February 11, 2015 10:54
@SergioCrisostomo SergioCrisostomo added this to the 1.5.2 milestone Feb 22, 2015
@SergioCrisostomo SergioCrisostomo force-pushed the fix-2682 branch 2 times, most recently from 249e7da to 54ca50d Compare February 24, 2015 06:52
@SergioCrisostomo
Copy link
Member Author

any comments on this?... 👂

for (var i = 0; i < domreadyCallbacks.length; ++i){
describe("DOMReady", function(){

var win, frame, cb, ready;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a fan of abbreviated variable names. code should be readable; let the minimizer worry about brevity. So, callback here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reading a big more I see this is the spec file, so I'm less pedantic about it... can leave it.

@SergioCrisostomo
Copy link
Member Author

@arian updated so the server is now inside a Grunt task.

@SergioCrisostomo SergioCrisostomo modified the milestones: 1.5.2, 1.5.3 Sep 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DomReady does not work when it reloads page.
4 participants