LayoutTests:
authorjusting <justing@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 5 Apr 2006 21:53:23 +0000 (21:53 +0000)
committerjusting <justing@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 5 Apr 2006 21:53:23 +0000 (21:53 +0000)
        Reviewed by darin

        <http://bugzilla.opendarwin.org/show_bug.cgi?id=8198>
        Hitting an assert on undo paste

        * editing/pasteboard/undoable-fragment-removes-expected.checksum: Added.
        * editing/pasteboard/undoable-fragment-removes-expected.png: Added.
        * editing/pasteboard/undoable-fragment-removes-expected.txt: Added.
        * editing/pasteboard/undoable-fragment-removes.html: Added.

WebCore:

        Reviewed by darin

        <http://bugzilla.opendarwin.org/show_bug.cgi?id=8198>
        Hitting an assert on undo paste

        ReplaceSelectionCommand was doing a combination of undoable and non-undoable
        removes from the ReplacementFragment.  On Undo Paste, the undoable removes
        couldn't be undone because the tree was in a different state than it was
        at the time of the remove.  This patch makes all the removes from the fragment
        non-undoable.  We could make them all undoable, but I can't think of any reason
        why we'd want the fragment to be reconstructed on an Undo Paste.

        * editing/AppendNodeCommand.cpp:
        (WebCore::AppendNodeCommand::doApply):
        Assert that the node to append isn't already in a tree, since if it is, it will
        be removed in a non-undoable way.
        * editing/InsertNodeBeforeCommand.cpp:
        (WebCore::InsertNodeBeforeCommand::doApply): Ditto.
        * editing/ReplaceSelectionCommand.cpp:
        (WebCore::ReplaceSelectionCommand::doApply):
        Nodes were being moved from the fragment to the document with undoable inserts.
        Undoable inserts implicitly remove the node (in a non-undoable way) from its
        old location if it is already in a tree.  I now explicitly remove the nodes
        from the fragment before inserting them into the document to make it clear that
        they are being removed in a non-non-undoable way.  I also changed the one undoable
        remove from the fragment to a non-undoable remove.
        * editing/ReplaceSelectionCommand.h:
        Made ReplacementFragment's non-undoable removeNode public.

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

LayoutTests/ChangeLog
LayoutTests/editing/pasteboard/undoable-fragment-removes-expected.checksum [new file with mode: 0644]
LayoutTests/editing/pasteboard/undoable-fragment-removes-expected.png [new file with mode: 0644]
LayoutTests/editing/pasteboard/undoable-fragment-removes-expected.txt [new file with mode: 0644]
LayoutTests/editing/pasteboard/undoable-fragment-removes.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/editing/AppendNodeCommand.cpp
WebCore/editing/InsertNodeBeforeCommand.cpp
WebCore/editing/ReplaceSelectionCommand.cpp
WebCore/editing/ReplaceSelectionCommand.h

index c9e6a64..214e5d1 100644 (file)
@@ -1,3 +1,15 @@
+2006-04-05  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by darin
+        
+        <http://bugzilla.opendarwin.org/show_bug.cgi?id=8198>
+        Hitting an assert on undo paste
+
+        * editing/pasteboard/undoable-fragment-removes-expected.checksum: Added.
+        * editing/pasteboard/undoable-fragment-removes-expected.png: Added.
+        * editing/pasteboard/undoable-fragment-removes-expected.txt: Added.
+        * editing/pasteboard/undoable-fragment-removes.html: Added.
+
 2006-04-05  Alexey Proskuryakov  <ap@nypop.com>
 
         Reviewed by Darin.
