Reviewed by Chris
authorkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 19 Feb 2005 22:53:43 +0000 (22:53 +0000)
committerkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 19 Feb 2005 22:53:43 +0000 (22:53 +0000)
        Fix for these bugs:

        <rdar://problem/4014228> REGRESSION (186-187) extra, uneditable lines inserted above and below a line of pasted quoted text
        <rdar://problem/4014393> REGRESSION (186-187) pasted quoted text gets extra <cr>s when pasted at top of document

        * khtml/editing/htmlediting.cpp:
        (khtml::ReplacementFragment::ReplacementFragment): Part of a general refactoring of how
        the fragment is inserted into the document, rendered, and then tested for certain
        important pieces of information that are required for pasting.
        (khtml::ReplacementFragment::insertFragmentForTestRendering): New helper. Handles inserting
        the fragment nodes into the document.
        (khtml::ReplacementFragment::restoreTestRenderingNodesToFragment): Removes nodes from the
        document, and restores them to the fragment.
        (khtml::ReplacementFragment::computeStylesUsingTestRendering): Factored out code that
        did this before into its own function.
        (khtml::ReplacementFragment::removeUnrenderedNodesUsingTestRendering): Ditto.
        (khtml::ReplacementFragment::countRenderedBlocks): This is a real improvement, as it
        eliminates a major use of the isProbablyBlock() function. Now, the blocks that are
        counted are real, rendered blocks.
        (khtml::ReplacementFragment::removeStyleNodes): Made this function retain margin-zeroing
        CSS properties on paragraphs. This does two things: 1) It helps us to maintain good behavior
        in the short term while there are still versions of Mail out there that use <p> elements
        instead of <div> elements for new paragraphs; and 2) It will help to maintain the compatibility
        with other mail clients that use <p> elements for their paragraphs but render them themselves
        with no margins as the result of quirks.
        (khtml::ReplaceSelectionCommand::doApply): Do some work to fix up and improve the handling
        of blank lines, be they <p> elements or <br> elements, that can be removed after pasting. This,
        coupled with the refactoring, fixes 4014393.
        * khtml/editing/htmlediting.h: Updated for new functions.
        * layout-tests/editing/pasteboard/paste-text-010-expected.txt: Updated results, actually improved with this change.
        * layout-tests/editing/pasteboard/paste-text-011-expected.txt: Ditto.

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

LayoutTests/editing/pasteboard/paste-text-010-expected.txt
LayoutTests/editing/pasteboard/paste-text-011-expected.txt
WebCore/ChangeLog-2005-08-23
WebCore/khtml/editing/htmlediting.cpp
WebCore/khtml/editing/htmlediting.h

index 710a95e327b5c4c75e20862132cd4b44caa05275..de42b7140159e15f80a496e51cd9016ee13f19cf 100644 (file)
@@ -12,8 +12,7 @@ layer at (0,0) size 800x600
           text run at (14,70) width 74: "of men."
         RenderText {TEXT} at (88,70) size 74x28
           text run at (88,70) width 74: "of men."
-        RenderBR {BR} at (0,0) size 0x0
 selection is CARET:
 start:      position 7 of child 5 {TEXT} of child 2 {DIV} of root {BODY}
 upstream:   position 7 of child 5 {TEXT} of child 2 {DIV} of root {BODY}
-downstream: position 0 of child 6 {BR} of child 2 {DIV} of root {BODY}
+downstream: position 7 of child 5 {TEXT} of child 2 {DIV} of root {BODY}
index b08a1af58904c108db27f2ca9311afcd64a436cb..38bfea51b21d5f16a148effe5fb6b9e605a0a8c1 100644 (file)
@@ -23,9 +23,7 @@ layer at (0,0) size 800x600
           RenderInline {SPAN} at (0,0) size 39x18
             RenderText {TEXT} at (0,0) size 39x18
               text run at (0,0) width 39: "there"
