Reviewed by Darin
authorkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 5 Mar 2005 22:40:17 +0000 (22:40 +0000)
committerkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 5 Mar 2005 22:40:17 +0000 (22:40 +0000)
        Fix for this bug:

        <rdar://problem/4038267> REGRESSION (Mail): Crash copying and pasting end of paragraph

        Code to handle content that has a "logical" newline at the end of the pasted content, and the code
        to adjust the selection at the end of the paste operation made an assumption that at least one
        node had been inserted by the paste command. This is not necessarily true in the case where the sole content
        in the pasted content is one of these "logical" newlines. Adjust some code around so that we don't deref
        null, but still adjust the selection correctly for this case. In each of the two functions below, some
        null checks have been added, and some code has been rearranged a little bit to continue on through
        the end of completeHTMLReplacement, even if no nodes have been inserted. The patch looks bigger and more
        complicated than the conceptual change.

        * khtml/editing/htmlediting.cpp:
        (khtml::ReplaceSelectionCommand::doApply)
        (khtml::ReplaceSelectionCommand::completeHTMLReplacement)

        * layout-tests/editing/pasteboard/paste-4038267-fix-expected.txt: Added.
        * layout-tests/editing/pasteboard/paste-4038267-fix.html: Added.

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

LayoutTests/editing/pasteboard/paste-4038267-fix-expected.txt [new file with mode: 0644]
LayoutTests/editing/pasteboard/paste-4038267-fix.html [new file with mode: 0644]
WebCore/ChangeLog-2005-08-23
WebCore/khtml/editing/htmlediting.cpp

