Reviewed by Maciej
authorkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 6 Dec 2004 23:57:52 +0000 (23:57 +0000)
committerkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 6 Dec 2004 23:57:52 +0000 (23:57 +0000)
        Fix for this bug:

        <rdar://problem/3890955> 8A314: Forward delete sometimes fails to delete the selected quoted text

        * khtml/editing/htmlediting.cpp:
        (khtml::DeleteSelectionCommand::handleSpecialCaseBRDelete): Fixed bonehead coding mistake in the
        check for one of the special cases being checked for in this function. The specific case
        intends to check for a selection that is only a <br> after a block ends (as in </div><br>). If it
        sees such markup, it deletes only the <br> and bails. However, this code would run in *any*
        case where a selection ended in a <br> after a block and would not delete any part of the
        selection preceding the <br>. Bad. I have tightened the check to see that only a <br> is
        selected.

        Fixing the bug above was accomplished with an additional call to DOM::Position::downstream. This
        new use of the function exposed this bug:

        <rdar://problem/3907666> Incorrectly coded loop in Position::downstream can lead to infinite loop

        * khtml/xml/dom_position.cpp:
        (DOM::Position::downstream): I am ashamed of my first cut at this. Rewrote the loop so it does
        not have this fatal flaw. It is a much better design as well.

        * layout-tests/editing/deleting/delete-3800834-fix-expected.txt: Changes made this test
        have what I consider to be a better result. Going with it.

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

LayoutTests/editing/deleting/delete-3800834-fix-expected.txt
WebCore/ChangeLog-2005-08-23
WebCore/khtml/editing/htmlediting.cpp
WebCore/khtml/xml/dom_position.cpp

index 757c9bb532d27fe06d548dd1eae35000c89a34a6..8cc4f31d4baff3ccf3dffe1677cf98dbbaadb6c7 100644 (file)
@@ -3,18 +3,16 @@ layer 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 784x80 [border: (2px solid #FF0000)]
+      RenderBlock {DIV} at (0,0) size 784x56 [border: (2px solid #FF0000)]
         RenderBlock (anonymous) at (14,14) size 756x28
           RenderInline {SPAN} at (0,0) size 37x28
             RenderText {TEXT} at (0,0) size 37x28
               text run at (0,0) width 37: "Foo"
-            RenderBR {BR} at (0,0) size 0x0
-        RenderBlock (anonymous) at (14,66) size 756x0
-          RenderBlock {BLOCKQUOTE} at (40,0) size 676x0
-        RenderBlock (anonymous) at (14,66) size 756x0
+        RenderBlock (anonymous) at (14,42) size 756x0
+        RenderBlock (anonymous) at (14,42) size 756x0
           RenderInline {SPAN} at (0,0) size 0x0
           RenderText {TEXT} at (0,0) size 0x0
 selection is CARET:
 start:      position 3 of child 1 {TEXT} of child 2 {SPAN} of root {DIV}
 upstream:   position 3 of child 1 {TEXT} of child 2 {SPAN} of root {DIV}
-downstream: position 0 of child 2 {BR} of child 2 {SPAN} of root {DIV}
+downstream: position 1 of child 3 {TEXT} of root {DIV}
index ee6307d47c708357914b846c7df239b7d46b0293..7d038c7827a924a3352821d17680ccb187fdf4b5 100644 (file)
@@ -1,3 +1,32 @@
+2004-12-06  Ken Kocienda  <kocienda@apple.com>
+
+        Reviewed by Maciej
+
+        Fix for this bug:
+        
+        <rdar://problem/3890955> 8A314: Forward delete sometimes fails to delete the selected quoted text
+
+        * khtml/editing/htmlediting.cpp:
+        (khtml::DeleteSelectionCommand::handleSpecialCaseBRDelete): Fixed bonehead coding mistake in the
+        check for one of the special cases being checked for in this function. The specific case 
+        intends to check for a selection that is only a <br> after a block ends (as in </div><br>). If it
+        sees such markup, it deletes only the <br> and bails. However, this code would run in *any*
+        case where a selection ended in a <br> after a block and would not delete any part of the
+        selection preceding the <br>. Bad. I have tightened the check to see that only a <br> is
+        selected.
+
+        Fixing the bug above was accomplished with an additional call to DOM::Position::downstream. This
+        new use of the function exposed this bug:
+        
+        <rdar://problem/3907666> Incorrectly coded loop in Position::downstream can lead to infinite loop
+
+        * khtml/xml/dom_position.cpp:
+        (DOM::Position::downstream): I am ashamed of my first cut at this. Rewrote the loop so it does 
+        not have this fatal flaw. It is a much better design as well.
+        
+        * layout-tests/editing/deleting/delete-3800834-fix-expected.txt: Changes made this test
+        have what I consider to be a better result. Going with it.
+
 2004-12-06  Chris Blumenberg  <cblu@apple.com>
 
        Fixed: <rdar://problem/3871718> REGRESSION (125-168): text marked bold with font that does not have bold variant copies as non-bold
index 4cfa1033388cc16c2978ae516c573d0995717d95..d5ea1271ffb47e722f85763ec537bd4e755a1199 100644 (file)
@@ -1581,7 +1581,9 @@ bool DeleteSelectionCommand::handleSpecialCaseBRDelete()
     // Check for special-case where the selection contains only a BR right after a block ended.
     bool downstreamEndIsBR = m_downstreamEnd.node()->id() == ID_BR;
     Position upstreamFromBR = m_downstreamEnd.upstream();
-    bool startIsBRAfterBlock = downstreamEndIsBR && m_downstreamEnd.node()->enclosingBlockFlowElement() != upstreamFromBR.node()->enclosingBlockFlowElement();
+    Position downstreamFromStart = m_downstreamStart.downstream();
+    bool startIsBRAfterBlock = downstreamEndIsBR && downstreamFromStart.node() == m_downstreamEnd.node() &&
+        m_downstreamEnd.node()->enclosingBlockFlowElement() != upstreamFromBR.node()->enclosingBlockFlowElement();
     if (startIsBRAfterBlock) {
         removeNode(m_downstreamEnd.node());
         m_endingPosition = upstreamFromBR;
index 358b4d3faff9606fc45438afdda3225d333a7929..6530bb887f0b9ec30fcd5f20eea469809dc1d2d0 100644 (file)
@@ -440,15 +440,20 @@ Position Position::downstream(EStayInBlock stayInBlock) const
 
         if (currentNode != startNode && renderer->isBlockFlow()) {
             if (it.current().offset() == 0) {
-                NodeImpl *node = currentNode;
-                while (NodeImpl *firstChild = node->firstChild()) {
-                    if (node->renderer()->style()->visibility() == VISIBLE && node->renderer()->isBlockFlow())
-                        node = firstChild;
+                // If no first child, or first visible child is a not a block, return; otherwise continue.
+                if (!currentNode->firstChild())
+                    return Position(currentNode, 0);
+                for (NodeImpl *child = currentNode->firstChild(); child; child = child->nextSibling()) {
+                    RenderObject *r = child->renderer();
+                    if (r && r->style()->visibility() == VISIBLE) {
+                         if (r->isBlockFlow())
+                            break; // break causes continue code below to run.
+                         else
+                            return Position(child, 0);
+                    }
                 }
-                return Position(node, 0);
-            }
-            else
                 continue;
+            }
         }
 
         if (renderer->isReplaced() || renderer->isBR()) {