Make command events not composed

[josepharhar]

Based on feedback here, there may be a better alternative to making this event composed and we should remove composed now: #11148

• At least two implementers are interested (and none opposed):
  • Chromium
  • …
• Tests are written and can be reviewed and commented upon at:
  • Update CommandEvent tests to expect an uncomposed event web-platform-tests/wpt#52276
• Implementation bugs are filed:
  • Chromium: https://issues.chromium.org/issues/414826954
  • Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1856430
  • WebKit: https://bugs.webkit.org/show_bug.cgi?id=292368
• Corresponding HTML AAM & ARIA in HTML issues & PRs: Not needed
• MDN issue is filed: I don't see "composed" anywhere here, so maybe no changes are needed? https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/command_event
• The top of this comment includes a clear commit message to use.

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

---

/form-elements.html ( diff )

[jakearchibald]

I assume tests will need to be updated too?

[keithamus]

I assume tests will need to be updated too?

In particular https://github.com/web-platform-tests/wpt/blob/master/html/semantics/the-button-element/command-and-commandfor/button-event-dispatch.html will need to be refactored.

[annevk]

This is editorially fine. OP needs to be completed and I recall @smaug---- raising concerns about this earlier. Is he okay with this now?

[smaug----]

If we are planning to change .source attribute handling to be similar to .relatedTarget, then this is fine (event would propagate if .target and .source are in different subtrees).

[jakearchibald]

@alice has been exploring solutions for reference target #11148 (comment)

[alice]

If we are planning to change .source attribute handling to be similar to .relatedTarget, then this is fine (event would propagate if .target and .source are in different subtrees).

Is this something relatedTarget does? I was trying to understand how related target affects event propagation, but I couldn't decipher it from the text.

[annevk]

If we want to make it work similar to relatedTarget that would be a bit more work. See https://dom.spec.whatwg.org/#concept-event-dispatch for how we deal with relatedTarget today.

[alice]

@annevk Could you explain how relatedTarget affects dispatch in non-spec terms? I've read through the spec a bunch of times, but I'm sadly none the wiser.

[annevk]

At this point I would have to deduce it from first principles again as well I'm afraid.

[alice]

My reading of this WPT is that relatedTarget doesn't affect the event propagation in anything approximating the way I'm proposing source would.

Worked example of event dispatch steps with relatedTarget

If I run through the event dispatch steps on the example from #11148 (comment) with the <button> as relatedTarget rather than source:

<outer-component id="outerComponent">
  <!-- outer shadow --><template shadowRootMode="open">
    <button id="relatedTarget">Foo</button>
    <middle-component id="middleComponent">
      <!-- middle shadow --><template shadowRootMode="open" shadowRootReferenceTarget="innerComponent">
        <inner-component id="innerComponent">
          <!-- inner shadow --><template shadowRootMode="open" shadowRootReferenceTarget="target">
            <div id="target"></div>
          </template>
        </inner-component>
      </template>
    </middle-component>
  </template>
</outer-component>

Dispatching event (with composed: false) to target target inside inner shadow:

4. Let relatedTarget be the result of retargeting event’s relatedTarget against target.

• relatedTarget is still relatedTarget, because relatedTarget's root (outer shadow) is a shadow-including inclusive ancestor of target.

6. If target (target) is not relatedTarget (relatedTarget) or target is event’s relatedTarget:

6.3. Append to an event path with event, target (target), targetOverride (target), relatedTarget (relatedTarget), touchTargets, and false.

6.8. Let parent be the result of invoking target’s get the parent with event.

• parent is inner shadow.

6.9. While parent (inner shadow) is non-null:

6.9.3. Let relatedTarget be the result of retargeting event’s relatedTarget (relatedTarget) against parent (inner shadow).

• relatedTarget is still relatedTarget.

6.9.6. If parent (inner shadow) is a Window object (false), or parent (inner shadow) is a node (true) and target’s (target) root (inner shadow) is a shadow-including inclusive ancestor of parent (inner shadow) (true):

6.9.6.2. Append to an event path with event, parent (inner shadow), null, relatedTarget (relatedTarget), touchTargets, and slot-in-closed-tree.

6.9.7 Otherwise, if parent is relatedTarget, then set parent to null.

• Since the previous condition was false, this condition is not met.

6.9.8. Otherwise:

• Also not met for the same reason.

6.9.9. If parent (inner shadow) is non-null, then set parent to the result of invoking parent’s get the parent with event.

• parent is null, because composed is false.

6.10. Let clearTargetsStruct be the last struct in event’s path whose shadow-adjusted target is non-null.

