Implement imperative slotting API changes

The original implementation of the imperative slot distribution API was done before the final spec PRs landed. In the process of landing those PRs, several changes were made to the way the API works. Primarily, there are two changes: 1. The "auto" slotAssignment mode was renamed to "named". 2. The "linkage" that is created by HTMLSlotElement.assign() was made more permanent. Previously, moving either the <slot> or the assigned node around in the tree (or across documents) would "break" the linkage. Now, the linkage is more permanent, and the only way to break it is through another call to .assign(). See [1] for the chromestatus entry, [2] for the intent to ship, [3], [4], and [5] for the spec PRs, and [6]/[7] for the landed spec. [1] https://chromestatus.com/feature/4979822998585344 [2] https://groups.google.com/a/chromium.org/g/blink-dev/c/6U78F3KWJ78 [3] whatwg/html#6561 [4] whatwg/html#6585 [5] whatwg/dom#966 [6] https://dom.spec.whatwg.org/#find-slotables [7] https://html.spec.whatwg.org/#dom-slot-assign Fixed: 1196842 Fixed: 1067153 Bug: 1067157 Change-Id: I0ee71043c23f3b49a1461296d722045f06eca540 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2824763 Reviewed-by: Joey Arhar <jarhar@chromium.org> Commit-Queue: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/master@{#874413} GitOrigin-RevId: 821490f6968ba4cb60d1c937d1efc6e79cc6626b
mjfroman · Apr 20, 2021 · 7c2e16f · 7c2e16f
1 parent 0d4b5c9
commit 7c2e16f
Show file tree

Hide file tree

