Bug 42580 - DOMFocusIn/DOMFocusOut return focusin/focusout Event.type
Summary: DOMFocusIn/DOMFocusOut return focusin/focusout Event.type
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords: InRadar
: 46949 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-07-19 11:40 PDT by Nate Chapin
Modified: 2010-10-01 13:59 PDT (History)
7 users (show)

See Also:


Attachments
patch + layout test (4.31 KB, patch)
2010-07-19 11:47 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
patch #2 (7.55 KB, patch)
2010-07-19 17:07 PDT, Nate Chapin
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
(hopefully) demo to illustrate difference between dispatching aliased events and two distinct events (693 bytes, text/html)
2010-09-30 21:25 PDT, Ryosuke Niwa
no flags Details
Patch (6.40 KB, patch)
2010-10-01 12:36 PDT, Enrica Casucci
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2010-07-19 11:40:21 PDT
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.
Comment 1 Nate Chapin 2010-07-19 11:47:41 PDT
Created attachment 61972 [details]
patch + layout test
Comment 2 WebKit Review Bot 2010-07-19 11:48:51 PDT
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.
Comment 3 Darin Adler 2010-07-19 11:52:10 PDT
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?
Comment 4 Nate Chapin 2010-07-19 11:59:27 PDT
(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.
Comment 5 Nate Chapin 2010-07-19 17:07:57 PDT
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.
Comment 6 Darin Adler 2010-07-19 17:47:18 PDT
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?
Comment 7 Nate Chapin 2010-07-20 09:26:30 PDT
(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).
Comment 8 Darin Adler 2010-07-20 09:28:15 PDT
Sam, Hyatt, can we just dump this "aliased event" concept?
Comment 9 Darin Adler 2010-08-29 12:35:59 PDT
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.
Comment 10 Darin Adler 2010-09-30 20:37:02 PDT
*** Bug 46949 has been marked as a duplicate of this bug. ***
Comment 11 Ryosuke Niwa 2010-09-30 21:25:29 PDT
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.
Comment 12 Darin Adler 2010-09-30 21:27:33 PDT
(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!
Comment 13 Ryosuke Niwa 2010-09-30 21:46:59 PDT
(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.
Comment 14 Enrica Casucci 2010-10-01 12:36:23 PDT
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.
Comment 15 Darin Adler 2010-10-01 12:40:35 PDT
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.
Comment 16 Ryosuke Niwa 2010-10-01 13:02:09 PDT
(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?
Comment 17 Enrica Casucci 2010-10-01 13:26:02 PDT
(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.
Comment 18 Enrica Casucci 2010-10-01 13:51:38 PDT
Committed revision 68923.
Comment 19 Ryosuke Niwa 2010-10-01 13:59:13 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > Will my test say undefined for both events with your patch?
> 
> Yes.

Great!