-
Notifications
You must be signed in to change notification settings - Fork 512
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
base: master
Are you sure you want to change the base?
Fix 2682 #2693
Conversation
@@ -77,6 +77,7 @@ if ('onreadystatechange' in document) document.addListener('readystatechange', c | |||
else shouldPoll = true; | |||
|
|||
if (shouldPoll) poll(); | |||
if (!ready) check(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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
513f9b0
to
3720df5
Compare
Updated using @kentaromiura's Specs suggestion. |
3720df5
to
aeb4c38
Compare
4628a96
to
ba054dd
Compare
249e7da
to
54ca50d
Compare
any comments on this?... 👂 |
for (var i = 0; i < domreadyCallbacks.length; ++i){ | ||
describe("DOMReady", function(){ | ||
|
||
var win, frame, cb, ready; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
54ca50d
to
e9359e8
Compare
@arian updated so the server is now inside a Grunt task. |
fixes #2682 (DOMReady not firing when MooTools is loaded to a ready page)
DOMReady.js
specs that were still from pre-Grunt times.Input or corrections welcome