Reviewed by John
authorkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 Mar 2005 16:19:15 +0000 (16:19 +0000)
committerkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 Mar 2005 16:19:15 +0000 (16:19 +0000)
        Fix for this bug:

        <rdar://problem/4035648> REGRESSION (Mail): line feed in source HTML file causes bad copy/paste behavior

        The createMarkup() function in markup.cpp iterates over the nodes in a range,
        and does some bookkeeping to figure out when to add close tags to the markup.
        Some code added at the start of the loop to prevent markup from being written
        for unrendered nodes short-circuited the rest of the loop, and so prevented
        the close-tag-writing code from running when it should.

        This is why the "plain" text wound up inside of the bold tag in the example
        above. The addition of the unrendered return character caused an incorrect
        delay in the close tag for the bold element from being written out, with the
        result being that it wound up including additional content.

        The fix is to add checks for node renderers throughout the loop at the points
        where markup is written out for each node. This allows the additional close
        tag logic to run as needed.

        All layout tests pass with this change.

        * khtml/editing/markup.cpp:
        (khtml::createMarkup)

        New test:

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

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

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

diff --git a/LayoutTests/editing/pasteboard/paste-4035648-fix-expected.txt b/LayoutTests/editing/pasteboard/paste-4035648-fix-expected.txt
new file mode 100644 (file)
index 0000000..4d08446
--- /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 784x240 [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/4035648>"
+          RenderText {TEXT} at (373,28) size 717x56
+            text run at (373,28) width 344: " REGRESSION (Mail): line feed in"
+            text run at (0,56) width 487: "source HTML file causes bad copy/paste behavior"
+        RenderBlock {DIV} at (14,114) size 756x112
+          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 521x28
+            text run at (0,28) width 521: "Should see the text self-documenting itself correctly: "
+          RenderBR {BR} at (0,0) size 0x0
+          RenderInline {B} at (0,0) size 45x28
+            RenderText {TEXT} at (0,56) size 45x28
+              text run at (0,56) width 45: "bold"
+            RenderBR {BR} at (0,0) size 0x0
+          RenderText {TEXT} at (0,84) size 49x28
+            text run at (0,84) width 49: "plain"
+      RenderBlock {DIV} at (0,264) size 784x88
+        RenderBlock {DIV} at (0,0) size 784x88 [border: (2px solid #FF0000)]
+          RenderBR {BR} at (2,2) size 0x28
+          RenderInline {B} at (0,0) size 45x28
+            RenderText {TEXT} at (2,30) size 45x28
+              text run at (2,30) width 45: "bold"
+            RenderBR {BR} at (0,0) size 0x0
+          RenderText {TEXT} at (2,58) size 49x28
+            text run at (2,58) width 49: "plain"
+selection is CARET:
+start:      position 5 of child 3 {TEXT} of child 1 {DIV} of root {DIV}
+upstream:   position 5 of child 3 {TEXT} of child 1 {DIV} of root {DIV}
+downstream: position 5 of child 3 {TEXT} of child 1 {DIV} of root {DIV}
diff --git a/LayoutTests/editing/pasteboard/paste-4035648-fix.html b/LayoutTests/editing/pasteboard/paste-4035648-fix.html
new file mode 100644 (file)
index 0000000..13184d5
--- /dev/null
@@ -0,0 +1,67 @@
+<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++)
+        extendSelectionForwardByLineCommand();
+    cutCommand();
+    pasteCommand();
+}
+
+</script>
+
+<title>Editing Test</title> 
+</head> 
+<body>
+
+<div class="explanation">
+<div class="scenario">
+Tests: 
+<br>
+Bug fix for <a href="rdar://problem/4035648">&lt;rdar://problem/4035648&gt;</a> REGRESSION (Mail): line feed in source HTML file causes bad copy/paste behavior
+</div>
+<div class="expected-results">
+Expected Results:
+<br>
+Should see the text self-documenting itself correctly:
+<BR>
+<B>bold<BR>
+</B>
+plain
+</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">
+<BR>
+<B>bold<BR>
+</B>
+plain
+</div>
+</div>
+
+<script>
+runEditingTest();
+</script>
+
+</body>
+</html>
index 5d6088ab32c36b44f71d5978ef571085d96a75d1..345fb5bf738e05d4f420ddeb352b8ce917d98056 100644 (file)
@@ -1,3 +1,36 @@
+2005-03-07  Ken Kocienda  <kocienda@apple.com>
+
+        Reviewed by John
+
+        Fix for this bug:
+        
+        <rdar://problem/4035648> REGRESSION (Mail): line feed in source HTML file causes bad copy/paste behavior
+
+        The createMarkup() function in markup.cpp iterates over the nodes in a range,
+        and does some bookkeeping to figure out when to add close tags to the markup.
+        Some code added at the start of the loop to prevent markup from being written
+        for unrendered nodes short-circuited the rest of the loop, and so prevented
+        the close-tag-writing code from running when it should.
+
+        This is why the "plain" text wound up inside of the bold tag in the example
+        above. The addition of the unrendered return character caused an incorrect
+        delay in the close tag for the bold element from being written out, with the
+        result being that it wound up including additional content.
+
+        The fix is to add checks for node renderers throughout the loop at the points
+        where markup is written out for each node. This allows the additional close
+        tag logic to run as needed.
+
+        All layout tests pass with this change.
+
+        * khtml/editing/markup.cpp:
+        (khtml::createMarkup)
+
+        New test:
+        
+        * layout-tests/editing/pasteboard/paste-4035648-fix-expected.txt: Added.
+        * layout-tests/editing/pasteboard/paste-4035648-fix.html: Added.
+
 2005-03-06  Christy Warren  kali@appple.com
 
         Reviewed by Ken
index 0de02ff7394c448efa15d75584d8df254eec9ca3..bfe8b101a0d4968a0b68ca9c3317e7954f8cc65e 100644 (file)
@@ -330,8 +330,6 @@ QString createMarkup(const RangeImpl *range, QPtrList<NodeImpl> *nodes, EAnnotat
     NodeImpl *next;
     for (NodeImpl *n = range->startNode(); n != pastEnd; n = next) {
         next = n->traverseNextNode();
-        if (!n->renderer())
-            continue;
 
         if (n->isBlockFlow() && next == pastEnd) {
             // Don't write out an empty block.
@@ -339,15 +337,19 @@ QString createMarkup(const RangeImpl *range, QPtrList<NodeImpl> *nodes, EAnnotat
         }
         
         // Add the node to the markup.
-        markups.append(startMarkup(n, range, annotate, defaultStyle));
-        if (nodes) {
-            nodes->append(n);
+        if (n->renderer()) {
+            markups.append(startMarkup(n, range, annotate, defaultStyle));
+            if (nodes) {
+                nodes->append(n);
+            }
         }
         
         if (n->firstChild() == 0) {
             // Node has no children, add its close tag now.
-            markups.append(endMarkup(n));
-            lastClosed = n;
+            if (n->renderer()) {
+                markups.append(endMarkup(n));
+                lastClosed = n;
+            }
             
             // Check if the node is the last leaf of a tree.
             if (n->nextSibling() == 0 || next == pastEnd) {
@@ -379,7 +381,7 @@ QString createMarkup(const RangeImpl *range, QPtrList<NodeImpl> *nodes, EAnnotat
                     }
                 }
             }
-        } else {
+        } else if (n->renderer()) {
             // Node is an ancestor, set it to close eventually.
             ancestorsToClose.append(n);
         }