diff --git a/LayoutTests/editing/pasteboard/undoable-fragment-removes-expected.checksum b/LayoutTests/editing/pasteboard/undoable-fragment-removes-expected.checksum
new file mode 100644 (file)
index 0000000..61bedba
--- /dev/null
@@ -0,0 +1 @@
+672cd6f0fb4cc7faf63476604e7d3994
\ No newline at end of file
diff --git a/LayoutTests/editing/pasteboard/undoable-fragment-removes-expected.png b/LayoutTests/editing/pasteboard/undoable-fragment-removes-expected.png
new file mode 100644 (file)
index 0000000..0390ca7
Binary files /dev/null and b/LayoutTests/editing/pasteboard/undoable-fragment-removes-expected.png differ
diff --git a/LayoutTests/editing/pasteboard/undoable-fragment-removes-expected.txt b/LayoutTests/editing/pasteboard/undoable-fragment-removes-expected.txt
new file mode 100644 (file)
index 0000000..486ef2c
--- /dev/null
@@ -0,0 +1,37 @@
+EDITING DELEGATE: shouldBeginEditingInDOMRange:range from 0 of DIV > BODY > HTML > #document to 3 of DIV > BODY > HTML > #document
+EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > DIV > BODY > HTML > #document to 0 of DIV > DIV > BODY > HTML > #document toDOMRange:range from 0 of #text > DIV > DIV > BODY > HTML > #document to 3 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldEndEditingInDOMRange:range from 0 of DIV > BODY > HTML > #document to 3 of DIV > BODY > HTML > #document
+EDITING DELEGATE: webViewDidEndEditing:WebViewDidEndEditingNotification
+EDITING DELEGATE: shouldBeginEditingInDOMRange:range from 0 of DIV > BODY > HTML > #document to 0 of DIV > BODY > HTML > #document
+EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldInsertNode:#document-fragment replacingDOMRange:range from 0 of DIV > BODY > HTML > #document to 0 of DIV > BODY > HTML > #document givenAction:WebViewInsertActionPasted
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > BODY > HTML > #document to 0 of DIV > BODY > HTML > #document toDOMRange:range from 3 of #text > DIV > DIV > BODY > HTML > #document to 3 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of DIV > BODY > HTML > #document to 0 of DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+layer at (0,0) size 800x600
+  RenderCanvas at (0,0) size 800x600
+layer at (0,0) size 800x600
+  RenderBlock {HTML} at (0,0) size 800x600
+    RenderBody {BODY} at (8,8) size 784x584
+      RenderBlock {P} at (0,0) size 784x36
+        RenderText {TEXT} at (0,0) size 750x36
+          text run at (0,0) width 573: "This is a testcase for 8193, hitting an assert inside RemoveNodeCommand on Undo Paste. "
+          text run at (573,0) width 177: "Both editable regions below"
+          text run at (0,18) width 217: "should look the same after the test."
+      RenderBlock {HR} at (0,52) size 784x2 [border: (1px inset #000000)]
+      RenderBlock {DIV} at (0,62) size 784x38 [border: (1px solid #000000)]
+        RenderBlock {DIV} at (1,1) size 782x18
+          RenderText {TEXT} at (0,0) size 21x18
+            text run at (0,0) width 21: "foo"
+        RenderBlock {DIV} at (1,19) size 782x18
+          RenderText {TEXT} at (0,0) size 20x18
+            text run at (0,0) width 20: "bar"
+      RenderBlock {DIV} at (0,100) size 784x20 [border: (1px solid #000000)]
+caret: position 0 of child 6 {DIV} of child 0 {BODY} of child 0 {HTML} of document
diff --git a/LayoutTests/editing/pasteboard/undoable-fragment-removes.html b/LayoutTests/editing/pasteboard/undoable-fragment-removes.html
new file mode 100644 (file)
index 0000000..a8f0d12
--- /dev/null
@@ -0,0 +1,17 @@
+<p>This is a testcase for 8193, hitting an assert inside RemoveNodeCommand on Undo Paste.  Both editable regions below should look the same after the test.</p>
+<hr>
+<div style="border: 1px solid black;" id="copy" contenteditable="true">
+<div>foo</div><div>bar</div></div>
+<div style="border: 1px solid black;" id="paste" contenteditable="true"></div>
+
+<script>
+var c = document.getElementById("copy");
+var p = document.getElementById("paste");
+var s = window.getSelection();
+s.setPosition(c, 0);
+document.execCommand("SelectAll");
+document.execCommand("Copy");
+s.setPosition(p, 0);
+document.execCommand("Paste");
+document.execCommand("Undo");
+</script>
\ No newline at end of file
index db3fe83..85f81f1 100644 (file)
@@ -1,3 +1,34 @@
+2006-04-05  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by darin
+        
+        <http://bugzilla.opendarwin.org/show_bug.cgi?id=8198>
+        Hitting an assert on undo paste
+        
+        ReplaceSelectionCommand was doing a combination of undoable and non-undoable 
+        removes from the ReplacementFragment.  On Undo Paste, the undoable removes 
+        couldn't be undone because the tree was in a different state than it was
+        at the time of the remove.  This patch makes all the removes from the fragment 
+        non-undoable.  We could make them all undoable, but I can't think of any reason 
+        why we'd want the fragment to be reconstructed on an Undo Paste.
+
+        * editing/AppendNodeCommand.cpp:
+        (WebCore::AppendNodeCommand::doApply):
+        Assert that the node to append isn't already in a tree, since if it is, it will 
+        be removed in a non-undoable way.
+        * editing/InsertNodeBeforeCommand.cpp:
+        (WebCore::InsertNodeBeforeCommand::doApply): Ditto.
+        * editing/ReplaceSelectionCommand.cpp:
+        (WebCore::ReplaceSelectionCommand::doApply): 
+        Nodes were being moved from the fragment to the document with undoable inserts.  
+        Undoable inserts implicitly remove the node (in a non-undoable way) from its 
+        old location if it is already in a tree.  I now explicitly remove the nodes 
+        from the fragment before inserting them into the document to make it clear that 
+        they are being removed in a non-non-undoable way.  I also changed the one undoable 
+        remove from the fragment to a non-undoable remove.
+        * editing/ReplaceSelectionCommand.h: 
+        Made ReplacementFragment's non-undoable removeNode public.
+
 2006-04-05  Darin Adler  <darin@apple.com>
 
         - fixed the build
index f40c129..8311481 100644 (file)
@@ -42,6 +42,9 @@ void AppendNodeCommand::doApply()
 {
     ASSERT(m_appendChild);
     ASSERT(m_parentNode);
+    // If the child to append is already in a tree, appending it will remove it from it's old location
+    // in an non-undoable way.  We might eventually find it useful to do an undoable remove in this case.
+    ASSERT(!m_appendChild->parent());
 
     ExceptionCode ec = 0;
     m_parentNode->appendChild(m_appendChild.get(), ec);
index bc97847..a33200e 100644 (file)
@@ -43,6 +43,9 @@ void InsertNodeBeforeCommand::doApply()
     ASSERT(m_insertChild);
     ASSERT(m_refChild);
     ASSERT(m_refChild->parentNode());
+    // If the child to insert is already in a tree, inserting it will remove it from it's old location
+    // in an non-undoable way.  We might eventually find it useful to do an undoable remove in this case.
+    ASSERT(!m_appendChild->parent());
 
     ExceptionCode ec = 0;
     m_refChild->parentNode()->insertBefore(m_insertChild.get(), m_refChild.get(), ec);
index a2cf0b7..f8987fd 100644 (file)
@@ -647,14 +647,16 @@ void ReplaceSelectionCommand::doApply()
 
     // step 1: merge content into the start block
     if (mergeStart) {
-        Node *refNode = fragment.mergeStartNode();
+        RefPtr<Node> refNode = fragment.mergeStartNode();
         if (refNode) {
             Node *parent = refNode->parentNode();
-            Node *node = refNode->nextSibling();
-            insertNodeAtAndUpdateNodesInserted(refNode, startPos.node(), startPos.offset());
-            while (node && !fragment.isBlockFlow(node)) {
+            RefPtr<Node> node = refNode->nextSibling();
+            fragment.removeNode(refNode);
+            insertNodeAtAndUpdateNodesInserted(refNode.get(), startPos.node(), startPos.offset());
+            while (node && !fragment.isBlockFlow(node.get())) {
                 Node *next = node->nextSibling();
-                insertNodeAfterAndUpdateNodesInserted(node, refNode);
+                fragment.removeNode(node);
+                insertNodeAfterAndUpdateNodesInserted(node.get(), refNode.get());
                 refNode = node;
                 node = next;
             }
@@ -662,7 +664,7 @@ void ReplaceSelectionCommand::doApply()
             // remove any ancestors we emptied, except the root itself which cannot be removed
             while (parent && parent->parentNode() && parent->childNodeCount() == 0) {
                 Node *nextParent = parent->parentNode();
-                removeNode(parent);
+                fragment.removeNode(parent);
                 parent = nextParent;
             }
         }
@@ -676,25 +678,26 @@ void ReplaceSelectionCommand::doApply()
     
     // step 2 : merge everything remaining in the fragment
     if (fragment.firstChild()) {
-        Node *refNode = fragment.firstChild();
-        Node *node = refNode ? refNode->nextSibling() : 0;
+        RefPtr<Node> refNode = fragment.firstChild();
+        RefPtr<Node> node = refNode ? refNode->nextSibling() : 0;
         Node *insertionBlock = insertionPos.node()->enclosingBlockFlowElement();
         bool insertionBlockIsRoot = insertionBlock == insertionBlock->rootEditableElement();
         VisiblePosition visiblePos(insertionPos, DOWNSTREAM);
-        if (!insertionBlockIsRoot && fragment.isBlockFlow(refNode) && isStartOfBlock(visiblePos))
-            insertNodeBeforeAndUpdateNodesInserted(refNode, insertionBlock);
-        else if (!insertionBlockIsRoot && fragment.isBlockFlow(refNode) && isEndOfBlock(visiblePos)) {
-            insertNodeAfterAndUpdateNodesInserted(refNode, insertionBlock);
-        } else if (m_lastNodeInserted && !fragment.isBlockFlow(refNode)) {
+        fragment.removeNode(refNode);
+        if (!insertionBlockIsRoot && fragment.isBlockFlow(refNode.get()) && isStartOfBlock(visiblePos))
+            insertNodeBeforeAndUpdateNodesInserted(refNode.get(), insertionBlock);
+        else if (!insertionBlockIsRoot && fragment.isBlockFlow(refNode.get()) && isEndOfBlock(visiblePos)) {
+            insertNodeAfterAndUpdateNodesInserted(refNode.get(), insertionBlock);
+        } else if (m_lastNodeInserted && !fragment.isBlockFlow(refNode.get())) {
             Position pos = visiblePos.next().deepEquivalent().downstream();
-            insertNodeAtAndUpdateNodesInserted(refNode, pos.node(), pos.offset());
+            insertNodeAtAndUpdateNodesInserted(refNode.get(), pos.node(), pos.offset());
         } else {
-            insertNodeAtAndUpdateNodesInserted(refNode, insertionPos.node(), insertionPos.offset());
+            insertNodeAtAndUpdateNodesInserted(refNode.get(), insertionPos.node(), insertionPos.offset());
         }
         
         while (node) {
             Node *next = node->nextSibling();
-            insertNodeAfterAndUpdateNodesInserted(node, refNode);
+            insertNodeAfterAndUpdateNodesInserted(node.get(), refNode.get());
             refNode = node;
             node = next;
         }
index bd0b8ce..cfe3343 100644 (file)
@@ -77,6 +77,8 @@ public:
     bool hasInterchangeNewlineAtEnd() const { return m_hasInterchangeNewlineAtEnd; }
     
     bool isBlockFlow(Node*) const;
+    
+    void removeNode(PassRefPtr<Node>);
 
 private:
     // no copy construction or assignment
@@ -94,9 +96,7 @@ private:
     int renderedBlocks(Node*);
     void removeStyleNodes();
 
-    // A couple simple DOM helpers
     Node *enclosingBlock(Node *) const;
-    void removeNode(PassRefPtr<Node>);
     void removeNodePreservingChildren(Node *);
     void insertNodeBefore(Node *node, Node *refNode);