Fix slotchange event issues for imperative shadowdom API

In this CL, We add the functions that support to call slotchange event when nodes are assigned from assign function. Previous implementation CL: crrev.com/c/1201513 Bug: 869308 Change-Id: I44cfc94c26141630aa82ea725566a8c7273eacf5 Reviewed-on: https://chromium-review.googlesource.com/1226477 Commit-Queue: Kyoko Muto <kymuto@google.com> Reviewed-by: Hayato Ito <hayato@chromium.org> Cr-Commit-Position: refs/heads/master@{#593166}

Fix slotchange event issues for imperative shadowdom API
In this CL, We add the functions that support to call slotchange event when nodes are assigned from assign function. Previous implementation CL: crrev.com/c/1201513 Bug: 869308 Change-Id: I44cfc94c26141630aa82ea725566a8c7273eacf5 Reviewed-on: https://chromium-review.googlesource.com/1226477 Commit-Queue: Kyoko Muto <kymuto@google.com> Reviewed-by: Hayato Ito <hayato@chromium.org> Cr-Commit-Position: refs/heads/master@{#593166}
d23d2a34 · Kyoko Muto · Commit Bot · 9acff93f · d23d2a34 · d23d2a34
Commit d23d2a34 authored Sep 21, 2018 by Kyoko Muto Committed by Commit Bot Sep 21, 2018
4 changed files
--- a/third_party/WebKit/LayoutTests/shadow-dom/imperative-api-notslotchange.html
+++ b/third_party/WebKit/LayoutTests/shadow-dom/imperative-api-notslotchange.html
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+</head>
+<body>
+<div id="log"></div>
+<script>
+    const test = async_test('slotchange event must not be fired');
+    let eventCount = 0;
+    let slot1;
+    let child1;
+    test.step(function () {
+        const host = document.createElement('div');
+        document.body.appendChild(host);
+        const shadowRoot = host.attachShadow({mode: 'open', slotting: 'manual'});
+        slot1 = document.createElement('slot');
+        slot1.addEventListener('slotchange', function (event) {
+            eventCount++;
+        });
+        shadowRoot.appendChild(slot1);
+        child1 = document.createElement('div');
+        host.appendChild(child1);
+        slot1.assign([child1]);
+    });
+    setTimeout(function () {
+        test.step(function () {
+            assert_equals(eventCount, 1, 'slotchange event must be fired  when node is assigned');
+            slot1.assign([child1]);
+        });
+        setTimeout(function () {
+            test.step(function () {
+                assert_equals(eventCount, 1, 'slotchange event must not be fired  when slot is assigned same node');
+            });
+            test.done();
+        }, 1);
+    }, 1);
+</script>
+</body>
+</html>
--- a/third_party/blink/renderer/core/dom/slot_assignment.cc
+++ b/third_party/blink/renderer/core/dom/slot_assignment.cc
@@ -127,7 +127,7 @@ void SlotAssignment::DidAddSlotInternalInManualMode(HTMLSlotElement& slot) {
  for (auto node : slot.AssignedNodesCandidate()) {
    InsertSlotInChildSlotMap(slot, *node);
  }
-  CallSlotChangeIfNeeded(slot);
+  CallSlotChangeAfterAddition(slot);
 }
 void SlotAssignment::DidRemoveSlotInternal(
@@ -415,16 +415,40 @@ HTMLSlotElement* SlotAssignment::FindSlotChange(HTMLSlotElement& slot,
  return nullptr;
 }
-void SlotAssignment::CallSlotChangeIfNeeded(HTMLSlotElement& slot) {
+void SlotAssignment::CallSlotChangeAfterRemovedFromAssignFunction(
+    HTMLSlotElement& slot) {
  for (Node& child : NodeTraversal::ChildrenOf(owner_->host())) {
-    auto* changed_slot = FindSlotChange(slot, child);
+    if (slot.AssignedNodesCandidate().Contains(&child)) {
-    if (changed_slot) {
+      CallSlotChangeIfNeeded(slot, child);
-      slot.SignalSlotChange();
+    }
-      if (changed_slot != slot)
+  }
-        changed_slot->SignalSlotChange();
+}
+void SlotAssignment::CallSlotChangeAfterAdditionFromAssignFunction(
+    HTMLSlotElement& slot,
+    const HeapVector<Member<Node>>& added_assign_nodes) {
+  for (Node& child : NodeTraversal::ChildrenOf(owner_->host())) {
+    if (added_assign_nodes.Contains(&child)) {
+      CallSlotChangeIfNeeded(slot, child);
    }
  }
 }
+void SlotAssignment::CallSlotChangeAfterAddition(HTMLSlotElement& slot) {
+  for (Node& child : NodeTraversal::ChildrenOf(owner_->host())) {
+    CallSlotChangeIfNeeded(slot, child);
+  }
+}
+void SlotAssignment::CallSlotChangeIfNeeded(HTMLSlotElement& slot,
+                                            Node& child) {
+  auto* changed_slot = FindSlotChange(slot, child);
+  if (changed_slot) {
+    slot.SignalSlotChange();
+    if (changed_slot != slot)
+      changed_slot->SignalSlotChange();
+  }
+}
 void SlotAssignment::CallSlotChangeAfterRemoved(HTMLSlotElement& slot) {
  for (Node& child : NodeTraversal::ChildrenOf(owner_->host())) {
    auto* changed_slot = FindSlotChange(slot, child);

--- a/third_party/blink/renderer/core/dom/slot_assignment.h
+++ b/third_party/blink/renderer/core/dom/slot_assignment.h
@@ -43,8 +43,14 @@ class SlotAssignment final : public GarbageCollected<SlotAssignment> {
                                  const AtomicString& new_value);
  bool FindHostChildBySlotName(const AtomicString& slot_name) const;
-  void CallSlotChangeIfNeeded(HTMLSlotElement& slot);
+  void CallSlotChangeAfterRemovedFromAssignFunction(HTMLSlotElement& slot);
+  void CallSlotChangeAfterAdditionFromAssignFunction(
+      HTMLSlotElement& slot,
+      const HeapVector<Member<Node>>& added_assign_nodes);
+  void CallSlotChangeAfterAddition(HTMLSlotElement& slot);
  void CallSlotChangeAfterRemoved(HTMLSlotElement& slot);
+  void CallSlotChangeIfNeeded(HTMLSlotElement& slot, Node& child);
  HTMLSlotElement* FindSlotChange(HTMLSlotElement& slot, Node& child);
  void DeleteSlotInChildSlotMap(HTMLSlotElement& slot);

--- a/third_party/blink/renderer/core/html/html_slot_element.cc
+++ b/third_party/blink/renderer/core/html/html_slot_element.cc
@@ -217,7 +217,9 @@ void HTMLSlotElement::assign(HeapVector<Member<Node>> nodes) {
    if (!assigned_nodes_candidates_.IsEmpty()) {
      added_assigned_nodes =
          DeleteCommonAssignedNodeAndReturnAddedAssignedNode(nodes);
-      ContainingShadowRoot()->GetSlotAssignment().CallSlotChangeIfNeeded(*this);
+      ContainingShadowRoot()
+          ->GetSlotAssignment()
+          .CallSlotChangeAfterRemovedFromAssignFunction(*this);
      ContainingShadowRoot()->GetSlotAssignment().DeleteSlotInChildSlotMap(
          *this);
    } else {
@@ -238,7 +240,10 @@ void HTMLSlotElement::assign(HeapVector<Member<Node>> nodes) {
      ContainingShadowRoot()->GetSlotAssignment().InsertSlotInChildSlotMap(
          *this, *child);
    }
-    ContainingShadowRoot()->GetSlotAssignment().CallSlotChangeIfNeeded(*this);
+    ContainingShadowRoot()
+        ->GetSlotAssignment()
+        .CallSlotChangeAfterAdditionFromAssignFunction(*this,
+                                                       added_assigned_nodes);
  }
 }