Bug 76216

Summary: Focus events should use FocusEvent interface (instead of UIEvents) and expose relatedTarget property
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: Tools / TestsAssignee: Terry Anderson <tdanderson>
Status: RESOLVED FIXED    
Severity: Normal CC: 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
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 76214    
Attachments:
Description Flags
Layout test for 76216
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
WIP: to see bot results
none
another try
none
Patch
none
patch for landing none

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
DetailsFormatted DiffDiff
Patch (34.93 KB, patch)
2012-02-09 15:03 PST, Terry Anderson 		no flags
DetailsFormatted DiffDiff
Patch (45.62 KB, patch)
2012-02-13 15:19 PST, Terry Anderson 		no flags
DetailsFormatted DiffDiff
Patch (45.62 KB, patch)
2012-02-13 16:00 PST, Terry Anderson 		no flags
DetailsFormatted DiffDiff
Patch (57.46 KB, patch)
2012-02-14 11:22 PST, Terry Anderson 		no flags
DetailsFormatted DiffDiff
Patch (51.58 KB, patch)
2012-02-14 13:29 PST, Terry Anderson 		no flags
DetailsFormatted DiffDiff
Patch (51.58 KB, patch)
2012-02-15 07:06 PST, Terry Anderson 		no flags
DetailsFormatted DiffDiff
Patch (51.72 KB, patch)
2012-02-16 08:23 PST, Terry Anderson 		no flags
DetailsFormatted DiffDiff
Patch (38.07 KB, patch)
2013-01-29 01:31 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
Patch (51.83 KB, patch)
2013-01-29 01:55 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
Patch (52.15 KB, patch)
2013-01-29 06:17 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
Patch (31.15 KB, patch)
2013-02-03 22:02 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
Patch (33.37 KB, patch)
2013-02-03 22:15 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
WIP: to see bot results (33.24 KB, patch)
2013-02-04 17:40 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
another try (33.14 KB, patch)
2013-02-06 04:46 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
Patch (32.77 KB, patch)
2013-02-06 16:46 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
patch for landing (32.76 KB, patch)
2013-02-06 17:03 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
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.