76216 – Focus events should use FocusEvent interface (instead of UIEvents) and expose relatedTarget property

RESOLVED FIXED 76216

Focus events should use FocusEvent interface (instead of UIEvents) and expose relatedTarget property

https://bugs.webkit.org/show_bug.cgi?id=76216

Summary Focus events should use FocusEvent interface (instead of UIEvents) and expose...

Eric Seidel (no email)

Reported 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

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
Show Obsolete (16) View All Add attachment proposed patch, testcase, etc.

Terry Anderson

Comment 1 2012-01-19 13:30:43 PST

I will be taking a look at this

Eric Seidel (no email)

Comment 2 2012-01-24 21:49:03 PST

Great! Let us know if we can help.

Terry Anderson

Comment 3 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.

Terry Anderson

Comment 4 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.

Terry Anderson

Comment 5 2012-02-09 14:45:13 PST

Created attachment 126378 [details] Patch

Eric Seidel (no email)

Comment 6 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?

Philippe Normand

Comment 7 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

Terry Anderson

Comment 8 2012-02-09 15:03:10 PST

Created attachment 126381 [details] Patch

Eric Seidel (no email)

Comment 9 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.

Early Warning System Bot

Comment 10 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

Gustavo Noronha (kov)

Comment 11 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

Gyuyoung Kim

Comment 12 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

WebKit Review Bot

Comment 13 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

Terry Anderson

Comment 14 2012-02-13 15:19:10 PST

Created attachment 126847 [details] Patch

Terry Anderson

Comment 15 2012-02-13 16:00:56 PST

Created attachment 126853 [details] Patch

Terry Anderson

Comment 16 2012-02-14 11:22:07 PST

Created attachment 127003 [details] Patch

Eric Seidel (no email)

Comment 17 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.

Terry Anderson

Comment 18 2012-02-14 13:29:16 PST

Created attachment 127030 [details] Patch

Eric Seidel (no email)

Comment 19 2012-02-14 13:32:06 PST

Comment on attachment 127030 [details] Patch LGTM. Please let the EWS bots munch on it before landing.

WebKit Review Bot

Comment 20 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.

Terry Anderson

Comment 21 2012-02-15 07:06:17 PST

Created attachment 127181 [details] Patch

WebKit Review Bot

Comment 22 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.

WebKit Review Bot

Comment 23 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

WebKit Review Bot

Comment 24 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.

WebKit Review Bot

Comment 25 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

Terry Anderson

Comment 26 2012-02-16 08:23:46 PST

Created attachment 127383 [details] Patch

Eric Seidel (no email)

Comment 27 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.

WebKit Review Bot

Comment 28 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>

WebKit Review Bot

Comment 29 2012-02-16 09:51:51 PST

All reviewed patches have been landed. Closing bug.

Kentaro Hara

Comment 30 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.)

Terry Anderson

Comment 31 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.

Kentaro Hara

Comment 32 2013-01-29 01:31:01 PST

Created attachment 185197 [details] Patch

Early Warning System Bot

Comment 33 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

Kentaro Hara

Comment 34 2013-01-29 01:55:21 PST

Created attachment 185203 [details] Patch

WebKit Review Bot

Comment 35 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

WebKit Review Bot

Comment 36 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

Kentaro Hara

Comment 37 2013-01-29 06:17:08 PST

Created attachment 185233 [details] Patch

Terry Anderson

Comment 38 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.

WebKit Review Bot

Comment 39 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

Build Bot

Comment 40 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

Build Bot

Comment 41 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

Sam Weinig

Comment 42 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?

Kentaro Hara

Comment 43 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?

Eric Seidel (no email)

Comment 44 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. :)

Eric Seidel (no email)

Comment 45 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.

Kentaro Hara

Comment 46 2013-01-29 22:54:54 PST

Because it's in the spec is the only reason for me.

Eric Seidel (no email)

Comment 47 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?

Eric Seidel (no email)

Comment 48 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. :)

Eric Seidel (no email)

Comment 49 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.

Ryosuke Niwa

Comment 50 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.

Eric Seidel (no email)

Comment 51 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. :)

Ryosuke Niwa

Comment 52 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?

Kentaro Hara

Comment 53 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.

Sam Weinig

Comment 54 2013-01-30 08:39:31 PST

Heh. Glad I asked.

Ojan Vafai

Comment 55 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?

Ryosuke Niwa

Comment 56 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.

Kentaro Hara

Comment 57 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.

Kentaro Hara

Comment 58 2013-02-03 22:02:49 PST

Created attachment 186302 [details] Patch probably fixed all crashes

WebKit Review Bot

Comment 59 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

Early Warning System Bot

Comment 60 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

Kentaro Hara

Comment 61 2013-02-03 22:15:46 PST

Created attachment 186305 [details] Patch

Eric Seidel (no email)

Comment 62 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?

Kentaro Hara

Comment 63 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.)

Eric Seidel (no email)

Comment 64 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?

Kentaro Hara

Comment 65 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.

Build Bot

Comment 66 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

Build Bot

Comment 67 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

WebKit Review Bot

Comment 68 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

WebKit Review Bot

Comment 69 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

Kentaro Hara

Comment 70 2013-02-04 17:40:53 PST

Created attachment 186509 [details] WIP: to see bot results

WebKit Review Bot

Comment 71 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

Kentaro Hara

Comment 72 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?

WebKit Review Bot

Comment 73 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

Kentaro Hara

Comment 74 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...

Kentaro Hara

Comment 75 2013-02-06 04:46:24 PST

Created attachment 186829 [details] another try

Eric Seidel (no email)

Comment 76 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. :(

Christian Biesinger

Comment 77 2013-02-06 14:04:50 PST

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

Christian Biesinger

Comment 78 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; +};

Kentaro Hara

Comment 79 2013-02-06 16:46:12 PST

Created attachment 186947 [details] Patch

Kentaro Hara

Comment 80 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.

Kentaro Hara

Comment 81 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.

Eric Seidel (no email)

Comment 82 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 ;

Kentaro Hara

Comment 83 2013-02-06 17:03:10 PST

Created attachment 186951 [details] patch for landing

Ojan Vafai

Comment 84 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.

Kentaro Hara

Comment 85 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:-)

Kentaro Hara

Comment 86 2013-02-07 00:31:41 PST

Committed r142072: <http://trac.webkit.org/changeset/142072>

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Tools / Tests

Assignee

Terry Anderson

Reported

2012-01-12 14:46 PST

Modified

2013-02-07 00:41 PST History

CC List

28 users Show

URL

Keywords

Duplicates (1)

100785 View as bug list

Depends on

Blocks

76214

Dependencies

tree graph

abarth
buildbot
cbiesinger
dglazkov
dominicc
eric.carlson
feature-media-reviews
fsamuel
gustavo
gyuyoung.kim
haraken
japhet
macpherson
menard
ojan.autocc
ojan
philn
rakuco
rjkroege
rniwa
sam
syoichi
tdanderson
tdresser
vestbo
webkit-ews
webkit.review.bot
xan.lopez