Shadow: Specify when slotchange fires

[surma]

Safari, Chrome and ShadyDOM currently exhibit different behavior when slotchange does and does not fire. As far as I understand the spec, it is not precisely spec’d, meaning all these current behaviors are technically spec compliant, but can be very frustrating for developers.

Demo 1: Slots are created in the constructor
Demo 2: Slots are creatred in connectedCallback

Which browser fires slotchange under what circumstances?

	Slots in constructor, parser creates element	Slots in constructor, element gets upgraded	Slots in connectedCallback, parser creates element	Slots in connectedCallback, element gets upgraded
Chrome	Fire	No fire	Fire	No Fire
Safari	Fire	No fire	No Fire	No Fire
ShadyDOM	Fire	Fire	Fire	Fire

(ShadyDOM was tested using Firefox).

While my main concern is consistency across browsers, I like ShadyDOM’s behavior best in terms of DX as it is consistent with how attributeChangeCallback in that it fires upon initialization. This would allow developers to have their update logic in the slotchange handler and they wouldn’t need to manually detect silently slotted elements in connectedCallback().

/cc @hayatoito

[annevk]

Why is it not precise?

[surma]

@annevk Maybe I am not understanding the spec, that is entirely possible. If it is specced, do you mind telling me which browser is behaving correctly so I can file bugs against the others?

[surma]

However, maybe we can have a discussion if it is possible/feasible to change the spec to the behavior I describe below or help me understand why that is not a good idea™️

[annevk]

As far as I can tell the specification consistently fires slotchange however the tree was created. Meaning only "ShadyDOM" is correct (though I haven't looked at the timing). If your understanding is different you'll have to tell me how you arrived at that conclusion.

[hayatoito]

I think the following chromium bug is related:
https://bugs.chromium.org/p/chromium/issues/detail?id=696659

I believe that the current Blink's behavior satisfies spec-conformance.
As I mentioned in the bug, I guess what is being proposed here is:

From #229 (comment)

When inserting a slot, we set a suppress signaling flag.

If we change it to:

When inserting a slot, we do not set a suppress signaling flag.

Then, I think the spec's behavior matches that of Shady DOM.

[annevk]

See WICG/webcomponents#288 (comment) by @rniwa for why we suppress in that case. I'm okay with changing this if you're all agreed (including @rniwa) we should change this.

Anyone interested in writing tests?

[surma]

Just want to reemphasize that I am arguing from a developer experience perspective here, and that the current state is unexpected (and inconvenient).

I’d actually be happy to take this opportunity learn to write tests and help out that way. I’m sure one of my colleagues could help me get started here. (@domenic?)

[annevk]

I appreciate you filing the issue. I just wasn't sure at first what the problem was, but @hayatoito stepping through the algorithms in the Chrome bug made it more clear.

[annevk]

http://web-platform-tests.org/ has documentation on writing tests. It's probably worth looking at existing tests once you've got the framework running locally as the easiest way to figure out how to write them.

[domenic]

The other thing to do is check through the existing tests to make sure none of them test the currently-specced behavior; if they do they need to change.

Happy to help more over IRC or Hangouts or whatever once I'm in the office (~an hour or so). Although those docs should help you get started.

[surma]

I opened a PR for a web-platform-tests that tests for the ShadyDOM behavior. Pending consensus in this issue :)

[hayatoito]

See WICG/webcomponents#288 (comment) by @rniwa for why we suppress in that case.

Thanks. I need some time on investigating how Blink can fire a event in such a case. I thought that is doable, but I am not 100% sure. I need to do an experiment.

I think the interoperability is the most important here. I am open to any opinions.

[hayatoito]

I have roughly implemented the behavior which I think is being proposed; when inserting a slot, and one or more slotables are assigned to the slot at this timing, we fire slotchange on the slot.
As far as I experimented, Blink doesn't have any significant difficulties to implement this behavior. I can say Blink is okay to change the spec and implement it so that Blink can fire slotchange for Demo2 as well as for Demo1 in #447 (comment).

If we can agree this change, I am happy to send a PR to DOM Standard, write a WPT, and update Blink's behavior. I think backward compatibility risk is very low.

[rniwa]

WebKit's behavior is not to fire slotchange on a newly created slot when the slot got assigned of nodes as it got inserted into the tree as I mentioned in WICG/webcomponents#288 (comment). I don't know Chrome fires only in demo1 but not in demo2.

Here's my thought.

1. Other DOM events such as change don't fire when the initial value is set.
2. A well written component probably have an efficient code path for when it's constructing shadow tree instead of relying on slotchange event

Firing slotchange event when slot is inserted would make (2) harder because now we have to remember to ignore the very first slotchange being fired. So I'm still slightly inclined towards not firing for the initial insertion.

[surma]