-          RenderInline {SPAN} at (0,0) size 0x18
-            RenderInline {B} at (0,0) size 0x18
 selection is CARET:
 start:      position 5 of child 1 {TEXT} of child 1 {SPAN} of child 1 {B} of child 5 {P} of root {BODY}
 upstream:   position 5 of child 1 {TEXT} of child 1 {SPAN} of child 1 {B} of child 5 {P} of root {BODY}
-downstream: position 1 of child 1 {B} of child 2 {SPAN} of child 1 {B} of child 5 {P} of root {BODY}
+downstream: position 5 of child 1 {TEXT} of child 1 {SPAN} of child 1 {B} of child 5 {P} of root {BODY}
index 501128355a5e9f621056d4e3677b24e387223232..a683759288bed4ed57a048e73188e0e225166941 100644 (file)
@@ -1,3 +1,39 @@
+2005-02-19  Ken Kocienda  <kocienda@apple.com>
+
+        Reviewed by Chris
+
+        Fix for these bugs:
+        
+        <rdar://problem/4014228> REGRESSION (186-187) extra, uneditable lines inserted above and below a line of pasted quoted text
+        <rdar://problem/4014393> REGRESSION (186-187) pasted quoted text gets extra <cr>s when pasted at top of document
+
+        * khtml/editing/htmlediting.cpp:
+        (khtml::ReplacementFragment::ReplacementFragment): Part of a general refactoring of how
+        the fragment is inserted into the document, rendered, and then tested for certain
+        important pieces of information that are required for pasting.
+        (khtml::ReplacementFragment::insertFragmentForTestRendering): New helper. Handles inserting
+        the fragment nodes into the document.
+        (khtml::ReplacementFragment::restoreTestRenderingNodesToFragment): Removes nodes from the
+        document, and restores them to the fragment.
+        (khtml::ReplacementFragment::computeStylesUsingTestRendering): Factored out code that
+        did this before into its own function.
+        (khtml::ReplacementFragment::removeUnrenderedNodesUsingTestRendering): Ditto.
+        (khtml::ReplacementFragment::countRenderedBlocks): This is a real improvement, as it
+        eliminates a major use of the isProbablyBlock() function. Now, the blocks that are
+        counted are real, rendered blocks.
+        (khtml::ReplacementFragment::removeStyleNodes): Made this function retain margin-zeroing
+        CSS properties on paragraphs. This does two things: 1) It helps us to maintain good behavior
+        in the short term while there are still versions of Mail out there that use <p> elements
+        instead of <div> elements for new paragraphs; and 2) It will help to maintain the compatibility
+        with other mail clients that use <p> elements for their paragraphs but render them themselves
+        with no margins as the result of quirks.
+        (khtml::ReplaceSelectionCommand::doApply): Do some work to fix up and improve the handling
+        of blank lines, be they <p> elements or <br> elements, that can be removed after pasting. This, 
+        coupled with the refactoring, fixes 4014393.
+        * khtml/editing/htmlediting.h: Updated for new functions.
+        * layout-tests/editing/pasteboard/paste-text-010-expected.txt: Updated results, actually improved with this change.
+        * layout-tests/editing/pasteboard/paste-text-011-expected.txt: Ditto.
+
 2005-02-19  Kevin Decker  <kdecker@apple.com>
 
         Reviewed by Chris.
