whatwg / dom

DOM Standard
https://dom.spec.whatwg.org/
Other
1.56k stars 288 forks source link

Shadow: Specify when `slotchange` fires #447

Closed surma closed 7 years ago

surma commented 7 years ago

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

hayatoito commented 7 years ago

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

rniwa commented 7 years ago

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

hayatoito commented 7 years ago

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 commented 7 years ago

Are you also updating the web platform tests?

hayatoito commented 7 years ago

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

hayatoito commented 7 years ago

FYI: PR for web platform tests is https://github.com/w3c/web-platform-tests/pull/5954

trusktr commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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

trusktr commented 6 years ago

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?