#127 ✓resolved
javier

Use doScroll approach for a more accurate dom:loaded event

Reported by javier | August 25th, 2008 @ 08:07 AM | in 1.6.1

Explorer randomly don't fire the window.load or dom:loaded event. It wait to load on element. I was able to fix it by chanching line 4025 (from prototype 1.6.0.2) to:


document.write("<script type=\"text/javascript\" id=\"__onDOMContentLoaded\" defer=\"defer\" src=\"javascript:void(0)\"><\/script>");
@22

Comments and changes to this ticket

  • Tobie Langel

    Tobie Langel October 6th, 2008 @ 08:43 AM

    • State changed from “new” to “bug”
    • Milestone set to 1.6.1
    • Tag set to dom, ie, ie6, ie7, needs_patch, needs_tests
  • Andrew Dupont

    Andrew Dupont October 6th, 2008 @ 04:51 PM

    God, this freaking URL thing again. I think we need to change to jQuery's "doScroll" approach just so that we never have to deal with this headache again.

  • Andrew Dupont

    Andrew Dupont October 6th, 2008 @ 04:52 PM

    • Title changed from “Explorer no firing window.load or dom:loaded” to “Explorer not firing window.load or dom:loaded”
  • javier

    javier October 6th, 2008 @ 05:35 PM

    I found this at 1.6.0.2, but i'm pretty sure it was fixed on 1.6.0.3

  • Nick Stakenburg

    Nick Stakenburg October 6th, 2008 @ 06:54 PM

    I'm not sure why the 1.6.0.2 approach that used doScroll didn't make it into 1.6.0.3 but it fixed problems like this and other issues such as "Operation aborted" errors. I'll attach a patch with that approach.

    Here's the mailing list discussion that let to it, http://groups.google.com/group/p...

  • Diego Perini

    Diego Perini December 2nd, 2008 @ 02:23 AM

    Nick, I don't know what happened either, I am sure I did my best to push that patch in Prototype but I never got real feedback and I didn't wanted to insist trying to walk over somebody else.

    Anyway, glad you took it again...hope it goes this time.

    I suggest you go a bit back when the "isCSSLoaded" was not there yet. There are bugs in how I implemented that CSS test on Opera initially and it showed up that moving stylesheets before the scripts in the source was enough to solve in Safari.

    I will have to review that part in Opera and see if it is still worth to include. The most important part is the IE part, from what we see here the real problem (blocker) is only on IE.

    -- Diego

  • Tobie Langel

    Tobie Langel December 2nd, 2008 @ 04:50 AM

    Diego, Nick, we definitely want this in Prototype asap, unfortunately, we had to revert some commits to ship a stable version of 1.6.0.3. And that meant that some otherwise valuable changes didn't make it in.

    I think the consensus was to separate this event into two dom:loaded and css:loaded. Let us know what you think.

  • Nick Stakenburg

    Nick Stakenburg December 2nd, 2008 @ 01:08 PM

    Tobie, Diego, here's the patch without isCssLoaded so that part can be added separately as css:loaded.

    Without the patch IE Unit tests fail on testDocumentContentLoadedEventFiresBeforeWindowLoad.

  • Tobie Langel

    Tobie Langel December 2nd, 2008 @ 01:18 PM

    Could you possibly reformat the above without using arguments.callee (for ES 3.1 and Caja compliance). Thanks!

  • Nick Stakenburg
  • Diego Perini

    Diego Perini December 3rd, 2008 @ 01:36 AM

    Nick, I did a quick visual review of the code and seems good to me.

    Good you have taken the work, I imagine you have good use for it, I will leave a couple of notes that may be of help. Our only objective here is IE.

    I see you have a test for this in your unit, it will make things easier for people taking it deep in future.

    Keeping separate "dom:loaded" and "css:loaded" patches is a good thing, I hope both will go away one day, don't know which one will be partied first though. :-)

    I am sure you already know how to check IE and what are the failing test cases, but if you go through this again I suggest you put a couple of console.log() lines in each of the "doScroll()" and "onreadystatechange" methods to see in which occasions each of the two method is invoked.

    There you will see difference between cached and first visited pages and occasionally between reloads of cached pages even through the Back/Forward and Go buttons or F5, ALT+F5, and Refresh from context menu or ALT+ArrowLeft and ALT+ArrowRight.

    Bring with me if you find some of the information is repeated.

    Will do some test too as soon as time permits and will leave "css:loaded" for later, overlaying problems is to avoid now.

    -- Diego

  • Tobie Langel

    Tobie Langel December 3rd, 2008 @ 02:16 AM

    Nick, I just noticed that you have a function declaration inside an if...else block. That's not ecmascript-compliant and behaves differently depending on the implementation. It is best avoided.

    Other than that, LGTM.

  • Nick Stakenburg

    Nick Stakenburg December 3rd, 2008 @ 02:48 AM

    Diego, thanks for the pointers. I'll keep them in mind the next time I go over this. Right now it seems pretty solid. The unit test it's fixing on IE is available in Prototype.

    Tobie, thanks I wasn't aware of that, changed it just in case.

  • Tobie Langel

    Tobie Langel December 3rd, 2008 @ 11:55 AM

    • Assigned user set to “Tobie Langel”
  • Diego Perini

    Diego Perini December 3rd, 2008 @ 12:10 PM

    • Assigned user cleared.

    Nick, don't want to seem picky but since we are looking at all these compatibility issues I have a couple of things to add:

    • seems to me a check for "document.attachEvent" is also good
    • we could use setTimeout instead of setInterval, seems better
    • remove the branch for Safari < 3.1 we have an onload fall back

    I would hear what you and others think about the above points.

    -- Diego

  • Tobie Langel

    Tobie Langel December 4th, 2008 @ 01:44 AM

    • Assigned user set to “Tobie Langel”

    Being picky is good :)

  • Nick Stakenburg

    Nick Stakenburg December 8th, 2008 @ 09:15 PM

    Diego, I'm okay with removing the Safari < 3.1 code but left it in since I figured someone pushed it into the deprecated 1.6.0.3 release for a reason. A check for document.attachEvent isn't required. Why does setTimeout seem better?

  • Diego Perini

    Diego Perini December 9th, 2008 @ 04:40 PM

    Nick, didn't want to force anything just underlining my own thoughts here.

    I believe checking for every method (especially naif ones) before using it is a good approach, not that I expect MS to remove this soon...it is in IE8.

    By not checking this we are supposing all browsers have one or the other methods, on the desktop that may be nearly true (lynx supports javascript but not events), on other devices it may be much different.

    For the setTimout, I believe Bobby Vandersluis of "swfobject" wrote the reasons I agree with in the most concise and clear way:

    http://code.google.com/p/swfobje...

    but again feel free to do your tests, in that #issue report you can probably find other interesting bits.

    -- Diego

  • Diego Perini

    Diego Perini December 9th, 2008 @ 04:58 PM

    Nick, I meant to say ELinks/Links2 supports some javascript and I did mention those because was the first I came to think, not that it should really matter to support those text browsers...

    -- Diego

  • Nick Stakenburg

    Nick Stakenburg December 10th, 2008 @ 03:20 AM

    Diego, thanks for the info, seems like a good idea to use setTimeout.

    The current dom:loaded polls for document.readyState on WebKit so I think it's best to leave the extra check in and remove it once Safari < 3.1 is no longer supported. Otherwise dom:loaded on WebKit wouldn't stay the same.

    Leaving out the attachEvent check should be okay since Event.observe also doesn't check for it. I'll use document.observe instead of document.attachEvent so this should no longer concern dom:loaded.

    Here's the patch using setTimeout as close as I can probably take it to the existing dom:loaded.

  • Nick Stakenburg
  • Jesus Garcia

    Jesus Garcia December 11th, 2008 @ 03:27 AM

    I am having no luck patching prototype 1.6.0.3. Would someone be kind enough to post a pre-patched file?

    Thanks in advance.

  • Diego Perini

    Diego Perini December 11th, 2008 @ 04:02 PM

    Nick,

    Good you did the change to use "observe()/stopObserving".

    In reality DOM0 events will fire before "attachEvent()" but we should not care nor use DOM0 events since they can be used by user code or by other snippets added with Prototype.

    It is the best choice and to enforce that here is the same talk on ExtJS forum where I suggested the same:

    http://extjs.com/forum/showthrea...

    Maybe in a standalone snippet it could be used as a DOM0 inline "onreadystatechange" but in a framework like Prototype a different reasoning should be taken.

    The Safari part will have let us completely remove browser sniffing in this delicate part of code, and as far as I know in Safari 2 the "onload" event will fire before images are loaded anyway.

    So I doubt this is a problem, since until now (1.6.0.3) Safari < 3.1 would have gone through the fall back "onload" which for the above reason would have fired before images (correct or wrong).

    I don't think you can fix this behavior without removing the "onload" fall back for Safari. But as always I may not be able too see obvious reason to keep it.

    Would be nice to hear somebody having Safari 2.0 though, others too.

    Good work Nick,

    -- Diego

    P.S. In the docs it should be stated clearly that it is mandatory to put external stylesheets before scripts to achieve an useful "dom:loaded" cross browser...

  • Nick Stakenburg

    Nick Stakenburg December 16th, 2008 @ 03:57 PM

    • Tag changed from dom, ie, ie6, ie7, needs_patch, needs_tests to dom, dom:loaded, ie, ie6, ie7, patched, tested
    • Title changed from “Explorer not firing window.load or dom:loaded” to “Use doScroll approach for a more accurate dom:loaded event”

    @Diego, I'll take out the Safari < 3.1 specific code. If you are right about the older Safari versions that should be safe then.

    I'm not able to test on Safari 2, maybe someone can check if leaving out the extra code is okay? Can always put it back in.

    @Jesus, That probably has to do with recent changes on git.

    Here's an updated patch that'll work now that the rewrite branch merged in.

  • Jesus Garcia

    Jesus Garcia December 17th, 2008 @ 04:14 AM

    Nick,

    The new patch works great, thank you very much.

  • GitHub Robot

    GitHub Robot December 17th, 2008 @ 07:21 AM

    • State changed from “bug” to “resolved”

    (from [e9e8c7fbe51e6c8cdc3dbb17efe1cce7c805911e]) Switch to the "doScroll approach" for the dom:loaded custom event. [#127 state:resolved] http://github.com/sstephenson/pr...

  • Paul Egan

    Paul Egan January 8th, 2009 @ 06:05 AM

    Between the last patch posted here and the commit the (window == top) test was changed to (window === top)... is that ever true on IE?

  • Tobie Langel

    Tobie Langel January 9th, 2009 @ 08:47 AM

    @paulegan: good point. Have you tested it?

  • Paul Egan

    Paul Egan January 13th, 2009 @ 03:13 AM

    A few quick tests on IE6 & 7 suggest that it's never true, however window == top works as expected.

    
    	window == top;	# true
    	window === top;	# false
    	window.window === window.top;	# true
    
  • Diego Perini

    Diego Perini January 28th, 2009 @ 12:33 AM

    Tobie, I confirm what Paul Egan said above. This popped up lately in other places.

    So beware the test there should be:

    window == top

    or

    window == window.top

    on IE this is strictly necessary, see the original here:

    http://javascript.nwbox.com/Cont...

    The patch done by Nick seems to have that correctly typed.

    Have not checked how it currently is in master but the above is mandatory, else the doScroll() will be short-circuited and will always fall back to the "onreadystatechange" event, still good (before onload) but sometime late for the scope.

    Diego

  • skaurus

    skaurus February 9th, 2009 @ 08:18 PM

    Sorry, will new method fire correctly when page loaded in iframe with scrollbars?

  • Andrew Dupont

    Andrew Dupont February 11th, 2009 @ 09:33 AM

    • Assigned user changed from “Tobie Langel” to “Andrew Dupont”
    • State changed from “resolved” to “bug”

    Re-opening this and assigning it to me to make sure this gets fixed before 1.6.0.4.

  • GitHub Robot

    GitHub Robot February 18th, 2009 @ 05:41 PM

    • State changed from “bug” to “resolved”

    (from [d3189652a679c57c05ed63633ff57434298e033c]) Fixed incorrect usage of === operator. [#127 state:resolved] http://github.com/sstephenson/pr...

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 02:10 AM

    • Tag changed from dom, dom:loaded, ie, ie6, ie7, patched, tested to ie, ie6, ie7, patched, section:dom

    [not-tagged:"dom" tagged:"section:dom" bulk edit command]

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 04:05 AM

    • Tag changed from ie, ie6, ie7, patched, section:dom to patched, section:dom

    [not-tagged:"ie" not-tagged:"ie6" not-tagged:"ie7" not-tagged:"ie8" bulk edit command]

  • Paul McCulloch

    Paul McCulloch November 23rd, 2010 @ 04:14 PM

    • Importance changed from “” to “”

    IE intermitent occasional sometimes window.onload onload() fails doesn't run

    (Just an additonal comment to try & help people searching for this issue. I only found this page after I'd tracked down the issue to prototype.js!)

  • chenlina1

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

The Prototype JavaScript library.

Shared Ticket Bins

Referenced by

Pages