index 47490cb58da9310959bca900acd28950ca353ae3..9a8995429ffd6aadf874a5ee65630db4e2c11cf8 100644 (file)
@@ -3969,7 +3969,6 @@ ReplacementFragment::ReplacementFragment(DocumentImpl *document, DocumentFragmen
     m_type = TreeFragment;
 
     NodeImpl *node = m_fragment->firstChild();
-    int realBlockCount = 0;
     NodeImpl *nodeToDelete = 0;
     while (node) {
         NodeImpl *next = node->traverseNextNode();
@@ -3989,30 +3988,24 @@ ReplacementFragment::ReplacementFragment(DocumentImpl *document, DocumentFragmen
             if (n)
                 next = n->traverseNextNode();
         }
-        else if (isProbablyBlock(node))
-            realBlockCount++;    
         node = next;
     }
 
     if (nodeToDelete)
         removeNode(nodeToDelete);
 
-    // Prepare this fragment to merge styles correctly into the destination.
+    
+    NodeImpl *holder = insertFragmentForTestRendering();
+    holder->ref();
     if (!m_matchStyle) {
-        computeStylesAndRemoveUnrendered();
+        computeStylesUsingTestRendering(holder);
     }
+    removeUnrenderedNodesUsingTestRendering(holder);
+    m_hasMoreThanOneBlock = countRenderedBlocks(holder) > 1;
+    restoreTestRenderingNodesToFragment(holder);
+    removeNode(holder);
+    holder->deref();
     removeStyleNodes();
-
-    int blockCount = realBlockCount;
-    firstChild = m_fragment->firstChild();
-    lastChild = m_fragment->lastChild();
-    if (!isProbablyBlock(firstChild))
-        blockCount++;
-    if (!isProbablyBlock(lastChild) && realBlockCount > 0)
-        blockCount++;
-
-    if (blockCount > 1)
-        m_hasMoreThanOneBlock = true;
 }
 
 ReplacementFragment::~ReplacementFragment()
@@ -4136,45 +4129,32 @@ void ReplacementFragment::insertNodeBefore(NodeImpl *node, NodeImpl *refNode)
     ASSERT(exceptionCode == 0);
 }
 
-void ReplacementFragment::computeStylesAndRemoveUnrendered()
+NodeImpl *ReplacementFragment::insertFragmentForTestRendering()
 {
-    // This function inserts the nodes from the fragment into the destination
-    // document and computes the style for each. This information will be
-    // used later to apply the necessary styles to the nodes after placing
-    // them in their intended location.
     NodeImpl *body = m_document->body();
     if (!body)
-        return;
+        return 0;
 
     ElementImpl *holder = createDefaultParagraphElement(m_document);
-    holder->ref();
-
-    QPtrList<NodeImpl> unrendered;
+    
     int exceptionCode = 0;
     holder->appendChild(m_fragment, exceptionCode);
     ASSERT(exceptionCode == 0);
+    
     body->appendChild(holder, exceptionCode);
     ASSERT(exceptionCode == 0);
+    
     m_document->updateLayout();
-    for (NodeImpl *node = holder->firstChild(); node; node = node->traverseNextNode()) {
-        if (!isNodeRendered(node)) {
-            unrendered.append(node);
-        }
-        else {
-            CSSComputedStyleDeclarationImpl *computedStyle = Position(node, 0).computedStyle();
-            computedStyle->ref();
-            CSSMutableStyleDeclarationImpl *style = computedStyle->copyInheritableProperties();
-            style->ref();
-            node->ref();
-            removeBlockquoteColorsIfNeeded(node, style);
-            m_styles[node] = style;
-            computedStyle->deref();
-        }
-    }
-    body->removeChild(holder, exceptionCode);
-    ASSERT(exceptionCode == 0);
-    m_document->updateLayout();
+    
+    return holder;
+}
 