Note: While preparing a demo for this reply, I found out that things become even more inconsistent depending on whether you create your slots in the constructor or connectedCallback. I augmented the original table!

	Slots in constructor, parser creates element	Slots in constructor, element gets upgraded	Slots in connectedCallback, parser creates element	Slots in connectedCallback, element gets upgraded
Chrome	Fire	No fire	Fire	No Fire
Safari	Fire	No fire	No Fire	No Fire
ShadyDOM	Fire	Fire	Fire	Fire

---

@hayatoito Thanks for investigating. Very glad to hear that the behavior is technically possible and feasible.

@rniwa As I said, my main goal is consistency, with a preference for changing the behavior to always firing slotchange. Let me address these two things separately:

Consistency

The behavior of slotchange depends on:

• Whether you define your element before or after using the element (upgrade vs parser creation)
• Whether you define your slots in the constructor or connectedCallback

Unless you are intimately familiar with the intricacies of Custom Elements and ShadowDOM, this is going to trip up and annoy developers. When developing generic components, you might write code that works fine during development, but because users include your CE script at a different point in the page, things suddenly stop working.

A similar argument can be made for slotchange behaving differently depending on whether you create the slot in the constructor or in connectedCallback.

Always firing

Take a look at this demo, which is a stripped down version of a tab panel component I am working on. I’m using ShadowDOM to “reshuffle” the children as a technique for graceful degradation. If JS is disabled, the tabs function as headings for the different sections. So in the markup tabs and panels are interleaved. I use ShadowDOM do group tabs and panels.

I need to be notified about new elements because I need to generate IDs and semantically link tabs and panels using aria-controls and aria-labelledby. Right now, I’d have to manually call into the linking code in the connectedCallback() in addition to linking it to the slotchange event.

I am aware that CE lifecycle callbacks are something different from ShadowDOM events, but the fact that attributeChangedCallback does get called for initialization purposes and slotchange does not, is confusing and somewhat unexpected. A MutationObserver would help, but ShadowDOM already has an internal MO so I think it’d be nice to be able to make use of that (apart from clearing up the aforementioned inconsistency with attributeChangeCallback).

[rniwa]

Those are some good points but I think we need to hear opinions of other stake holders. If they're all okay with this change, we can go ahead and make the spec change.

[sorvell]

Definitely agree with making this change. It's much easier to create an element when the developer only has to consider one processing model for basic lifecycle.

A well written component probably have an efficient code path for when it's constructing shadow tree instead of relying on slotchange event

One could similarly argue that connectedCallback should not run if the element is connected when the constructor runs because the constructor could just check isConnected. As with slotchange, this has the unfortunate effect of requiring a developer to consider multiple processing models to handle basic lifecycle. This is just fundamentally cumbersome.

[rniwa]

Okay, given the use cases and multiple developers saying the current behavior is confusing, I'd now argue that we should make this spec change.

[surma]

Sounds like noone is opposed to the change 🎉

@hayatoito Do you mind creating the PR for the spec? I’ll happily take care of the PR for the tests (web-platform-tests/wpt#5705).

[hayatoito]

Sure. Let me make a PR to DOM Standard. I'll update Blink's behavior after it gets merged.

Thank you all for making this change happen. Especially, huge kudos for @surma. 👍

[annevk]

@hayatoito @surma @rniwa so should we keep suppressing for removal slots or should we just remove the concept of suppressing altogether?

[hayatoito]

I don't have any opinion about that of removing. It looks no one is interested in that.

@surma @sorvell
Do you have any preference for that? If either is okay, we could remove the concept of suppressing signal altogether.

[hayatoito]

From the implementor's perspective, if we remove suppressing for slots removal, the engine will need to do an additional check (and additional potential slotchange event firing). Unless there is a good use case for slotchange at "slot removal", it might be better to keep suppressing it.

[bicknellr]

should we keep suppressing for removal slots or should we just remove the concept of suppressing altogether?

IMHO, it's better to have the same behavior in all cases: "If the nodes distributed to the slot changed, then 'slotchange' fires." I don't think the inconsistency introduced by adding the special case "However, if the nodes distributed to a slot changed because that slot was removed from a tree, then 'slotchange' does not fire." is justified by avoiding the performance cost of an extra event.

[annevk]

@bicknellr why not?

[annevk]

Okay, if someone still feels strongly we should remove suppression for slots removal I suggest they file a new issue and we can tackle that separately.

[rniwa]

I think we should get rid of the suppression completely given that would simplify the spec & implementation. I can't think of a reason why we'd want to keep it just for the removal case.

[annevk]

It seems somewhat reasonable. I think it would be even better if slot outside shadow trees didn't do anything else either then. No functional API whatsoever, but maybe that's more trouble than it's worth.

[hayatoito]

I see. I am okay to disable <slot>'s functional APIs completely if it is not in a shadow tree. The sounds more consistent.

As of now, the functional APIs which are visible to outside are:

1. <slot> can fire a slotchange even if it is not in a shadow tree, as I explained
2. slot.getAssignedNodes({flatten: true}): This might return non-empty if the slot has a child even if the slot is not in a shadow tree.

I think that is all. We should update DOM Standard to disable 1 and 2.

I am happy to update PR to DOM Standard if this is okay. I think this change doesn't have any practical impact on web developers.

[surma]

I like this. Disabling <slot> functionality outside of a ShadowRoot seems very reasonable to me and will simplify explaining behavior should someone run into one of the edge cases.

@hayatoito I think that change should be covered in a separate WPT for ShadowDOM. Do you want to add those?

[hayatoito]

@surma I will add (and/or update) WPT to cover these changes.

[rniwa]

Yeah, I really doubt that anyone is relying on slot element working when it's detached from a shadow tree.

[hayatoito]

I roughly implemented what we roughly agreed here.
From an implementation point of view, I can see that the code became much clear and simple. I think we are in a right direction.

I am now feeling that supporting <slot> in non-shadow tree is like a divided-by-zero error, which we should have excluded from the beginning. :)

