Bug 76216 - Focus events should use FocusEvent interface (instead of UIEvents) and expose relatedTarget property
Summary: Focus events should use FocusEvent interface (instead of UIEvents) and expose...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Terry Anderson
URL:
Keywords:
: 100785 (view as bug list)
Depends on:
Blocks: 76214
  Show dependency treegraph
 
Reported: 2012-01-12 14:46 PST by Eric Seidel (no email)
Modified: 2013-02-07 00:41 PST (History)
28 users (show)

See Also:


Attachments
Layout test for 76216 (2.78 KB, text/html)
2012-02-09 07:50 PST, Terry Anderson
no flags Details
Patch (34.61 KB, patch)
2012-02-09 14:45 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (34.93 KB, patch)
2012-02-09 15:03 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (45.62 KB, patch)
2012-02-13 15:19 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (45.62 KB, patch)
2012-02-13 16:00 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (57.46 KB, patch)
2012-02-14 11:22 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (51.58 KB, patch)
2012-02-14 13:29 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (51.58 KB, patch)
2012-02-15 07:06 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (51.72 KB, patch)
2012-02-16 08:23 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (38.07 KB, patch)
2013-01-29 01:31 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (51.83 KB, patch)
2013-01-29 01:55 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (52.15 KB, patch)
2013-01-29 06:17 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (31.15 KB, patch)
2013-02-03 22:02 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (33.37 KB, patch)
2013-02-03 22:15 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
WIP: to see bot results (33.24 KB, patch)
2013-02-04 17:40 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
another try (33.14 KB, patch)
2013-02-06 04:46 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (32.77 KB, patch)
2013-02-06 16:46 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for landing (32.76 KB, patch)
2013-02-06 17:03 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-01-12 14:46:39 PST
WebKit does not support DOM 3 Events FocusEvent