+void ReplacementFragment::restoreTestRenderingNodesToFragment(NodeImpl *holder)
+{
+    if (!holder)
+        return;
+
+    int exceptionCode = 0;
     while (NodeImpl *node = holder->firstChild()) {
         node->ref();
         holder->removeChild(node, exceptionCode);
@@ -4183,11 +4163,67 @@ void ReplacementFragment::computeStylesAndRemoveUnrendered()
         ASSERT(exceptionCode == 0);
         node->deref();
     }
+}
+
+void ReplacementFragment::computeStylesUsingTestRendering(NodeImpl *holder)
+{
+    if (!holder)
+        return;
+
+    m_document->updateLayout();
+
+    for (NodeImpl *node = holder->firstChild(); node; node = node->traverseNextNode(holder)) {
+        CSSComputedStyleDeclarationImpl *computedStyle = Position(node, 0).computedStyle();
+        computedStyle->ref();
+        CSSMutableStyleDeclarationImpl *style = computedStyle->copyInheritableProperties();
+        style->ref();
+        node->ref();
+        removeBlockquoteColorsIfNeeded(node, style);
+        m_styles[node] = style;
+        computedStyle->deref();
+    }
+}
+
+void ReplacementFragment::removeUnrenderedNodesUsingTestRendering(NodeImpl *holder)
+{
+    if (!holder)
+        return;
+
+    QPtrList<NodeImpl> unrendered;
+
+    for (NodeImpl *node = holder->firstChild(); node; node = node->traverseNextNode(holder)) {
+        if (!isNodeRendered(node))
+            unrendered.append(node);
+    }
 
     for (QPtrListIterator<NodeImpl> it(unrendered); it.current(); ++it)
         removeNode(it.current());
+}
 
-    holder->deref();
+int ReplacementFragment::countRenderedBlocks(NodeImpl *holder)
+{
+    if (!holder)
+        return 0;
+    
+    int count = 0;
+    NodeImpl *prev = 0;
+    for (NodeImpl *node = holder->firstChild(); node; node = node->traverseNextNode(holder)) {
+        if (node->isBlockFlow()) {
+            if (!prev) {
+                count++;
+                prev = node;
+            }
+        }
+        else {
+            NodeImpl *block = node->enclosingBlockFlowElement();
+            if (block != prev) {
+                count++;
+                prev = block;
+            }
+        }
+    }
+    
+    return count;
 }
 
 void ReplacementFragment::removeStyleNodes()
@@ -4217,8 +4253,22 @@ void ReplacementFragment::removeStyleNodes()
         else if (node->isHTMLElement()) {
             HTMLElementImpl *elem = static_cast<HTMLElementImpl *>(node);
             CSSMutableStyleDeclarationImpl *inlineStyleDecl = elem->inlineStyleDecl();
-            if (inlineStyleDecl)
-                inlineStyleDecl->clear();
+            if (inlineStyleDecl) {
+                if (elem->id() == ID_P) {
+                    // For <p> elements, we want to retain any margin-cancelling properties
+                    // that have been added by our old editing code or by current Mail.app code.
+                    DOMString marginTop = inlineStyleDecl->getPropertyValue(CSS_PROP_MARGIN_TOP);                        
+                    DOMString marginBottom = inlineStyleDecl->getPropertyValue(CSS_PROP_MARGIN_BOTTOM);
+                    inlineStyleDecl->clear();
+                    if (!marginTop.isEmpty())
+                        inlineStyleDecl->setProperty(CSS_PROP_MARGIN_TOP, marginTop);
+                    if (!marginBottom.isEmpty())
+                        inlineStyleDecl->setProperty(CSS_PROP_MARGIN_BOTTOM, marginBottom);
+                }
+                else {
+                    inlineStyleDecl->clear();
+                }
+            }
         }
         node = next;
     }
@@ -4295,7 +4345,11 @@ void ReplaceSelectionCommand::doApply()
     }
     
     // decide whether to later append nodes to the end
-    bool moveNodesAfterEnd = !m_fragment.hasInterchangeNewline() && (startBlock != endBlock || m_fragment.hasMoreThanOneBlock());
+    NodeImpl *beyondEndNode = 0;
+    if (!isEndOfParagraph(visibleEnd)) {
+        beyondEndNode = selection.end().downstream(StayInBlock).node();
+    }
+    bool moveNodesAfterEnd = beyondEndNode && !m_fragment.hasInterchangeNewline() && (startBlock != endBlock || m_fragment.hasMoreThanOneBlock());
     
     Position startPos = Position(selection.start().node()->enclosingBlockFlowElement(), 0);
     Position endPos; 
