Bug 42580 – DOMFocusIn/DOMFocusOut return focusin/focusout Event.type

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

Original report: http://code.google.com/p/chromium/issues/detail?id=42631

Because of the way we alias focusin/DOMFocusIn and focusout/DOMFocusOut to each other, DOMFocusIn and DOMFocusOut events return the wrong type attribute to script.

[Nate Chapin]

Created attachment 61972 [details]
patch + layout test

[WebKit Review Bot]

Attachment 61972 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/dom/Event.h:81:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 61972 [details]
patch + layout test

This seems a bit too clever to me. The kind of cleverness that often leads to unhappiness later. There are a couple things I don't understand:

    1) How does calling switchToAliasedType a second time return the type to the original? I don't see any code in that function that will do it.

    2) When calling these listeners, what's the guarantee that some other code doesn't already have a pointer to the event? The listener could call any code at all, and you could actually observe the event object in both states. Seems unnecessary.

How about having two separate event objects instead? Do we really need to mutate a single event for this?

[Nate Chapin]

(In reply to comment #3)
> (From update of attachment 61972 [details])
> This seems a bit too clever to me. The kind of cleverness that often leads to unhappiness later. There are a couple things I don't understand:
> 
>     1) How does calling switchToAliasedType a second time return the type to the original? I don't see any code in that function that will do it.

It's dependent on an existing assumption (that only 2 events can be aliased to each other, and that there's no such thing as a one-way alias).  With the existing aliasedType relationships, calling switchToAliasedType() a second time will always reverse the first call.  If that's safe to leave in, it should probably at least be commented.

> 
>     2) When calling these listeners, what's the guarantee that some other code doesn't already have a pointer to the event? The listener could call any code at all, and you could actually observe the event object in both states. Seems unnecessary.
> 
> How about having two separate event objects instead? Do we really need to mutate a single event for this?

This could work, but it would require cleverness of a different flavor to ensure that we don't end up generating incorrect events (or in an endless loop of aliased events).  I'm happy to move to that solution if you feel it's more sane than this one.

[Nate Chapin]

Created attachment 62014 [details]
patch #2

This patch creates an entirely new Event for the aliased event and ensures that an aliased event will be created exactly once per original event.

Note that this reverses the order of the events (aliased first, then original).  I don't think that's an issue, but I can do some more research on the topic if that would be helpful.

[Darin Adler]

Comment on attachment 62014 [details]
patch #2

>          const AtomicString& aliasedType() const;
>          bool hasAliasedType() const;
> +        virtual void dispatchAliasedEvent() { }

I missed when all this aliased event machinery was added to Event. I don't understand why this is being baked in at such a low level.

It seems to me that we could simply dispatch both pairs of events in Document::setFocusedNode and get exactly the same results.

What are the test cases that aliased events support that can't be done by simply dispatching two different events one after another?

[Nate Chapin]

