682554 - Yandex maps doesn't load (onreadystatechange should be on the document only)

Status: VERIFIED FIXED

(Core :: DOM: Events, defect, P1)

Description

[Alexander L. Slovesnik]

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]

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]

Local track down using Linux x86_64:

The first bad revision is:
changeset:   75838:1f58f9ed470c
user:        Boris Zbarsky <[e-mail]>
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]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

Attachment: Fire a readystatechange event on script elements after firing load and implement a readyState getter on HTML script elements.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: A simple implementation of option 3

Comment 9

[Azat Razetdinov]

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 [:bzbarsky]]

> 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]

See also https://bugzilla.mozilla.org/show_bug.cgi?id=300992

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yes, iirc the spec calls for that and also onreadystatechange on XHR and media elements... but nothing else.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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)]

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 [:bzbarsky]]

Ian, could you please link me to the relevant spec section?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Er, nevermind.  Links are in http://www.w3.org/Bugs/Public/show_bug.cgi?id=13965

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: 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) No longer reading bugmail consistently]

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][[e-mail]]]

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 [:bzbarsky]]

> Tests?

See comment 17.  Writing them right now.  ;)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: The test

Comment 22

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Attachment: Move onreadystatechange to live on Document only.

Comment 24

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2dbed30dd67d

Sorry for the lag on landing this...

Comment 25

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/2dbed30dd67d

Comment 26

[Alexander L. Slovesnik]

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)]

Tracking for Firefox Update 9 so if it gets re-opened we will notice.

Comment 28

[Eric Shepherd [:sheppy]]

The DOM event reference now says readystatechange is on document only, and this is mentioned on Firefox 9 for developers.