WebCore:
authorjustin.garcia@apple.com <justin.garcia@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Dec 2007 19:55:37 +0000 (19:55 +0000)
committerjustin.garcia@apple.com <justin.garcia@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Dec 2007 19:55:37 +0000 (19:55 +0000)
        Reviewed by Darin Adler.

        <rdar://problem/5575101> GoogleDocs: Hang in SplitElementCommand::doApply when outdenting a list item in a particular list

        * editing/CompositeEditCommand.cpp:
        (WebCore::CompositeEditCommand::splitTreeToNode): Moved here.
        * editing/CompositeEditCommand.h:
        * editing/IndentOutdentCommand.cpp: Moved splitTreeToNode.
        * editing/IndentOutdentCommand.h: Ditto, and removed unimplemented splitTreeTo.
        * editing/InsertListCommand.cpp:
        (WebCore::InsertListCommand::doApply): Split ancestors of listChildNode between
        it and listNode, if they exists, so that moving listChildNode doesn't put it out
        of order.  Added a test case to cover each change.
        * editing/SplitElementCommand.cpp:
        (WebCore::SplitElementCommand::doApply): Added an ASSERT to catch code that
        tries to split a container at a bogus child, and an early return to avoid a
        hang in that case.

LayoutTests:

        Reviewed by Darin Adler.

        <rdar://problem/5575101> Hang in SplitElementCommand::doApply when outdenting a list item in a particular list

        * editing/execCommand/5575101-1-expected.txt: Added.
        * editing/execCommand/5575101-1.html: Added.
        * editing/execCommand/5575101-2-expected.txt: Added.
        * editing/execCommand/5575101-2.html: Added.
        * editing/execCommand/5575101-3-expected.txt: Added.
        * editing/execCommand/5575101-3.html: Added.

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/editing/execCommand/5575101-1-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/5575101-1.html [new file with mode: 0644]
LayoutTests/editing/execCommand/5575101-2-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/5575101-2.html [new file with mode: 0644]
LayoutTests/editing/execCommand/5575101-3-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/5575101-3.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/editing/CompositeEditCommand.cpp
WebCore/editing/CompositeEditCommand.h
WebCore/editing/IndentOutdentCommand.cpp
WebCore/editing/IndentOutdentCommand.h
WebCore/editing/InsertListCommand.cpp
WebCore/editing/SplitElementCommand.cpp

index c4d29df..e5c04cb 100644 (file)
@@ -1,3 +1,16 @@
+2007-12-14  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by Darin Adler.
+        
+        <rdar://problem/5575101> Hang in SplitElementCommand::doApply when outdenting a list item in a particular list
+
+        * editing/execCommand/5575101-1-expected.txt: Added.
+        * editing/execCommand/5575101-1.html: Added.
+        * editing/execCommand/5575101-2-expected.txt: Added.
+        * editing/execCommand/5575101-2.html: Added.
+        * editing/execCommand/5575101-3-expected.txt: Added.
+        * editing/execCommand/5575101-3.html: Added.
+
 2007-12-14  Anders Carlsson  <andersca@apple.com>
 
         Reviewed by Darin and Geoff.