Showing 16 changed files with 184 additions and 280 deletions.
diff --git a/blink/renderer/core/dom/element.cc b/blink/renderer/core/dom/element.cc
@@ -3500,8 +3500,7 @@ ShadowRoot* Element::attachShadow(const ShadowRootInit* shadow_root_init_dict,
   auto slot_assignment = (shadow_root_init_dict->hasSlotAssignment() &&
                           shadow_root_init_dict->slotAssignment() == "manual")
                              ? SlotAssignmentMode::kManual
-                             : SlotAssignmentMode::kAuto;
-
+                             : SlotAssignmentMode::kNamed;
   if (const char* error_message = ErrorMessageForAttachShadow()) {
     exception_state.ThrowDOMException(DOMExceptionCode::kNotSupportedError,
                                       error_message);

diff --git a/blink/renderer/core/dom/element.h b/blink/renderer/core/dom/element.h
@@ -121,7 +121,7 @@ enum class ElementFlags {
 
 enum class ShadowRootType;
 
-enum class SlotAssignmentMode { kManual, kAuto };
+enum class SlotAssignmentMode { kManual, kNamed };
 enum class FocusDelegation { kNone, kDelegateFocus };
 
 enum class SelectionBehaviorOnFocus {
@@ -552,7 +552,7 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable {
   ShadowRoot& AttachShadowRootInternal(
       ShadowRootType,
       FocusDelegation focus_delegation = FocusDelegation::kNone,
-      SlotAssignmentMode slot_assignment_mode = SlotAssignmentMode::kAuto);
+      SlotAssignmentMode slot_assignment_mode = SlotAssignmentMode::kNamed);
 
   // Returns the shadow root attached to this element if it is a shadow host.
   ShadowRoot* GetShadowRoot() const;

diff --git a/blink/renderer/core/dom/flat_tree_node_data.cc b/blink/renderer/core/dom/flat_tree_node_data.cc
@@ -12,6 +12,7 @@ void FlatTreeNodeData::Trace(Visitor* visitor) const {
   visitor->Trace(assigned_slot_);
   visitor->Trace(previous_in_assigned_nodes_);
   visitor->Trace(next_in_assigned_nodes_);
+  visitor->Trace(manually_assigned_slot_);
 }
 
 }  // namespace blink
diff --git a/blink/renderer/core/dom/flat_tree_node_data.h b/blink/renderer/core/dom/flat_tree_node_data.h
@@ -42,20 +42,32 @@ class FlatTreeNodeData final : public GarbageCollected<FlatTreeNodeData> {
   }
   void SetNextInAssignedNodes(Node* next) { next_in_assigned_nodes_ = next; }
 
+  void SetManuallyAssignedSlot(HTMLSlotElement* slot) {
+    manually_assigned_slot_ = slot;
+  }
+
   HTMLSlotElement* AssignedSlot() { return assigned_slot_; }
   Node* PreviousInAssignedNodes() { return previous_in_assigned_nodes_; }
   Node* NextInAssignedNodes() { return next_in_assigned_nodes_; }
 
+  HTMLSlotElement* ManuallyAssignedSlot() const {
+    return manually_assigned_slot_;
+  }
+
   friend class FlatTreeTraversal;
   friend class HTMLSlotElement;
   friend HTMLSlotElement* Node::AssignedSlot() const;
   friend HTMLSlotElement* Node::AssignedSlotWithoutRecalc() const;
   friend void Node::ClearFlatTreeNodeDataIfHostChanged(const ContainerNode&);
+  friend void Node::SetManuallyAssignedSlot(HTMLSlotElement* slot);
+  friend HTMLSlotElement* Node::ManuallyAssignedSlot();
   friend Element* Node::FlatTreeParentForChildDirty() const;
 
   WeakMember<HTMLSlotElement> assigned_slot_;
   WeakMember<Node> previous_in_assigned_nodes_;
   WeakMember<Node> next_in_assigned_nodes_;
+  // Used by the imperative slot distribution API (not cleared by Clear()).
+  WeakMember<HTMLSlotElement> manually_assigned_slot_;
 };
 
 }  // namespace blink

diff --git a/blink/renderer/core/dom/node.cc b/blink/renderer/core/dom/node.cc
@@ -3096,7 +3096,8 @@ HTMLSlotElement* Node::AssignedSlot() const {
   // dirty. RecalcAssignment() is almost no-op if we don't need to recalc.
   root->GetSlotAssignment().RecalcAssignment();
   if (FlatTreeNodeData* data = GetFlatTreeNodeData()) {
-    DCHECK_EQ(root->AssignedSlotFor(*this), data->AssignedSlot());
+    DCHECK_EQ(root->AssignedSlotFor(*this), data->AssignedSlot())
+        << "Assigned slot mismatch for node " << this;
     return data->AssignedSlot();
   }
   return nullptr;
@@ -3343,6 +3344,15 @@ void Node::UnregisterScrollTimeline(ScrollTimeline* timeline) {
   EnsureRareData().UnregisterScrollTimeline(timeline);
 }
 
+void Node::SetManuallyAssignedSlot(HTMLSlotElement* slot) {
+  EnsureFlatTreeNodeData().SetManuallyAssignedSlot(slot);
+}
+HTMLSlotElement* Node::ManuallyAssignedSlot() {
+  if (FlatTreeNodeData* data = GetFlatTreeNodeData())
+    return data->ManuallyAssignedSlot();
+  return nullptr;
+}
+
 void Node::Trace(Visitor* visitor) const {
   visitor->Trace(parent_or_shadow_host_node_);
   visitor->Trace(previous_);

diff --git a/blink/renderer/core/dom/node.h b/blink/renderer/core/dom/node.h
@@ -908,6 +908,10 @@ class CORE_EXPORT Node : public EventTarget {
   void RegisterScrollTimeline(ScrollTimeline*);
   void UnregisterScrollTimeline(ScrollTimeline*);
 
+  // For the imperative slot distribution API.
+  void SetManuallyAssignedSlot(HTMLSlotElement* slot);
+  HTMLSlotElement* ManuallyAssignedSlot();
+
   // For Element.
   void SetHasDisplayLockContext() { SetFlag(kHasDisplayLockContext); }
   bool HasDisplayLockContext() const { return GetFlag(kHasDisplayLockContext); }

diff --git a/blink/renderer/core/dom/shadow_root.cc b/blink/renderer/core/dom/shadow_root.cc
@@ -62,7 +62,7 @@ ShadowRoot::ShadowRoot(Document& document, ShadowRootType type)
       type_(static_cast<unsigned>(type)),
       registered_with_parent_shadow_root_(false),
       delegates_focus_(false),
-      slot_assignment_mode_(static_cast<unsigned>(SlotAssignmentMode::kAuto)),
+      slot_assignment_mode_(static_cast<unsigned>(SlotAssignmentMode::kNamed)),
       needs_dir_auto_attribute_update_(false),
       supports_name_based_slot_assignment_(false),
       unused_(0) {}

diff --git a/blink/renderer/core/dom/shadow_root_init.idl b/blink/renderer/core/dom/shadow_root_init.idl
@@ -5,7 +5,7 @@
 // Spec: https://w3c.github.io/webcomponents/spec/shadow/#shadowrootinit-dictionary
 
 enum ShadowRootMode { "open", "closed" };
-enum SlotAssignmentMode { "manual", "auto" };
+enum SlotAssignmentMode { "manual", "named" };
 
 dictionary ShadowRootInit {
     required ShadowRootMode mode;

diff --git a/blink/renderer/core/dom/slot_assignment.cc b/blink/renderer/core/dom/slot_assignment.cc
@@ -67,10 +67,8 @@ void SlotAssignment::DidRemoveSlot(HTMLSlotElement& slot) {
   needs_collect_slots_ = true;
 
   if (owner_->IsManualSlotting()) {
-    auto& candidates = slot.AssignedNodesCandidates();
+    auto& candidates = slot.ManuallyAssignedNodes();
     if (candidates.size()) {
-      ClearCandidateNodes(candidates);
-      slot.ClearAssignedNodesCandidates();
       SetNeedsAssignmentRecalc();
       slot.DidSlotChangeAfterRemovedFromShadowTree();
     }
@@ -278,56 +276,58 @@ void SlotAssignment::RecalcAssignment() {
           FindSlotByName(HTMLSlotElement::UserAgentCustomAssignSlotName());
     }
 
-    bool is_manual_slot_assignment = owner_->IsManualSlotting();
-    // Replaces candidate_assigned_slot_map_ after the loop, to avoid stale
-    // references resulting from calls to slot->DidRecalcAssignedNodes().
-    HeapHashMap<Member<Node>, Member<HTMLSlotElement>> candidate_map;
-
-    for (Node& child : NodeTraversal::ChildrenOf(owner_->host())) {
-      if (!child.IsSlotable())
-        continue;
-
-      HTMLSlotElement* slot = nullptr;
-      if (supports_name_based_slot_assignment) {
-        if (is_manual_slot_assignment) {
-          if (auto* candidate_slot = candidate_assigned_slot_map_.at(&child)) {
-            if (candidate_slot->ContainingShadowRoot() == owner_) {
-              slot = candidate_slot;
-            } else {
-              candidate_assigned_slot_map_.erase(&child);
-              const AtomicString& slot_name =
-                  (candidate_slot->GetName() != g_empty_atom)
-                      ? candidate_slot->GetName()
-                      : "SLOT";
-              owner_->GetDocument().AddConsoleMessage(MakeGarbageCollected<
-                                                      ConsoleMessage>(
-                  mojom::blink::ConsoleMessageSource::kRendering,
-                  mojom::blink::ConsoleMessageLevel::kWarning,
-                  "This code triggered a slot assignment recalculation. At "
-                  "the time of this recalculation, the assigned node '" +
-                      child.nodeName() + "' was no longer a child of '" +
-                      slot_name +
-                      "'s parent shadow host, so it could not be assigned."));
-            }
+    if (owner_->IsManualSlotting()) {
+      // |children_to_clear| starts with the list of all light-dom children of
+      // the host that are *currently slotted*. Any of those that aren't slotted
+      // during this recalc will then have their flat tree data cleared.
+      HeapHashSet<Member<Node>> children_to_clear;
+      for (Node& child : NodeTraversal::ChildrenOf(owner_->host())) {
+        if (!child.GetFlatTreeNodeData())
+          continue;
+        children_to_clear.insert(&child);
+      }
+
+      for (Member<HTMLSlotElement> slot : Slots()) {
+        for (Node* slottable : slot->ManuallyAssignedNodes()) {
+          // Some of the manually assigned nodes might have been moved
+          // to other trees or documents. In that case, don't assign them
+          // here, but also don't remove/invalidate them in the manually
+          // assigned nodes list, in case they come back later.
+          if (slottable && slottable->IsChildOfShadowHost() &&
+              slottable->parentElement() == owner_->host()) {
+            slot->AppendAssignedNode(*slottable);
+            children_to_clear.erase(slottable);
           }
-        } else {
-          slot = FindSlotByName(child.SlotName());
         }
-      } else {
-        if (user_agent_custom_assign_slot && ShouldAssignToCustomSlot(child)) {
-          slot = user_agent_custom_assign_slot;
+      }
+
+      for (auto child : children_to_clear) {
+        child->ClearFlatTreeNodeData();
+        child->RemovedFromFlatTree();
+      }
+    } else {
+      for (Node& child : NodeTraversal::ChildrenOf(owner_->host())) {
+        if (!child.IsSlotable())
+          continue;
+
+        HTMLSlotElement* slot = nullptr;
+        if (supports_name_based_slot_assignment) {
+          slot = FindSlotByName(child.SlotName());
         } else {
-          slot = user_agent_default_slot;
+          if (user_agent_custom_assign_slot &&
+              ShouldAssignToCustomSlot(child)) {
+            slot = user_agent_custom_assign_slot;
+          } else {
+            slot = user_agent_default_slot;
+          }
         }
-      }
 
-      if (slot) {
-        slot->AppendAssignedNode(child);
-        if (is_manual_slot_assignment)
-          candidate_map.Set(&child, slot);
-      } else {
-        child.ClearFlatTreeNodeData();
-        child.RemovedFromFlatTree();
+        if (slot) {
+          slot->AppendAssignedNode(child);
+        } else {
+          child.ClearFlatTreeNodeData();
+          child.RemovedFromFlatTree();
+        }
       }
     }
 
@@ -339,9 +339,6 @@ void SlotAssignment::RecalcAssignment() {
 
     for (auto& slot : Slots())
       slot->DidRecalcAssignedNodes();
-
-    if (is_manual_slot_assignment)
-      candidate_assigned_slot_map_.swap(candidate_map);
   }
 
   // Update an dir=auto flag from a host of slots to its all descendants.
@@ -393,9 +390,10 @@ HTMLSlotElement* SlotAssignment::FindSlotInUserAgentShadow(
   return user_agent_default_slot;
 }
 
-HTMLSlotElement* SlotAssignment::FindSlotInManualSlotting(const Node& node) {
-  auto* slot = candidate_assigned_slot_map_.at(const_cast<Node*>(&node));
-  if (slot && slot->ContainingShadowRoot() == owner_)
+HTMLSlotElement* SlotAssignment::FindSlotInManualSlotting(Node& node) {
+  auto* slot = node.ManuallyAssignedSlot();
+  if (slot && slot->ContainingShadowRoot() == owner_ &&
+      node.IsChildOfShadowHost() && node.parentElement() == owner_->host())
     return slot;
 
   return nullptr;
@@ -423,29 +421,10 @@ HTMLSlotElement* SlotAssignment::GetCachedFirstSlotWithoutAccessingNodeTree(
   return nullptr;
 }
 
-bool SlotAssignment::UpdateCandidateNodeAssignedSlot(Node& node,
-                                                     HTMLSlotElement& slot) {
-  bool updated = false;
-  auto* prev_slot = candidate_assigned_slot_map_.at(&node);
-  if (prev_slot && prev_slot != &slot) {
-    prev_slot->RemoveAssignedNodeCandidate(node);
-    updated = true;
-  }
-
-  candidate_assigned_slot_map_.Set(&node, &slot);
-  return updated;
-}
-
-void SlotAssignment::ClearCandidateNodes(
-    const HeapLinkedHashSet<Member<Node>>& candidates) {
-  candidate_assigned_slot_map_.RemoveAll(candidates);
-}
-
 void SlotAssignment::Trace(Visitor* visitor) const {
   visitor->Trace(slots_);
   visitor->Trace(slot_map_);
   visitor->Trace(owner_);
-  visitor->Trace(candidate_assigned_slot_map_);
   visitor->Trace(candidate_directionality_set_);
 }
 

diff --git a/blink/renderer/core/dom/slot_assignment.h b/blink/renderer/core/dom/slot_assignment.h
@@ -54,8 +54,6 @@ class SlotAssignment final : public GarbageCollected<SlotAssignment> {
   bool NeedsAssignmentRecalc() const { return needs_assignment_recalc_; }
   void SetNeedsAssignmentRecalc();
   void RecalcAssignment();
-  bool UpdateCandidateNodeAssignedSlot(Node&, HTMLSlotElement&);
-  void ClearCandidateNodes(const HeapLinkedHashSet<Member<Node>>& candidates);
   HeapHashSet<Member<Node>>& GetCandidateDirectionality() {
     return candidate_directionality_set_;
   }
@@ -66,7 +64,7 @@ class SlotAssignment final : public GarbageCollected<SlotAssignment> {
     kRenamed,
   };
 
-  HTMLSlotElement* FindSlotInManualSlotting(const Node&);
+  HTMLSlotElement* FindSlotInManualSlotting(Node&);
   HTMLSlotElement* FindSlotInUserAgentShadow(const Node&) const;
 
   void CollectSlots();
@@ -84,9 +82,6 @@ class SlotAssignment final : public GarbageCollected<SlotAssignment> {
   unsigned needs_collect_slots_ : 1;
   unsigned needs_assignment_recalc_ : 1;
   unsigned slot_count_ : 30;
-  // TODO: (1067157) Ensure references inside the map are GCed.
-  HeapHashMap<Member<Node>, Member<HTMLSlotElement>>
-      candidate_assigned_slot_map_;
   HeapHashSet<Member<Node>> candidate_directionality_set_;
 };