Improve CommandEvent and ToggleEvent source retargeting

[josepharhar]

This change prevents CommandEvent.source from leaking nodes across shadow boundaries. More context here:
#11255 (comment)

Event.currentTarget and Event.target already have retargeting capabilities built into them to prevent leaking nodes across shadow boundaries. By retargeting against those, we can make sure that Event.source does not leak across shadow boundaries.

• At least two implementers are interested (and none opposed):
  • Chromium
  • …
• Tests are written and can be reviewed and commented upon at:
  • …
• Implementation bugs are filed:
  • Chromium: https://issues.chromium.org/issues/420639769
  • Gecko: …
  • WebKit: …
• Corresponding HTML AAM & ARIA in HTML issues & PRs: Not needed
• MDN issue is filed: I'm not sure that this needs MDN documentation.
• The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

---

/interaction.html ( diff )

[domenic]

Editorially LGTM, but I'd appreciate someone more familiar with the intent checking the logic.

[keithamus]

I think this looks good, it would certainly solve the issue. I'm a little curious if this can be simplified to "if target is null or currentTarget is null, return null, otherwise retarget source against currentTarget". I'm not sure of the performance cost of re-targeting twice.

[josepharhar]

I think this looks good, it would certainly solve the issue. I'm a little curious if this can be simplified to "if target is null or currentTarget is null, return null, otherwise retarget source against currentTarget". I'm not sure of the performance cost of re-targeting twice.

I think there are cases where currentTarget is null but target is not null after event dispatch, so we should continue considering target in addition to currentTarget. See the "clearTargets" variable here and how it only removes the target from the event in some situations (this prevents leaking nodes): https://dom.spec.whatwg.org/#concept-event-dispatch

[josepharhar]

I just pushed a change to move the retargeting logic into a separate algorithm so it can also be called for the ToggleEvent.source getter

[smaug----]

I don't understand this setup. We want events which have been dispatched in light DOM to have their .source retargeted? What is the use case for that. Events dispatched in shadow DOM would get just null.

[josepharhar]

We want events which have been dispatched in light DOM to have their .source retargeted?

If the event has been dispatched in light DOM, then retargeting won't change the resulting source element that we return; we are effectively not retargeting events that are dispatched in light DOM.

Events dispatched in shadow DOM would get just null.

If the event was dispatched in shadow DOM, then source will become null after event dispatch is done, just like target and relatedTarget.

[smaug----]

If the event has been dispatched in light dom and source is in shadow DOM, retargeting will happen.

But what are we trying to achieve with this rather unusual setup? Why do we need to fall back to 'target'?

[josepharhar]

If the event has been dispatched in light dom and source is in shadow DOM, retargeting will happen.

Ah yes, that is true. In this case, I'm assuming that you still don't want source to be leaked somewhere since it is in a shadow root, so we should do retargeting.

But what are we trying to achieve with this rather unusual setup? Why do we need to fall back to 'target'?

Target is better than currentTarget because after event dispatch target is likely still set to something we can retarget against which won't leak any nodes, whereas currentTarget just becomes null.

Now that I think about it, maybe we can just always retarget against target instead of currentTarget

[josepharhar]

I removed the usage of currentTarget and now it just uses target. It shouldn't change the behavior.

[annevk]

Is that also what happens for relatedTarget? Ideally source works the same way as relatedTarget.

[smaug----]

retargetAgainst isn't defined anywhere

[smaug----]

But what is actually wrong with the current .source handling. I'm still not quite sure what we're trying to fix here. The current spec retargets shadow course to first light DOM host after dispatch. And during dispatch it is retargeted so that .source will be in the same or "higher" subtree than currentTarget/target.

The discussion about having relatedTarget type of behavior is more about tweaking the propagation path if we kept command event as composed, since once relatedTarget and target are in the same subtree, the event won't propagate higher up.
https://dom.spec.whatwg.org/#dispatching-events 6.9.7

[josepharhar]

retargetAgainst isn't defined anywhere

Thanks, I fixed that.

But what is actually wrong with the current .source handling.

Since it returns the original source element when currentTarget is not set, after event dispatch you can get the original element without retargeting: #11255 (comment)

The discussion about having relatedTarget type of behavior is more about tweaking the propagation path if we kept command event as composed, since once relatedTarget and target are in the same subtree, the event won't propagate higher up.

I agree, I made this more simple assuming that we are going to make the command event not composed.

[smaug----]

Since it returns the original source element when currentTarget is not set

Well, only if the original source element is in light DOM.

[smaug----]

I still don't understand what we're trying to fix here. The current setup doesn't leak shadow DOM .source, or I don't understand how it does that. Could you explain?

[annevk]

