Adds ability for slot to preserve order of its manually assigned nodes.

Prior to this CL, the ordering of the manually assigned nodes is not
preserved. Nodes were assigned in tree-order. In addition, assignment
is not absolute. A slotable node can be assign to multiple slots, and
where it appears is when the assigned slot first appear in a tree-order
traversal, not the last slot the node is assigned to.

This CL does two things. One, the order of manually assigned node is
preserved. Two, assignment is absolute. The implementation uses a
HeapHashMap, candidate_assigned_slot_map_, to keep track of
assignment node -> slot. It uses this map during node assignment to
find if another assigned slot exists. When found, the node is
removed from the previously assigned slot.

Preserving the ordering of manually assigned nodes is done in
HTMLSlotElement::UpdateManuallyAssignedNodesOrdering(). This is called
at the end of SlotAssignment::RecalcAssignment(). RecalcAssignment()
walks the ShadowHost's children in tree-order, so the assigned node
could be out of order from how these nodes were assigned.  I thought
about making a separate function for manually assigned nodes, looping
though assigned nodes instead. But I decided against it at this point
due to the complexity of the recalc function.

For Reference: point 1 in this comment is addressed by this CL.
https://github.com/whatwg/html/issues/3534#issuecomment-537802687


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

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

@@ -17,6 +17,7 @@
 #include "third_party/blink/renderer/core/html/html_details_element.h"
 #include "third_party/blink/renderer/core/html/html_slot_element.h"
 #include "third_party/blink/renderer/core/html/parser/nesting_level_incrementer.h"
+#include "third_party/blink/renderer/core/inspector/console_message.h"
 
 namespace blink {
 
@@ -266,10 +267,24 @@ void SlotAssignment::RecalcAssignment() {
     HTMLSlotElement* slot = nullptr;
     if (!is_user_agent) {
       if (owner_->IsManualSlotting()) {
-        for (auto candidate_slot : Slots()) {
-          if (candidate_slot->AssignedNodesCandidate().Contains(&child)) {
+        if (auto* candidate_slot = candidate_assigned_slot_map_.at(&child)) {
+          if (candidate_slot->ContainingShadowRoot() == owner_) {
             slot = candidate_slot;
-            break;
+          } 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."));
           }
         }
       } else {
@@ -334,10 +349,10 @@ HTMLSlotElement* SlotAssignment::FindSlotInUserAgentShadow(
 }
 
 HTMLSlotElement* SlotAssignment::FindSlotInManualSlotting(const Node& node) {
-  for (auto& slot : Slots()) {
-    if (slot->AssignedNodesCandidate().Contains(const_cast<Node*>(&node)))
-      return slot;
-  }
+  auto* slot = candidate_assigned_slot_map_.at(const_cast<Node*>(&node));
+  if (slot && slot->ContainingShadowRoot() == owner_)
+    return slot;
+
   return nullptr;
 }
 
@@ -363,10 +378,28 @@ HTMLSlotElement* SlotAssignment::GetCachedFirstSlotWithoutAccessingNodeTree(
   return nullptr;
 }
 
+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);
+  }
+  candidate_assigned_slot_map_.Set(&node, &slot);
+}
+
+void SlotAssignment::ClearCandidateNodes(
+    const HeapLinkedHashSet<Member<Node>>& candidates) {
+  candidate_assigned_slot_map_.RemoveAll(candidates);
+}
+
 void SlotAssignment::Trace(Visitor* visitor) {
   visitor->Trace(slots_);
   visitor->Trace(slot_map_);
   visitor->Trace(owner_);
+  visitor->Trace(candidate_assigned_slot_map_);
 }
 
 }  // namespace blink

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

@@ -56,6 +56,8 @@ class SlotAssignment final : public GarbageCollected<SlotAssignment> {
   bool NeedsAssignmentRecalc() const { return needs_assignment_recalc_; }
   void SetNeedsAssignmentRecalc();
   void RecalcAssignment();
+  void UpdateCandidateNodeAssignedSlot(Node&, HTMLSlotElement&);
+  void ClearCandidateNodes(const HeapLinkedHashSet<Member<Node>>& candidates);
 
  private:
   enum class SlotMutationType {
@@ -81,6 +83,9 @@ 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.
+  HeapHashMap<Member<Node>, Member<HTMLSlotElement>>
+      candidate_assigned_slot_map_;
 };
 
 }  // namespace blink

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

@@ -180,6 +180,8 @@ void HTMLSlotElement::assign(HeapVector<Member<Node>> nodes,
     return;
   }
 
+  ContainingShadowRoot()->GetSlotAssignment().ClearCandidateNodes(
+      assigned_nodes_candidates_);
   assigned_nodes_candidates_.clear();
   auto* host = OwnerShadowHost();
   bool has_invalid_node = false;
@@ -193,7 +195,11 @@ void HTMLSlotElement::assign(HeapVector<Member<Node>> nodes,
       has_invalid_node = true;
       break;
     }
-    assigned_nodes_candidates_.insert(node);
+
+    // Before assignment, see if this node belongs to another slot.
+    ContainingShadowRoot()->GetSlotAssignment().UpdateCandidateNodeAssignedSlot(
+        *node, *this);
+    assigned_nodes_candidates_.AppendOrMoveToLast(node);
   }
 
   if (!has_invalid_node)