We dispatch blur/focus events, but they are not themselves FocusEvent objects (they're not even UIEvent objects), thus they don't have things like relatedTarget set on them.

http://www.w3.org/TR/DOM-Level-3-Events/#webidl-events-FocusEvent

I noticed this when investigating these IETC tests:
http://samples.msdn.microsoft.com/ietestcenter/domevents/focusin.relatedTarget.html
http://samples.msdn.microsoft.com/ietestcenter/domevents/focusout.relatedTarget.html
Comment 1 Terry Anderson 2012-01-19 13:30:43 PST
I will be taking a look at this
Comment 2 Eric Seidel (no email) 2012-01-24 21:49:03 PST
Great!  Let us know if we can help.
Comment 3 Terry Anderson 2012-02-03 15:25:33 PST
Running http://samples.msdn.microsoft.com/ietestcenter/domevents/focusin.relatedTarget.html on chromium linux: clicking on the button does not actually dispatch a focusin event, but tabbing to the button does.
Comment 4 Terry Anderson 2012-02-09 07:50:47 PST
Created attachment 126303 [details]
Layout test for 76216

Layout test to verify that relatedTarget is set appropriately when focusin and focusout events are dispatched by tabbing.
Comment 5 Terry Anderson 2012-02-09 14:45:13 PST
Created attachment 126378 [details]
Patch
Comment 6 Eric Seidel (no email) 2012-02-09 14:47:52 PST
Comment on attachment 126378 [details]
Patch

The change looks OK, but the ChangeLog needs work.  I assume your'e just moving the *Mediator classes?  Can you explain what all your doing?
Comment 7 Philippe Normand 2012-02-09 14:54:56 PST
Comment on attachment 126378 [details]
Patch

Attachment 126378 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11487448
Comment 8 Terry Anderson 2012-02-09 15:03:10 PST
Created attachment 126381 [details]
Patch
Comment 9 Eric Seidel (no email) 2012-02-09 15:06:10 PST
Comment on attachment 126381 [details]
Patch

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

So you made no logic changes besides adding the FocusEvent class?  Is that correct?

> Source/WebCore/ChangeLog:9
> +        Created a new FocusEvent class (extends UIEvent) with a relatedTarget attribute.  Moved
> +        the {Focus,Blur,FocusIn,FocusOut}EventDispatchMediator classes inside FocusEvent.  Now when
> +        focusin or focusout events are dispatched, a FocusEvent is created with the relatedTarget
> +        attribute set accordingly.

I probably would have done these two things in separate changes...

> Source/WebCore/dom/EventFactory.in:12
> +FocusEvents interfaceName=FocusEvent

FocusEvents?  I don't see this used.  Sounds like a separate change.
Comment 10 Early Warning System Bot 2012-02-09 15:51:25 PST
Comment on attachment 126381 [details]
Patch

Attachment 126381 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11485417
Comment 11 Gustavo Noronha (kov) 2012-02-09 16:07:39 PST
Comment on attachment 126381 [details]
Patch

Attachment 126381 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11487457
Comment 12 Gyuyoung Kim 2012-02-09 16:29:38 PST
Comment on attachment 126381 [details]
Patch

Attachment 126381 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11487470
Comment 13 WebKit Review Bot 2012-02-09 16:40:21 PST
Comment on attachment 126381 [details]
Patch

Attachment 126381 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11496118

New failing tests:
fast/dom/shadow/shadow-boundary-events.html
Comment 14 Terry Anderson 2012-02-13 15:19:10 PST
Created attachment 126847 [details]
Patch
Comment 15 Terry Anderson 2012-02-13 16:00:56 PST
Created attachment 126853 [details]
Patch
Comment 16 Terry Anderson 2012-02-14 11:22:07 PST
Created attachment 127003 [details]
Patch
Comment 17 Eric Seidel (no email) 2012-02-14 12:26:00 PST
Comment on attachment 127003 [details]
Patch

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

The diff is a bit broken, but otherwise looks OK.  Upload a new patch and we're good to go.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:996
> -		4123081B138C429700BCCFCA /* WebCore.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 93F19B1A08245E5A001E9ABC /* WebCore.framework */; };
> +		4123081B138C429700BCCFCA /* .framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 93F19B1A08245E5A001E9ABC /* .framework */; };

I think this is an error.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:7819
> -		417DA6D013734E02007C57FB /* libWebCoreTestSupport.dylib */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.dylib"; includeInIndex = 0; path = libWebCoreTestSupport.dylib; sourceTree = BUILT_PRODUCTS_DIR; };
> +		417DA6D013734E02007C57FB /* .dylib */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.dylib"; includeInIndex = 0; path = .dylib; sourceTree = BUILT_PRODUCTS_DIR; };
>  		417DA71B13735DFA007C57FB /* JSInternals.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSInternals.cpp; sourceTree = "<group>"; };

This too.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:10142
> -		93F19B1A08245E5A001E9ABC /* WebCore.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = WebCore.framework; sourceTree = BUILT_PRODUCTS_DIR; };
> +		93F19B1A08245E5A001E9ABC /* .framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = .framework; sourceTree = BUILT_PRODUCTS_DIR; };

Same error.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:24278
> +<<<<<<< HEAD
> +				B6D9D23514EABD260090D75E /* FocusEvent.h in Headers */,
> +				B6D9D27B14EAC0860090D75E /* JSFocusEvent.h in Headers */,
> +=======
>  				C598905714E9C28000E8D18B /* PasteboardStrategy.h in Headers */,
>  				C598905814E9C29900E8D18B /* PlatformPasteboard.h in Headers */,
> +>>>>>>> gclient

Merge confict.
Comment 18 Terry Anderson 2012-02-14 13:29:16 PST
Created attachment 127030 [details]
Patch
Comment 19 Eric Seidel (no email) 2012-02-14 13:32:06 PST
Comment on attachment 127030 [details]
Patch

