Last Comment Bug 689564 - n.maps.yandex.ru fails
: n.maps.yandex.ru fails
Status: VERIFIED FIXED
[qa!]
: dev-doc-complete, regression, verified-aurora, verified-beta
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: 9 Branch
: All All
: -- normal (vote)
: mozilla10
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
http://n.maps.yandex.ru/
Depends on: 696020
Blocks: 659350
  Show dependency treegraph
 
Reported: 2011-09-27 07:27 PDT by Maxim Dementyev
Modified: 2013-12-27 14:24 PST (History)
16 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Proposed fix, aligns with spec (11.94 KB, patch)
2011-09-28 07:55 PDT, Boris Zbarsky [:bz]
bugs: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Review

Description Maxim Dementyev 2011-09-27 07:27:24 PDT
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 2011-09-27 08:03:01 PDT
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 [:bz] 2011-09-27 08:53:43 PDT
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 [:bz] 2011-09-27 21:16:16 PDT
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 [:bz] 2011-09-27 21:16:52 PDT
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 [:bz] 2011-09-27 21:49:36 PDT
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 [:bz] 2011-09-27 22:01:48 PDT
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 [:bz] 2011-09-27 22:52:33 PDT
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 [:bz] 2011-09-27 23:30:51 PDT
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 [:bz] 2011-09-27 23:52:34 PDT
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 [:bz] 2011-09-27 23:55:33 PDT
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 [:bz] 2011-09-28 00:22:51 PDT
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 2011-09-28 00:26:12 PDT
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] 2011-09-28 00:54:47 PDT
(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 [:bz] 2011-09-28 00:57:34 PDT
  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 [:bz] 2011-09-28 00:58:38 PDT
And in particular, per spec, that attribute doesn't add any event listeners anywhere.
Comment 16 Olli Pettay [:smaug] 2011-09-28 01:32:17 PDT
Per spec that listener is added to window, if I read the spec correctly.
Comment 17 Boris Zbarsky [:bz] 2011-09-28 07:02:21 PDT
> 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 [:bz] 2011-09-28 07:13:16 PDT
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 [:bz] 2011-09-28 07:55:42 PDT
Created attachment 563071 [details] [diff] [review]
Proposed fix, aligns with spec

We should raise the issue of clicks outside the body as a spec issue....
Comment 20 Olli Pettay [:smaug] 2011-09-28 08:11:53 PDT
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 [:bz] 2011-09-28 08:14:44 PDT
Er, yes.  I meant to do that.  Will do.
Comment 23 Boris Zbarsky [:bz] 2011-09-28 08:57:10 PDT
Comment on attachment 563071 [details] [diff] [review]
Proposed fix, aligns with spec

Requesting aurora approval for this regression fix.
Comment 24 Michael Wu [:mwu] 2011-09-29 01:28:16 PDT
https://hg.mozilla.org/mozilla-central/rev/09d60465831c
Comment 25 christian 2011-09-29 14:39:49 PDT
We want this to bake a little more on central before we approve. Leaving the request for now.
Comment 26 christian 2011-10-04 14:58:15 PDT
Comment on attachment 563071 [details] [diff] [review]
Proposed fix, aligns with spec

Approved, please land on aurora
Comment 28 Mihaela Velimiroviciu (:mihaelav) 2011-11-22 01:47:10 PST
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] 2011-12-16 10:09:30 PST
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 [:bz] 2011-12-16 10:17:33 PST
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] 2011-12-16 15:15:32 PST
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 [:bz] 2011-12-16 17:02:27 PST
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] 2011-12-19 07:20:39 PST
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.

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