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

VERIFIED FIXED in mozilla9

Status

()

Core
DOM: Events
P1
major
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Alexander L. Slovesnik, Assigned: bz)

Tracking

({dev-doc-complete, regression, top100})

Trunk
mozilla9
dev-doc-complete, regression, top100
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox9+)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
Summary: Yandex maps doesn't load since Aug 25 → Yandex maps doesn't load
(Reporter)

Comment 1

6 years ago
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

6 years ago
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
(Reporter)

Updated

6 years ago
Component: General → DOM: Events
Product: Firefox → Core
QA Contact: general → events
(Reporter)

Updated

6 years ago
Blocks: 659350
(Reporter)

Comment 3

6 years ago
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)
tracking-firefox9: --- → ?
(Assignee)

Comment 4

6 years ago
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.
Assignee: nobody → bzbarsky
(Assignee)

Comment 5

6 years ago
Though given the cc list... maybe #1 is an option too.   ;)  mash, what do things look like on your end here?
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

6 years ago
Created attachment 556750 [details] [diff] [review]
A simple implementation of option 3
(Assignee)

Updated

6 years ago
Attachment #556749 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #556750 - Flags: feedback?(jonas)

Comment 9

6 years ago
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.
(Assignee)

Comment 10

6 years ago
> 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

6 years ago
See also https://bugzilla.mozilla.org/show_bug.cgi?id=300992
(Assignee)

Comment 12

6 years ago
Yes, iirc the spec calls for that and also onreadystatechange on XHR and media elements... but nothing else.
(Assignee)

Comment 13

6 years ago
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.
I've updated the spec. I believe it's compatible with legacy content, but then I thought the onfoo* changes were too, so...
(Assignee)

Comment 15

6 years ago
Ian, could you please link me to the relevant spec section?
(Assignee)

Comment 16

6 years ago
Er, nevermind.  Links are in http://www.w3.org/Bugs/Public/show_bug.cgi?id=13965
(Assignee)

Comment 17

6 years ago
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.
Attachment #559034 - Flags: review?(jonas)
(Assignee)

Updated

6 years ago
Attachment #556750 - Attachment is obsolete: true
Attachment #556750 - Flags: feedback?(jonas)
(Assignee)

Updated

6 years ago
Priority: -- → P1
Whiteboard: [need review]
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?
Attachment #559034 - Flags: review?(jonas) → review?(Olli.Pettay)
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.
Attachment #559034 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 20

6 years ago
> Tests?

See comment 17.  Writing them right now.  ;)
(Assignee)

Comment 21

6 years ago
Created attachment 559309 [details] [diff] [review]
The test
Attachment #559309 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 22

6 years ago
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.
Attachment #559309 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 23

6 years ago
Created attachment 559658 [details] [diff] [review]
Move onreadystatechange to live on Document only.
Attachment #559658 - Flags: review?(Olli.Pettay)
Attachment #559658 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Updated

6 years ago
Blocks: 684671
(Assignee)

Comment 24

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

Sorry for the lag on landing this...
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla9
https://hg.mozilla.org/mozilla-central/rev/2dbed30dd67d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Comment 26

6 years ago
Verified fixed on Mozilla/5.0 (X11; Linux i686; rv:9.0a1) Gecko/20110920 Firefox/9.0a1 ID:20110920085517
Thanks a lot, Boris!
Status: RESOLVED → VERIFIED
Summary: Yandex maps doesn't load → Yandex maps doesn't load (onreadystatechange should be on the document only)
Keywords: dev-doc-needed
Tracking for Firefox Update 9 so if it gets re-opened we will notice.
tracking-firefox9: ? → +
The DOM event reference now says readystatechange is on document only, and this is mentioned on Firefox 9 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.