It would only leak if you create a synthetic event with source set to something in a shadow tree. Or if a new specification comes along that does something similar and reuses this event class. The question has become if we should put in protections for these cases as well. I kinda think we should.

I still don't understand why this is not the same as relatedTarget though. It seems weird to have a completely different mechanism.

[smaug----]

Why would it leak even with synthetic events?

[annevk]

If you don't retarget wouldn't it just return the node it was set to? Which could be from a shadow tree?

[keithamus]

It actually looks like neither Chrome nor Firefox follow the existing spec, as is written today. The spec today says:

The source getter steps are to return the result of retargeting source against this's currentTarget.

But both implementations do more like:

The source getter steps are to return the result of retargeting source against this's currentTarget if currentTarget is not null, otherwise return source.

This can be seen in the Chrome implementation of command_event.cc L46-50:

  if (current) {
    return &current->ToNode()->GetTreeScope().Retarget(*source);
  }
  DCHECK_EQ(eventPhase(), Event::PhaseType::kNone);
  return source;

And in the Firefox implementation CommandEvent.cpp L83-93:

  if (currentTarget) {
    nsINode* currentTargetNode = currentTarget->GetAsNode();
    if (!currentTargetNode) {
      return nullptr;
    }
    nsINode* retargeted = nsContentUtils::Retarget(
        static_cast<nsINode*>(mSource), currentTargetNode);
    return retargeted ? retargeted->AsElement() : nullptr;
  }
  MOZ_ASSERT(!mEvent->mFlags.mIsBeingDispatched);
  return mSource;

So the spec is right and two impls are wrong. If the implementations followed the spec this would be a non-issue, and this PR is perhaps redundant.

[smaug----]

If you don't retarget wouldn't it just return the node it was set to? Which could be from a shadow tree?

there is already the existing retarget in the spec.

[annevk]

I see, your concern is with the rewrite in particular, not with retargeting in general. Glad we're in agreement on retargeting in general. @keithamus or @josepharhar will have to answer this then.

[keithamus]

After looking at this a lot more closely with @smaug---- I don't think this is necessary any more, I think implementations have misinterpreted the spec to cause a bug that we're now trying to fix with more spec but actually simply need to fix with implementation changes.

[annevk]

Can we capture the bug in a WPT?

[keithamus]

I believe https://github.com/web-platform-tests/wpt/blob/master/html/semantics/the-button-element/command-and-commandfor/source-attribute-retargeting.tentative.html does this, but I guess @josepharhar hasn't gotten around to updating the PR template.

[annevk]

So is the proposed resolution here that we rename that away from tentative and close this PR?

[keithamus]

So is the proposed resolution here that we rename that away from tentative and close this PR?

I think so. If I change the implementation to unconditionally retarget it passes those tests. Here's the Firefox patch: https://phabricator.services.mozilla.com/D261248. I believe Chrome's could be similar, without the need for these spec changes.

[josepharhar]

Sounds good to me

[annevk]

Let's close it once there's a PR to rename the test.

[josepharhar]

@keithamus I made the change to retarget against currentTarget and otherwise return null, which still sounds like a good implementation to me, but it required making these WPT changes: https://chromium-review.googlesource.com/c/chromium/src/+/6853807

Do those changes look good to you? Is firefox passing those tests without the changes I'm proposing?

[keithamus]

@josepharhar firefox passes those tests without the changes (module the popover toggle-source ones as that's still todo for us).

With the changes in your patch applied button-event-dispatch.html also passes but source-attribute-retargeting.tentative.html fails:

FAIL CommandEvent.source should be retargeted when manually dispatched with composed set to true. - assert_equals: host2: event.source after dispatch. expected null but got Element node <div id="host2">

Looking at your patch I think this might not be correct; AIUI it should be <div id="host2> because the re-targeting can always take place. I think the Chromium code leads you to avoid retargeting in some cases, instead returning nullptr when current_target is nullptr, which is IMO incorrect.

If I read through the spec and assume that A (.source) is an element inside a shadow root and B (current_target) is null, then I am left to understand that we hit step 2 and the host of A (.source) should be returned. The retargeting algorithm should only return null when A (.source) is null.

I think Chrome's implementation of Retarget predicates itself on the existence of B (CurrentTarget()) which takes you down the path of returning nullptr if B is nullptr. It is my opinion that Chromium's retarget algorithm should be refactored such that the provided element is the element you're retargeting against (so B), not A, IOW I think it should be closer to:

Element* Event::Retarget(Element* element) const {
  EventTarget* current_target = currentTarget();
  if (element) {
    return &element->GetTreeScope().Retarget(current_target ? current_target->ToNode() :  nullptr);
  }
  return nullptr;
} 

[josepharhar]

Thanks Keith! I think I have an implementation that works now, I'll try getting it shipped and remove tentative from the test and report back here.