DOM APIs which replace all children end up firing superfluous slotchange events

[rniwa]

When replacing all children of a node, via textContent for example, removing each child of the node results in the invocation of the concept to assign slotables for a tree.

For example, consider the following shadow tree:
<p id="target"><slot id="slot1"></slot><slot id="slot2"></slot><slot id="slot3"></slot></p>

If target.textContent = '' is evaluated, it would result in the removal of slot1, followed by that of slot2 and slot3. As a result, we would fire slotchange event on slot2 and slot3 despite of the fact those slots would have never been "rendered" with assigned nodes in practice.

This results in slotchange event handlers of slot2 and slot3 potentially doing unnecessary & useless work.

Relatedly, Firefox's implementation seems to fire slotchange on slot2 & slot3 when target.remove() is evaluated, which seems wrong because the time at which the concept to assign slotables for a tree is evaluated, slot2 and slot3 are no longer in the tree.

See https://gist.github.com/rniwa/1a24c77317a50785ae8885e58f35966c for the demo.

[rniwa]

@annevk @domenic @JanMiksovsky @hayatoito @travisleithead @sorvell

[hayatoito]

It looks an expected behavior to me; Fire slotchange events on every slots if their assigned nodes are changed.

I wonder what is a practical issue for the current expected behavior? Could you share that with us?

Unless there is a strong practical issue, I would prefer a consistent behavior.

[emilio]

cc @smaug----

[smaug----]

Ok, Gecko may have an issue, but other than that, looks reasonable and consistent.

[rniwa]

The issue here is that slot elements get superfluous slotchange events which are of no use. The fact replace all is implemented as a sequence of removing each node is more of a spec fiction. The author shouldn't have to deal with it. There is also a serious performance cost in firing slotchange event on all those slot elements. In fact, if we were to implement this in WebKit, we would slow down all operations of replace all so it's simply not acceptable. As such, the WebKit team would object to the currently specified behavior.

Note that neither Gecko nor WebKit implement the currently specified behavior, and Chrome doesn't fire slotchange in the tree order in other cases. It just so happens to match this particular spec behavior but the order of elements in other cases are not spec conforming.

[hayatoito]

Thanks. So the expected behavior from the author's perspective would be:

shadowRoot: <p><slot id="slot1"></slot><b><slot id="slot2"></slot></b><slot id="slot3"></slot></p>
Removing p results in slotchange events on: ["slot1"]
shadowRoot: <p><slot id="slot1"></slot><slot id="slot2"></slot><slot id="slot3"></slot></p>
Setting p.textContent to "" results in slotchange events on: ["slot1"]

, right? (Both cases fire a slotchange event only on slot1). I agree that this behavior looks consistent from the autors' perspective.

From the spec's perspective, can the spec (or maybe the implementation too?) become complicated if we try to align the current spec to the author's expected behavior?

[rniwa]

@hayatoito : I think implementation / spec just needs to special-case replace-all case, e.g. add a flag to the concept to remove a node similar to an optional suppress observers flag and skip steps 12, 12.1 and 12.2 which assign spottables for a tree. replace all would then invoke it later.

It would add a slight complexity but I think it's an overall win for developer ergonomics & perf since resolving slots after removing each subtree is very expensive.

[annevk]

What exactly would replace all invoke? "Run assign slotables for a tree with parent's root."? Or node's root? I guess they're always equivalent if we do this at the end. Presumably we would have to adjust the insert operation as well, which also has a suppress observers flag? Or is the "problem" non-existent there somehow? (Unfortunately I don't have all the logic in my head anymore, I hope you all don't mind the questions.)

[caridy]

cc @ekashida

[rniwa]

We'd have to run assign slotables for a tree with parent’s root, and then run it again for the children we just removed.

[rniwa]

There is a minor complication for implementors that mutation events and unload evnets in iframes could insert those removed child back into somewhere else but it's not really an issue for the spec since it doesn't have mutation events (WebKit and Blink also disables mutation events in shadow trees).

