Bug 164609 - event.composedPath() does not include window
Summary: event.composedPath() does not include window
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 148695
  Show dependency treegraph
 
Reported: 2016-11-10 12:41 PST by Keanu Lee
Modified: 2016-11-11 21:06 PST (History)
10 users (show)

See Also:


Attachments
Fixes the bug (20.20 KB, patch)
2016-11-11 16:50 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.16 MB, application/zip)
2016-11-11 17:42 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-yosemite (929.39 KB, application/zip)
2016-11-11 18:02 PST, Build Bot
no flags Details
Rebaselined the test (20.48 KB, patch)
2016-11-11 18:06 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixes the bug (20.48 KB, patch)
2016-11-11 18:38 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (20.47 KB, patch)
2016-11-11 19:48 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keanu Lee 2016-11-10 12:41:23 PST
event.composedPath() should include the window object, as defined in the spec (https://dom.spec.whatwg.org/#dom-event-composedpath):

http://jsbin.com/gebebag/edit?html,output
Comment 1 Radar WebKit Bug Importer 2016-11-10 16:48:52 PST
<rdar://problem/29210383>
Comment 2 Ryosuke Niwa 2016-11-11 16:50:24 PST
Created attachment 294568 [details]
Fixes the bug
Comment 3 Build Bot 2016-11-11 17:42:30 PST
Comment on attachment 294568 [details]
Fixes the bug

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

New failing tests:
fast/shadow-dom/event-path-with-window.html
Comment 4 Build Bot 2016-11-11 17:42:35 PST
Created attachment 294572 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 5 Build Bot 2016-11-11 18:02:36 PST
Comment on attachment 294568 [details]
Fixes the bug

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

New failing tests:
fast/shadow-dom/event-path-with-window.html
Comment 6 Build Bot 2016-11-11 18:02:40 PST
Created attachment 294573 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 7 Ryosuke Niwa 2016-11-11 18:06:14 PST
Created attachment 294574 [details]
Rebaselined the test
Comment 8 Ryosuke Niwa 2016-11-11 18:38:10 PST
Created attachment 294579 [details]
Fixes the bug
Comment 9 Antti Koivisto 2016-11-11 19:15:43 PST
Comment on attachment 294579 [details]
Fixes the bug

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

> Source/WebCore/dom/EventPath.cpp:176
> +        if (!ambgiousContext.isMouseOrFocusEventContext())
> +            continue;
> +        auto& context = downcast<MouseOrFocusEventContext>(ambgiousContext);

is<MouseOrFocusEventContext>() pairs better with the cast than using a member test function.

> Source/WebCore/dom/EventPath.cpp:221
> +        if (context->isTouchEventContext()) {
> +            Node* currentRelatedNode = retargeter.currentNode(currentTarget);
> +            downcast<TouchEventContext>(*context).touchList(touchListType)->append(touch.cloneWithNewTarget(currentRelatedNode));

Same here with is<TouchEventContext>()

> Source/WebCore/dom/EventPath.cpp:267
> +        if (const DOMWindow* domWindow = const_cast<EventTarget&>(target).toDOMWindow()) {
> +            targetNode = domWindow->document();
> +            ASSERT(targetNode);
> +        } else
> +            return path;

I'd prefer this in early return style without else.
Comment 10 Ryosuke Niwa 2016-11-11 19:48:56 PST
Created attachment 294584 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2016-11-11 19:50:50 PST
Comment on attachment 294584 [details]
Patch for landing

Rejecting attachment 294584 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 294584, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2501394
Comment 12 Ryosuke Niwa 2016-11-11 20:00:25 PST
Comment on attachment 294584 [details]
Patch for landing

Will land with the right reviewer name after waiting for EWS.
Comment 13 Ryosuke Niwa 2016-11-11 21:06:33 PST
Committed r208641: <http://trac.webkit.org/changeset/208641>