(In reply to comment #6)
> (From update of attachment 62014 [details])
> >          const AtomicString& aliasedType() const;
> >          bool hasAliasedType() const;
> > +        virtual void dispatchAliasedEvent() { }
> 
> I missed when all this aliased event machinery was added to Event. I don't understand why this is being baked in at such a low level.
> 
> It seems to me that we could simply dispatch both pairs of events in Document::setFocusedNode and get exactly the same results.
> 
> What are the test cases that aliased events support that can't be done by simply dispatching two different events one after another?

I don't know, but it appears it was introduced in http://trac.webkit.org/changeset/56402. (cc:ing hyatt and weinig, who wrote and reviewed it).

[Darin Adler]

Sam, Hyatt, can we just dump this "aliased event" concept?

[Darin Adler]

Comment on attachment 62014 [details]
patch #2

Instead of tis patch, we should fix this by getting rid of the aliased event concept entirely. That should be relatively straightforward.

[Darin Adler]

*** Bug 46949 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

Created attachment 69419 [details]
(hopefully) demo to illustrate difference between dispatching aliased events and two distinct events

(In reply to comment #6)
> I missed when all this aliased event machinery was added to Event. I don't understand why this is being baked in at such a low level.
> It seems to me that we could simply dispatch both pairs of events in Document::setFocusedNode and get exactly the same results.

That sounds like the simplest solution to this problem.  I'm curious as to know why we're reusing the same event.

> What are the test cases that aliased events support that can't be done by simply dispatching two different events one after another?

What if JavaScript added some information to the event object?  If we dispatched two different events, won't we get two different JS objects as well?  I wanted to see what happens in other browsers but no other browser seems to implement FocusIn so I can't even tell whether or not my test is correct.  The question (if my assumption is right)then becomes whether or not JS event listeners should see the same event object.

[Darin Adler]

(In reply to comment #11)
> What if JavaScript added some information to the event object?  If we dispatched two different events, won't we get two different JS objects as well?  I wanted to see what happens in other browsers but no other browser seems to implement FocusIn so I can't even tell whether or not my test is correct.  The question (if my assumption is right)then becomes whether or not JS event listeners should see the same event object.

It seems quite clear to me that two events of different types are not the same event. I can’t imagine someone making code that would depend on them being the same event object!

[Ryosuke Niwa]

(In reply to comment #12)
> (In reply to comment #11)
> > What if JavaScript added some information to the event object?  If we dispatched two different events, won't we get two different JS objects as well?  I wanted to see what happens in other browsers but no other browser seems to implement FocusIn so I can't even tell whether or not my test is correct.  The question (if my assumption is right)then becomes whether or not JS event listeners should see the same event object.
> 
> It seems quite clear to me that two events of different types are not the same event. I can’t imagine someone making code that would depend on them being the same event object!

That's my intuition as well.  And I can't find any remarks about sharing the same objects for focusIn and DOMFocusIn in http://www.w3.org/TR/DOM-Level-3-Events/#event-type-DOMFocusIn either.  So I think you're right that we shouldn't be reusing the same event object for DOMFocusIn and FocusIn and likewise for DOMFocusOut and FocusOut.

[Enrica Casucci]

Created attachment 69503 [details]
Patch

As discussed with Dave, Darin and Dan, this patch removes the aliasedType concept and fires 2 events, one with the new name and one with the old name.

[Darin Adler]

Comment on attachment 69503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69503&action=review

> WebCore/dom/Document.cpp:3108
> +        // FIXME: We should remove firing the old event in the future.

I’m not sure “the old event” and “in the future” are clear enough. A better comment would be more specific that DOMFocusOut is “the old event” and use wording that talks specifically about when this would be appropriate. For example we could say, “when we are sure no content depends on it”, and perhaps even mention the Mail issue or point to a bug number that mentions that. We want the FIXME to be actionable.

> LayoutTests/fast/events/focusinout.html:22
> +        writeFailed('result1', "Wrong even type");

Typo: "even type" instead of "event type". Same typo in all three places.

[Ryosuke Niwa]

(In reply to comment #14)
> Created an attachment (id=69503) [details]
> Patch
> 
> As discussed with Dave, Darin and Dan, this patch removes the aliasedType concept and fires 2 events, one with the new name and one with the old name.

Will my test say undefined for both events with your patch?

[Enrica Casucci]

(In reply to comment #16)
> (In reply to comment #14)
> > Created an attachment (id=69503) [details] [details]
> > Patch
> > 
> > As discussed with Dave, Darin and Dan, this patch removes the aliasedType concept and fires 2 events, one with the new name and one with the old name.
> 
> Will my test say undefined for both events with your patch?

Yes.

[Enrica Casucci]

Committed revision 68923.

[Ryosuke Niwa]

(In reply to comment #17)
> (In reply to comment #16)
> > Will my test say undefined for both events with your patch?
> 
> Yes.

Great!