Changeset 189906 in webkit

Timestamp:

Sep 16, 2015 11:12:06 PM (9 years ago)

Author:

rniwa@webkit.org

Message:

removeShadow shouldn't call ChildNodeRemovalNotifier with the shadow host as the removal point
​https://bugs.webkit.org/show_bug.cgi?id=149244

Reviewed by Antti Koivisto.

Since a shadow host is in a different tree than nodes in its shadow tree, it's incorrect to call
removedFrom with the shadow host as the removal point. This causes HTMLSlotElement::removedFrom
which will be added in the bug 149241 to call methods on a wrong ShadowRoot.

We still keep the ad-hoc behavior of using the shadow host as the insertion/removal point when
calling insertedInto and removedFrom on the shadow root itself to update the InDocument node flag.
We may want to re-visit this design in the future.

No new tests since I couldn't quite create a reduction. However, tests I'm adding in the bug 149241
will crash without this change.

I separated this patch from the bug 149241 to isolate the high-risk code change here.

• dom/Element.cpp:

(WebCore::Element::addShadowRoot): Call insertedInto on ShadowRoot, and then call it on all its
children separately with the insertion point set to the shadow root since insertedInto relies on
insertion point's inDocument flag to be true when the shadow host is in the document.
(WebCore::Element::removeShadowRoot): Ditto in the reverse order.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/Element.cpp (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2015-09-16  Ryosuke Niwa  <rniwa@webkit.org>
+
+        removeShadow shouldn't call ChildNodeRemovalNotifier with the shadow host as the removal point
+        https://bugs.webkit.org/show_bug.cgi?id=149244
+
+        Reviewed by Antti Koivisto.
+
+        Since a shadow host is in a different tree than nodes in its shadow tree, it's incorrect to call
+        removedFrom with the shadow host as the removal point. This causes HTMLSlotElement::removedFrom
+        which will be added in the bug 149241 to call methods on a wrong ShadowRoot.
+
+        We still keep the ad-hoc behavior of using the shadow host as the insertion/removal point when
+        calling insertedInto and removedFrom on the shadow root itself to update the InDocument node flag.
+        We may want to re-visit this design in the future.
+
+        No new tests since I couldn't quite create a reduction. However, tests I'm adding in the bug 149241
+        will crash without this change.
+
+        I separated this patch from the bug 149241 to isolate the high-risk code change here.
+
+        * dom/Element.cpp:
+        (WebCore::Element::addShadowRoot): Call insertedInto on ShadowRoot, and then call it on all its
+        children separately with the insertion point set to the shadow root since insertedInto relies on
+        insertion point's inDocument flag to be true when the shadow host is in the document.
+        (WebCore::Element::removeShadowRoot): Ditto in the reverse order.
+
 2015-09-16  Gyuyoung Kim  <gyuyoung.kim@webkit.org>
 

trunk/Source/WebCore/dom/Element.cpp

@@ -1616 +1616 @@
     shadowRoot.setParentTreeScope(&treeScope());
 
-    NodeVector postInsertionNotificationTargets;
-    notifyChildNodeInserted(*this, shadowRoot, postInsertionNotificationTargets);
-
-    for (auto& target : postInsertionNotificationTargets)
-        target->finishedInsertingSubtree();
+    auto shadowRootInsertionResult = shadowRoot.insertedInto(*this);
+    ASSERT_UNUSED(shadowRootInsertionResult, shadowRootInsertionResult == InsertionDone);
+    if (auto* firstChild = shadowRoot.firstChild()) {
+        NodeVector postInsertionNotificationTargets;
+        {
+            NoEventDispatchAssertion assertNoEventDispatch;
+            for (auto* child = firstChild; child; child = child->nextSibling())
+                notifyChildNodeInserted(shadowRoot, *child, postInsertionNotificationTargets);
+        }
+
+        for (auto& target : postInsertionNotificationTargets)
+            target->finishedInsertingSubtree();
+    }
 
     resetNeedsNodeRenderingTraversalSlowPath();
@@ -1647 +1655 @@
     oldRoot->setParentTreeScope(&document());
 
-    notifyChildNodeRemoved(*this, *oldRoot);
+    {
+        NoEventDispatchAssertion assertNoEventDispatch;
+        for (auto* child = oldRoot->firstChild(); child; child = child->nextSibling())
+            notifyChildNodeRemoved(*oldRoot, *child);
+    }
+
+    oldRoot->removedFrom(*this);
 }