Do not dispatch a trusted event when target and reletedTarget are identical

[hayatoito]

…Target are identical

Change the behavior of event dispatch as follows:

1. Make an event dispatch happen until the root of the tree when target
   and relatedTarget are identical in synthetic events.
2. Do not dispatch an event when retargeted-target and
   retargeted-reletedTarget are identical, but target and relatedTarget are
   not identical.

(2) is a bug fix. This sitution happens in UA events. For example,
if a mouse pointer moves from an element in a shadow tree to
its shadow host, a "mouseover" event should not be dispatched on the
shadow host. The current spec allows a non-empty event path in this case.

Fixes WICG/webcomponents#577

[rniwa]

Do not dispatch an event when retargeted-target and
retargeted-reletedTarget are identical, but target and relatedTarget are
not identical.

This has the same issue as the one I pointed out in WICG/webcomponents#577. In general, you can't just compare target and related target or their retargeted counterparts. You need to look at whether we're crossing a shadow boundary, and if both target and related target are in the shadow tree from which we're exiting.

For example, it's perfectly fine to dispatch an event with target set to a shadow host and related target set to a node inside that shadow tree, and the event MUST be dispatched.

[hayatoito]

Yeah, I am fully aware of that. The point is UA events. There is a case where we should not dispatch a UA event with such a condition. Could you have a chance to see the example in #336 (comment)?

Maybe https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/dom/shadow/shadow-boundary-events.html?l=150 might be helpful to understand the case. In this case, we should not dispatch an mouseover event on the shadow host (shadowF).

That is the motivation of this change. If we are to dispatch a synthetic event which has the same condition (with target=shadowF and relatedTarget=shadowI), and want to make sure that event is dispatched, we have to check whether an event is a synthetic event or not explicitly in the spec. I do not want to introduce the divergence between synthetic events and UA events in the spec (and the implementation) only because we want to address this very minor case. The benefit looks small, though I am not 100% sure.

Thus, in summary, the point is which we prefer (1 or 2):

1. Introduce the divergence (between synthetic events and UA events) so that synthetic events are always dispatched in the first tree.
2. We do not want to introduce the divergence between synthetic events and UA events in any case!

The PR stands for 2.

[rniwa]

This is not a thing the generic event dispatch algorithm should be dealing with; the same way composed flag shouldn't be inferred by the type of the event we're firing. Instead, we need to update UI events spec so that we don't fire events when relatedTarget is inside a shadow tree of target.

[hayatoito]

Hmm. Maybe that would be possible. However, is it really worth doing?
Users can fire a synthetic UI events. Thus, that would not address the issue here.

[rniwa]

I don't understand. You said earlier that the only case you care about with regards to the related target being inside the shadow tree of the target is when UA fires a trusted event. Specifying this in the UI events spec would specifically solves this issue. Note that UI events spec specifies when and how events gets fired by UA. e.g. mouseout wouldn't get fired constantly whenever mouseover happens.

[hayatoito]

I do not think that's a good idea.
Rather, I would do adding one "if .. else .." statement in DOM Standard, instead of adding a lot of normative information to UI Events spec. That would make our lives much simpler, I think.

BTW, do you prefer 1? Is my understanding correct?

1. Introduce the divergence (between synthetic events and UA events) so that synthetic events are always dispatched in the first tree.
2. We do not want to introduce the divergence between synthetic events and UA events in any case!

[rniwa]

I do not think that's a good idea.

The last time this came up for composed, people have argued that we should specify this at where events are dispatched instead of treating all trusted events the same so we need to check with @domenic and @annevk what they think about this case.

[hayatoito]

We do not have to do the same way as we did for composed this time because we can specify the condition easily in general event dispatch section in DOM Standard. For example, "a trusted event which has a non-null relatedTarget property" might be good enough.

[hayatoito]

Anyway, let me update the PR so that a synthetic event is always dispatched in the first tree, at least until the root of the tree. I am fine with this change. The PR would become simpler because the current spec already handles that well.

[hayatoito]

I have updated the PR, re-using this thread. Please ignore the first comment in this discussion. The PR became much simpler.

[hayatoito]

Is that okay to terminate these steps without a return value?
That would be no difference in practice, but I am not sure what is the best.

[annevk]

Is this actually how we would implement this in practice? It seems somewhat weird to use the isTrusted attribute. Also in part because I still don't quite know when it's set and when it's not. Does element.click() set it?

[smaug----]

element.click() doesn't dispatch trusted events.

[hayatoito]

DOM Standard defines isTrusted as follows:

event . isTrusted
Returns true if event was dispatched by the user agent, and false otherwise.

I am not sure how we could interpret "event was dispatched by the user agent" strictly.
I tend to agree @SmauG: A click event triggered by element.click() is dispatched by users, instead of UA. I confirmed that element.click() doesn't dispatch trusted events in Blink.

