Adds ability to clear out assigned nodes to slot when slot is removed from shadow root.

Prior to this CL, when a slot with imperatively assigned nodes is
removed from its shadow root, assignment recalc is deferred until
an API (Slot.assignedNodes, Node.assignedSlot) or a LifeCycleUpdate
triggers the recalc. If no assignment recalc is triggered and the
slot is added back to its original shadow root, its imperatively
assigned nodes will reappear. This is an unwanted side effect
because if an assignment recalc is trigger before slot is added back,
its assigned nodes will be cleared.

This CL fixes this side effect by clearing a slot's imperatively
assigned nodes whenever it's removed from its parent node. Thus,
regardless of whether a recalc is trigger before the slot is added back
or not, the end result is that the slot's assigned nodes are empty.

Note: With this CL, even moving the slot inside it's shadow root will
clear out its imperatively assigned nodes. See example:
   host
       ---sr
           --s1
           --s2
       --c1

s2.assign([c1]);
sr.insertBefore(s2, s1);
assert_array_equals(s2.assignedNodes(), []);

I believe this is correct behavior. The output should be the same as
removed = sr.removeChild(s2)
sr.appendChild(removed);

Bug: 869308
Change-Id: Ie1247684b77d5c4b8e5f113421807c0c187224f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2135371Reviewed-by: Mason Freed <masonfreed@chromium.org>
Commit-Queue: Yu Han <yuzhehan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758334}

third_party/blink/renderer/core/dom/slot_assignment.cc

