1412775 - Implement Event.composedPath

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Ben Tian (inactive)]

https://dom.spec.whatwg.org/#dom-event-composedpath

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

I could take this one.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

And as always, spec bugs to solve first, https://github.com/whatwg/dom/issues/525

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/f89d8b878d48ae3526fdf45029a118cceded9d7d
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=f89d8b878d48ae3526fdf45029a118cceded9d7d

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8936645 [details] [diff] [review]
wip

stone, perhaps you could start to look at this.
https://dom.spec.whatwg.org/#dom-event-composedpath
The spec to look at is https://dom.spec.whatwg.org/#concept-event-dispatch


I need to still write mochitest for Gecko specific parts, but at least manual testing shows that part should be fine too.

Sorry that I didn't split this to pieces. I could if you want. Though the actual change here is rather small. Just
adding couple of new flags increases the patch size.
 But if this is ok, I'd review different parts separately:
1. aVisitor.mParentTarget -> aVisitor.SetParentParent().
Had to make that change so that mParentIsChromeHandler is set correctly.
Parent is chrome handler whenever event propagation crosses content->chrome boundary.
In most cases this change is pure mechanical.

2. Read also about the other new boolean variables in Visitor object. They are documented in EventDispatcher.h

3. The rest.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

> true if mParentTarget is a chrome handler in the event path.
For consistency s/true/True/

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: composed_path_chrome_test.diff

This just tests that the composedPath() returns reasonable values in content and in chrome. The implementation of composedPath() limits path to the same window basically.
We may want to change that later if chrome code needs something special - or rather add some new [ChromeOnly] method.

Comment 7

[Ming-Chou Shih [:stone]]

Comment on attachment 8936875 [details] [diff] [review]
composed_path_chrome_test.diff

Review of attachment 8936875 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks.

Comment 8

[Ming-Chou Shih [:stone]]

Comment on attachment 8936645 [details] [diff] [review]
wip

Review of attachment 8936645 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/EventDispatcher.cpp
@@ +865,3 @@
>          EventTargetChainItem::HandleEventTargetChain(chain, postVisitor,
>                                                       aCallback, cd);
> +        aEvent->mPath = nullptr;

Wondering what is the expected result if content holds the event and calls composedPath()?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Empty sequence, since currentTarget is then null.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Or, I guess from spec point of view it is path which is just empty.
"Set event’s path to the empty list. " happens at the end of dispatch.

Comment 11

[Ming-Chou Shih [:stone]]

Comment on attachment 8936645 [details] [diff] [review]
wip

Review of attachment 8936645 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks.

::: dom/base/nsInProcessTabChildGlobal.cpp
@@ +284,5 @@
>    if (mIsBrowserFrame &&
>        (!mOwner || !nsContentUtils::IsInChromeDocshell(mOwner->OwnerDoc()))) {
>      if (mOwner) {
>        if (nsPIDOMWindowInner* innerWindow = mOwner->OwnerDoc()->GetInnerWindow()) {
> +        // 'this' is already a "chrome handler", so we consider window's 

extra space here

::: dom/events/EventDispatcher.cpp
@@ +1157,5 @@
>    return nullptr;
>  }
>  
> +// static 
> +void

ditto

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +nits

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c0350b68863
Implement Event.composedPath, r=stone
https://hg.mozilla.org/integration/mozilla-inbound/rev/45ed85bd1b6a
Implement Event.composedPath, tests for chrome handling, r=stone

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/9c0350b68863
https://hg.mozilla.org/mozilla-central/rev/45ed85bd1b6a

Comment 15

[Chris Mills [:cmills]]

Documented: 

https://developer.mozilla.org/en-US/docs/Web/API/Event/composedPath
https://developer.mozilla.org/en-US/Firefox/Releases/59#DOM_events