Closed Bug 689564 Opened 9 years ago Closed 9 years ago fails


(Core :: DOM: Events, defect)

9 Branch
Not set



Tracking Status
firefox9 + fixed


(Reporter: orofarne, Assigned: bzbarsky)




(4 keywords, Whiteboard: [qa!])


(1 file)

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

Actual results:

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

Expected results:

There is should be map.
Confirmed on
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110927 Firefox/9.0a1 ID:20110927030845

Regression window(m-c hourly),
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110824 Firefox/9.0a1 ID:20110824095659
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110825 Firefox/9.0a1 ID:20110825030823

Regression window(m-i hourly),
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110823 Firefox/9.0a1 ID:20110824124236
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110824 Firefox/9.0a1 ID:20110824125354

Suspected bug:
Bug 659350 - don't rely on slots to hold event handlers, IDL-ify event handlers
Blocks: 659350
Component: General → DOM: Events
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
QA Contact: general → events
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.
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.
Also works if I use jquery 1.6.0.  There are no versions between 1.5.2 and 1.6.0.
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.....
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.  :(
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...
Assignee: nobody → 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))
        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.
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.
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?
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.
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.
(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.
  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.
And in particular, per spec, that attribute doesn't add any event listeners anywhere.
Per spec that listener is added to window, if I read the spec correctly.
> Per spec that listener is added to window, if I read the spec correctly.

You're right.  I was misreading the spec.
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.
We should raise the issue of clicks outside the body as a spec issue....
Attachment #563071 - Flags: review?(Olli.Pettay)
Whiteboard: [need review]
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.
Attachment #563071 - Flags: review?(Olli.Pettay) → review+
Er, yes.  I meant to do that.  Will do.
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla10
Comment on attachment 563071 [details] [diff] [review]
Proposed fix, aligns with spec

Requesting aurora approval for this regression fix.
Attachment #563071 - Flags: approval-mozilla-aurora?
Closed: 9 years ago
Resolution: --- → FIXED
OS: Linux → All
Hardware: x86_64 → All
We want this to bake a little more on central before we approve. Leaving the request for now.
Comment on attachment 563071 [details] [diff] [review]
Proposed fix, aligns with spec

Approved, please land on aurora
Attachment #563071 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 696020
Whiteboard: [qa+]
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
Whiteboard: [qa+] → [qa!]
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.
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,-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.
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...
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.
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.

I've also mentioned this on Firefox 9 for developers.
You need to log in before you can comment on or make changes to this bug.