Closed
Bug 1449560
Opened 7 years ago
Closed 7 years ago
Implement shadow-related event dispatch changes
Categories
(Core :: DOM: Core & HTML, enhancement, P3)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: annevk, Assigned: smaug)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
7.84 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
See http://w3c-test.org/dom/events/relatedTarget.window.html and https://github.com/whatwg/dom/pull/585 for all the algorithm changes (it's a big change).
Comment 1•7 years ago
|
||
Do we need this to ship or can it be post-shipping?
Reporter | ||
Comment 2•7 years ago
|
||
It's not clear how compatible we are with other browsers today, but if we are we could fix this later. I suspect Olli wants to fix this though since he filed the 3 or so issues that led to this change.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bugs
Assignee | ||
Comment 3•7 years ago
|
||
Not sure who could review this atm. Masayuki is still away, I think.
The first ShouldClearTargets(aEvent); call is need in case no event target will handle the event because event.target and event.relatedTarget got retargeted to the same node, as an example.
wpt change is required since the test is buggy. The listener is never called.
It is possible that the spec is still tweaked a bit regarding that behavior.
remote: View your change here:
remote: https://hg.mozilla.org/try/rev/1c167f3d87545f66242bac02d3c984b0943c0be5
remote:
remote: Follow the progress of your build on Treeherder:
remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c167f3d87545f66242bac02d3c984b0943c0be5
remote: recorded changegroup in replication log in 0.112s
Attachment #8973261 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 4•7 years ago
|
||
This bug blocks activation behavior and touch object bugs.
Comment 5•7 years ago
|
||
Comment on attachment 8973261 [details] [diff] [review]
shadow_event_dispatch_tweaks.diff
Man, the spec around this stuff is hard to read... In particular, the fact that the "target" in the dispatch algorithm becomes the "item" in the event path structs. :( I _really_ hope "the last tuple in event’s path whose target is non-null" is in fact the thing that we are thinking of as the target of the event... Is it?
>+ } else if ((finalRelatedTarget =
Else after return.
>+++ b/testing/web-platform/tests/dom/events/relatedTarget.window.js
>- document.body.dispatchEvent(event);
>+ shadowChild2.dispatchEvent(event);
This seems like it's changing the test a lot. The idea here was to test a case when the target is not in a shadow tree but the relatedTarget was, right? But now both are.
We should move the listener to document.body (and maybe make it a { once: true } listener), keep dispatching to document.body, and remove the shadowChild2 bits, I would think...
Attachment #8973261 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 6•7 years ago
|
||
I did first move listener to document.body, but the issue is that relatedTarget is then host which is not in shadow dom
Assignee | ||
Comment 7•7 years ago
|
||
I'll file a test and/or spec issue about the test
Comment 8•7 years ago
|
||
> but the issue is that relatedTarget is then host which is not in shadow dom
Oh, because relatedTarget also gets retargeted?
Assignee | ||
Comment 9•7 years ago
|
||
Assignee | ||
Comment 10•7 years ago
|
||
oh, is there some non-e10s issue still :/
Assignee | ||
Comment 11•7 years ago
|
||
The issue is of course that we dispatch to Window in content page and then propagate to chrome document and we still try to retarget there. This is not something really covered in the spec and shouldn't be.
Assignee | ||
Comment 12•7 years ago
|
||
The only change (except that else-after-return thingie) is added 'if' in FragmentOrElement.cpp. That case should happen only if we're propagating events from a document to another, which is content->chrome case only.
Hopefully the comment there is clear enough.
(I think we may want to change retargeting to check owner document or so, but that is a spec issue, so later.)
remote: View your change here:
remote: https://hg.mozilla.org/try/rev/4e5c4ce1a1a288bbe20758c94cade7085a535e01
remote:
remote: Follow the progress of your build on Treeherder:
remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e5c4ce1a1a288bbe20758c94cade7085a535e01
remote: recorded changegroup in replication log in 0.108s
Attachment #8973261 -
Attachment is obsolete: true
Attachment #8973342 -
Flags: review?(bzbarsky)
Comment 13•7 years ago
|
||
Comment on attachment 8973342 [details] [diff] [review]
shadow_event_dispatch_tweaks_3.diff
r=me
Attachment #8973342 -
Flags: review?(bzbarsky) → review+
Comment 14•7 years ago
|
||
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd8b80c9a29
clear event.target and .relatedTarget in case they would otherwise reveal targets in shadow DOM, r=bz
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10859 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/w3c/web-platform-tests/pull/10859
* continuous-integration/travis-ci/pr (https://travis-ci.org/w3c/web-platform-tests/builds/375138277?utm_source=github_status&utm_medium=notification)
Comment 17•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Upstream PR merged
Reporter | ||
Comment 19•7 years ago
|
||
I filed https://github.com/whatwg/dom/issues/645 as a follow-up for comment 5.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•