Ben Tian (inactive) Reporter
	Description • 7 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 1 • 7 years ago

I could take this one.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 2 • 7 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 3 • 6 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 4 • 6 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 5 • 6 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 6 • 6 years ago

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.

	Ming-Chou Shih [:stone]
	Comment 7 • 6 years ago

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

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

LGTM, thanks.

	Ming-Chou Shih [:stone]
	Comment 8 • 6 years ago

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()?

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 9 • 6 years ago

Empty sequence, since currentTarget is then null.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 10 • 6 years ago

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.

	Ming-Chou Shih [:stone]
	Comment 11 • 6 years ago

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

	Pulsebot
	Comment 13 • 6 years ago

Pushed by opettay@mozilla.com:
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

	Raul Gurzau (:RaulG)
	Comment 14 • 6 years ago
bugherder

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

	Chris Mills [:cmills]
	Comment 15 • 6 years ago

Documented: 

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