Bug 186266

Summary: Improve window.event compliance: Should not be set when target is in shadow tree
Product: WebKit Reporter: Anne van Kesteren <annevk>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, ews-watchlist, ggaren, koivisto, rniwa, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=233834
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews107 for mac-sierra-wk2 none

Description Anne van Kesteren 2018-06-04 04:55:18 PDT
See https://github.com/whatwg/dom/issues/334 for the discussion that led to this, https://github.com/whatwg/dom/pull/407 for the DOM Standard change, and https://github.com/web-platform-tests/wpt/pull/4790 & https://github.com/web-platform-tests/wpt/pull/10329/files for tests that Safari fails.
Comment 1 Chris Dumez 2018-07-03 15:22:46 PDT
Created attachment 344229 [details]
Patch
Comment 2 Ryosuke Niwa 2018-07-03 16:00:10 PDT
Comment on attachment 344229 [details]
Patch

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

> Source/WebCore/bindings/js/JSEventListener.cpp:163
> +        if (!isTargetInsideShadowTree)

It probably doesn't matter but maybe we should also avoid calling setCurrentEvent later when we restore the saved event to maintain the symmetry?
Comment 3 Chris Dumez 2018-07-03 16:04:54 PDT
Comment on attachment 344229 [details]
Patch

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

>> Source/WebCore/bindings/js/JSEventListener.cpp:163
>> +        if (!isTargetInsideShadowTree)
> 
> It probably doesn't matter but maybe we should also avoid calling setCurrentEvent later when we restore the saved event to maintain the symmetry?

I prefer it this way for a couple of reasons:
- Trying to avoid the setting / restore adds a little bit of code complexity / branching
- Setting the event is cheap
- The current behavior matches the spec exactly (https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke)

I am not opposed to doing it if you feel strongly about it though.
Comment 4 EWS Watchlist 2018-07-03 16:35:07 PDT
Comment on attachment 344229 [details]
Patch

Attachment 344229 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8429641

New failing tests:
accessibility/smart-invert-reference.html
Comment 5 EWS Watchlist 2018-07-03 16:35:09 PDT
Created attachment 344240 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 6 Chris Dumez 2018-07-03 16:44:56 PDT
Comment on attachment 344240 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

Failure does not look related
Comment 7 WebKit Commit Bot 2018-07-03 17:06:38 PDT
Comment on attachment 344229 [details]
Patch

Clearing flags on attachment: 344229

Committed r233489: <https://trac.webkit.org/changeset/233489>
Comment 8 WebKit Commit Bot 2018-07-03 17:06:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-07-03 17:07:31 PDT
<rdar://problem/41799255>