[annevk]

That is not a normative description of isTrusted though. That's just sort of what it does. We don't really have a normative description anywhere. It's currently left to those dispatching events in various specifications.

I thought maybe the ECMAScript stack being empty would be it, but postMessage() results in untrusted events, so that's not it either.

[smaug----]

postMessage() dispatching untrusted events? That is a bug.
click is the special case similarly to dispatching MouseEvent click explicitly needing to trigger default handling.

[hayatoito]

Thanks. Instead of using isTrusted here, just saying "If event is dispatched by the user agent" would be safer?

[smaug----]

Could we please not use PRs for discussing spec issues.

[smaug----]

FWIW, after reading comments here, I was about to propose the same stuff what hayato then proposes in #336 (review) , I think.
My current thinking is that UIEvents should tell what kind of events should be fired, and it plays in a layer above actual event dispatch. Then if shadow DOM affects to event dispatch, that is something to deal with in DOM spec.
(in a way even .composed could be thought to be part of dispatch, not really about the caller of dispatch method, even though, currently .composed is in UI spec events)

Also, I would imagine #336 (review) is what would be implemented, not some per-event-firing-location relatedTarget check.

[annevk]

@smaug---- I don't really understand what you're proposing. I'd think we want to make the specification match how this is actually implemented in practice. When implementation and specification diverge, there's often room for bugs.

@hayatoito we can't say "dispatched by the user agent" since that is too vague. It sounds like what @smaug---- is saying there is that click() is no different from dispatching a synthetic MouseEvent object whose name is click. If that is the case making sure that is defined correctly depends on #325 I think.

[smaug----]

@smaug---- I don't really understand what you're proposing.

The same thing what @hayatoito proposed

[hayatoito]

Can we merge this PR without waiting for #325?
Since this PR only affects an event which has relatedTarget, the result of #325, click(), will not affect the behavior, I think.

[annevk]

Apologies, I got confused and you are correct. I rebased the PR and applied some minor nit fixes. I have two questions:

1. Is @rniwa now okay with this approach?
2. We're sure no specification relies on the return value of dispatch (when relatedTarget is set somehow) and would get tripped up by it not returning anything here?

[hayatoito]

Thank you!

We're sure no specification relies on the return value of dispatch (when relatedTarget is set somehow) and would get tripped up by it not returning anything here?

I am not sure. Maybe we can return true here, instead of not returning anything. That might be better.

As far as I confirmed in Blink, Blink returns DispatchEventResult::NotCanceled in event dispatch if the event path is empty, and I have not seen any problem for that so far.

[annevk]

Yeah, returning true is probably better, especially if that matches implementations. I can make that change before landing.

@rniwa could you please review one more time? If I don't hear anything by tomorrow I'm just going to assume you're okay with this. We can always revisit post-commit.

[rniwa]

No, I don't think we should be specifying this in the DOM specification. Also, this would affect every UI event dispatch beyond just ones inside shadow trees, and I'm not okay with that.

Please don't merge this.

[hayatoito]

Also, this would affect every UI event dispatch beyond just ones inside shadow trees

We don't have to worry about such a case. UA's UI Event which happens in a document tree never meets this condition. Such an UI Event's target and relatedTarget are always different, as far as I see. A target and a relatedTarget could be identical only if retargeting happens.

[annevk]

@rniwa what do you suggest we do instead? And how would/do you implement this in WebKit?

[rniwa]

@rniwa what do you suggest we do instead? And how would/do you implement this in WebKit?

I don't think this is a thing that belongs to the DOM specification. It should be specified in https://w3c.github.io/uievents/ which already specifies other conditions under which an event should be dispatched or not dispatched.

Adding this text to the DOM means that any event regardless of whether they're UI event or not would not be dispatched by UA if it happens to have a related target and it's equal to the target.

[annevk]

How would you implement this though? Also, the whole reason DOM has relatedTarget logic is because there is no clean layering.

[rniwa]

How would you implement this though? Also, the whole reason DOM has relatedTarget logic is because there is no clean layering.

I don't know how relevant how each implementation does this check since that'll be an implementation detail but WebKit checks this condition when figuring out which node to fire mouse events.

[hayatoito]

Adding this text to the DOM means that any event regardless of whether they're UI event or not would not be dispatched by UA if it happens to have a related target and it's equal to the target.

The PR does not have an intention to distinguish UI events or not. That is not what we agreed at all.
This PR is just a bug fix which I introduced in upstreaming Shadow DOM into DOM Standard in the past, where whether having relatedTarget or not only matters.

Could you file another bug if you have another concrete proposal? We should not discuss that in this PR because your proposal sounds a new proposal which we have not heard.

