Closed Bug 1412775 Opened 3 years ago Closed 3 years ago

Implement Event.composedPath

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: ben.tian, Assigned: smaug)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files)

I could take this one.
Assignee: nobody → bugs
And as always, spec bugs to solve first, https://github.com/whatwg/dom/issues/525
Priority: -- → P2
Status: NEW → ASSIGNED
Attached patch wipSplinter Review
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 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.
Attachment #8936645 - Flags: review?(sshih)
> true if mParentTarget is a chrome handler in the event path.
For consistency s/true/True/
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.
Attachment #8936875 - Flags: review?(sshih)
Blocks: 1413102
Comment on attachment 8936875 [details] [diff] [review]
composed_path_chrome_test.diff

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

LGTM, thanks.
Attachment #8936875 - Flags: review?(sshih) → review+
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()?
Empty sequence, since currentTarget is then null.
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 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
Attachment #8936645 - Flags: review?(sshih) → review+
Attached patch +nitsSplinter Review
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
https://hg.mozilla.org/mozilla-central/rev/9c0350b68863
https://hg.mozilla.org/mozilla-central/rev/45ed85bd1b6a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.