Last Comment Bug 682554 - Yandex maps doesn't load (onreadystatechange should be on the document only)
: Yandex maps doesn't load (onreadystatechange should be on the document only)
Status: VERIFIED FIXED
: dev-doc-complete, regression, top100
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: P1 major with 10 votes (vote)
: mozilla9
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
http://maps.yandex.ru/
Depends on:
Blocks: 659350 684671
  Show dependency treegraph
 
Reported: 2011-08-27 06:57 PDT by Alexander L. Slovesnik
Modified: 2011-12-19 07:43 PST (History)
19 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Fire a readystatechange event on script elements after firing load and implement a readyState getter on HTML script elements. (5.96 KB, patch)
2011-08-29 21:45 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
A simple implementation of option 3 (5.96 KB, patch)
2011-08-29 21:46 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
Implementation of the new spec (7.85 KB, patch)
2011-09-07 20:32 PDT, Boris Zbarsky [:bz]
bugs: review+
Details | Diff | Review
The test (4.60 KB, patch)
2011-09-08 15:42 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
Move onreadystatechange to live on Document only. (14.56 KB, patch)
2011-09-09 22:55 PDT, Boris Zbarsky [:bz]
bugs: review+
Details | Diff | Review

Description Alexander L. Slovesnik 2011-08-27 06:57:44 PDT
STR:
1) Open http://maps.yandex.ru/

Expected results:
Map loads

Actual results:
Map doesn't load

Last good revision:
20110824030813 - http://hg.mozilla.org/mozilla-central/rev/198c7de0699d

First bad revision:
20110825030823 - http://hg.mozilla.org/mozilla-central/rev/e58e98a89827

Regression window:
hg.mozilla.org/mozilla-central/pushloghtml?fromchange=198c7de0699d&tochange=e58e98a89827
Comment 1 Alexander L. Slovesnik 2011-08-27 07:11:22 PDT
Regression range for SeaMonkey builds:

Last good revision:
20110824003041 - http://hg.mozilla.org/mozilla-central/rev/5d9989c3bff6

First bad revision:
20110825003151 - http://hg.mozilla.org/mozilla-central/rev/c5b2cbf6e31b

Regression window:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5d9989c3bff6&tochange=c5b2cbf6e31b
Comment 2 Thomas Ahlblom 2011-08-27 10:04:25 PDT
Local track down using Linux x86_64:

The first bad revision is:
changeset:   75838:1f58f9ed470c
user:        Boris Zbarsky <bzbarsky@mit.edu>
date:        Wed Aug 24 15:49:25 2011 -0400
summary:     Bug 659350 part 5.  The guts of the change to move from storing inline event handlers on the JSObject to storing them in the event listener manager directly, so we can easily set/read them via IDL.

http://hg.mozilla.org/mozilla-central/rev/1f58f9ed470c
Comment 3 Alexander L. Slovesnik 2011-08-28 06:34:54 PDT
Setting tracking-firefox9 to ?. FWIW, http://maps.yandex.ru/ is very popular map service in Russia and part of Yandex.ru portal (#21 in Alexa - http://www.alexa.com/siteinfo/yandex.ru)
Comment 4 Boris Zbarsky [:bz] 2011-08-29 14:32:16 PDT
Looks like Yandex uses a dynamic script loader to load the map scripts.  When prettyprinted and some irrelevant stuff elided, that looks like this:

  (function (c, a, e) {
    var d = a.getElementsByTagName("head")[0],
    b = {};
    c.load = function (g, i, h) {
      var f = a.createElement("script");
      f.src = g;
      if (i) {
        if (f[e] === null) {
          f[e] = function () {
            var j = this.readyState;
            if (j === "loaded" || j === "complete") {
              f[e] = null;
              i(b[h])
            }
          }
        } else {
          f.onload = f.onerror = function () {
            f.onload = f.onerror = null;
            i(b[h])
          }
        }
      }
      d.insertBefore(f, d.lastChild)
    };
  }(require, document, "onreadystatechange"));

where |require| is the loader object, |g| is the script URI, |h| is some sort of closure object consumers can pass in, |i| is the callback function to trigger when the script loads.

In any case, per the current HTML5 spec f[e] would be the .onreadystatechange of the <script> which would be null.  So we end up on the f[e] codepath instead of the f.onload codepath.  But we don't actually fire readystatechange events on <script> (also per spec), so |i| never gets invoked.

Apparently, IE6-8 don't fire onload on scripts, which is why the hoops are being jumped through.

The obvious options here are:

1) Evangelize yandex to detect IE more stringently (which is what RequireJS, say, does). 
   The problem with that is that the above seems like perfectly reasonable capability
   detection to me.
2) Change the spec so that various on* event handlers for events that will never fire on
   an element are not present on that element.  This would make the above sort of
   capability detection actually useful.
3) Implement support for readyState on <script> and fire at least some readystatechange
   events for <script> (at least the one indicating the script is done loading).  This
   would match IE and Opera, at first glance.  This, too, needs a spec change.

I'm going to file a bug on the spec here to decide between 2 & 3, I guess, and in the meantime start working on a patch to do #3.  Doing #2 would be a lot more work from where our code is right now (though doing it for just onreadystatechange would not be too bad; it needs to be on Document and MediaElement but not elsewhere, necessarily).  Doing #1 seems like punishing yandex.  #3 seems like the simplest path forward that doesn't involve the sudden outbreak of sanity of #2.
Comment 5 Boris Zbarsky [:bz] 2011-08-29 14:33:09 PDT
Though given the cc list... maybe #1 is an option too.   ;)  mash, what do things look like on your end here?
Comment 6 Boris Zbarsky [:bz] 2011-08-29 14:43:16 PDT
http://www.w3.org/Bugs/Public/show_bug.cgi?id=13965 filed.

