Implement Event.composedPath

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: ben.tian, Assigned: smaug)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(3 attachments)

Assignee

Comment 1

2 years ago
I could take this one.
Assignee: nobody → bugs
Assignee

Comment 2

2 years ago
And as always, spec bugs to solve first, https://github.com/whatwg/dom/issues/525
Priority: -- → P2
Assignee

Updated

2 years ago
Status: NEW → ASSIGNED
Assignee

Comment 3

2 years ago
Posted 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
Assignee

Comment 4

2 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.
Attachment #8936645 - Flags: review?(sshih)
Assignee

Comment 5

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

Comment 6

2 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.
Attachment #8936875 - Flags: review?(sshih)
Assignee

Updated

2 years ago
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()?
Assignee

Comment 9

2 years ago
Empty sequence, since currentTarget is then null.
Assignee

Comment 10

2 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.
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+
Assignee

Comment 12

2 years ago
Posted patch +nitsSplinter Review

Comment 13

2 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

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9c0350b68863
https://hg.mozilla.org/mozilla-central/rev/45ed85bd1b6a
Status: ASSIGNED → RESOLVED
Closed: 2 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.