@@ -4341,7 +4395,12 @@ void ReplaceSelectionCommand::doApply()
     // now that we are about to add content, check whether a placeholder element can be removed
     // if so, do it and update startPos and endPos
     NodeImpl *block = startPos.node()->enclosingBlockFlowElement();
-    NodeImpl *placeholderBlock = findBlockPlaceholder(block);
+    NodeImpl *linePlaceholder = findBlockPlaceholder(block);
+    if (!linePlaceholder) {
+        Position downstream = startPos.downstream(StayInBlock);
+        if (downstream.node()->id() == ID_BR && downstream.offset() == 0)
+            linePlaceholder = downstream.node();
+    }
     
     // check whether to "smart replace" needs to add leading and/or trailing space
     bool addLeadingSpace = false;
@@ -4482,19 +4541,16 @@ void ReplaceSelectionCommand::doApply()
 
         if (moveNodesAfterEnd) {
             QPtrList<NodeImpl> blocks;
-            NodeImpl *node = Position(m_lastNodeInserted, m_lastNodeInserted->caretMaxOffset()).downstream().node();
-            if (node != m_lastNodeInserted) {
-                NodeImpl *refNode = m_lastNodeInserted;
-                while (node) {
-                    NodeImpl *next = node->nextSibling();
-                    blocks.append(node->enclosingBlockFlowElement());
-                    removeNode(node);
-                    // Call the base class insert after code here.
-                    // No need to update inserted node variables.
-                    insertNodeAfter(node, refNode);
-                    refNode = node;
-                    node = next;
-                }
+            NodeImpl *node = beyondEndNode;
+            NodeImpl *refNode = m_lastNodeInserted;
+            while (node) {
+                NodeImpl *next = node->nextSibling();
+                blocks.append(node->enclosingBlockFlowElement());
+                removeNode(node);
+                // No need to update inserted node variables.
+                insertNodeAfter(node, refNode);
+                refNode = node;
+                node = next;
             }
             document()->updateLayout();
             for (QPtrListIterator<NodeImpl> it(blocks); it.current(); ++it) {
@@ -4518,10 +4574,20 @@ void ReplaceSelectionCommand::doApply()
     }
     
     // step 5 : mop up
-    if (placeholderBlock) {
+    if (linePlaceholder) {
         document()->updateLayout();
-        if (placeholderBlock->inDocument() && (!placeholderBlock->renderer() || placeholderBlock->renderer()->height() == 0))
-            removeNode(placeholderBlock);
+        if (linePlaceholder->inDocument()) {
+            if (!linePlaceholder->renderer() || linePlaceholder->renderer()->height() == 0) {
+                removeNode(linePlaceholder);
+            }
+            else if (!mergeStart && !m_fragment.hasInterchangeNewline()) {
+                NodeImpl *block = linePlaceholder->enclosingBlockFlowElement();
+                removeNode(linePlaceholder);
+                document()->updateLayout();
+                if (!block->renderer() || block->renderer()->height() == 0)
+                    removeNode(block);
+            }
+        }
     }
 }
 
index babcd1c82333287124c89e808afc66c6bbed0d8a..16789622796c0b7efe79fb1111e2f9d4b9876a7a 100644 (file)
@@ -690,7 +690,11 @@ private:
     static bool isInterchangeNewlineNode(const DOM::NodeImpl *);
     static bool isInterchangeConvertedSpaceSpan(const DOM::NodeImpl *);
 
-    void computeStylesAndRemoveUnrendered();
+    DOM::NodeImpl *insertFragmentForTestRendering();
+    void restoreTestRenderingNodesToFragment(DOM::NodeImpl *);
+    void computeStylesUsingTestRendering(DOM::NodeImpl *);
+    void removeUnrenderedNodesUsingTestRendering(DOM::NodeImpl *);
+    int countRenderedBlocks(DOM::NodeImpl *holder);
     void removeStyleNodes();
     void removeBlockquoteColorsIfNeeded(DOM::NodeImpl *, DOM::CSSMutableStyleDeclarationImpl *);