Executing "insertunorderedlist" while selecting a contenteditable element inside...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 29 Nov 2018 21:50:43 +0000 (21:50 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 29 Nov 2018 21:50:43 +0000 (21:50 +0000)
https://bugs.webkit.org/show_bug.cgi?id=184049
<rdar://problem/38931033>

Reviewed by Antti Koivisto.

Source/WebCore:

The primary hung was caused by TextIterator::advance traversing the next node in the tree order using
NodeTraversal::next which doesn't take the composed tree into account. Fixed the bug by traversing
the composed tree while sharing code with StylizedMarkupAccumulator.

This revealed a second hang in InsertListCommand::doApply() caused by endingSelection() being null,
which was caused by CompositeEditCommand::moveParagraphs failing to restore the ending selection properly
because it was computing the text indices difference from the beginning of the document until the destination.

Fixed this second bug by computing the indices against the beginning of the root editable element.
Note that editability never crosses a shadow boundary.

Test: editing/execCommand/insert-unordered-list-in-shadow-tree.html

* dom/ComposedTreeIterator.h:
(WebCore::nextSkippingChildrenInComposedTreeIgnoringUserAgentShadow): Extracted out of nextSkippingChildren
in markup.cpp.
(WebCore::nextInComposedTreeIgnoringUserAgentShadow): Added.
* editing/CompositeEditCommand.cpp:
(WebCore::CompositeEditCommand::moveParagraphs): Compute the index from the beginning of the root editable
element as opposed to the beginning of the document.
* editing/TextIterator.cpp:
(WebCore::nextNode): Added.
(WebCore::isDescendantOf): Added.
(WebCore::TextIterator::advance): Use the newly added functions to traverse the composed tree when the options
contains TextIteratorTraversesFlatTree.
* editing/markup.cpp:

LayoutTests:

Added a regression test for executing InsertUnorderedList inside a shadow tree.

* editing/execCommand/insert-ordered-list-in-shadow-tree-expected.txt: Added.
* editing/execCommand/insert-ordered-list-in-shadow-tree.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@238693 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/editing/execCommand/insert-unordered-list-in-shadow-tree-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/insert-unordered-list-in-shadow-tree.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/ComposedTreeIterator.h
Source/WebCore/editing/CompositeEditCommand.cpp
Source/WebCore/editing/TextIterator.cpp
Source/WebCore/editing/markup.cpp

index b3e8dd2..cd16611 100644 (file)
@@ -1,3 +1,16 @@
+2018-11-29  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Executing "insertunorderedlist" while selecting a contenteditable element inside a shadow dom hangs the browser
+        https://bugs.webkit.org/show_bug.cgi?id=184049
+        <rdar://problem/38931033>
+
+        Reviewed by Antti Koivisto.
+
+        Added a regression test for executing InsertUnorderedList inside a shadow tree.
+
+        * editing/execCommand/insert-ordered-list-in-shadow-tree-expected.txt: Added.
+        * editing/execCommand/insert-ordered-list-in-shadow-tree.html: Added.
+
 2018-11-29  Justin Fan  <justin_fan@apple.com>
 
         [WebGPU] WebGPURenderPassEncoder::setPipeline, draw, and endPass prototypes
diff --git a/LayoutTests/editing/execCommand/insert-unordered-list-in-shadow-tree-expected.txt b/LayoutTests/editing/execCommand/insert-unordered-list-in-shadow-tree-expected.txt
new file mode 100644 (file)
index 0000000..f9989ee
--- /dev/null
@@ -0,0 +1,28 @@
+This tests inserting an ordered list inside a shadow tree. WebKit should not hang.
+
+Before:
+| <div>
+|   contenteditable=""
+|   id="editor"
+|   "
+    one"
+|   <br>
+|   "
+    two"
+|   <br>
+|   "
+"
+
+After:
+| <div>
+|   contenteditable=""
+|   id="editor"
+|   <ol>
+|     <li>
+|       "one"
+|       <br>
+|     <li>
+|       "two"
+|       <br>
+|   "
+"
diff --git a/LayoutTests/editing/execCommand/insert-unordered-list-in-shadow-tree.html b/LayoutTests/editing/execCommand/insert-unordered-list-in-shadow-tree.html
new file mode 100644 (file)
index 0000000..c7a1063
--- /dev/null
@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/dump-as-markup.js"></script>
+<script>
+
+Markup.description('This tests inserting an ordered list inside a shadow tree. WebKit should not hang.');
+
+const host = document.createElement('div');
+document.body.appendChild(host);
+const shadowRoot = host.attachShadow({mode: 'closed'});
+shadowRoot.innerHTML = `<div id="editor" contenteditable>
+    one<br>
+    two<br>
+</div>`;
+
+shadowRoot.querySelector('#editor').focus();
+document.execCommand('selectAll', false, null);
+Markup.dump(shadowRoot, 'Before');
+
+document.execCommand('insertOrderedList', false, null);
+Markup.dump(shadowRoot, 'After');
+
+</script>
+</body>
+</html>
index decd2b1..069997b 100644 (file)
@@ -1,3 +1,38 @@
+2018-11-29  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Executing "insertunorderedlist" while selecting a contenteditable element inside a shadow dom hangs the browser
+        https://bugs.webkit.org/show_bug.cgi?id=184049
+        <rdar://problem/38931033>
+
+        Reviewed by Antti Koivisto.
+
+        The primary hung was caused by TextIterator::advance traversing the next node in the tree order using
+        NodeTraversal::next which doesn't take the composed tree into account. Fixed the bug by traversing
+        the composed tree while sharing code with StylizedMarkupAccumulator.
+
+        This revealed a second hang in InsertListCommand::doApply() caused by endingSelection() being null,
+        which was caused by CompositeEditCommand::moveParagraphs failing to restore the ending selection properly
+        because it was computing the text indices difference from the beginning of the document until the destination.
+
+        Fixed this second bug by computing the indices against the beginning of the root editable element.
+        Note that editability never crosses a shadow boundary.
+
+        Test: editing/execCommand/insert-unordered-list-in-shadow-tree.html
+
+        * dom/ComposedTreeIterator.h:
+        (WebCore::nextSkippingChildrenInComposedTreeIgnoringUserAgentShadow): Extracted out of nextSkippingChildren
+        in markup.cpp.
+        (WebCore::nextInComposedTreeIgnoringUserAgentShadow): Added.
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::CompositeEditCommand::moveParagraphs): Compute the index from the beginning of the root editable
+        element as opposed to the beginning of the document.
+        * editing/TextIterator.cpp:
+        (WebCore::nextNode): Added.
+        (WebCore::isDescendantOf): Added.
+        (WebCore::TextIterator::advance): Use the newly added functions to traverse the composed tree when the options
+        contains TextIteratorTraversesFlatTree.
+        * editing/markup.cpp:
+
 2018-11-29  Zalan Bujtas  <zalan@apple.com>
 
         [ContentObservation] Decouple content change and DOM timer scheduling observation
