Anne (:annevk) Reporter
	Description • 6 years ago

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).

	Andrew Overholt [:overholt]
	Comment 1 • 6 years ago

Do we need this to ship or can it be post-shipping?

	Anne (:annevk) Reporter
	Comment 2 • 6 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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 3 • 6 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

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

This bug blocks activation behavior and touch object bugs.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 6 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...

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 6 • 6 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

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

I'll file a test and/or spec issue about the test

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 6 years ago

> but the issue is that relatedTarget is then host which is not in shadow dom

Oh, because relatedTarget also gets retargeted?

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

https://github.com/w3c/web-platform-tests/issues/10854

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

oh, is there some non-e10s issue still :/

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 11 • 6 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.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 12 • 6 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

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 6 years ago

Comment on attachment 8973342 [details] [diff] [review]
shadow_event_dispatch_tweaks_3.diff

r=me

	Pulsebot
	Comment 14 • 6 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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 15 • 6 years ago

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10859 for changes under testing/web-platform/tests

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 16 • 6 years ago

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)

	Natalia Csoregi [:nataliaCs]
	Comment 17 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7bd8b80c9a29

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 18 • 6 years ago

Upstream PR merged

	Anne (:annevk) Reporter
	Comment 19 • 6 years ago

I filed https://github.com/whatwg/dom/issues/645 as a follow-up for comment 5.