diff --git a/LayoutTests/editing/execCommand/5575101-1-expected.txt b/LayoutTests/editing/execCommand/5575101-1-expected.txt
new file mode 100644 (file)
index 0000000..a2dff07
--- /dev/null
@@ -0,0 +1,6 @@
+This tests for a hang when outdenting a list item within a sublist twice.
+
+one
+two
+three
+four
diff --git a/LayoutTests/editing/execCommand/5575101-1.html b/LayoutTests/editing/execCommand/5575101-1.html
new file mode 100644 (file)
index 0000000..c2017ca
--- /dev/null
@@ -0,0 +1,16 @@
+<p>This tests for a hang when outdenting a list item within a sublist twice.</p>
+<div contenteditable="true">
+<ol>
+    <li>one</li>
+    <li id="insert">two<ol><li id="outdent">three</li><li>four</li></ol></li>
+</ol>
+</div>
+
+<script>
+if (window.layoutTestController)
+    window.layoutTestController.dumpAsText();
+outdent = document.getElementById("outdent");
+window.getSelection().setPosition(outdent, 0);
+document.execCommand("Outdent");
+document.execCommand("Outdent");
+</script>
diff --git a/LayoutTests/editing/execCommand/5575101-2-expected.txt b/LayoutTests/editing/execCommand/5575101-2-expected.txt
new file mode 100644 (file)
index 0000000..2a8ba80
--- /dev/null
@@ -0,0 +1,5 @@
+This tests for a bug where outdenting a list item twice would incorrectly move it above the list it was outdented from. Below you should see 'two' in order with 'one' and 'three' and it should not be in a list.
+
+one
+two
+three
diff --git a/LayoutTests/editing/execCommand/5575101-2.html b/LayoutTests/editing/execCommand/5575101-2.html
new file mode 100644 (file)
index 0000000..ac60416
--- /dev/null
@@ -0,0 +1,16 @@
+<p>This tests for a bug where outdenting a list item twice would incorrectly move it above the list it was outdented from. Below you should see 'two' in order with 'one' and 'three' and it should not be in a list.</p>
+<div contenteditable="true">
+<ol>
+    <li>one</li>
+    <li id="insert"><ol><li id="outdent">two</li><li>three</li></ol></li>
+</ol>
+</div>
+
+<script>
+if (window.layoutTestController)
+    window.layoutTestController.dumpAsText();
+outdent = document.getElementById("outdent");
+window.getSelection().setPosition(outdent, 0);
+document.execCommand("Outdent");
+document.execCommand("Outdent");
+</script>
diff --git a/LayoutTests/editing/execCommand/5575101-3-expected.txt b/LayoutTests/editing/execCommand/5575101-3-expected.txt
new file mode 100644 (file)
index 0000000..2a8ba80
--- /dev/null
@@ -0,0 +1,5 @@
+This tests for a bug where outdenting a list item twice would incorrectly move it above the list it was outdented from. Below you should see 'two' in order with 'one' and 'three' and it should not be in a list.
+
+one
+two
+three
diff --git a/LayoutTests/editing/execCommand/5575101-3.html b/LayoutTests/editing/execCommand/5575101-3.html
new file mode 100644 (file)
index 0000000..e170009
--- /dev/null
@@ -0,0 +1,17 @@
+<p>This tests for a bug where outdenting a list item twice would incorrectly move it above the list it was outdented from. Below you should see 'two' in order with 'one' and 'three' and it should not be in a list.</p>
+<div contenteditable="true">
+<ol>
+    <li>one</li>
+    <li id="insert"><ol><li id="outdent">two</li></ol></li>
+    <li>three</li>
+</ol>
+</div>
+
+<script>
+if (window.layoutTestController)
+    window.layoutTestController.dumpAsText();
+outdent = document.getElementById("outdent");
+window.getSelection().setPosition(outdent, 0);
+document.execCommand("Outdent");
+document.execCommand("Outdent");
+</script>
index 0d16d18..ce7af3f 100644 (file)
@@ -1,3 +1,23 @@
+2007-12-14  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by Darin Adler.
+
+        <rdar://problem/5575101> GoogleDocs: Hang in SplitElementCommand::doApply when outdenting a list item in a particular list
+
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::CompositeEditCommand::splitTreeToNode): Moved here.
+        * editing/CompositeEditCommand.h:
+        * editing/IndentOutdentCommand.cpp: Moved splitTreeToNode.
+        * editing/IndentOutdentCommand.h: Ditto, and removed unimplemented splitTreeTo.
+        * editing/InsertListCommand.cpp:
+        (WebCore::InsertListCommand::doApply): Split ancestors of listChildNode between
+        it and listNode, if they exists, so that moving listChildNode doesn't put it out
+        of order.  Added a test case to cover each change.
+        * editing/SplitElementCommand.cpp:
+        (WebCore::SplitElementCommand::doApply): Added an ASSERT to catch code that
+        tries to split a container at a bogus child, and an early return to avoid a
+        hang in that case.
+
 2007-12-14  Anders Carlsson  <andersca@apple.com>
 
         Reviewed by Darin and Geoff.
index a630d38..bf4be75 100644 (file)
@@ -911,6 +911,22 @@ Position CompositeEditCommand::positionAvoidingSpecialElementBoundary(const Posi
     return result;
 }
 