index 2c3d76f..a461b9a 100644 (file)
@@ -245,4 +245,22 @@ inline Node* nextSiblingInComposedTreeIgnoringUserAgentShadow(Node& node)
     return node.nextSibling();
 }
 
+inline Node* nextSkippingChildrenInComposedTreeIgnoringUserAgentShadow(Node& node)
+{
+    if (auto* sibling = nextSiblingInComposedTreeIgnoringUserAgentShadow(node))
+        return sibling;
+    for (auto* ancestor = node.parentInComposedTree(); ancestor; ancestor = ancestor->parentInComposedTree()) {
+        if (auto* sibling = nextSiblingInComposedTreeIgnoringUserAgentShadow(*ancestor))
+            return sibling;
+    }
+    return nullptr;
+}
+
+inline Node* nextInComposedTreeIgnoringUserAgentShadow(Node& node)
+{
+    if (auto* firstChild = firstChildInComposedTreeIgnoringUserAgentShadow(node))
+        return firstChild;
+    return nextSkippingChildrenInComposedTreeIgnoringUserAgentShadow(node);
+}
+
 } // namespace WebCore
index 99c0d4b..ac1f271 100644 (file)
@@ -1487,7 +1487,8 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
         document().updateLayoutIgnorePendingStylesheets();
     }
 