[annevk]

(To be clear, mutation events might well become an issue if they are not removed: whatwg/dom#305. There was some talk of firing them at a similar time as custom element reactions however, which would reduce a number of issues.)

[annevk]

@rniwa I looked at this some more and I wonder if we should basically extend the "suppress observers flag" to also cover running "assign slotables for a tree". E.g., if you do replaceChild() (defined via https://dom.spec.whatwg.org/#concept-node-replace) with two <slot>s, it seems we should also run the algorithm once for parent's root and once for the removed node, right?

I guess it might only be observable for the case you brought up, but it seems conceptually cleaner if we manage them all in the same way.

[rniwa]

Sure, re-using the suppress observers flag makes senes to me.

[annevk]

@rniwa I created whatwg/dom#695, but I'm a little worried as slotchange is intertwined with assigning slotables to slots.

[orstavik]

You also get slotchange events that can be perceived as redundant when you construct layered custom elements with slots. This seems to be correct behavior according to spec, and it happens similarly in both Chrome, Firefox and Safari. But from the developer standpoint they are somewhat unexpected and can cause confusion.

The minimal situation:

1. You have custom element B that uses custom A in its shadowRoot.
2. Both A and B have slots, and adds a slotchange listener to their shadowRoot in their constructor.
3. The slots are nested. B puts its slot in the slottable position of A.
4. If you create B, with something in its slottable position, then that will fire:
   a. 2 slotchange event in A and
   b. 1 slotchange events in B.

https://codepen.io/orstavik/pen/jeEqpe

This problem also grows as you nest slot and web components deeper. If you have 3 layers, C -> B -> A, then you will get a total of 6 slotchange events:
3 slotchange event in A
2 slotchange events in B
1 slotchange event in C

https://codepen.io/orstavik/pen/bmNpLv

The content of the last codepen (that can be used to create both situations) I also add as an attachment.
redundantSlotchangeEvents.txt

[orstavik]

The principle of this problem is a bit tricky. Thinking out loud here of how I make sense of it.

When you construct the element, with children elements, and slotted children, it is a sequence of multiple actions. The inner constructor sets up elements with empty slottables in the first DOM constellation.
That triggers a slotchange event. Then the middle constructor sets up a second DOM constellation includes the inner element. That triggers a second slotchange event that bubbles to both the middle and inner slot element. Then the outer constructor sets up a third DOM constellation. That triggers a third slotchange event that bubbles to all three slot elements. 3 events fired. But 6 events caught. 3 events caught by the inner slot. 2 events by the middle. 1 event by the outer. And(!), in this case, and I think most such cases, the inner and middle slot is really only interested in the last slotchange event that they receive indirectly and that originates from the outermost slot.

Issue 1.
As a developer, I do not see the construction of a single custom element as the creation of a sequence of multiple DOM constellations. Although this is what the browser does, it took me a long time to envisage this DOM constellation sequence triggering 3 different slotchange events that bubble via the slots. And I am not sure I am right.

As a developer, I anticipate that the slotchange callback function of the two inner slots is only triggered once. I see the construction of this DOM branch as "a single slotchange moment".
In this particular scenario, I would also anticipate that the assignedNodes of the inner elements never would be empty. And I would not have thought about efficiency concerns for the inner or middle slot listener callback due to them being called twice.

Issue 2.
The last slotchange event that bubbles up to the innermost slot element, and that is the relevant slotchange event, does not originate from the innermost slot. That means that the event detail is confusing, as for example its .composedPath()[0] is the outermost slot.

If I attach the event listener for slotchange on the shadowRoot, and have multiple slots in my shadowRoot, I would have to search the event detail/ composedPath of the event detail to find the slot that is in my shadowRoot. If I simply assumed that .composedPath()[0] was the relevant slot element, and for example called .assignedNodes({flatten: true}) on it, then I could sometimes get the right data (as in this instance) and sometimes the wrong data (as more assigned nodes for that slot could be added in the middle layer).