ContextNode: automatically re-discover changes in the shadow DOM on slotchange event #30321
Conversation
| @@ -218,6 +218,22 @@ export class ContextNode { | |||
| setTimeout | |||
| ); | |||
|
|
|||
| // Shadow root: track slot changes. | |||
| if (node.nodeType == FRAGMENT_NODE) { | |||
There was a problem hiding this comment.
FMI: FRAGMENT_NODE means that its the root right?
There was a problem hiding this comment.
Curtesy of web spec, it means that it's a shadow root.
| if (node.nodeType == FRAGMENT_NODE) { | ||
| node.addEventListener('slotchange', (e) => { | ||
| const slot = /** @type {!HTMLSlotElement} */ (e.target); | ||
| // Rediscover newly assigned nodes. |
There was a problem hiding this comment.
Nit: s/Rediscover/Discover (they've never been discovered before right?)
There was a problem hiding this comment.
Could have been discovered. Even likely. Shadow DOM only can distribute the nodes that were there before. So when a slot is added/changed the node's state changes. For instance, if we start with:
<div id="host">
#shadow-root
<slot name="slot1">
<div id="child1"></div>
Then child1 is essentially undisplayed and it's only connected to the host. But as soon as we change a slot assignment on it:
<div id="child1" slot="slot1"></div>
It gets connected to the shadow root via slot1 and it potentially becomes displayed.
There was a problem hiding this comment.
A bit more context. At this stage we can't even discover new nodes that were not discovered before. But if a new node is discovered later (for instance if it's upgraded as a custom element later), then it will be discovered for the first time and will find the slot/shadow root automatically since they are now ready and expecting new nodes.
| child.isDiscoverable() && | ||
| this.node.contains(child.node) | ||
| ) { | ||
| if (child != this && child.isDiscoverable()) { |
There was a problem hiding this comment.
Yeah, not super happy about it, tbh. But I didn't see any other way. But shadow root "breaks" the contains() API a bit for us. For a slot slot.contains(assignedNode) is always false because they are in different subtrees. So this.node.contains(child.node) would prevent an assigned node to be rediscovered. This change will cause more rediscovers. However, the cost of trying to rediscover a node is relatively low. So the correctness of code won out for me. There's a way to do a more complicated contains over the boundaries of light/shadow subtrees. But that code is exactly what discover does. So this also avoids executing that code twice.
|
|
||
| it('should reassign to slot parent becomes a context node', async () => { | ||
| const sibling1Wait = waitForDiscover(sibling1); | ||
| await rediscover(slot1Parent); |
There was a problem hiding this comment.
nit: it was unclear to me at first that rediscover is being used here to create context nodes
There was a problem hiding this comment.
I get caught up in this sometimes too. The thing is that from the public API perspective, ContextNode interface doesn't exist - it's completely hidden by other APIs such as setProp and addComponent. Thus I don't have a clean "create new context node" API exposed. And the discover is closer to that intent than some other APIs.
No description provided.