Bug Tracker

Opened 12 years ago

Closed 11 years ago

#2614 closed bug (fixed)

IE implementation of DOMContentLoaded failing when no assets

Reported by: diego Owned by: john
Priority: major Milestone: 1.3
Component: event Version: 1.2.6
Keywords: Cc: diego, zachleat
Blocked by: Blocking:

Description

The "doScroll()" trick is not enough in all situations to ensure that the ".ready()" function will be fired before "onload" event. This is due to the fact that we are not using a precisely timed event but an slightly unpredictable javascript timeout.

The order in which these event are fired should always be "ready" event first and then the "onload" event. This is to ensure that user's functions are fired in the same order across different browser. This behavior is needed to be able to overwrite some javascript function before the "onload" event. For example during unit testing it is desirable to overwrite the "alert()" function before the "onload" event to avoid these synchronous blockers.

This patch does exactly that by completely implementing the IEContentLoaded trick which also needs an "onreadystatechange" event to be used in combinations with the "doScroll()" trick.

This is the thread where I left the first note.

http://groups.google.com/group/jquery-dev/browse_thread/thread/7c6a479ac24c4e77

I also adjusted the "ready()" method to be triggered automatically on jQuery startup so "isReady" is always set to true even if Lazy Loaded after the "onload" event.

http://groups.google.com/group/jquery-dev/browse_thread/thread/c713fa86d4a28785/

Here is the small patch.

Diego Perini

Attachments (2)

event-ready.patch (1.7 KB) - added by diego 11 years ago.
Patch for event.js in trunk
ready_load_order_jquery.html (715 bytes) - added by diego 11 years ago.
Test firing order for the ready() function

Download all attachments as: .zip

Change History (21)

comment:1 Changed 11 years ago by Andrea Giamm

Hi Diego, I have read the ml and I though about this alternative function. If I am not wrong, the problem is unpredictable setTimeout execution order, so my proposal is to use a single interval for every kind of purpose (but we could use a setTimeout as well, but in this case an interval could be faster) using a simple stack queue:

IEContentLoaded = function(events){
    function    index(w){
         for(var i = 0, length = events.length; i < length; i++)
                if(events[i].w === w)
                    return  i;
         return -1;
    };
    function    init(i){
        for(var j = 0, length = events[i].fn.length; j < length; j++)
            events[i].fn[j]();
        events.splice(i, 1);
        // or // events = events.slice(0, i).concat(events.slice(++i))
    };
    var i;
    return  function(w, fn){
        var j = index(w);
        ~j ? events[j].fn.push(fn) : events.push({w:w,fn:[fn]});
        if(!i)
            i = setInterval(function(){
                for(var j = 0, length = events.length; j < length; j++)
                    try {
                        events[j].w.document.documentElement.doScroll('left');
                        init(j);
                    } catch (e) {}
                if(length === 0)
                    clearInterval(i);
            }, 10);
    }
}([]);

It seems to work as expected in IE 5.5, IE 6, IE 7, and IE 8 beta 1 For IE5 I suppose we should use the concat slice instead of splice, but it is only a minor detail.

Kind Regards

comment:2 Changed 11 years ago by Andrea Giamm

Sorry, I forgot the readyState ...

IEContentLoaded = function(events){
    function    index(w){
         for(var i = 0, length = events.length; i < length; i++)
                if(events[i].w === w)
                    return  i;
         return -1;
    };
    function    init(i){
        for(var j = 0, length = events[i].fn.length; j < length; j++)
            events[i].fn[j]();
        events.splice(i, 1);
        // or // events = events.slice(0, i).concat(events.slice(++i))
    };
    var i;
    return  function(w, fn){
        var j = index(w), d = w.document;
        if(~j)
            events[j].fn.push(fn);
        else {
            j = events.push({w:w,fn:[fn]}) - 1;
            d.onreadystatechange = function() {
        		if (d.readyState == 'complete') {
        			d.onreadystatechange = null;
                    if(~index(w))
                        init(j);
        		}
        	};
        };
        if(!i)
            i = setInterval(function(){
                for(var j = 0, length = events.length; j < length; j++)
                    try {
                        events[j].w.document.documentElement.doScroll('left');
                        init(j);
                    } catch (e) {}
                if(length === 0)
                    clearInterval(i);
            }, 10);
    }
}([]);

