WebCore:

        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

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index c4d29df8c2ad061a3ee5c87047432685c0b757c7..e5c04cb782e3191790722760c1f23710727ec7fe 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -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
+++ b/LayoutTests/editing/execCommand/5575101-1-expected.txt
@@ -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
+++ b/LayoutTests/editing/execCommand/5575101-1.html
@@ -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
+++ b/LayoutTests/editing/execCommand/5575101-2-expected.txt
@@ -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
+++ b/LayoutTests/editing/execCommand/5575101-2.html
@@ -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
+++ b/LayoutTests/editing/execCommand/5575101-3-expected.txt
@@ -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
+++ b/LayoutTests/editing/execCommand/5575101-3.html
@@ -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>

diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
index 0d16d186f7801f47c57db416585a31a3be0e2da4..ce7af3f5ce85e0b292830755c81a4e0036eadaf7 100644 (file)
--- a/WebCore/ChangeLog
+++ b/WebCore/ChangeLog
@@ -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.

diff --git a/WebCore/editing/CompositeEditCommand.cpp b/WebCore/editing/CompositeEditCommand.cpp
index a630d38bd766b7bd09dfb64d35b019d138cd059b..bf4be75356b8290eb24f6a5b7189e8c9e43814a0 100644 (file)
--- a/WebCore/editing/CompositeEditCommand.cpp
+++ b/WebCore/editing/CompositeEditCommand.cpp
@@ -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;

diff --git a/WebCore/editing/CompositeEditCommand.h b/WebCore/editing/CompositeEditCommand.h
index e099fcfdf6c4974a1f09170994c4edfd9853f690..9be8ee716919238bbfe6d0ad13c4ae7bf507d019 100644 (file)
--- a/WebCore/editing/CompositeEditCommand.h
+++ b/WebCore/editing/CompositeEditCommand.h
@@ -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;
 

diff --git a/WebCore/editing/IndentOutdentCommand.cpp b/WebCore/editing/IndentOutdentCommand.cpp
index ce0620e5c49d7054169c9913235517317e4ea57f..a7ab6449ae07ad090b6a70fb493baf8c1b1bdada 100644 (file)
--- a/WebCore/editing/IndentOutdentCommand.cpp
+++ b/WebCore/editing/IndentOutdentCommand.cpp
@@ -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())

diff --git a/WebCore/editing/IndentOutdentCommand.h b/WebCore/editing/IndentOutdentCommand.h
index 6319ffbfae1bfa48a47312678bba030e9e9604e4..403297ae2479175ed0657d4e7af4adbadcac70a3 100644 (file)
--- a/WebCore/editing/IndentOutdentCommand.h
+++ b/WebCore/editing/IndentOutdentCommand.h
@@ -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**);
 };
 

diff --git a/WebCore/editing/InsertListCommand.cpp b/WebCore/editing/InsertListCommand.cpp
index 03fd6349ab29380f7451b3f4e8aa957f7b8a7422..d46274be4ce8ce13bf5f9606974a21a2ca86671e 100644 (file)
--- a/WebCore/editing/InsertListCommand.cpp
+++ b/WebCore/editing/InsertListCommand.cpp
@@ -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);
     }

diff --git a/WebCore/editing/SplitElementCommand.cpp b/WebCore/editing/SplitElementCommand.cpp
index f6060c74e642a006f679c1421f34fec8f2230842..9b4241dff5a719c763d26ed00b0e193e31caee77 100644 (file)
--- a/WebCore/editing/SplitElementCommand.cpp
+++ b/WebCore/editing/SplitElementCommand.cpp
@@ -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);