LGTM.  Please let the EWS bots munch on it before landing.
Comment 20 WebKit Review Bot 2012-02-15 01:30:05 PST
Attachment 127003 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   a9730c4..2ce77d7  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 107794 = c59b9dab74417b86c7b5b60a4408ab3e5d1659a8
r107793 = 2ce77d76874b9ee77991fecb540696485733202a
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/platform/graphics/FontCache.cpp
107794 = c59b9dab74417b86c7b5b60a4408ab3e5d1659a8 already exists! Why are we refetching it?
 at /usr/lib/git-core/git-svn line 5210

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Terry Anderson 2012-02-15 07:06:17 PST
Created attachment 127181 [details]
Patch
Comment 22 WebKit Review Bot 2012-02-15 08:30:07 PST
Comment on attachment 127181 [details]
Patch

Rejecting attachment 127181 [details] from commit-queue.

tdanderson@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 23 WebKit Review Bot 2012-02-15 12:31:29 PST
Comment on attachment 127181 [details]
Patch

Rejecting attachment 127181 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
sing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 9
ntinue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/11532272
Comment 24 WebKit Review Bot 2012-02-15 13:34:32 PST
The commit-queue encountered the following flaky tests while processing attachment 127181 [details]:

perf/mouse-event.html bug 78736 (author: eae@chromium.org)
inspector/debugger/script-formatter-console.html bug 78737 (author: pfeldman@chromium.org)
The commit-queue is continuing to process your patch.
Comment 25 WebKit Review Bot 2012-02-15 17:51:05 PST
Comment on attachment 127181 [details]
Patch

Rejecting attachment 127181 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
kit-commit-queue/Source/WebKit/chromium/tools/win --revision 122122 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
43>At revision 122122.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/11527929
Comment 26 Terry Anderson 2012-02-16 08:23:46 PST
Created attachment 127383 [details]
Patch
Comment 27 Eric Seidel (no email) 2012-02-16 08:37:04 PST
Comment on attachment 127383 [details]
Patch

The cq got sick yesterday with the git.webkit.org outage.  The failures had nothing to do with your patch as far as I can tell.
Comment 28 WebKit Review Bot 2012-02-16 09:51:42 PST
Comment on attachment 127383 [details]
Patch

Clearing flags on attachment: 127383

Committed r107952: <http://trac.webkit.org/changeset/107952>
Comment 29 WebKit Review Bot 2012-02-16 09:51:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 30 Kentaro Hara 2013-01-24 22:20:18 PST
This change has been rolled out in r108034.

Do you have any plan to support FocusEvent in WebKit?

