script: Don't unregister slots from their shadow root when the whole shadow tree is being unbound (servo#40278)

Each shadow root holds a map of slot names to slot elements. Slot
elements dynamically register and unregister themselves at their
respective shadow root when they are bound and unbound from the tree.

Previously, they were doing this too often. When a slot element is
unbound from the tree then it might still be connected to its shadow
root, in case the entire shadow tree is being unbound.

Fixing this requires the slot element to know whether it was in a shadow
tree prior to `bind_to_tree` being called (then it's already registered
and doesn't need to do so again), and whether it will be in a shadow
tree after `unbind_from_tree` is called (then there's no need to
unregister).

Testing: This change adds a new web platform test
Fixes: servo#40242

---------

Signed-off-by: Simon Wülker <[email protected]>

Author: simonwuelker

components/script/dom/element.rs

@@ -157,8 +157,8 @@ use crate::dom::intersectionobserver::{IntersectionObserver, IntersectionObserve
 use crate::dom::mutationobserver::{Mutation, MutationObserver};
 use crate::dom::namednodemap::NamedNodeMap;
 use crate::dom::node::{
-    BindContext, ChildrenMutation, CloneChildrenFlag, LayoutNodeHelpers, Node, NodeDamage,
-    NodeFlags, NodeTraits, ShadowIncluding, UnbindContext,
+    BindContext, ChildrenMutation, CloneChildrenFlag, IsShadowTree, LayoutNodeHelpers, Node,
+    NodeDamage, NodeFlags, NodeTraits, ShadowIncluding, UnbindContext,
 };
 use crate::dom::nodelist::NodeList;
 use crate::dom::promise::Promise;
@@ -695,7 +695,7 @@ impl Element {
             .upcast::<Node>()
             .set_containing_shadow_root(Some(&shadow_root));
 
-        let bind_context = BindContext::new(&self.node);
+        let bind_context = BindContext::new(self.upcast(), IsShadowTree::Yes);
         shadow_root.bind_to_tree(&bind_context, can_gc);
 
         let node = self.upcast::<Node>();

components/script/dom/html/htmlslotelement.rs

@@ -28,7 +28,9 @@ use crate::dom::document::Document;
 use crate::dom::element::{AttributeMutation, Element};
 use crate::dom::globalscope::GlobalScope;
 use crate::dom::html::htmlelement::HTMLElement;
-use crate::dom::node::{BindContext, Node, NodeDamage, NodeTraits, ShadowIncluding, UnbindContext};
+use crate::dom::node::{
+    BindContext, IsShadowTree, Node, NodeDamage, NodeTraits, ShadowIncluding, UnbindContext,
+};
 use crate::dom::virtualmethods::VirtualMethods;
 use crate::script_runtime::CanGc;
 
@@ -495,22 +497,24 @@ impl VirtualMethods for HTMLSlotElement {
             s.bind_to_tree(context, can_gc);
         }
 
-        if !context.tree_is_in_a_shadow_tree {
-            return;
+        let was_already_in_shadow_tree = context.is_shadow_tree == IsShadowTree::Yes;
+        if !was_already_in_shadow_tree {
+            if let Some(shadow_root) = self.containing_shadow_root() {
+                shadow_root.register_slot(self);
+            }
         }
-
-        self.containing_shadow_root()
-            .expect("not in a shadow tree")
-            .register_slot(self);
     }
 
     fn unbind_from_tree(&self, context: &UnbindContext, can_gc: CanGc) {
         if let Some(s) = self.super_type() {
             s.unbind_from_tree(context, can_gc);
         }
 
-        if let Some(shadow_root) = self.containing_shadow_root() {
-            shadow_root.unregister_slot(self.Name(), self);
+        if !self.upcast::<Node>().is_in_a_shadow_tree() {
+            if let Some(old_shadow_root) = self.containing_shadow_root() {
+                // If we used to be in a shadow root, but aren't anymore, then unregister this slot
+                old_shadow_root.unregister_slot(self.Name(), self);
+            }
         }
     }
 }

components/script/dom/node.rs

@@ -301,7 +301,7 @@ impl Node {
         let parent_is_connected = self.is_connected();
         let parent_is_in_ua_widget = self.is_in_ua_widget();
 
-        let context = BindContext::new(self);
+        let context = BindContext::new(self, IsShadowTree::No);
 
         for node in new_child.traverse_preorder(ShadowIncluding::No) {
             if parent_in_shadow_tree {
@@ -4305,16 +4305,29 @@ pub(crate) struct BindContext<'a> {
 
     /// Whether the tree's root is a shadow root
     pub(crate) tree_is_in_a_shadow_tree: bool,
+
+    /// Whether the root of the subtree that is being bound to the parent is a shadow root.
+    ///
+    /// This implies that all elements whose "bind_to_tree" method are called were already
+    /// in a shadow tree beforehand.
+    pub(crate) is_shadow_tree: IsShadowTree,
+}
+
+#[derive(Debug, Eq, PartialEq)]
+pub(crate) enum IsShadowTree {
+    Yes,
+    No,
 }
 
 impl<'a> BindContext<'a> {
     /// Create a new `BindContext` value.
-    pub(crate) fn new(parent: &'a Node) -> Self {
+    pub(crate) fn new(parent: &'a Node, is_shadow_tree: IsShadowTree) -> Self {
         BindContext {
             parent,
             tree_connected: parent.is_connected(),
             tree_is_in_a_document_tree: parent.is_in_a_document_tree(),
             tree_is_in_a_shadow_tree: parent.is_in_a_shadow_tree(),
+            is_shadow_tree,
         }
     }
 

components/script/dom/shadowroot.rs

@@ -47,8 +47,8 @@ use crate::dom::documentorshadowroot::{
 use crate::dom::element::Element;
 use crate::dom::html::htmlslotelement::HTMLSlotElement;
 use crate::dom::node::{
-    BindContext, Node, NodeDamage, NodeFlags, NodeTraits, ShadowIncluding, UnbindContext,
-    VecPreOrderInsertionHelper,
+    BindContext, IsShadowTree, Node, NodeDamage, NodeFlags, NodeTraits, ShadowIncluding,
+    UnbindContext, VecPreOrderInsertionHelper,
 };
 use crate::dom::trustedhtml::TrustedHTML;
 use crate::dom::types::EventTarget;
@@ -575,7 +575,7 @@ impl VirtualMethods for ShadowRoot {
 
         shadow_root.set_flag(NodeFlags::IS_CONNECTED, context.tree_connected);
 
-        let context = BindContext::new(shadow_root);
+        let context = BindContext::new(shadow_root, IsShadowTree::Yes);
 
         // avoid iterate over the shadow root itself
         for node in shadow_root.traverse_preorder(ShadowIncluding::Yes).skip(1) {

tests/wpt/meta/MANIFEST.json

@@ -822432,6 +822432,13 @@
       }
      ]
     ],
+    "assign-slottables-after-removing-shadow-tree-from-document.html": [
+     "216c5747625695b58666d32a04d0f3037b6b690b",
+     [
+      null,
+      {}
+     ]
+    ],
     "attachShadow-with-ShadowRoot.html": [
      "994053822909ff9b1adc077cb7d912ecb54c430b",
      [

tests/wpt/tests/shadow-dom/assign-slottables-after-removing-shadow-tree-from-document.html

@@ -0,0 +1,34 @@
+<!DOCTYPE html>
+<head>
+    <title>Slottables should match slots when the shadow host is removed from the document</title>
+    <meta name="author" title="Simon Wülker" href="mailto:[email protected]">
+    <meta name="assert" content="Slottables should be assigned to slots even after the shadow host has been removed from the document">
+    <link rel="help" href="https://github.com/servo/servo/issues/40242">
+
+    <script src="/resources/testharness.js"></script>
+    <script src="/resources/testharnessreport.js"></script>
+</head>
+
+<body>
+    <script defer>
+      test((t) => {
+        // Attach a shadow root with a slot named "foo" inside it
+        let host = document.createElement("div");
+        document.body.appendChild(host);
+        let root = host.attachShadow({mode: "closed"});
+        let slot = document.createElement("slot");
+        slot.name = "foo";
+        root.appendChild(slot);
+
+        // Remove the host from the document tree
+        host.remove();
+        assert_array_equals(slot.assignedNodes(), []);
+
+        // Create a slottable that should be slotted into "foo"
+        let slottable = document.createElement("div");
+        slottable.slot = "foo"
+        host.appendChild(slottable);
+        assert_array_equals(slot.assignedNodes(), [slottable]);
+      }, "Slottables should be assigned to slots even after the shadow host has been removed from the document");
+    </script>
+</body>