• This is the first struct added to the path: { event: event, target: target, shadow-adjusted-target: target, relatedTarget: relatedTarget, touchTargets: [], slot-in-closed-tree: false }

6.11. If clearTargetsStruct’s shadow-adjusted target (target), clearTargetsStruct’s relatedTarget (relatedTarget), or an EventTarget object in clearTargetsStruct’s touch target list is a node whose root is a shadow root: set clearTargets to true.

• clearTargets is true.

6.13. For each struct of event’s path, in reverse order: [fire at capturing/target phase]

6.14. For each struct of event’s path: [fire at target/bubbling phase]

7-13. [cleanup, run activation behaviour and return with clearTargets set to true]

---

The only time relatedTarget seems to actually impact the event path is step 6.9.7:

Otherwise, if parent is relatedTarget, then set parent to null.

This seems to have been added in whatwg/dom@afac504, with the commit message:

Shadow: do not dispatch an event when target is relatedTarget

For example, when 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.

We might (?) need to handle a similar case here, but I don't think that logic is sufficient for how we'd like to propagate events with a source.

[lukewarlow]

I think web-platform-tests/wpt#52276 should cover the test changes?

[lukewarlow]

I can't edit OP so I'll just comment the relevant links.

Chromium Implementation Bug: https://issues.chromium.org/u/1/issues/414826954

WebKit Implementation Bug: https://bugs.webkit.org/show_bug.cgi?id=292368

Firefox Implementation Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1856430 (meta bug for whole feature)

[josepharhar]

Thanks luke! I put your links in the description

[annevk]

Adding "do not merge yet" as it seems like we need to figure out to what extent we need the same model as event's relatedTarget concept has.

[lukewarlow]

Do we need to work out the specifics to that before we change the composedness?

I think it'd be good to change this sooner rather than later?

[annevk]

I suppose I'll defer to @smaug---- on that, though generally we don't want the specification to be in a known in-between state.

[lukewarlow]

@smaug---- or @keithamus does Mozilla have a perspective on whether we should make this change now and address the problem in a better way (such as that proposed by Alice) in a future change? I don't think in this case that the spec would be in a known in-between state, as this relates to the event composability and that other change would be something novel for multiple such properties (e.g. the new ToggleEvent.source proposal).

I would like to get clarification on this so I know whether to change WebKit before I enable by default there.

[keithamus]

If we can find a way to resolve the re-targeting of .source such that it does not expose elements in a shadow tree, including manually dispatching events with a source & composed: true, I personally think this is reasonable. I discussed this with @josepharhar who I believe has a way to handle this. My personal opinion is that both of these changes could land as one spec PR, so that we don't leave the spec in a "known not-good state".

[alice]

@keithamus The spec already retargets the source getter:

https://html.spec.whatwg.org/#dom-commandevent-source

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

[lukewarlow]

Yeah I think this change which is removing composed: true, is separate to a potential future change to how retargeting works, so again I don't personally think this is leaving the spec in a known bad state.

If this is the intended direction we're going to go in I'm leaning towards making them not composed in Safari before releasing it? @annevk what's your thinking regarding that?

[jakearchibald]

Fwiw I agree with @lukewarlow here. The potential problems relate to another feature in development, so the solutions should land with that other feature. This PR should land now.

[keithamus]

@keithamus The spec already retargets the source getter:

https://html.spec.whatwg.org/#dom-commandevent-source

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

Perhaps it's a separate issue, but there needs to be additional work to solve issues of retargeting when currentTarget is null:

data:text/html,<div id=a><template shadowrootmode=open><button command=show-modal>Click</template><dialog id=p><script>a.shadowRoot.querySelector('button').commandForElement = p;</script><script>p.addEventListener('command', async e => new Promise(setTimeout).then(()=>console.log(e.source)))</script>

<div id=a>
<template shadowrootmode=open><button command=show-modal>Click</template>
<dialog id=p>
<script>a.shadowRoot.querySelector('button').commandForElement = p;</script>
<script>
p.addEventListener('command', async e =>
  new Promise(setTimeout).then(()=>console.log(e.source)))
</script>

I think issues like this are where the preference for .source to retarget like .relatedTarget are coming from. This remains an issue regardless of composed being true or false, but we should settle on a direction for what we're doing with .source. Perhaps this doesn't block making command events non composed but it is why I left my comment.

[josepharhar]

I found the same issue that keith mentioned, here is a chromium patch that addresses it. I can make a spec PR for it soon: https://chromium-review.googlesource.com/c/chromium/src/+/6526266