@@ -205,6 +211,22 @@ void HTMLSlotElement::AppendAssignedNode(Node& host_child) {
   assigned_nodes_.push_back(&host_child);
 }
 
+void HTMLSlotElement::UpdateManuallyAssignedNodesOrdering() {
+  if (assigned_nodes_.IsEmpty() || assigned_nodes_candidates_.IsEmpty())
+    return;
+
+  // TODO: (crbug.com/1067153) Add perf benchmark test for large assigned list.
+  HeapHashSet<Member<Node>> prev_nodes;
+  for (auto& node : assigned_nodes_) {
+    prev_nodes.insert(node);
+  }
+  assigned_nodes_.clear();
+  for (auto& node : assigned_nodes_candidates_) {
+    if (prev_nodes.Contains(node))
+      assigned_nodes_.push_back(node);
+  }
+}
+
 void HTMLSlotElement::ClearAssignedNodes() {
   assigned_nodes_.clear();
 }

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

@@ -73,6 +73,7 @@ class CORE_EXPORT HTMLSlotElement final : public HTMLElement {
 
   void WillRecalcAssignedNodes() { ClearAssignedNodes(); }
   void DidRecalcAssignedNodes() {
+    UpdateManuallyAssignedNodesOrdering();
     UpdateFlatTreeNodeDataForAssignedNodes();
     RecalcFlatTreeChildren();
   }
@@ -111,7 +112,7 @@ class CORE_EXPORT HTMLSlotElement final : public HTMLElement {
 
   // For imperative Shadow DOM distribution APIs
   void assign(HeapVector<Member<Node>> nodes, ExceptionState&);
-  const HeapHashSet<Member<Node>>& AssignedNodesCandidate() const {
+  const HeapLinkedHashSet<Member<Node>>& AssignedNodesCandidates() const {
     return assigned_nodes_candidates_;
   }
 
@@ -138,6 +139,8 @@ class CORE_EXPORT HTMLSlotElement final : public HTMLElement {
 
   void SetNeedsDistributionRecalcWillBeSetNeedsAssignmentRecalc();
 
+  // SlotAssignnment:recalc runs in tree order. Update to assigned order.
+  void UpdateManuallyAssignedNodesOrdering();
   void RecalcFlatTreeChildren();
   void UpdateFlatTreeNodeDataForAssignedNodes();
   void ClearAssignedNodesAndFlatTreeChildren();
@@ -147,8 +150,9 @@ class CORE_EXPORT HTMLSlotElement final : public HTMLElement {
 
   bool slotchange_event_enqueued_ = false;
 
-  // For imperative Shadow DOM distribution APIs
-  HeapHashSet<Member<Node>> assigned_nodes_candidates_;
+  // For imperative Shadow DOM distribution APIs.
+  // LinkedHashSet because candidates are ordered.
+  HeapLinkedHashSet<Member<Node>> assigned_nodes_candidates_;
 
   template <typename T, wtf_size_t S>
   struct LCSArray {

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

@@ -3,13 +3,16 @@ 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.
-FAIL Order of slotables is preserved in manual slot assignment. assert_array_equals: expected property 0 to be Element node <div id="c2"></div> but got Element node <div id="c1"></div> (expected array [Element node <div id="c2"></div>, Element node <div id="c3"></div>, Element node <div id="c1"></div>] got [Element node <div id="c1"></div>, Element node <div id="c2"></div>, Element node <div id="c3"></div>])
-FAIL Previously assigned slotable is moved to new slot when it's reassigned. assert_array_equals: expected property 0 to be Element node <div id="c2"></div> but got Element node <div id="c1"></div> (expected array [Element node <div id="c2"></div>, Element node <div id="c3"></div>, Element node <div id="c1"></div>] got [Element node <div id="c1"></div>, Element node <div id="c2"></div>, Element node <div id="c3"></div>])
-FAIL Assigning invalid nodes causes exception and slot returns to its previous state. assert_array_equals: expected property 0 to be Element node <div id="c2"></div> but got Element node <div id="c1"></div> (expected array [Element node <div id="c2"></div>, Element node <div id="c3"></div>, Element node <div id="c1"></div>] got [Element node <div id="c1"></div>, Element node <div id="c2"></div>, Element node <div id="c3"></div>])
+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.
+FAIL Assigning invalid nodes causes exception and slot returns to its previous state. assert_array_equals: lengths differ, expected array [] length 0, got [Element node <div id="c1"></div>, Element node <div id="c2"></div>] length 2
 PASS Moving a slot to a new host, the slot loses its previously assigned slotables.
-FAIL Moving a slot's tree order position within a shadow host has no impact on its assigned slotables. assert_array_equals: lengths differ, expected array [Element node <div id="c1"></div>, Element node <div id="c2"></div>, Element node <div id="c3"></div>] length 3, got [] length 0
-FAIL Appending slotable to different host, it loses slot assignment. It can be re-assigned within a new host. Failed to execute 'assign' on 'HTMLSlotElement': The object must have a callable @@iterator property.
-FAIL Assignment with the same node in parameters should be ignored, last one wins. assert_array_equals: expected property 0 to be Element node <div id="c2"></div> but got Element node <div id="c1"></div> (expected array [Element node <div id="c2"></div>, Element node <div id="c1"></div>] got [Element node <div id="c1"></div>, Element node <div id="c2"></div>])
+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

@@ -136,6 +136,21 @@ test(() => {
   assert_equals(tTree.c3.assignedSlot, tTree.s3);
 }, 'Previously assigned slotable is moved to new slot when it\'s reassigned.');
 
+test(() => {
+ let tTree = createTestTree(test_assign);
+
+  tTree.s1.assign([tTree.c1]);
+  tTree.s2.assign([tTree.c2, tTree.c1]);
+  tTree.s3.assign([tTree.c1, tTree.c3]);
+
+  assert_array_equals(tTree.s1.assignedNodes(), []);
+  assert_array_equals(tTree.s2.assignedNodes(), [tTree.c2]);
+  assert_array_equals(tTree.s3.assignedNodes(), [tTree.c1, tTree.c3]);
+  assert_equals(tTree.c1.assignedSlot, tTree.s3);
+  assert_equals(tTree.c2.assignedSlot, tTree.s2);
+  assert_equals(tTree.c3.assignedSlot, tTree.s3);
+}, 'Order and assignment of nodes are preserved during multiple assignment in a row.');
+
 test(() => {
   let tTree = createTestTree(test_assign);
 
@@ -194,7 +209,7 @@ test(() => {
   assert_array_equals(tTree.s4.assignedNodes(), []);
 
   tTree.ns1.append(tTree.s1);
-  assert_array_equals(tTree.s1.assignedNodes(), [tTree.c1, tTree.c2, tTree.c3]);
+  assert_array_equals(tTree.s1.assignedNodes(), []);
 }, 'Moving a slot\'s tree order position within a shadow host has no impact on its assigned slotables.');
 
 test(() => {
@@ -207,11 +222,41 @@ test(() => {
   assert_array_equals(tTree.s1.assignedNodes(), [tTree.c2, tTree.c3]);
   assert_array_equals(tTree.s4.assignedNodes(), []);
 
-  tTree.s4.assign(tTree.c1);
+  tTree.s4.assign([tTree.c1]);
   assert_array_equals(tTree.s4.assignedNodes(), [tTree.c1]);
   assert_equals(tTree.c1.assignedSlot, tTree.s4);
 }, 'Appending slotable to different host, it loses slot assignment. It can be re-assigned within a new host.');
 
+test(() => {
+  let tTree = createTestTree(test_assign);
+
+  tTree.s1.assign([tTree.c1]);
+  assert_array_equals(tTree.s1.assignedNodes(), [tTree.c1]);
+
+  tTree.shadow_root4.insertBefore(tTree.s1, tTree.s4);
+  assert_array_equals(tTree.s1.assignedNodes(), []);
+  // Don't trigger slot assignment on previous shadow root.
+  // assert_array_equals(tTree.s2.assignedNodes(), []);
+
+  tTree.shadow_root.insertBefore(tTree.s1, tTree.s2);
+  assert_array_equals(tTree.s1.assignedNodes(), []);
+}, '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.');
+
+test(() => {
+  let tTree = createTestTree(test_assign);
+
+  tTree.s1.assign([tTree.c1]);
+  assert_array_equals(tTree.s1.assignedNodes(), [tTree.c1]);
+
+  tTree.shadow_root4.insertBefore(tTree.s1, tTree.s4);
+  assert_array_equals(tTree.s1.assignedNodes(), []);
+  // Trigger slot assignment on previous shadow root.
+  assert_array_equals(tTree.s2.assignedNodes(), []);
+
+  tTree.shadow_root.insertBefore(tTree.s1, tTree.s2);
+  assert_array_equals(tTree.s1.assignedNodes(), []);
+}, 'Previously assigned node should not be assigned if slot moved to a new shadow root. The slot remains empty when moved back, trigger recalc.');
+
 test(() => {
   let tTree = createTestTree(test_assign);
 

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

@@ -27,37 +27,6 @@ See https://crbug.com/869308
   </div>
 </div>
 <script>
-test(() => {
-  let n = createTestTree(test2);
-  assert_array_equals(n.s2.assignedElements(), []);
-  assert_equals(n.c1.assignedSlot, null);
-
-  n.s2.assign([n.c1]);
-
-  assert_array_equals(n.s2.assignedNodes(), [n.c1]);
-  assert_equals(n.c1.assignedSlot, n.s2);
-
-  n.s1.assign([n.c2,n.c1]);
-  assert_array_equals(n.s1.assignedNodes(), [n.c1,n.c2]);
-  assert_array_equals(n.s2.assignedNodes(), []);
-  assert_equals(n.c1.assignedSlot, n.s1);
-  assert_equals(n.c2.assignedSlot, n.s1);
-
-  n.s1.assign([n.c2]);
-  assert_array_equals(n.s1.assignedNodes(), [n.c2]);
-  assert_array_equals(n.s2.assignedNodes(), [n.c1]);
-}, 'assignedNodes/Slot can be used in manual slotting');
-
-test(() => {
-  let n = createTestTree(test2);
-  n.s1.assign([n.c1]);
-  n.s2.assign([n.c1]);
-  assert_array_equals(n.s2.assignedNodes(), []);
-
-  n.shadow_root.insertBefore(n.s2,n.s1);
-  assert_array_equals(n.s2.assignedNodes(), [n.c1]);
-}, 'A node should be assigned to a slot when the slot is inserted before the assigned slot');
-
 test(() => {
   let n = createTestTree(test2);
   n.s1.assign([n.c1]);
@@ -72,19 +41,6 @@ test(() => {
   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.s1.assign([n.c1]);
-  assert_array_equals(n.s1.assignedNodes(), [n.c1]);
-  n.shadow_root1.insertBefore(n.s1,n.s5);
-
-  assert_array_equals(n.s1.assignedNodes(), []);
-  n.s1.assign([]);
-  n.shadow_root.insertBefore(n.s1,n.s2);
-
-  assert_array_equals(n.s1.assignedNodes(), []);
-}, 'A slot should not be assigned to a node once after the slot is inserted in another shadow root, and assigned another slot.')
-
 test(() => {
   let n = createTestTree(test2);
   n.shadow_root.insertBefore(n.s1,n.s2);

third_party/blink/web_tests/shadow-dom/imperative-apis/custom-detail-summary.js

@@ -27,11 +27,13 @@ customElements.define("my-detail", class extends HTMLElement {
         slot2.style.display = "block";
         child2.innerHTML = "▾ ";
         child1.innerText = "";
+        slot2.assign(target.childNodes);
       });
       child2.addEventListener('click', (e) => {
         slot2.style.display = "none";
         child1.innerHTML = "▸ ";
         child2.innerText = "";
+        slot1.assign([child1,child2, target.querySelector(':scope > my-summary')]);
       });
       const shadowRoot = target.shadowRoot;
       shadowRoot.appendChild(slot1);
@@ -45,8 +47,6 @@ customElements.define("my-detail", class extends HTMLElement {
         } else {
           slot1.assign([child1,child2]);
         }
-        slot2.assign(target.childNodes);
-        slot2.style.display = "none";
       });
     observer.observe(this, {childList: true});
     }