689564 - n.maps.yandex.ru fails

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Maxim Dementyev]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110926 Firefox/9.0a1
Build ID: 20110926030901

Steps to reproduce:

go to http://n.maps.yandex.ru/


Actual results:

nothing. "Loading..." and no more.


Expected results:

There is should be map.

Comment 1

[Alice0775 White]

Confirmed on
http://hg.mozilla.org/mozilla-central/rev/1f800c226837
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110927 Firefox/9.0a1 ID:20110927030845

Regression window(m-c hourly),
Works:
http://hg.mozilla.org/mozilla-central/rev/b354d9b3e9e1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110824 Firefox/9.0a1 ID:20110824095659
Fails:
http://hg.mozilla.org/mozilla-central/rev/e58e98a89827
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110825 Firefox/9.0a1 ID:20110825030823
Pushloh:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b354d9b3e9e1&tochange=e58e98a89827


Regression window(m-i hourly),
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7254c4f4a805
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110823 Firefox/9.0a1 ID:20110824124236
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/3fbd9eaf9deb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110824 Firefox/9.0a1 ID:20110824125354
Pushloh:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7254c4f4a805&tochange=3fbd9eaf9deb

Suspected bug:
Bug 659350 - don't rely on slots to hold event handlers, IDL-ify event handlers

Comment 2

[Boris Zbarsky [:bzbarsky]]

Azat, I'm looking into this on my end, but if you can give me any idea of what this code is doing and why it's failing, that would be really nice!

Requesting tracking for the regression.

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK, first interesting bit of info: if I replace jquery 1.5.2 with jquery 1.6.4 in the page, it seems to start working.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Also works if I use jquery 1.6.0.  There are no versions between 1.5.2 and 1.6.0.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So far all I can tell is that we end up trying to init the map _twice_ in the working build (both times under the same each() call, just in two different callback invocations) but only once in the non-working build.....

Comment 6

[Boris Zbarsky [:bzbarsky]]

More precisely, Ya.WM.App.init is reached exactly once in the working build and not at all in the non-working one.  But it's reached via an afterCurrentEvent() callback and it seems that the _second_ afterCurrentEvent call is the one that ends up reaching it.

That second call happens when doing an each() when the name of the current prop is "b-page".  The first one happens when the name is "b-wm-app".