comment:3 Changed 11 years ago by Andrea Giamm

Please ignore above example, I did not think about for loop problems. This should be the last proposal:

IEContentLoaded = function(events){
    function    index(w){
         for(var i = 0, length = events.length; i < length; i++)
                if(events[i].w === w)
                    return  i;
         return -1;
    };
    function    init(i){
        for(var j = 0, length = events[i].fn.length; j < length; j++)
            events[i].fn[j]();
        events.splice(i, 1); // IE5 => events = events.slice(0, i).concat(events.slice(++i))
    };
    var i;
    return  function(w, fn){
        var j = index(w), d = w.document;
        if(~j)
            events[j].fn.push(fn);
        else {
            j = events.push({w:w,fn:[fn]}) - 1;
            d.onreadystatechange = function() {
                if (d.readyState == 'complete') {
                    d.onreadystatechange = null;
                    j = index(w);
                    if(~j)
                        init(j);
                }
            };
        };
        if(!i)
            i = setInterval(function(){
                for(var j = 0, length = events.length; j < length; j++)
                    try {
                        events[j].w.document.documentElement.doScroll('left');
                        init(--length && j--);
                    } catch (e) {}
                if(length === 0)
                    clearInterval(i);
            }, 10);
    }
}([]);

Kind Regards

comment:4 Changed 11 years ago by Andrea Giamm

... init(length-- && j--); ... (I need more coffee)

comment:5 Changed 11 years ago by diego

I rearranged the original patch so for iframes we can use a fall back that ensure that the ready() method will fire before the standard "onload" event on Internet Explorer.

Attached a new event-ready.patch for jquery-1.2.6.js.

comment:6 Changed 11 years ago by john

Owner: set to john

comment:7 Changed 11 years ago by john

Milestone: 1.2.41.3
Version: 1.2.31.2.6

comment:8 Changed 11 years ago by diego

Revised patch as discussed. Avoid using jQuery.browser.msie.

Re-submitted the attached patch, diffs made for event.js in current trunk.

Changed 11 years ago by diego

Attachment: event-ready.patch added

Patch for event.js in trunk

comment:9 Changed 11 years ago by john

Resolution: fixed
Status: newclosed

Fixed in SVN commit [5941].

comment:11 Changed 11 years ago by zachleat

Resolution: fixed
Status: closedreopened

This fix is not correct. As reported in

http://www.thefutureoftheweb.com/blog/adddomloadevent#comment16

and I have confirmed in my own tests, adding "onreadystatechange" to the document element will fire after images have loaded, not before.

What you need to do is branch based on window == top, if they are equal use the doScroll("left") hack, if not use the same code that was used in jQuery 1.2.1 to fire the DOMContentLoaded event in IE (document.write a deferred script tag).

Or, you could just use the deferred script tag all the time for IE, but anything that avoids document.write's would be ideal.

comment:12 Changed 11 years ago by diego

zachleat, that message is two years old. The two events produce similar results but are not the same. "onreadystatechange" will fire before the "onload" event, preceding it by just few milliseconds. That is exactly what we need, especially for iframes but not only for that...

Using "document.write" is risky because it is possible to completely overwrite the document, leaving the users with a white page or other similar problems on IE (operation aborted).

In normal windows the "doScroll()" trick fire before images are loaded.

This technique is solid enough and works very well on IE.

comment:13 Changed 11 years ago by flesler

Cc: diego zachleat added
need: ReviewTest Case

Can anyone provide an example or counter-example proving this works well or doesn't work ?

comment:14 Changed 11 years ago by diego

Ok Ariel, here is one test for current jQuery 1.2.6.

It shows the order of events is not what we would expect.

We expect ready() to fire before onload() on all browsers, so the order in the above example should be:

  • ready
  • load

instead from Internet Explorer we get:

  • load
  • ready