+// Splits the tree parent by parent until we reach the specified ancestor. We use VisiblePositions
+// to determine if the split is necessary. Returns the last split node.
+Node* CompositeEditCommand::splitTreeToNode(Node* start, Node* end, bool splitAncestor)
+{
+    Node* node;
+    for (node = start; node && node->parent() != end; node = node->parent()) {
+        VisiblePosition positionInParent(Position(node->parent(), 0), DOWNSTREAM);
+        VisiblePosition positionInNode(Position(node, 0), DOWNSTREAM);
+        if (positionInParent != positionInNode)
+            applyCommandToComposite(new SplitElementCommand(static_cast<Element*>(node->parent()), node));
+    }
+    if (splitAncestor)
+        return splitTreeToNode(end, end->parent());
+    return node;
+}
+
 PassRefPtr<Element> createBlockPlaceholderElement(Document* document)
 {
     ExceptionCode ec = 0;
index e099fcf..9be8ee7 100644 (file)
@@ -101,6 +101,8 @@ protected:
     bool breakOutOfEmptyListItem();
     
     Position positionAvoidingSpecialElementBoundary(const Position&, bool alwaysAvoidAnchors = true);
+    
+    Node* splitTreeToNode(Node*, Node*, bool splitAncestor = false);
 
     Vector<RefPtr<EditCommand> > m_commands;
 
index ce0620e..a7ab644 100644 (file)
@@ -104,22 +104,6 @@ Node* IndentOutdentCommand::prepareBlockquoteLevelForInsertion(VisiblePosition&
     return placeholder.get();
 }
 
-// Splits the tree parent by parent until we reach the specified ancestor. We use VisiblePositions
-// to determine if the split is necessary. Returns the last split node.
-Node* IndentOutdentCommand::splitTreeToNode(Node* start, Node* end, bool splitAncestor)
-{
-    Node* node;
-    for (node = start; node && node->parent() != end; node = node->parent()) {
-        VisiblePosition positionInParent(Position(node->parent(), 0), DOWNSTREAM);
-        VisiblePosition positionInNode(Position(node, 0), DOWNSTREAM);
-        if (positionInParent != positionInNode)
-            applyCommandToComposite(new SplitElementCommand(static_cast<Element*>(node->parent()), node));
-    }
-    if (splitAncestor)
-        return splitTreeToNode(end, end->parent());
-    return node;
-}
-
 static int indexForVisiblePosition(VisiblePosition& visiblePosition)
 {
     if (visiblePosition.isNull())
index 6319ffb..403297a 100644 (file)
@@ -38,14 +38,12 @@ public:
     virtual void doApply();
     virtual EditAction editingAction() const { return m_typeOfAction == Indent ? EditActionIndent : EditActionOutdent; }
 private:
-    void splitTreeTo(Node* start, Node* stop);
     bool modifyRange();
     EIndentType m_typeOfAction;
     int m_marginInPixels;
     void indentRegion();
     void outdentRegion();
     void outdentParagraph();
-    Node* splitTreeToNode(Node*, Node*, bool splitAncestor = false);
     Node* prepareBlockquoteLevelForInsertion(VisiblePosition&, Node**);
 };
 
index 03fd634..d46274b 100644 (file)
@@ -169,13 +169,28 @@ void InsertListCommand::doApply()
             nodeToInsert = createListItemElement(document());
             appendNode(placeholder.get(), nodeToInsert.get());
         }
+        
         if (nextListChild && previousListChild) {
-            splitElement(static_cast<Element *>(listNode), nextListChild);
+            // We want to pull listChildNode out of listNode, and place it before nextListChild 
+            // and after previousListChild, so we split listNode and insert it between the two lists.  
+            // But to split listNode, we must first split ancestors of listChildNode between it and listNode,
+            // if any exist.
+            // FIXME: We appear to split at nextListChild as opposed to listChildNode so that when we remove
+            // listChildNode below in moveParagraphs, previousListChild will be removed along with it if it is 
+            // unrendered. But we ought to remove nextListChild too, if it is unrendered.
+            splitElement(static_cast<Element *>(listNode), splitTreeToNode(nextListChild, listNode));
             insertNodeBefore(nodeToInsert.get(), listNode);
-        } else if (nextListChild)
+        } else if (nextListChild || listChildNode->parentNode() != listNode) {
+            // Just because listChildNode has no previousListChild doesn't mean there isn't any content
+            // in listNode that comes before listChildNode, as listChildNode could have ancestors
+            // between it and listNode. So, we split up to listNode before inserting the placeholder
+            // where we're about to move listChildNode to.
+            if (listChildNode->parentNode() != listNode)
+                splitElement(static_cast<Element *>(listNode), splitTreeToNode(listChildNode, listNode));
             insertNodeBefore(nodeToInsert.get(), listNode);
-        else
+        else
             insertNodeAfter(nodeToInsert.get(), listNode);
+        
         VisiblePosition insertionPoint = VisiblePosition(Position(placeholder.get(), 0));
         moveParagraphs(start, end, insertionPoint, true);
     }
index f6060c7..9b4241d 100644 (file)
@@ -42,6 +42,7 @@ void SplitElementCommand::doApply()
 {
     ASSERT(m_element2);
     ASSERT(m_atChild);
+    ASSERT(m_atChild->parentNode() == m_element2);
 
     ExceptionCode ec = 0;
 
@@ -55,6 +56,10 @@ void SplitElementCommand::doApply()
     m_element2->parent()->insertBefore(m_element1.get(), m_element2.get(), ec);
     ASSERT(ec == 0);
     
+    // Bail if we were asked to split at a bogus child, to avoid hanging below.
+    if (!m_atChild || m_atChild->parentNode() != m_element2)
+        return;
+    
     while (m_element2->firstChild() != m_atChild) {
         ASSERT(m_element2->firstChild());
         m_element1->appendChild(m_element2->firstChild(), ec);