Debugging this would be a lot simpler with unminified code for the Yandex stuff.  :(

Comment 7

[Boris Zbarsky [:bzbarsky]]

So we reach this code:

  BEM.DOM.decl("b-page", {
    onSetMod: {
        js: function () {
            Ya.WM.App.init(this.findBlockOn("b-wm-app"), this.params)
        }
    }
  });

in the wikimaps script in both cases.  But the above onSetMod.js doesn't seem to be called in the non-working build.

I did some digging, and that also ends up called from afterCurrentEvent.  So I really need to figure out where in the mess of minified foreach spaghetti things are going wrong there.  :(

Azat, I could really use some help from your end here...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Aha!  Finally some progress!

    function g(C) {
        var B = C.onclick || C.ondblclick;
	if (C.tagName.toLowerCase() == "body") alert(B);
        if (!B && C.tagName.toLowerCase() == "body") {
            var D = e(C),
                A = D.attr("onclick") || D.attr("ondblclick");
            A && (B = Function(A))
	    alert(D);
	    alert(A);
	    alert(B);
        }
        return B ? B() : {}
    }

I added the alerts.  In a working build, I see it alert undefined, then [object Object] then some long string, then a Function object.  In a non-working build, I get null, [object Obeject], null, null.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK, looks like D is just a jquery wrapper for the <body>.

And it has getAttribute("onclick") returning the string that ends up in 'A' in a working build.

So the key part here is that attr() in jQuery 1.5.2 looks like this (trimming out the irrelevant bits):

  if ( (name in elem || elem[ name ] !== undefined) && notxml && !special ) {
    return elem[name];
  }
  // Fall through to getAttribute

while jQuery 1.6 has a blacklist of things that shouldn't get the getAttribute treatment and does getAttribute by default.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I see what's going on here...  The problem is that the onclick attribute leads to an event handler being added to the _window_.  We do that for all attributes on <body>.  This does not actually match the HTML5 spec, sadly.

But the forwarding to window only happens for the on* getters and setters for particular event names, per spec.

So we do compile that attribute into a function, and stick it on the window.  But body.onclick returns null, not that function.

We need to either change the attribute forwarding to forward only that whitelist of attributes (not that hard, actually) or change body and frameset to forward all on* gets/sets to the window (also not that hard).  Or back out the event changes on aurora, of course....  This last would entail a noticeable DOM performance regression at this point, with type inference enabled.

Jonas, Olli, preferences for which approach to take here?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Oh, and my general take on the risks of those approaches....

Implementing what the spec says right now is a no-go in my opinion.  For example, it lists "pageshow" as an event that does not work as an onpageshow attribute on <body>, which is not what either we or WebKit do.  We certainly have tests depending on it (though not many); I would assume there is web content depending on it too.

So my gut feeling is that if forward _fewer_ content attributes to the window we should still forward everything that's in FORWARDED_EVENT and WINDOW_EVENT in the event list.  These are all the _idl_ attributes that get forwarded to the window so the two lists will be consistent.  I suspect we want to actually spec it that way, except for the view events that are WINDOW_EVENT but not a supported on* content attribute at all (in this case, only "message").  There is some risk in this approach, since it will attach fewer listeners to the window than we do now, of course.

The other option is to forward all on* sets on the body and frameset to the window, as well as all the relevant attributes.  This would, I believe, effectively keep our old behavior, but I do think we want to change that behavior eventually (e.g. other UAs don't have it).  So it would just be putting things off.  But it's probably a safer change.

Perhaps the right thing to do is to forward fewer attributes on m-c and to forward more on* property on aurora.... I'm not sure what I think of landing a change on aurora without it really baking on m-c, though.

Comment 12

[Azat Razetdinov]

Seems that I got too late here due to time zone difference :) Thank you for your investigation, Boris. If you need any further help, I’m all ears.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky (:bz) from comment #11)
> For
> example, it lists "pageshow" as an event that does not work as an onpageshow
> attribute on <body>
I'm not sure what this means.

Comment 14

[Boris Zbarsky [:bzbarsky]]

  data:text/html,<body onpageshow="alert('foo')">

shows an alert in both Gecko and WebKit.  Per spec, if I read it right, it should not.

Comment 15

[Boris Zbarsky [:bzbarsky]]

And in particular, per spec, that attribute doesn't add any event listeners anywhere.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Per spec that listener is added to window, if I read the spec correctly.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Per spec that listener is added to window, if I read the spec correctly.

You're right.  I was misreading the spec.

Comment 18

[Boris Zbarsky [:bzbarsky]]

One other note: in the old code we do NOT forward any on* DOM properties from body to window (but we do register the on* attribute handlers on window).  The only reason the yandex code worked is that attr() tested false for |"onclick" in body| and fell through to getAttribute.  This is also why they have the silly new Function dance.

Given that, and given that I had misunderstood what the spec was saying, smaug and I think that it would probably be safest to align to the spec here.  I'll post a patch for that (passing try and all) in a few minutes.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix, aligns with spec

We should raise the issue of clicks outside the body as a spec issue....

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 563071 [details] [diff] [review]
Proposed fix, aligns with spec

Could you add some tests which check that FORWARDED_EVENTs and
WINDOW_EVENTs are handled correctly.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Er, yes.  I meant to do that.  Will do.

Comment 22

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/09d60465831c

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 563071 [details] [diff] [review]
Proposed fix, aligns with spec

Requesting aurora approval for this regression fix.

Comment 24

[Michael Wu [:mwu]]

https://hg.mozilla.org/mozilla-central/rev/09d60465831c

Comment 25

[LegNeato]

We want this to bake a little more on central before we approve. Leaving the request for now.

Comment 26

[LegNeato]

Comment on attachment 563071 [details] [diff] [review]
Proposed fix, aligns with spec

Approved, please land on aurora

Comment 27

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6acb018b5a1b

Comment 28

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Windows NT 6.1; rv:11.0a1) Gecko/20111114 Firefox/11.0a1
Mozilla/5.0 (Windows NT 6.1; rv:10.0a2) Gecko/20111115 Firefox/10.0a2
Mozilla/5.0 (Windows NT 6.1; rv:9.0) Gecko/20100101 Firefox/9.0

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0a2) Gecko/20111121 Firefox/10.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0a1) Gecko/20111118 Firefox/11.0a1

Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a2) Gecko/20111117 Firefox/10.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:11.0a1) Gecko/20111118 Firefox/11.0a1

Mozilla/5.0 (Windows NT 5.1; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 5.1; rv:10.0a2) Gecko/20111121 Firefox/10.0a2
Mozilla/5.0 (Windows NT 5.1; rv:11.0a1) Gecko/20111121 Firefox/11.0a1

Verified as fixed on the above builds: map is displayed

Comment 29

[Eric Shepherd [:sheppy]]

What's the summary of what changed here?  There are references to aligning with the spec with no link to which spec/section is being referred to.

Comment 30

[Boris Zbarsky [:bzbarsky]]

The change that was made is that HTML that looks like this:

  <body onfoo="something">

only sets up an event listener on the window for some values of foo (the ones listed in the spec), not all values of foo.  The relevant spec section is http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#event-handlers-on-elements,-document-objects,-and-window-objects and in particular the table following the "The following are the event handlers (and their corresponding event handler event types) that must be supported by Window objects, as IDL attributes on the Window object, and with corresponding content attributes and IDL attributes exposed on the body and frameset elements" text.

Comment 31

[Eric Shepherd [:sheppy]]

Can someone explain to me why some events are in both the second and third tables? It seems to me these should be mutually exclusive lists...

Comment 32

[Boris Zbarsky [:bzbarsky]]

The second table is for all elements except body (and frameset; that needs to be added to the spec) and for Document.

The third table is for Window, body, and frameset.

The behaviors are slightly different; an onload property on a <script> registers an event handler for that <script>, while an onload property on a <body> registers an event handler on the Window.

Comment 33

[Eric Shepherd [:sheppy]]

OK, I think the docs are updated. There are some fine points that need to be clarified on the docs for specific events, but documenting those events in detail is outside the scope of this particular bug.

https://developer.mozilla.org/en/DOM/DOM_event_reference

I've also mentioned this on Firefox 9 for developers.