As I pointed out in that bug, doing both #2 and #3 might be the best way forward.
Comment 7 Boris Zbarsky [:bz] 2011-08-29 21:45:41 PDT
Created attachment 556749 [details] [diff] [review]
Fire a readystatechange event on script elements after firing load and implement a readyState getter on HTML script elements.
Comment 8 Boris Zbarsky [:bz] 2011-08-29 21:46:02 PDT
Created attachment 556750 [details] [diff] [review]
A simple implementation of option 3
Comment 9 Azat Razetdinov 2011-08-30 04:03:30 PDT
Hi, I’m the author of the above code from Yandex. There had been a classical shared callback for both onload on onreadystatechange before a bug in Opera 9.6 was found. The browser fired onreadystatechange with readyState=loaded, but the script’s content wasn’t available at that moment. So we were forced to limit onreadystatechange usage to IE only. Luckily, onreadystatechange equals to null in IE, and to undefied in Opera.

See http://stackoverflow.com/questions/774752/dynamic-script-loading-synchronization

Although we do not support Opera 9.6 anymore, it seems counter-intuitive to return null for onreadystatechange without fully supporting the event. I would consider #2 and #3 options.
Comment 10 Boris Zbarsky [:bz] 2011-08-30 06:42:46 PDT
> Luckily, onreadystatechange equals to null in IE

The problem is that the HTML5 spec also requires it to be null.  I suggest that you comment in http://www.w3.org/Bugs/Public/show_bug.cgi?id=13965 as needed...
Comment 11 Azat Razetdinov 2011-08-30 07:29:01 PDT
See also https://bugzilla.mozilla.org/show_bug.cgi?id=300992
Comment 12 Boris Zbarsky [:bz] 2011-09-02 22:21:12 PDT
Yes, iirc the spec calls for that and also onreadystatechange on XHR and media elements... but nothing else.
Comment 13 Boris Zbarsky [:bz] 2011-09-02 22:33:30 PDT
Kyle says in http://www.w3.org/Bugs/Public/show_bug.cgi?id=13965#c5 that the above patch would break LabJS.  So what we probably want instead is the Opera behavior here, not the IE one.  Assuming we go there at all, which I'm less and less convinced of.
Comment 14 Hixie (not reading bugmail) 2011-09-07 15:41:22 PDT
I've updated the spec. I believe it's compatible with legacy content, but then I thought the onfoo* changes were too, so...
Comment 15 Boris Zbarsky [:bz] 2011-09-07 17:25:47 PDT
Ian, could you please link me to the relevant spec section?
Comment 16 Boris Zbarsky [:bz] 2011-09-07 17:52:49 PDT
Er, nevermind.  Links are in http://www.w3.org/Bugs/Public/show_bug.cgi?id=13965
Comment 17 Boris Zbarsky [:bz] 2011-09-07 20:32:52 PDT
Created attachment 559034 [details] [diff] [review]
Implementation of the new spec

I'll write some tests tomorrow too, but in the meantime I figured I'd get the review ball rolling.
Comment 18 Jonas Sicking (:sicking) 2011-09-07 22:33:22 PDT
Comment on attachment 559034 [details] [diff] [review]
Implementation of the new spec

I won't have time to review this anytime soon. Smaug, would you be able to do the honors?
Comment 19 Olli Pettay [:smaug] 2011-09-08 02:20:26 PDT
Comment on attachment 559034 [details] [diff] [review]
Implementation of the new spec

Tests?

So we don't support all the states, but the spec allows that.


r=me if there will be some tests.
Comment 20 Boris Zbarsky [:bz] 2011-09-08 07:21:38 PDT
> Tests?

See comment 17.  Writing them right now.  ;)
Comment 21 Boris Zbarsky [:bz] 2011-09-08 15:42:47 PDT
Created attachment 559309 [details] [diff] [review]
The test
Comment 22 Boris Zbarsky [:bz] 2011-09-09 21:44:37 PDT
Comment on attachment 559309 [details] [diff] [review]
The test

Opera says that doing this is not very web-compatible (which will be _great_ fun for IE).  Hixie has reverted the spec changes for this, and we're just going to put onreadystatechange on documents only.
Comment 23 Boris Zbarsky [:bz] 2011-09-09 22:55:36 PDT
Created attachment 559658 [details] [diff] [review]
Move onreadystatechange to live on Document only.
Comment 24 Boris Zbarsky [:bz] 2011-09-19 21:34:48 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dbed30dd67d

Sorry for the lag on landing this...
Comment 25 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-20 07:49:13 PDT
https://hg.mozilla.org/mozilla-central/rev/2dbed30dd67d
Comment 26 Alexander L. Slovesnik 2011-09-20 11:17:26 PDT
Verified fixed on Mozilla/5.0 (X11; Linux i686; rv:9.0a1) Gecko/20110920 Firefox/9.0a1 ID:20110920085517
Thanks a lot, Boris!
Comment 27 Christopher Blizzard (:blizzard) 2011-09-27 14:57:58 PDT
Tracking for Firefox Update 9 so if it gets re-opened we will notice.
Comment 28 Eric Shepherd [:sheppy] 2011-12-19 07:43:43 PST
The DOM event reference now says readystatechange is on document only, and this is mentioned on Firefox 9 for developers.

Note You need to log in before you can comment on or make changes to this bug.