Reactive-Extensions / RxJS-DOM

HTML DOM Bindings for the Reactive Extensions for JavaScript
http://reactivex.io
Other
437 stars 99 forks source link

Rx.DOM.ready() seems to not work. #106

Open newjam opened 8 years ago

newjam commented 8 years ago

according to the MDN documentation, when document.readyState === 'interactive' then the DOMContentLoaded has already fired.

The Rx.DOM.ready() observable checks to see if the readyState === 'complete', otherwise it listens for the DOMContentLoaded event. This seems incorrect to me, as the observable will be forever empty if the DOMContentLoaded event has already fired, and the readyState === 'interactive'.

sirbarrence commented 8 years ago

I've had the same problem. If I subscribe to Rx.DOM.ready() at just the wrong moment while readyState === "interactive", between readyState === "loading" and readyState === "complete", the subscription never sends a next or completion notification.

The WhatWG HTML Standard Section 12.2.6 "The end" details the order things are supposed to happen in. In short, the order is:

Therefore it is entirely possible that DOMContentLoaded has fired already but readyState is not yet "completed".

jQuery handles this case correctly by invoking the callback immediately (on next event loop iteration) if readyState is either "complete" or not "loading". Else it adds the DOMContentLoaded event listener.

In rx.dom.js, if I change:

if (root.document.readyState === 'complete') {

to

if (root.document.readyState === 'complete' || (root.document.readyState !== "loading" && !root.document.documentElement.doScroll )) {

then Rx.DOM.ready() works as expected.

Rx.DOM.ready() should handle this the same way jQuery.ready() does.

I'd submit a pull request for this, but there's currently no test for Rx.DOM.ready(). (The test link in the docs 404s.) I'm not sure how a test for this particular event should be done. Test with an actual page's / iframe's load events or completely fake the events like all the other event & event shortcut tests do?

andi1984 commented 8 years ago

Any update on that? Stumbled over the same issue today.