-    RefPtr<Range> startToDestinationRange(Range::create(document(), firstPositionInNode(document().documentElement()), destination.deepEquivalent().parentAnchoredEquivalent()));
+    auto editableRoot = destination.rootEditableElement();
+    RefPtr<Range> startToDestinationRange(Range::create(document(), firstPositionInNode(editableRoot), destination.deepEquivalent().parentAnchoredEquivalent()));
     destinationIndex = TextIterator::rangeLength(startToDestinationRange.get(), true);
 
     setEndingSelection(VisibleSelection(destination, originalIsDirectional));
@@ -1510,8 +1511,8 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
         // causes spaces to be collapsed during the move operation.  This results
         // in a call to rangeFromLocationAndLength with a location past the end
         // of the document (which will return null).
-        RefPtr<Range> start = TextIterator::rangeFromLocationAndLength(document().documentElement(), destinationIndex + startIndex, 0, true);
-        RefPtr<Range> end = TextIterator::rangeFromLocationAndLength(document().documentElement(), destinationIndex + endIndex, 0, true);
+        RefPtr<Range> start = TextIterator::rangeFromLocationAndLength(editableRoot, destinationIndex + startIndex, 0, true);
+        RefPtr<Range> end = TextIterator::rangeFromLocationAndLength(editableRoot, destinationIndex + endIndex, 0, true);
         if (start && end)
             setEndingSelection(VisibleSelection(start->startPosition(), end->startPosition(), DOWNSTREAM, originalIsDirectional));
     }
index e7ae75c..f381bd9 100644 (file)
@@ -424,6 +424,20 @@ static inline Node* nextSibling(TextIteratorBehavior options, Node& node)
     return node.nextSibling();
 }
 
+static inline Node* nextNode(TextIteratorBehavior options, Node& node)
+{
+    if (UNLIKELY(options & TextIteratorTraversesFlatTree))
+        return nextInComposedTreeIgnoringUserAgentShadow(node);
+    return NodeTraversal::next(node);
+}
+
+static inline bool isDescendantOf(TextIteratorBehavior options, Node& node, Node& possibleAncestor)
+{
+    if (UNLIKELY(options & TextIteratorTraversesFlatTree))
+        return node.isDescendantOrShadowDescendantOf(&possibleAncestor);
+    return node.isDescendantOf(&possibleAncestor);
+}
+
 static inline Node* parentNodeOrShadowHost(TextIteratorBehavior options, Node& node)
 {
     if (UNLIKELY(options & TextIteratorTraversesFlatTree))
@@ -507,10 +521,10 @@ void TextIterator::advance()
         if (!next) {
             next = nextSibling(m_behavior, *m_node);
             if (!next) {
-                bool pastEnd = NodeTraversal::next(*m_node) == m_pastEndNode;
+                bool pastEnd = nextNode(m_behavior, *m_node) == m_pastEndNode;
                 Node* parentNode = parentNodeOrShadowHost(m_behavior, *m_node);
                 while (!next && parentNode) {
-                    if ((pastEnd && parentNode == m_endContainer) || m_endContainer->isDescendantOf(*parentNode))
+                    if ((pastEnd && parentNode == m_endContainer) || isDescendantOf(m_behavior, *m_endContainer, *parentNode))
                         return;
                     bool haveRenderer = m_node->renderer();
                     Node* exitedNode = m_node;
index b7b2475..63bf458 100644 (file)
@@ -273,15 +273,8 @@ private:
     
     Node* nextSkippingChildren(Node& node)
     {
-        if (UNLIKELY(m_useComposedTree)) {
-            if (auto* sibling = nextSiblingInComposedTreeIgnoringUserAgentShadow(node))
-                return sibling;
-            for (auto* ancestor = node.parentInComposedTree(); ancestor; ancestor = ancestor->parentInComposedTree()) {
-                if (auto* sibling = nextSiblingInComposedTreeIgnoringUserAgentShadow(*ancestor))
-                    return sibling;
-            }
-            return nullptr;
-        }
+        if (UNLIKELY(m_useComposedTree))
+            return nextSkippingChildrenInComposedTreeIgnoringUserAgentShadow(node);
         return NodeTraversal::nextSkippingChildren(node);
     }