Reviewed by John
authorkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Oct 2004 23:21:42 +0000 (23:21 +0000)
committerkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Oct 2004 23:21:42 +0000 (23:21 +0000)
        Fix for this bug:

        <rdar://problem/3816768> REGRESSION (Mail): Deleting last character in block incorrectly
        moves caret out of block.

        The issue here is that an empty block with no explicit height set by style collapses
        to zero height, and does so immediately after the last bit of content is removed from
        it (as a result of deleting text with the delete key for instance). Since zero-height
        blocks are not eligible caret positions, the caret jumped to the closest eligible spot.

        The fix is to detect when a block has not been removed itself, but has had all its
        contents removed. In this case, a BR element is placed in the block, one that is
        specially marked as a placeholder. Later, if the block ever receives content, this
        placeholder is removed.

        * khtml/editing/htmlediting.cpp:
        (khtml::blockPlaceholerClassString): String which acts as a placeholder marker class.
        (khtml::CompositeEditCommand::insertBlockPlaceholderIfNeeded): Adds a placeholder BR if needed.
        (khtml::CompositeEditCommand::removeBlockPlaceholderIfNeeded): Removes a placeholder BR if needed.
        (khtml::DeleteSelectionCommand::moveNodesAfterNode): Call removeBlockPlaceholderIfNeeded.
        Also, do some cleanup on some old, crufty code in the move logic that is just so clearly wrong
        (it's very clear that we needs to be able to move more than just text nodes). This may expose
        bugs, but these bugs needs to be filed and fixed, not ducked. Besides, undoing this silliness
        made the test case in the bug work.
        (khtml::DeleteSelectionCommand::doApply): Call insertBlockPlaceholderIfNeeded and
        removeBlockPlaceholderIfNeeded.
        (khtml::InputTextCommand::input): Call removeBlockPlaceholderIfNeeded.
        (khtml::ReplaceSelectionCommand::doApply): Call removeBlockPlaceholderIfNeeded.
        * khtml/editing/htmlediting.h: Declare new functions.

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

WebCore/ChangeLog-2005-08-23
WebCore/khtml/editing/htmlediting.cpp
WebCore/khtml/editing/htmlediting.h

index 8e57af7409e7a18b9b076dd631f75e818425366a..1057cdafb48874f889821e76f9dfa37f3e824068 100644 (file)
@@ -1,3 +1,37 @@
+2004-10-13  Ken Kocienda  <kocienda@apple.com>
+
+        Reviewed by John
+
+        Fix for this bug:
+        
+        <rdar://problem/3816768> REGRESSION (Mail): Deleting last character in block incorrectly 
+        moves caret out of block.
+
+        The issue here is that an empty block with no explicit height set by style collapses
+        to zero height, and does so immediately after the last bit of content is removed from
+        it (as a result of deleting text with the delete key for instance). Since zero-height
+        blocks are not eligible caret positions, the caret jumped to the closest eligible spot.
+        
+        The fix is to detect when a block has not been removed itself, but has had all its 
+        contents removed. In this case, a BR element is placed in the block, one that is
+        specially marked as a placeholder. Later, if the block ever receives content, this
+        placeholder is removed.
+
+        * khtml/editing/htmlediting.cpp:
+        (khtml::blockPlaceholerClassString): String which acts as a placeholder marker class.
+        (khtml::CompositeEditCommand::insertBlockPlaceholderIfNeeded): Adds a placeholder BR if needed.
+        (khtml::CompositeEditCommand::removeBlockPlaceholderIfNeeded): Removes a placeholder BR if needed.
+        (khtml::DeleteSelectionCommand::moveNodesAfterNode): Call removeBlockPlaceholderIfNeeded.
+        Also, do some cleanup on some old, crufty code in the move logic that is just so clearly wrong 
+        (it's very clear that we needs to be able to move more than just text nodes). This may expose
+        bugs, but these bugs needs to be filed and fixed, not ducked. Besides, undoing this silliness
+        made the test case in the bug work.
+        (khtml::DeleteSelectionCommand::doApply): Call insertBlockPlaceholderIfNeeded and
+        removeBlockPlaceholderIfNeeded.
+        (khtml::InputTextCommand::input): Call removeBlockPlaceholderIfNeeded.
+        (khtml::ReplaceSelectionCommand::doApply): Call removeBlockPlaceholderIfNeeded.
+        * khtml/editing/htmlediting.h: Declare new functions.
+
 2004-10-13  Richard Williamson   <rjw@apple.com>
 
         Added support for -apple-dashboard-region:none.  And fixed
index ca197022b7f147c0a5144421c0767126baba1cfe..1f99dff511c32e2e223f44a7c216008f95e6660f 100644 (file)
@@ -168,6 +168,12 @@ static DOMString &styleSpanClassString()
     return styleSpanClassString;
 }
 
+static DOMString &blockPlaceholderClassString()
+{
+    static DOMString blockPlaceholderClassString = "khtml-block-placeholder";
+    return blockPlaceholderClassString;
+}
+
 static void debugPosition(const char *prefix, const Position &pos)
 {
     if (!prefix)
@@ -798,6 +804,44 @@ void CompositeEditCommand::deleteUnrenderedText(const Position &pos)
         setEndingSelection(block);
 }
 
+void CompositeEditCommand::insertBlockPlaceholderIfNeeded(NodeImpl *node)
+{
+    document()->updateLayout();
+
+    RenderObject *renderer = node->renderer();
+    if (!renderer->isBlockFlow())
+        return;
+    
+    if (renderer->height() > 0)
+        return;
+
+    int exceptionCode = 0;
+    ElementImpl *breakNode = document()->createHTMLElement("BR", exceptionCode);
+    ASSERT(exceptionCode == 0);
+    breakNode->setAttribute(ATTR_CLASS, blockPlaceholderClassString());
+    appendNode(breakNode, node);
+}
+
+void CompositeEditCommand::removeBlockPlaceholderIfNeeded(NodeImpl *node)
+{
+    document()->updateLayout();
+
+    RenderObject *renderer = node->renderer();
+    if (!renderer->isBlockFlow())
+        return;
+
+    // This code will remove a block placeholder if it still is at the end
+    // of a block, where we placed it in insertBlockPlaceholderIfNeeded().
+    // Of course, a person who hand-edits an HTML file could move a 
+    // placeholder around, but it seems OK to be unconcerned about that case.
+    NodeImpl *last = node->lastChild();
+    if (last->isHTMLElement()) {
+        ElementImpl *element = static_cast<ElementImpl *>(last);
+        if (element->getAttribute(ATTR_CLASS) == blockPlaceholderClassString())
+            removeNode(element);
+    }
+}
+
 //==========================================================================================
 // Concrete commands
 //------------------------------------------------------------------------------------------
@@ -1169,15 +1213,12 @@ void DeleteSelectionCommand::moveNodesAfterNode(NodeImpl *startNode, NodeImpl *d
         // Do not move content between parts of a table
         return;
 
+    // Now that we are about to add content, check to see if a placeholder element
+    // can be removed.
+    removeBlockPlaceholderIfNeeded(startBlock);
+
     NodeImpl *node = startNode == startBlock ? startBlock->firstChild() : startNode;
 
-    // Only do the move if node is a text node.
-    // This is done to duplicate the behavior in NSText, particularly to
-    // mimic the behavior in Mail where deletions are done between blocks
-    // that are quoted.
-    if (!node || !node->isTextNode())
-        return;
-    
     // Do the move.
     NodeImpl *refNode = dstNode;
     while (node && node->isAncestor(startBlock)) {
@@ -1200,9 +1241,9 @@ void DeleteSelectionCommand::moveNodesAfterNode(NodeImpl *startNode, NodeImpl *d
     // This will have the side effect of moving 'Three' on to the same line as 'One'
     // and 'Two'. This is undesirable. We fix this up by adding a BR before the 'Three'.
     // This may not be ideal, but it is better than nothing.
-    if (!startBlock->firstChild()) {
+    document()->updateLayout();
+    if (startBlock->renderer() && startBlock->renderer()->height() == 0) {
         removeNode(startBlock);
-        document()->updateLayout();
         if (refNode->renderer() && refNode->renderer()->inlineBox() && refNode->renderer()->inlineBox()->nextOnLineExists()) {
             int exceptionCode = 0;
             ElementImpl *breakNode = document()->createHTMLElement("BR", exceptionCode);
@@ -1417,6 +1458,10 @@ void DeleteSelectionCommand::doApply()
         }
     }
 
+    // If the delete emptied a block, add in a placeholder so the block does not
+    // seem to disappear.
+    insertBlockPlaceholderIfNeeded(endingPosition.node());
+
     // Compute the difference between the style before the delete and the style now
     // after the delete has been done. Set this style on the part, so other editing
     // commands being composed with this one will work, and also cache it on the command,
@@ -1735,6 +1780,10 @@ void InputTextCommand::input(const DOMString &text, bool selectInsertedText)
     
     TextImpl *textNode = static_cast<TextImpl *>(pos.node());
     long offset = pos.offset();
+
+    // Now that we are about to add content, check to see if a placeholder element
+    // can be removed.
+    removeBlockPlaceholderIfNeeded(textNode->enclosingBlockFlowElement());
     
     // These are temporary implementations for inserting adjoining spaces
     // into a document. We are working on a CSS-related whitespace solution
@@ -2013,6 +2062,10 @@ void ReplaceSelectionCommand::doApply()
     
     selection = endingSelection();
     ASSERT(selection.isCaret());
+
+    // Now that we are about to add content, check to see if a placeholder element
+    // can be removed.
+    removeBlockPlaceholderIfNeeded(selection.start().node()->enclosingBlockFlowElement());
     
     bool addLeadingSpace = false;
     bool addTrailingSpace = false;
index 2e5dc9e8014720fff819b44c4550bd06a104b0bb..d1cd0d7f797ec67eb706c60925edb22a9e15f42e 100644 (file)
@@ -198,6 +198,9 @@ protected:
     void deleteUnrenderedText(DOM::NodeImpl *);
     void deleteUnrenderedText(const DOM::Position &pos);
 
+    void insertBlockPlaceholderIfNeeded(DOM::NodeImpl *);
+    void removeBlockPlaceholderIfNeeded(DOM::NodeImpl *);
+
     QValueList<EditCommandPtr> m_cmds;
 };