this produces unexpected behavior from code running on IE, normally this is more of an issue when both event are used, each starting different parts of the main code.

I have to stress that the "onreadystatechange" addition is absolutely necessary to fix other unexpected behaviors in IE especially those bound to the difference between cached and un-cached pages and the use of the Back/Forward buttons. The "doScroll()" trick is not an event, it is just a polling loop and is really useful only on the first fresh read of the page. When the page and it's content are already cached you will notice the firing happens through the "onreadystatechange" event.

You may easily test this strange IE behavior by inserting some logging lines in both of them and see the differences when a page has no assets at all, when it does have assets but is the first visit, and when it is fetched directly from the browser internal cache.

You see, also the solution on IE is just a few line this is a very elaborate strategy to get what most people want.

Unfortunately for all of us IE is not a joke, it still has more than half of the market. This solution also works on IE8, so keep it handy for the next 10 year.

-- Diego

Changed 11 years ago by diego

Test firing order for the ready() function

comment:15 Changed 11 years ago by diego

I also suggest to change:

if ( document.attachEvent )

with:

if ( document.createEventObject )

to avoid Opera going through the IE path. Not sure if this would create misbehaviors in Opera, however it is better to ensure the conditional branch each browser will follow.

-- Diego

comment:16 Changed 11 years ago by zachleat

Please see the following test cases in Internet Explorer:

http://www.zachleat.com/Projects/jquery/original/parent.html http://www.zachleat.com/Projects/jquery/diego-patch/parent.html http://www.zachleat.com/Projects/jquery/my-patch/parent.html

image.php contains the following PHP code: sleep(2); which will cause the client to wait 2 seconds before it considers the image to be downloaded.

The times displayed are the date difference between the first line of JS execution and the event firing. Note how the original and diego-patch versions both fire AFTER the image has loaded in the child iframe. The only difference in my-patch is that it uses the isReady method that was used for IE in jQuery 1.2.1 (deferred script load and document.write). For comparison, the HTC method presented by Dean Edwards is also included. This method is described here: http://dean.edwards.name/weblog/2005/09/busted2/

I'm pretty sure the test is sound, please let me know if it is not.

Thanks, Zach

comment:17 Changed 11 years ago by diego

Zach, your tests are correct, this is a well known IE problem, we talked about it in several posts on the jquery-dev list. The reason the method changed from 1.2.1 is that it was giving too many problems on IE.

"document.write()" had to be excluded for several reasons:

  • - XHTML does not allows it, not compliant
  • - doesn't work with small pages or when no assets
  • - it sometime completely overrides the page content
  • - it gives problems when there are mixed HTTP/HTTPS requests

so the only alternative method we could resort to for IFRAMEs is to use Microsoft own HTC method where feasible, that is an extra external file that people try to avoid if possible.

There is nothing that stops you launching your code before the images are loaded if you know what you do on IE. Effectively it is possible in IE to append child to the BODY before the page is completely loaded.

There are other events that can be used in IE as a notification about the loading process, these are "activate" and "beforeactivate", but as said these are useful only to attach widget to the BODY before the page is completely loaded and would not work as a generic "ready()" method that fits everybody needs.

As an example if we used these "activation" events we would not be able to correctly execute a CSS selection on the page since the page may not be completely loaded yet.

The unique scope of this patch is to fix the order in which "ready" and "onload" will fire on Internet Explorer.

Hope I have clarified some aspect of this problem...

-- Diego

comment:18 Changed 11 years ago by zachleat

Ok, if that's the scope of this patch that's fine. I still think that jQuery should supply a DOMContentLoaded event for iframed content (in IE). Would it be okay with everyone if I opened a separate support ticket for that request?

comment:19 Changed 11 years ago by zachleat

As a side node, the current Prototype code base uses the deferred script method exclusively for iframe/non-iframe DOMContentLoaded in IE. Do you have test cases that show the examples you're describing (XHTML, override page content, http/https)?

comment:20 Changed 11 years ago by john

Resolution: fixed
Status: reopenedclosed

Fixed in SVN rev [5970].

Note: See TracTickets for help on using tickets.