@@ -44,7 +44,7 @@ void SlotAssignment::DidAddSlot(HTMLSlotElement& slot) {
   needs_collect_slots_ = true;
 
   if (owner_->IsManualSlotting()) {
-    SetNeedsAssignmentRecalc();
+    // Adding a new slot should not require assignment recalc.
     return;
   }
 
@@ -67,7 +67,12 @@ void SlotAssignment::DidRemoveSlot(HTMLSlotElement& slot) {
   needs_collect_slots_ = true;
 
   if (owner_->IsManualSlotting()) {
-    SetNeedsAssignmentRecalc();
+    auto& candidates = slot.AssignedNodesCandidates();
+    if (candidates.size()) {
+      ClearCandidateNodes(candidates);
+      slot.ClearAssignedNodesCandidates();
+      SetNeedsAssignmentRecalc();
+    }
     return;
   }
 
@@ -391,12 +396,9 @@ HTMLSlotElement* SlotAssignment::GetCachedFirstSlotWithoutAccessingNodeTree(
 void SlotAssignment::UpdateCandidateNodeAssignedSlot(Node& node,
                                                      HTMLSlotElement& slot) {
   auto* prev_slot = candidate_assigned_slot_map_.at(&node);
-  if (prev_slot && prev_slot != &slot) {
-    auto candidates = prev_slot->AssignedNodesCandidates();
-    auto it = candidates.find(&node);
-    if (it != candidates.end())
-      candidates.erase(it);
-  }
+  if (prev_slot && prev_slot != &slot)
+    prev_slot->RemoveAssignedNodeCandidate(node);
+
   candidate_assigned_slot_map_.Set(&node, &slot);
 }
 

third_party/blink/renderer/core/dom/slot_assignment.h

@@ -83,7 +83,7 @@ class SlotAssignment final : public GarbageCollected<SlotAssignment> {
   unsigned needs_collect_slots_ : 1;
   unsigned needs_assignment_recalc_ : 1;
   unsigned slot_count_ : 30;
-  // TODO: (crbug.com/1067157) Ensure references inside the map are GCed.
+  // TODO: (1067157) Ensure references inside the map are GCed.
   HeapHashMap<Member<Node>, Member<HTMLSlotElement>>
       candidate_assigned_slot_map_;
 };

third_party/blink/renderer/core/html/html_slot_element.cc

@@ -219,7 +219,7 @@ void HTMLSlotElement::UpdateManuallyAssignedNodesOrdering() {
   if (assigned_nodes_.IsEmpty() || assigned_nodes_candidates_.IsEmpty())
     return;
 
-  // TODO: (crbug.com/1067153) Add perf benchmark test for large assigned list.
+  // TODO: (1067153) Add perf benchmark test for large assigned list.
   HeapHashSet<Member<Node>> prev_nodes;
   for (auto& node : assigned_nodes_) {
     prev_nodes.insert(node);
@@ -231,6 +231,16 @@ void HTMLSlotElement::UpdateManuallyAssignedNodesOrdering() {
   }
 }
 
+void HTMLSlotElement::RemoveAssignedNodeCandidate(Node& node) {
+  auto it = assigned_nodes_candidates_.find(&node);
+  if (it != assigned_nodes_candidates_.end())
+    assigned_nodes_candidates_.erase(it);
+}
+
+void HTMLSlotElement::ClearAssignedNodesCandidates() {
+  assigned_nodes_candidates_.clear();
+}
+
 void HTMLSlotElement::ClearAssignedNodes() {
   assigned_nodes_.clear();
 }

third_party/blink/renderer/core/html/html_slot_element.h

@@ -115,6 +115,8 @@ class CORE_EXPORT HTMLSlotElement final : public HTMLElement {
   const HeapLinkedHashSet<Member<Node>>& AssignedNodesCandidates() const {
     return assigned_nodes_candidates_;
   }
+  void ClearAssignedNodesCandidates();
+  void RemoveAssignedNodeCandidate(Node&);
 
   void Trace(Visitor*) override;
 

third_party/blink/web_tests/external/wpt/shadow-dom/slots-imperative-slot-api.tentative-expected.txt

-This is a testharness.js-based test.
-PASS attachShadow can take slotAssignment parameter.
-PASS Imperative slot API throws exception when not slotAssignment != 'manual'.
-PASS Imperative slot API throws exception when slotable parentNode != slot's host.
-PASS Imperative slot API can assign nodes in manual slot assignment.
-PASS Order of slotables is preserved in manual slot assignment.
-PASS Previously assigned slotable is moved to new slot when it's reassigned.
-PASS Order and assignment of nodes are preserved during multiple assignment in a row.
-PASS Assigning invalid nodes causes exception and slot returns to its previous state.
-PASS Moving a slot to a new host, the slot loses its previously assigned slotables.
-PASS Moving a slot's tree order position within a shadow host has no impact on its assigned slotables.
-PASS Appending slotable to different host, it loses slot assignment. It can be re-assigned within a new host.
-FAIL Previously assigned node should not be assigned if slot moved to a new shadow root. The slot remains empty when moved back, no trigger recalc. assert_array_equals: lengths differ, expected array [] length 0, got [Element node <div id="c1"></div>] length 1
-PASS Previously assigned node should not be assigned if slot moved to a new shadow root. The slot remains empty when moved back, trigger recalc.
-PASS Assignment with the same node in parameters should be ignored, last one wins.
-PASS Removing a slot from DOM resets its slotable's slot assignment.
-Harness: the test ran to completion.
-

third_party/blink/web_tests/external/wpt/shadow-dom/slots-imperative-slot-api.tentative.html

@@ -277,4 +277,29 @@ test(() => {
   assert_equals(tTree.c2.assignedSlot, null);
   assert_equals(tTree.c3.assignedSlot, null);
 }, 'Removing a slot from DOM resets its slotable\'s slot assignment.');
+
+test(() => {
+  let tTree = createTestTree(test_assign);
+
+  tTree.s1.assign([tTree.c1]);
+  tTree.s2.assign([tTree.c2]);
+  tTree.s3.assign([tTree.c3]);
+  tTree.shadow_root.insertBefore(tTree.s2, tTree.s1);
+  tTree.shadow_root.insertBefore(tTree.s3, tTree.s1);
+
+  assert_array_equals(tTree.s1.assignedNodes(), [tTree.c1]);
+  assert_array_equals(tTree.s2.assignedNodes(), []);
+  assert_array_equals(tTree.s3.assignedNodes(), []);
+  assert_equals(tTree.c1.assignedSlot, tTree.s1);
+  assert_equals(tTree.c2.assignedSlot, null);
+  assert_equals(tTree.c3.assignedSlot, null);
+
+  tTree.s2.remove();
+
+  assert_array_equals(tTree.s1.assignedNodes(), [tTree.c1]);
+  assert_array_equals(tTree.s3.assignedNodes(), []);
+  assert_equals(tTree.c1.assignedSlot, tTree.s1);
+  assert_equals(tTree.c2.assignedSlot, null);
+  assert_equals(tTree.c3.assignedSlot, null);
+}, 'A slot should be cleared of assigned nodes even if it\'s re-inserted into the same shadow root.');
 </script>

third_party/blink/web_tests/shadow-dom/imperative-api.html

-<!DOCTYPE html>
-<!--
-Tests for Imperative Shadow DOM Distribution API.
-See https://crbug.com/869308
--->
-<script src="../resources/testharness.js"></script>
-<script src="../resources/testharnessreport.js"></script>
-<script src="resources/shadow-dom.js"></script>
-
-</script>
-<div id="test2">
-  <div id="host">
-    <template id="shadow_root" data-mode="open" data-slot-assignment="manual">
-      <slot id="s1"></slot>
-      <slot id="s2"></slot>
-      <slot id="s3"></slot>
-    </template>
-    <div id="c1"></div>
-    <div id="c2"></div>
-    <div id="c3"></div>
-  </div>
-  <div id="c4"></div>
-  <div id="host4">
-    <template id="shadow_root1" data-mode="open" data-slot-assignment="manual">
-      <slot id="s5" name="s5"></slot>
-    </template>
-  </div>
-</div>
-<script>
-test(() => {
-  let n = createTestTree(test2);
-  n.s1.assign([n.c1]);
-  n.s2.assign([n.c1]);
-  n.s3.assign([n.c1]);
-  n.shadow_root.insertBefore(n.s2,n.s1);
-  n.shadow_root.insertBefore(n.s3,n.s1);
-
-  n.s2.remove();
-
-  assert_array_equals(n.s1.assignedNodes(), []);
-  assert_array_equals(n.s3.assignedNodes(), [n.c1]);
-}, 'A slot should be assigned to a node even after the slot is inserted after the assigned slot, and the assigned slot are removed')
-
-test(() => {
-  let n = createTestTree(test2);
-  n.shadow_root.insertBefore(n.s1,n.s2);
-  n.s2.assign([n.c1,n.c1]);
-  assert_array_equals(n.s2.assignedNodes(), [n.c1]);
-
-  n.s1.assign([n.c1]);
-  assert_array_equals(n.s2.assignedNodes(), []);
-
-  n.shadow_root1.appendChild(n.s2);
-  assert_array_equals(n.s2.assignedNodes(), []);
-}, 'Same nodes should be ignored');
-
-test(() => {
-  let n = createTestTree(test2);
-  n.s1.assign([n.c3]);
-  n.s1.assign([n.c3]);
-
-  n.s1.remove();
-
-  assert_equals(n.c3.assignedSlot, null);
-}, 'Same assignment should be ignored');
-
-</script>
\ No newline at end of file