diff --git a/LayoutTests/editing/pasteboard/paste-4038267-fix-expected.txt b/LayoutTests/editing/pasteboard/paste-4038267-fix-expected.txt
new file mode 100644 (file)
index 0000000..ad10257
--- /dev/null
@@ -0,0 +1,44 @@
+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 {DIV} at (0,0) size 784x268 [border: (2px solid #0000FF)]
+        RenderBlock {DIV} at (14,14) size 756x84
+          RenderText {TEXT} at (0,0) size 67x28
+            text run at (0,0) width 67: "Tests: "
+          RenderBR {BR} at (0,0) size 0x0
+          RenderText {TEXT} at (0,28) size 113x28
+            text run at (0,28) width 113: "Bug fix for "
+          RenderInline {A} at (0,0) size 260x28 [color=#0000EE]
+            RenderText {TEXT} at (113,28) size 260x28
+              text run at (113,28) width 260: "<rdar://problem/4038267>"
+          RenderText {TEXT} at (373,28) size 747x56
+            text run at (373,28) width 374: " REGRESSION (Mail): Crash copying"
+            text run at (0,56) width 287: "and pasting end of paragraph."
+        RenderBlock {DIV} at (14,114) size 756x140
+          RenderBlock (anonymous) at (0,0) size 756x84
+            RenderText {TEXT} at (0,0) size 189x28
+              text run at (0,0) width 189: "Expected Results: "
+            RenderBR {BR} at (0,0) size 0x0
+            RenderText {TEXT} at (0,28) size 732x56
+              text run at (0,28) width 732: "Should see this content in the red box below (insertion point must be on the"
+              text run at (0,56) width 308: "second line, right before \"bar\"):"
+          RenderBlock {DIV} at (0,84) size 756x28
+            RenderText {TEXT} at (0,0) size 32x28
+              text run at (0,0) width 32: "foo"
+          RenderBlock {DIV} at (0,112) size 756x28
+            RenderText {TEXT} at (0,0) size 31x28
+              text run at (0,0) width 31: "bar"
+      RenderBlock {DIV} at (0,292) size 784x60
+        RenderBlock {DIV} at (0,0) size 784x60 [border: (2px solid #FF0000)]
+          RenderBlock {DIV} at (2,2) size 780x28
+            RenderText {TEXT} at (0,0) size 32x28
+              text run at (0,0) width 32: "foo"
+          RenderBlock {DIV} at (2,30) size 780x28
+            RenderText {TEXT} at (0,0) size 31x28
+              text run at (0,0) width 31: "bar"
+selection is CARET:
+start:      position 0 of child 1 {TEXT} of child 3 {DIV} of child 1 {DIV} of root {DIV}
+upstream:   position 0 of child 3 {DIV} of child 1 {DIV} of root {DIV}
+downstream: position 0 of child 1 {TEXT} of child 3 {DIV} of child 1 {DIV} of root {DIV}
diff --git a/LayoutTests/editing/pasteboard/paste-4038267-fix.html b/LayoutTests/editing/pasteboard/paste-4038267-fix.html
new file mode 100644 (file)
index 0000000..50c9941
--- /dev/null
@@ -0,0 +1,61 @@
+<html> 
+<head>
+
+<style>
+.editing { 
+    border: 2px solid red; 
+    font-size: 24px; 
+}
+.explanation { 
+    border: 2px solid blue; 
+    padding: 12px; 
+    font-size: 24px; 
+    margin-bottom: 24px;
+}
+.scenario { margin-bottom: 16px;}
+.scenario:first-line { font-weight: bold; margin-bottom: 16px;}
+.expected-results:first-line { font-weight: bold }
+</style>
+<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>
+
+<script>
+
+function editingTest() {
+    for (i = 0; i < 3; i++)
+        moveSelectionForwardByCharacterCommand();
+    extendSelectionForwardByCharacterCommand();
+    copyCommand();
+    pasteCommand();
+}
+
+</script>
+
+<title>Editing Test</title> 
+</head> 
+<body>
+
+<div class="explanation">
+<div class="scenario">
+Tests: 
+<br>
+Bug fix for <a href="rdar://problem/4038267">&lt;rdar://problem/4038267&gt;</a> REGRESSION (Mail): Crash copying and pasting end of paragraph.
+</div>
+<div class="expected-results">
+Expected Results:
+<br>
+Should see this content in the red box below (insertion point must be on the second line, right before "bar"): <div>foo</div><div>bar</div>
+</div>
+</div>
+
+<div contenteditable id="root" style="word-wrap: break-word; -khtml-nbsp-mode: space; -khtml-line-break: after-white-space;">
+<div id="test" class="editing">
+<div>foo</div><div>bar</div>
+</div>
+</div>
+
+<script>
+runEditingTest();
+</script>
+
+</body>
+</html>
index 56b16e54dc94fc61edc6c2f191b2b6104171ba9d..8712d22632a21146d2153c6db53caf036ffe491d 100644 (file)
@@ -1,3 +1,27 @@
+2005-03-05  Ken Kocienda  <kocienda@apple.com>
+
+        Reviewed by Darin
+
+        Fix for this bug:
+        
+        <rdar://problem/4038267> REGRESSION (Mail): Crash copying and pasting end of paragraph
+
+        Code to handle content that has a "logical" newline at the end of the pasted content, and the code
+        to adjust the selection at the end of the paste operation made an assumption that at least one
+        node had been inserted by the paste command. This is not necessarily true in the case where the sole content
+        in the pasted content is one of these "logical" newlines. Adjust some code around so that we don't deref
+        null, but still adjust the selection correctly for this case. In each of the two functions below, some
+        null checks have been added, and some code has been rearranged a little bit to continue on through
+        the end of completeHTMLReplacement, even if no nodes have been inserted. The patch looks bigger and more
+        complicated than the conceptual change.
+
+        * khtml/editing/htmlediting.cpp:
+        (khtml::ReplaceSelectionCommand::doApply)
+        (khtml::ReplaceSelectionCommand::completeHTMLReplacement)
+        
+        * layout-tests/editing/pasteboard/paste-4038267-fix-expected.txt: Added.
+        * layout-tests/editing/pasteboard/paste-4038267-fix.html: Added.
+
 2005-03-05  Darin Adler  <darin@apple.com>
 
         Reviewed by John.
index c3de16db87eca821b1156b2afed83621694049ab..598b04e3fbefdb910eaee77c3d226c18e6283f95 100644 (file)
@@ -4696,28 +4696,35 @@ void ReplaceSelectionCommand::doApply()
 
     // step 4 : handle trailing newline
     if (m_fragment.hasInterchangeNewline()) {
-        bool insertParagraph = false;
-        if (startBlock == endBlock && !isProbablyBlock(m_lastTopNodeInserted)) {
-            insertParagraph = true;
+        if (!m_lastNodeInserted) {
+            lastPositionToSelect = endingSelection().end().downstream();
         }
         else {
-            // Handle end-of-document case.
-            document()->updateLayout();
-            VisiblePosition pos(Position(m_lastNodeInserted, m_lastNodeInserted->caretMaxOffset()), DOWNSTREAM);
-            if (isEndOfDocument(pos))
+            bool insertParagraph = false;
+            if (startBlock == endBlock && !isProbablyBlock(m_lastTopNodeInserted)) {
                 insertParagraph = true;
+            }
+            else {
+                // Handle end-of-document case.
+                document()->updateLayout();
+                VisiblePosition pos(Position(m_lastNodeInserted, m_lastNodeInserted->caretMaxOffset()), DOWNSTREAM);
+                if (isEndOfDocument(pos))
+                    insertParagraph = true;
+            }
+            if (insertParagraph) {
+                setEndingSelection(insertionPos, DOWNSTREAM);
+                insertParagraphSeparator();
+                updateNodesInserted(endingSelection().end().downstream().node());
+                // Select up to the paragraph separator that was added.
+                lastPositionToSelect = endingSelection().end().downstream();
+            } 
+            else {
+                // Select up to the preexising paragraph separator.
+                lastPositionToSelect = Position(m_lastNodeInserted, m_lastNodeInserted->caretMaxOffset()).downstream();
+            }
         }
-        if (insertParagraph) {
-            setEndingSelection(insertionPos, DOWNSTREAM);
-            insertParagraphSeparator();
-            updateNodesInserted(endingSelection().end().downstream().node());
-            // Select up to the paragraph separator that was added.
-            lastPositionToSelect = endingSelection().end().downstream();
-        } else {
-            // Select up to the preexising paragraph separator.
-            lastPositionToSelect = Position(m_lastNodeInserted, m_lastNodeInserted->caretMaxOffset()).downstream();
-        }
-    } else {
+    } 
+    else {
         if (m_lastNodeInserted && m_lastNodeInserted->id() == ID_BR && !document()->inStrictMode()) {
             document()->updateLayout();
             VisiblePosition pos(Position(m_lastNodeInserted, 0), DOWNSTREAM);
@@ -4788,47 +4795,55 @@ void ReplaceSelectionCommand::doApply()
 
 void ReplaceSelectionCommand::completeHTMLReplacement(const Position &lastPositionToSelect)
 {
-    if (!m_firstNodeInserted || !m_firstNodeInserted->inDocument() ||
-        !m_lastNodeInserted || !m_lastNodeInserted->inDocument())
-        return;
+    Position start;
+    Position end;
 
-    // Find the last leaf.
-    NodeImpl *lastLeaf = m_lastNodeInserted;
-    while (1) {
-        NodeImpl *nextChild = lastLeaf->lastChild();
-        if (!nextChild)
-            break;
-        lastLeaf = nextChild;
-    }
+    if (m_firstNodeInserted && m_firstNodeInserted->inDocument() &&
+        m_lastNodeInserted && m_lastNodeInserted->inDocument()) {
 
-    // Find the first leaf.
-    NodeImpl *firstLeaf = m_firstNodeInserted;
-    while (1) {
-        NodeImpl *nextChild = firstLeaf->firstChild();
-        if (!nextChild)
-            break;
-        firstLeaf = nextChild;
-    }
-    
-    // Call updateLayout so caretMinOffset and caretMaxOffset return correct values.
-    document()->updateLayout();
-    Position start(firstLeaf, firstLeaf->caretMinOffset());
-    Position end(lastLeaf, lastLeaf->caretMaxOffset());
-    
-    if (m_matchStyle) {
-        assert(m_insertionStyle);
-        setEndingSelection(Selection(start, SEL_DEFAULT_AFFINITY, end, SEL_DEFAULT_AFFINITY));
-        applyStyle(m_insertionStyle);
-    }    
+        // Find the last leaf.
+        NodeImpl *lastLeaf = m_lastNodeInserted;
+        while (1) {
+            NodeImpl *nextChild = lastLeaf->lastChild();
+            if (!nextChild)
+                break;
+            lastLeaf = nextChild;
+        }
     
-    if (lastPositionToSelect.isNotNull())
-        end = lastPositionToSelect;
+        // Find the first leaf.
+        NodeImpl *firstLeaf = m_firstNodeInserted;
+        while (1) {
+            NodeImpl *nextChild = firstLeaf->firstChild();
+            if (!nextChild)
+                break;
+            firstLeaf = nextChild;
+        }
+        
+        // Call updateLayout so caretMinOffset and caretMaxOffset return correct values.
+        document()->updateLayout();
+        start = Position(firstLeaf, firstLeaf->caretMinOffset());
+        end = Position(lastLeaf, lastLeaf->caretMaxOffset());
+
+        if (m_matchStyle) {
+            assert(m_insertionStyle);
+            setEndingSelection(Selection(start, SEL_DEFAULT_AFFINITY, end, SEL_DEFAULT_AFFINITY));
+            applyStyle(m_insertionStyle);
+        }    
+        
+        if (lastPositionToSelect.isNotNull())
+            end = lastPositionToSelect;
+    }
+    else if (lastPositionToSelect.isNotNull()) {
+        start = end = lastPositionToSelect;
+    }
+    else {
+        return;
+    }
     
-    if (m_selectReplacement) {
+    if (m_selectReplacement)
         setEndingSelection(Selection(start, SEL_DEFAULT_AFFINITY, end, SEL_DEFAULT_AFFINITY));
-    } else {
+    else
         setEndingSelection(end, SEL_DEFAULT_AFFINITY);
-    }
     
     rebalanceWhitespace();
 }