[hayatoito]

Okay. Let's change the sentence as follows, and land this PR.

If event's isTrusted attribute is true and target is relatedTarget but target is not event's relatedTarget, then return true and terminate these steps.

@annevk I appreciate if you could merge this PR with this change.

[annevk]

@rniwa note that relatedTarget in DOM is not generic per se. Only if an event class decides to make use of that internal slot will they participate in the choices DOM makes for it. If you want to call something relatedTarget but have different semantics, you could in theory use your own internal slot for it. (This is all a little hairy since we haven't formalized internal slots yet and UI events is a hopelessly vague specification, but, it should all work...)

Anyway, I'll update the PR per the discussion and land it tonight / tomorrow.

[annevk]

PR updated.

[rniwa]

On my second thought, don't we want this behavior to non-trusted events as well? When someone manually fire a mouseout event, don't we want that event to stop when it reaches a point where retargeted target and retargeted relatedTarget are identical? (We can discuss this in a separate issue since this PR discussion is getting really long).

Now I'm re-reading the history of what's been discussed here, I've started to think that we may want some sort of a flag like composed. Maybe relatedTargetIsScoped? Anyway, we can discuss it in a separate issue about that.

[annevk]

The beginning of this PR thread does touch upon the synthetic event issue. And in #336 (comment) @hayatoito argued for not making them diverge, but then the thread went into a different direction.

I also realize I forgot to update the text to make it return true as mentioned in #336 (comment). Will do that now.

[annevk]

I'm going to land this with the isTrusted guard removed since it seems both @hayatoito and @rniwa are okay with that (I checked with @rniwa on IRC).

[annevk]

@hayatoito are you planning on writing tests for this?

[hayatoito]

Ah, I see. Okay to remove isTrusted guard since we added "target is not event's relatedTarget" guard. That would change the behavior slightly, but at least it would not bring an incompatible change unless Shadow DOM is used.

I can add tests for WPT. Thank you!

[dSalieri]

Ah, I see. Okay to remove isTrusted guard since we added "target is not event's relatedTarget" guard. That would change the behavior slightly, but at least it would not bring an incompatible change unless Shadow DOM is used.

I can add tests for WPT. Thank you!

@hayatoito Well, I've read your issue, I see you avoided in spec using isTrusted property, instead this you added "target is not event's relatedTarget".

/// Let "target" be the host of a shadow root in an another shadow root
/// Let "relatedTarget" be the node in shadow root of the host
const ev = new Event("click");
ev.relatedTarget = relatedTarget;
target.addEventListener("click", (e) => console.log(e));
target.dispatchEvent(ev)

This example will invoke callback but it shouldn't because condition returns true.

I see that browsers don't call callbacks if it conform that condition if it is not synthetic event and call if it is synthetic event.

If target is relatedTarget and target is not event’s relatedTarget, then return true.

---

But if I appoint relatedTarget the following way:

const ev = new MouseEvent("click", {relatedTarget});

then condition will work as if it will be an non-synthetic event, hence it won't invoke the callback of the event listener.

In theory, it doesn't matter how I set relatedTarget, the condition shouldn't allow callback functions to be called. But for some reason it doesn't match.

Could you give some comment about that?

[bathos]

In theory, it doesn't matter how I set relatedTarget, the condition shouldn't allow callback functions to be called. But for some reason it doesn't match.

The internally associated “relatedTarget” (not sure why it’s camel cased in the spec, I thought the norm for this was using regular spaces) isn’t the relatedTarget attribute of UIEvent. The UIEvent attribute (→ ES accessor property) exposes it for UIEvent instances specifically, but it exists for all events as private state. Setting a data property with the same name doesn’t do anything.

[dSalieri]

@bathos yeah I observed that relatedTarget is accessor property. But this UIEvent attribute exists for concrete UIEvents like MouseEvent or FocusEvent. Hence for them I can't do ordinary assignment because they are getters, I talk about relatedTarget properties. I only do initializing as I specify above. But what about CustomEvent? What if I want to emulate behaviour like a MouseEvent when my cursor point onto element of shadow root and then I do "move" to his host element that also placing in shadow root, I expect to see no one callback (callback won't be called).

Summarize: Inspite of that spec explicit says about relatedTarget as a property of event, there are differences. Your own created properties don't count in whatwg algos. Right?

[bathos]

Summarize: Inspite of that spec explicit says about relatedTarget as a property of event, there are differences. Your own created properties don't count in whatwg algos. Right?

Right. Despite sharing the same name as an attribute of UIEvent and a dictionary member that UIEvent constructors accept, when the spec says “event’s relatedTarget”, it isn’t referring to an ES property. It’s an abstract internal association, not something specific to the ECMAScript binding.