Let me update PR to DOM Standard.

[rniwa]

Are you also updating the web platform tests?

[hayatoito]

Yes, I am now working on that. Some amount of tests need to be updated.

[hayatoito]

FYI: PR for web platform tests is web-platform-tests/wpt#5954

[trusktr]

@hayatoito

Is slotchange for that really a useful event for web developers?

It's more useful for library authors.

In real worlds, does removing slots (or its one of ancestors) from the tree happen frequently?

It's easy to imagine scenarios. Using our imagination or example, imagine a game. When the user's monster character gets to a higher skill level, it earns an extra hand. Objects like guns or tools could be placed into this new hand (i.e. slotted into this new slot). If the character is injured, maybe it's hand gets severed from battle, then the hand is removed (the slot is removed but the perhaps the gun remains in inventory).

<monster-character position="20 40 50" rotation="0 40 0">
  <weapon-rifle slot="hand-1">
  </weapon-rifle>
  <weapon-pistol slot="hand-2">
  </weapon-pistol>
  <weapon-sword slot="hand-3">
  </weapon-sword>
</monster-character>

It would be great if we can see a concrete example of the current usage of a slotchange event listener. Do you know that?

In infamous, I use slotchange to detect the composed tree. Then I can traverse the composed tree to render a WebGL scene. Traversing the light tree is (obviously) naive, and will not work when users decide to compose my custom elements using shadow dom.

A-Frame mentioned they don't intend to support Shadow DOM... But, if they did... they would need to understand the composed tree in order to render that with Three.js. slotchange will be very handy for them if they want to add this feature now or in the future.

[trusktr]

I forgot to add, slotchange events could be used, for example, to properly connect or disconnect Three.js THREE.Object3D instances from each other if those, for example, were used to render <monster-character>. Simply relying on parentNode or watching children with MutationObserver won't work, the scene won't be rendered correctly unless we consider the composed tree.

[trusktr]

At the moment, in infamous, I'm observing addition/removal of slots with MO, and using slotchange for while the slots are in DOM (my impl is here and here and some cases still need to be handled).

If slotchanged fired when slots were removed and the removed nodes were observable, I might just move to using only slotchange and no MO.

This is my perspective as someone using Web Components. If I could easily detect distributed and undistributed nodes with only a slotchange event, that might be more helpful, but I'm not completely sure yet.

It seems to me that it might be more helpful in some cases to have a distributedchildren event on shadow roots, and in the handler we could detect which slots elements went to and which slots elements were undistributed from (including being able to observe which parent a slot was removed from).

A Custom Element (a reusable component) owns its whole shadow root, so it would be okay to have shadow root events like this, because the whole logic is still encapsulated inside the Custom Element.

The event of "elements being distributed into a shadow root" seems great for more generic whole-shadow-root logic, and slotchange could still be useful for fine tuning specific logic to specific areas of the tree.

[annevk]

@trusktr if you want a change in behavior it would be much better to file a new issue. We're not tracking closed issues.

[trusktr]

@annevk I don't have a new issue per se, I'm just pointing out use cases that might be useful to consider (because @hayatoito had asked, but not many were provided).

After the changes in #459, how does the above table change? Is everything Fire now?

[hayatoito]

@trusktr Blink has already implemented the spec change. You can confirm the new behavior in Blink. If you find a bug, I would appreciate if you can file a bug for Blink.

[dejan9393]

If anyone else is looking for a workaround for this issue on Safari, try checking for slot.assignedNodes().length in the connectedCallback

[trusktr]

I will test soon on my end when I circle back to ShadowDOM support for my lib. What should I expect from the changes? Is everything in the above table Fire, or are some of them still expected to be No Fire?