(I'm asking this just because I wanted to implement a constructor for FocusEvent.)
Comment 31 Terry Anderson 2013-01-25 09:50:49 PST
(In reply to comment #30)
> This change has been rolled out in r108034.
> 
> Do you have any plan to support FocusEvent in WebKit?
> 
> (I'm asking this just because I wanted to implement a constructor for FocusEvent.)

This was my starter bug from a year ago and to be honest I didn't realize it got rolled out until seeing your comment here!

If you want to take ownership of this issue and try to re-land this patch, please feel free to assign yourself to the bug.
Comment 32 Kentaro Hara 2013-01-29 01:31:01 PST
Created attachment 185197 [details]
Patch
Comment 33 Early Warning System Bot 2013-01-29 01:38:15 PST
Comment on attachment 185197 [details]
Patch

Attachment 185197 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16183073
Comment 34 Kentaro Hara 2013-01-29 01:55:21 PST
Created attachment 185203 [details]
Patch
Comment 35 WebKit Review Bot 2013-01-29 03:23:15 PST
Comment on attachment 185203 [details]
Patch

Attachment 185203 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16115145

New failing tests:
fast/events/related-target-focusevent.html
fast/forms/month-multiple-fields/month-multiple-fields-blur-and-focus-events.html
fast/forms/time-multiple-fields/time-multiple-fields-state-change-on-focus-or-blur.html
fast/forms/time-multiple-fields/time-multiple-fields-blur-and-focus-events.html
fast/forms/date-multiple-fields/date-multiple-fields-blur-and-focus-events.html
fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-blur-and-focus-events.html
fast/dom/shadow/shadow-boundary-events.html
fast/forms/week-multiple-fields/week-multiple-fields-blur-and-focus-events.html
Comment 36 WebKit Review Bot 2013-01-29 04:37:28 PST
Comment on attachment 185203 [details]
Patch

Attachment 185203 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16115169

New failing tests:
fast/events/related-target-focusevent.html
fast/forms/month-multiple-fields/month-multiple-fields-blur-and-focus-events.html
fast/forms/time-multiple-fields/time-multiple-fields-state-change-on-focus-or-blur.html
fast/forms/time-multiple-fields/time-multiple-fields-blur-and-focus-events.html
fast/forms/date-multiple-fields/date-multiple-fields-blur-and-focus-events.html
fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-blur-and-focus-events.html
fast/dom/shadow/shadow-boundary-events.html
fast/forms/week-multiple-fields/week-multiple-fields-blur-and-focus-events.html
Comment 37 Kentaro Hara 2013-01-29 06:17:08 PST
Created attachment 185233 [details]
Patch
Comment 38 Terry Anderson 2013-01-29 08:40:18 PST
(In reply to comment #37)
> Created an attachment (id=185233) [details]
> Patch

Be sure to keep an eye on the two chromium mac tests that failed for my patch: https://bugs.webkit.org/show_bug.cgi?id=78832. From looking at the logs, EWS didn't catch fast/dom/shadow/shadow-boundary-events.html.
Comment 39 WebKit Review Bot 2013-01-29 08:49:32 PST
Comment on attachment 185233 [details]
Patch

Attachment 185233 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16203186

New failing tests:
fast/forms/month-multiple-fields/month-multiple-fields-blur-and-focus-events.html
fast/forms/time-multiple-fields/time-multiple-fields-state-change-on-focus-or-blur.html
fast/forms/time-multiple-fields/time-multiple-fields-blur-and-focus-events.html
fast/forms/date-multiple-fields/date-multiple-fields-blur-and-focus-events.html
fast/dom/shadow/shadow-boundary-events.html
fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-blur-and-focus-events.html
fast/forms/week-multiple-fields/week-multiple-fields-blur-and-focus-events.html
Comment 40 Build Bot 2013-01-29 11:20:45 PST
Comment on attachment 185233 [details]
Patch

Attachment 185233 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16203237

New failing tests:
fast/events/related-target-focusevent.html
Comment 41 Build Bot 2013-01-29 13:23:20 PST
Comment on attachment 185233 [details]
Patch

Attachment 185233 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16201266

New failing tests:
fast/events/related-target-focusevent.html
Comment 42 Sam Weinig 2013-01-29 22:41:09 PST
Out of curiosity, why do we want to support this?  Is there a site that requires it?  Is it a good feature?
Comment 43 Kentaro Hara 2013-01-29 22:44:35 PST
(In reply to comment #42)
> Out of curiosity, why do we want to support this?  Is there a site that requires it?  Is it a good feature?

Terry, Eric: do you have any background context?
Comment 44 Eric Seidel (no email) 2013-01-29 22:50:58 PST
I filed this based on IE Test Center tests.  It's not at all clear to me that we want this.  Just because IE has it doesn't mean we should. :)
Comment 45 Eric Seidel (no email) 2013-01-29 22:52:50 PST
That said... it does kinda make sense that an Event would have its own class.  At least a DOM-level event generated by the browser. :)  My position was one of ambivalence.  I'm really neither for nor against w/o further research.
Comment 46 Kentaro Hara 2013-01-29 22:54:54 PST
Because it's in the spec is the only reason for me.
Comment 47 Eric Seidel (no email) 2013-01-29 22:56:22 PST
I just tested in Chrome and FF on my Mac and neither of them even seemed to dispatch the focus event for the button.  I'm not sure we send focus events for buttons on Mac?
Comment 48 Eric Seidel (no email) 2013-01-29 22:58:40 PST
The focusout test however, results in a UIEvent in Chrome, but nothing in Firefox.

It would appear that focus events are not universally supported in the first place, let alone with the spec'd prototype chain. :)
Comment 49 Eric Seidel (no email) 2013-01-29 23:01:17 PST
Again, I feel il-informed, and would love comment from one of the spec authors.  Or even what they believe their implementation status is.  It seems fine to wait on this bug until something actually needs it (which is inevitable now that one major browser supports these).

As noted from my testing above, it's possible that focus events don't even make sense on a system like Mac where buttons don't get focused from clicks!

Interestingly, testing fucus using tab (which can focus a button) doesn't dispatch the events in FF either, but again, does in Chrome, just with the "wrong" type.
Comment 50 Ryosuke Niwa 2013-01-29 23:18:15 PST
Adding focusin/focusout may introduce new security vulnerabilities if they can fire when focus/blur doesn't fire.

In general, these synchronous focus related events are evil and we should not try to support them.
Comment 51 Eric Seidel (no email) 2013-01-29 23:20:42 PST
I agree synchronous JS events are bad-news.  I should clarify that this bug is about what prototype chain we have on the event we send, whether or not we send it, btw. :)
Comment 52 Ryosuke Niwa 2013-01-29 23:21:19 PST
(In reply to comment #51)
> I agree synchronous JS events are bad-news.  I should clarify that this bug is about what prototype chain we have on the event we send, whether or not we send it, btw. :)

We should probably rename the bug title then?
Comment 53 Kentaro Hara 2013-01-29 23:24:59 PST
Comment on attachment 185233 [details]
Patch

Clearing r? for now, as the topic looks controversial and the patch breaks several tests.
Comment 54 Sam Weinig 2013-01-30 08:39:31 PST
Heh. Glad I asked.
Comment 55 Ojan Vafai 2013-01-30 10:57:59 PST
(In reply to comment #42)
> Out of curiosity, why do we want to support this?  Is there a site that requires it?  Is it a good feature?

It's a good feature and we absolutely should do it. Knowing which element is getting blurred during a focus event (and vice versa) is very useful and practically impossible to keep track of in JS code. 

I don't know of a site that requires it, but we get web developers asking for this all the time.

(In reply to comment #53)
> (From update of attachment 185233 [details])
> Clearing r? for now, as the topic looks controversial and the patch breaks several tests.

Lets try to get this in. There's no controversy. The working group discussed this and there was all-around support. Also, this is a dupe of bug 100785. The approach in bug 100785 seems much simpler. Is there a case that approach gets wrong?
Comment 56 Ryosuke Niwa 2013-01-30 12:04:26 PST
(In reply to comment #55)
> (In reply to comment #42)
> > Out of curiosity, why do we want to support this?  Is there a site that requires it?  Is it a good feature?
> 
> It's a good feature and we absolutely should do it. Knowing which element is getting blurred during a focus event (and vice versa) is very useful and practically impossible to keep track of in JS code. 
> 
> I don't know of a site that requires it, but we get web developers asking for this all the time.
> 
> (In reply to comment #53)
> > (From update of attachment 185233 [details] [details])
> > Clearing r? for now, as the topic looks controversial and the patch breaks several tests.
> 
> Lets try to get this in. There's no controversy. The working group discussed this and there was all-around support. Also, this is a dupe of bug 100785. The approach in bug 100785 seems much simpler. Is there a case that approach gets wrong?

Yeah, my concern was if we were adding new event or firing it often. It seems like we're not doing that, so using the right interface object seems like an obvious improvement.
Comment 57 Kentaro Hara 2013-01-31 22:29:58 PST
(In reply to comment #55)
> Lets try to get this in. There's no controversy. The working group discussed this and there was all-around support. Also, this is a dupe of bug 100785. The approach in bug 100785 seems much simpler. Is there a case that approach gets wrong?

The approach looks good but it looks like our approaches are the same. Tests about relatedTarget of focus/blur/focusin/focusout events are failing on both of our patches. Let me polish them up.
Comment 58 Kentaro Hara 2013-02-03 22:02:49 PST
Created attachment 186302 [details]
Patch

probably fixed all crashes
Comment 59 WebKit Review Bot 2013-02-03 22:13:23 PST
Comment on attachment 186302 [details]
Patch

Attachment 186302 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16369051
Comment 60 Early Warning System Bot 2013-02-03 22:15:12 PST
Comment on attachment 186302 [details]
Patch

Attachment 186302 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16366139
Comment 61 Kentaro Hara 2013-02-03 22:15:46 PST
Created attachment 186305 [details]
Patch
Comment 62 Eric Seidel (no email) 2013-02-03 22:24:57 PST
Comment on attachment 186305 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:19541
> +                                B6D9D27A14EAC0860090D75E /* JSFocusEvent.cpp */,
> +                                B6D9D27914EAC0860090D75E /* JSFocusEvent.h */,

Looks like hte indent is wrong.

> Source/WebCore/dom/FocusEvent.cpp:42
> +};

Extra ;

> Source/WebCore/dom/FocusEvent.h:54
> +    FocusEvent(const AtomicString& type, bool canBubble, bool cancelable, PassRefPtr<AbstractView>, int, PassRefPtr<EventTarget>);

What is int detail?  Is it ever used?
Comment 63 Kentaro Hara 2013-02-03 22:28:17 PST
Comment on attachment 186305 [details]
Patch

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

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:19541
>> +                                B6D9D27914EAC0860090D75E /* JSFocusEvent.h */,
> 
> Looks like hte indent is wrong.

Will fix.

>> Source/WebCore/dom/FocusEvent.h:54
>> +    FocusEvent(const AtomicString& type, bool canBubble, bool cancelable, PassRefPtr<AbstractView>, int, PassRefPtr<EventTarget>);
> 
> What is int detail?  Is it ever used?

detail is an attribute of UIEvent (https://dvcs.w3.org/hg/d4e/raw-file/tip/source_respec.htm#constructor-uievent). Regarding focusin/focusout events triggered inside WebCore, detail is always set to 0. (Please look at the diff of Node.cpp.)
Comment 64 Eric Seidel (no email) 2013-02-03 22:30:48 PST
I see, so detail is something that can be set from JS but not ever from our impl?
Comment 65 Kentaro Hara 2013-02-03 22:32:08 PST
(In reply to comment #64)
> I see, so detail is something that can be set from JS but not ever from our impl?

Yes, I think so.
Comment 66 Build Bot 2013-02-04 01:15:01 PST
Comment on attachment 186305 [details]
Patch

Attachment 186305 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16372095

New failing tests:
fast/events/related-target-focusevent.html
Comment 67 Build Bot 2013-02-04 01:23:39 PST
Comment on attachment 186305 [details]
Patch

Attachment 186305 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16371107

New failing tests:
fast/events/related-target-focusevent.html
Comment 68 WebKit Review Bot 2013-02-04 01:30:27 PST
Comment on attachment 186305 [details]
Patch

Attachment 186305 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16377015

New failing tests:
fast/dom/shadow/shadow-boundary-events.html
Comment 69 WebKit Review Bot 2013-02-04 03:00:21 PST
Comment on attachment 186305 [details]
Patch

Attachment 186305 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16368140

New failing tests:
fast/dom/shadow/shadow-boundary-events.html
Comment 70 Kentaro Hara 2013-02-04 17:40:53 PST
Created attachment 186509 [details]
WIP: to see bot results
Comment 71 WebKit Review Bot 2013-02-04 20:20:06 PST
Comment on attachment 186509 [details]
WIP: to see bot results

Attachment 186509 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16366568

New failing tests:
fast/dom/shadow/shadow-boundary-events.html
Comment 72 Kentaro Hara 2013-02-04 20:24:07 PST
(In reply to comment #71)
> (From update of attachment 186509 [details])
> Attachment 186509 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16366568
> 
> New failing tests:
> fast/dom/shadow/shadow-boundary-events.html

Now it looks like that this is the only failure. I don't understand why the test fails in the chromium-linux bot even though the test passes in my local linux release/debug environment. Any idea?
Comment 73 WebKit Review Bot 2013-02-04 21:14:28 PST
Comment on attachment 186509 [details]
WIP: to see bot results

Attachment 186509 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16375517

New failing tests:
fast/dom/shadow/shadow-boundary-events.html
Comment 74 Kentaro Hara 2013-02-05 17:53:27 PST
(In reply to comment #73)
> (From update of attachment 186509 [details])
> Attachment 186509 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16375517
> 
> New failing tests:
> fast/dom/shadow/shadow-boundary-events.html

Is there any way to retrieve the bot result? Given that the test is passing on my local chromium-release build, there is no hint for debugging...
Comment 75 Kentaro Hara 2013-02-06 04:46:24 PST
Created attachment 186829 [details]
another try
Comment 76 Eric Seidel (no email) 2013-02-06 12:49:55 PST
Comment on attachment 186829 [details]
another try

Lots of tabs in the XCode file. :(  I know XCode is a big pain in the ass to edit, sorry. :(
Comment 77 Christian Biesinger 2013-02-06 14:04:50 PST
*** Bug 100785 has been marked as a duplicate of this bug. ***
Comment 78 Christian Biesinger 2013-02-06 14:05:47 PST
Do you not need the event constructor attributes in the IDL file? Like what my patch had:

+[
+    ConstructorConditional=DOM4_EVENTS_CONSTRUCTOR,
+    ConstructorTemplate=Event
+]
+interface FocusEvent : UIEvent
+{
+    [InitializedByEventConstructor] readonly attribute EventTarget      relatedTarget;
+};
Comment 79 Kentaro Hara 2013-02-06 16:46:12 PST
Created attachment 186947 [details]
Patch
Comment 80 Kentaro Hara 2013-02-06 16:46:49 PST
(In reply to comment #76)
> (From update of attachment 186829 [details])
> Lots of tabs in the XCode file. :(  I know XCode is a big pain in the ass to edit, sorry. :(

eric: Thanks. Fixed the tab issue. Now bots are green.
Comment 81 Kentaro Hara 2013-02-06 16:47:47 PST
(In reply to comment #78)
> Do you not need the event constructor attributes in the IDL file? Like what my patch had:

Let me do it in a follow-up patch with test cases. I'll also upload a couple of follow-up patches for refactoring.
Comment 82 Eric Seidel (no email) 2013-02-06 16:58:00 PST
Comment on attachment 186947 [details]
Patch

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

This looks fantastic!

> Source/WebCore/dom/FocusEvent.cpp:42
> +};

Extra ;
Comment 83 Kentaro Hara 2013-02-06 17:03:10 PST
Created attachment 186951 [details]
patch for landing
Comment 84 Ojan Vafai 2013-02-06 17:04:14 PST
I think we should support these on focus/blur as well. See http://lists.w3.org/Archives/Public/www-dom/2012OctDec/0061.html for the www-dom discussion.

Doesn't need to block this patch, but it'd be a nice followup.
Comment 85 Kentaro Hara 2013-02-06 17:05:53 PST
(In reply to comment #84)
> I think we should support these on focus/blur as well. See http://lists.w3.org/Archives/Public/www-dom/2012OctDec/0061.html for the www-dom discussion.

Will do! (though I hit a bunch of crashes when I tried it:-)
Comment 86 Kentaro Hara 2013-02-07 00:31:41 PST
Committed r142072: <http://trac.webkit.org/changeset/142072>