Reviewed by John
authorkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 2 Mar 2005 20:58:48 +0000 (20:58 +0000)
committerkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 2 Mar 2005 20:58:48 +0000 (20:58 +0000)
        Fix for this bug:

        <rdar://problem/4020574> REGRESSION (Mail): copy/paste first part of reply-quoted text alters downstream style

        The problem was that the operation to move nodes following the newly-pasted nodes did not preserve the
        style of these moved nodes. I have generalized some of the functions that compute and preserve styles
        for nodes and then apply these styles after a DOM operation.

        * khtml/editing/htmlediting.cpp:
        (khtml::ReplacementFragment::~ReplacementFragment): Call new derefNodesAndStylesInMap() helper function
        in place of old code that had this deref'ing inline.
        (khtml::ReplacementFragment::computeStylesUsingTestRendering): Now calls new mapDesiredStyleForNode() helper
        function place of old code that had this style computation inline.
        (khtml::ReplacementFragment::removeStyleNodes): Updated comment for new helper name.
        (khtml::ReplaceSelectionCommand::doApply): Now calls new helpers in place of helpers whose names were changed,
        or in place of pre-refactored inline code.
        (khtml::ReplaceSelectionCommand::fixupNodeStyles): Renamed from applyStyleToInsertedNodes(). Now generalized
        to take the map of nodes to use for the fixup. This makes it possible to call this code with different maps,
        and that is needed to fix the bug.
        (khtml::mapDesiredStyleForNode): New helper function to compute the inheritable styles for a given node
        and map this style to the given node in the given map. This function now also includes the code that was
        in the removeBlockquoteColorsIfNeeded(). This latter helper has now been removed.
        (khtml::derefNodesAndStylesInMap): Simple helper to deref map members.
        * khtml/editing/htmlediting.h:
        (khtml::ReplacementFragment::desiredStyles): New helper to return map of nodes-to-desiredStyles.

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

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

index 6e119b8b2873fd1425dcab2ae8c6192486005f44..f31e51a3b665c0999eef1a159c56b33cf86096ee 100644 (file)
@@ -1,3 +1,33 @@
+2005-03-02  Ken Kocienda  <kocienda@apple.com>
+
+        Reviewed by John
+
+        Fix for this bug:
+        
+        <rdar://problem/4020574> REGRESSION (Mail): copy/paste first part of reply-quoted text alters downstream style
+        
+        The problem was that the operation to move nodes following the newly-pasted nodes did not preserve the
+        style of these moved nodes. I have generalized some of the functions that compute and preserve styles
+        for nodes and then apply these styles after a DOM operation.
+
+        * khtml/editing/htmlediting.cpp:
+        (khtml::ReplacementFragment::~ReplacementFragment): Call new derefNodesAndStylesInMap() helper function
+        in place of old code that had this deref'ing inline.
+        (khtml::ReplacementFragment::computeStylesUsingTestRendering): Now calls new mapDesiredStyleForNode() helper
+        function place of old code that had this style computation inline.
+        (khtml::ReplacementFragment::removeStyleNodes): Updated comment for new helper name.
+        (khtml::ReplaceSelectionCommand::doApply): Now calls new helpers in place of helpers whose names were changed,
+        or in place of pre-refactored inline code.
+        (khtml::ReplaceSelectionCommand::fixupNodeStyles): Renamed from applyStyleToInsertedNodes(). Now generalized
+        to take the map of nodes to use for the fixup. This makes it possible to call this code with different maps,
+        and that is needed to fix the bug.
+        (khtml::mapDesiredStyleForNode): New helper function to compute the inheritable styles for a given node
+        and map this style to the given node in the given map. This function now also includes the code that was
+        in the removeBlockquoteColorsIfNeeded(). This latter helper has now been removed.
+        (khtml::derefNodesAndStylesInMap): Simple helper to deref map members.
+        * khtml/editing/htmlediting.h:
+        (khtml::ReplacementFragment::desiredStyles): New helper to return map of nodes-to-desiredStyles.
+
 2005-03-01  Ken Kocienda  <kocienda@apple.com>
 
         Reviewed by Hyatt
index b0158863a0408727aac1ffbd13b55782cb9b4047..86bcc42d4b505937cbdf9eb6ed69aa3d69c4d15d 100644 (file)
@@ -4139,11 +4139,7 @@ ReplacementFragment::ReplacementFragment(DocumentImpl *document, DocumentFragmen
 
 ReplacementFragment::~ReplacementFragment()
 {
-    QMapIterator<NodeImpl *, CSSMutableStyleDeclarationImpl *> it;
-    for (it = m_styles.begin(); it != m_styles.end(); ++it) {
-        it.key()->deref();
-        it.data()->deref();
-    }
+    derefNodesAndStylesInMap(m_styles);
     if (m_document)
         m_document->deref();
     if (m_fragment)
@@ -4160,12 +4156,6 @@ NodeImpl *ReplacementFragment::lastChild() const
     return  m_fragment->lastChild(); 
 }
 
-CSSMutableStyleDeclarationImpl *ReplacementFragment::styleForNode(NodeImpl *node)
-{
-    QMapConstIterator<NodeImpl *,CSSMutableStyleDeclarationImpl *> it = m_styles.find(node);
-    return it != m_styles.end() ? *it : 0;
-}
-
 NodeImpl *ReplacementFragment::mergeStartNode() const
 {
     NodeImpl *node = m_fragment->firstChild();
@@ -4304,14 +4294,7 @@ void ReplacementFragment::computeStylesUsingTestRendering(NodeImpl *holder)
     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();
+        mapDesiredStyleForNode(node, m_styles);
     }
 }
 
@@ -4361,7 +4344,7 @@ void ReplacementFragment::removeStyleNodes()
 {
     // Since style information has been computed and cached away in
     // computeStylesForNodes(), these style nodes can be removed, since
-    // the correct styles will be added back in applyStyleToInsertedNodes().
+    // the correct styles will be added back in fixupNodeStyles().
     NodeImpl *node = m_fragment->firstChild();
     while (node) {
         NodeImpl *next = node->traverseNextNode();
@@ -4394,32 +4377,6 @@ void ReplacementFragment::removeStyleNodes()
     }
 }
 
-void ReplacementFragment::removeBlockquoteColorsIfNeeded(NodeImpl *node, CSSMutableStyleDeclarationImpl *style)
-{
-    if (!node || !style)
-        return;
-
-    // In either of the color-matching tests below, set the color to a pseudo-color that will
-    // make the content take on the color of the nearest-enclosing blockquote (if any) after
-    // being pasted in.
-        
-    if (NodeImpl *blockquote = nearestMailBlockquote(node)) {
-        CSSComputedStyleDeclarationImpl *blockquoteStyle = Position(blockquote, 0).computedStyle();
-        if (blockquoteStyle->getPropertyValue(CSS_PROP_COLOR) == style->getPropertyValue(CSS_PROP_COLOR)) {
-            style->setProperty(CSS_PROP__KHTML_MATCH_NEAREST_MAIL_BLOCKQUOTE_COLOR, matchNearestBlockquoteColorString());
-            return;
-        }
-    }
-    
-    NodeImpl *documentElement = node->getDocument() ? node->getDocument()->documentElement() : 0;
-    if (documentElement) {
-        CSSComputedStyleDeclarationImpl *documentStyle = Position(documentElement, 0).computedStyle();
-        if (documentStyle->getPropertyValue(CSS_PROP_COLOR) == style->getPropertyValue(CSS_PROP_COLOR)) {
-            style->setProperty(CSS_PROP__KHTML_MATCH_NEAREST_MAIL_BLOCKQUOTE_COLOR, matchNearestBlockquoteColorString());
-        }
-    }
-}
-
 ReplaceSelectionCommand::ReplaceSelectionCommand(DocumentImpl *document, DocumentFragmentImpl *fragment, bool selectReplacement, bool smartReplace, bool matchStyle) 
     : CompositeEditCommand(document), 
       m_fragment(document, fragment, matchStyle),
@@ -4660,7 +4617,7 @@ void ReplaceSelectionCommand::doApply()
             endPos = endingSelection().end().downstream();
         }
         if (!m_matchStyle) {
-            applyStyleToInsertedNodes();
+            fixupNodeStyles(m_fragment.desiredStyles());
         }
         completeHTMLReplacement(startPos, endPos);
     } else {
@@ -4678,12 +4635,15 @@ void ReplaceSelectionCommand::doApply()
         }
 
         if (moveNodesAfterEnd) {
+            document()->updateLayout();
+            QMap<NodeImpl *, CSSMutableStyleDeclarationImpl *> styles;
             QPtrList<NodeImpl> blocks;
             NodeImpl *node = beyondEndNode;
             NodeImpl *refNode = m_lastNodeInserted;
             while (node) {
                 NodeImpl *next = node->nextSibling();
                 blocks.append(node->enclosingBlockFlowElement());
+                mapDesiredStyleForNode(node, styles);
                 removeNode(node);
                 // No need to update inserted node variables.
                 insertNodeAfter(node, refNode);
@@ -4702,10 +4662,13 @@ void ReplaceSelectionCommand::doApply()
                     document()->updateLayout();
                 }
             }
+
+            fixupNodeStyles(styles);
+            derefNodesAndStylesInMap(styles);
         }
     
         if (!m_matchStyle) {
-            applyStyleToInsertedNodes();
+            fixupNodeStyles(m_fragment.desiredStyles());
         }
         
         completeHTMLReplacement();
@@ -4830,72 +4793,109 @@ void ReplaceSelectionCommand::updateNodesInserted(NodeImpl *node)
         old->deref();
 }
 
-void ReplaceSelectionCommand::applyStyleToInsertedNodes()
+void ReplaceSelectionCommand::fixupNodeStyles(const QMap<NodeImpl *, CSSMutableStyleDeclarationImpl *> &map)
 {
-    // This function uses the cached style information computed in by the 
-    // ReplacementFragment class to apply the styles necessary to make
-    // the document look right.
+    // This function uses the mapped "desired style" to apply the additional style needed, if any,
+    // to make the node have the desired style.
 
     document()->updateLayout();
 
-    NodeImpl *node = m_firstNodeInserted;
-    NodeImpl *beyondEnd = m_lastNodeInserted->traverseNextNode();
-    while (node && node != beyondEnd) {
-        NodeImpl *next = node->traverseNextNode();
-        
-        // See if this node was present in the fragment, or was added by the paste algorithm. 
-        // If it was in the fragment, then we need to check, and perhaps fix up, its style.
-        CSSMutableStyleDeclarationImpl *desiredStyle = m_fragment.styleForNode(node);
-        if (desiredStyle) {
-            // The desiredStyle declaration tells what style this node wants to be.
-            // Compare that to the style that it is right now in the document.
-            Position pos(node, 0);
-            CSSComputedStyleDeclarationImpl *currentStyle = pos.computedStyle();
-            currentStyle->ref();
-
-            // Check for the special "match nearest blockquote color" property and resolve to the correct
-            // color if necessary.
-            DOMString matchColorCheck = desiredStyle->getPropertyValue(CSS_PROP__KHTML_MATCH_NEAREST_MAIL_BLOCKQUOTE_COLOR);
-            if (matchColorCheck == matchNearestBlockquoteColorString()) {
-                NodeImpl *blockquote = nearestMailBlockquote(node);
-                Position pos(blockquote ? blockquote : node->getDocument()->documentElement(), 0);
-                CSSComputedStyleDeclarationImpl *style = pos.computedStyle();
-                DOMString desiredColor = desiredStyle->getPropertyValue(CSS_PROP_COLOR);
-                DOMString nearestColor = style->getPropertyValue(CSS_PROP_COLOR);
-                if (desiredColor != nearestColor)
-                    desiredStyle->setProperty(CSS_PROP_COLOR, nearestColor);
-            }
-            desiredStyle->removeProperty(CSS_PROP__KHTML_MATCH_NEAREST_MAIL_BLOCKQUOTE_COLOR);
+    QMapConstIterator<NodeImpl *, CSSMutableStyleDeclarationImpl *> it;
+    for (it = map.begin(); it != map.end(); ++it) {
+        NodeImpl *node = it.key();
+        CSSMutableStyleDeclarationImpl *desiredStyle = it.data();
+        ASSERT(desiredStyle);
 
-            currentStyle->diff(desiredStyle);
-            
-            // Only add in block properties if the node is at the start of a 
-            // paragraph. This matches AppKit.
-            if (!isStartOfParagraph(VisiblePosition(pos, DOWNSTREAM)))
-                desiredStyle->removeBlockProperties();
-            
-            // If the desiredStyle is non-zero length, that means the current style differs
-            // from the desired by the styles remaining in the desiredStyle declaration.
-            if (desiredStyle->length() > 0) {
-                DOM::RangeImpl *rangeAroundNode = document()->createRange();
-                rangeAroundNode->ref();
-                int exceptionCode = 0;
-                rangeAroundNode->selectNode(node, exceptionCode);
-                ASSERT(exceptionCode == 0);
-                               // affinity is not really important since this is a temp selection
-                               // just for calling applyStyle
-                setEndingSelection(Selection(rangeAroundNode, SEL_DEFAULT_AFFINITY, SEL_DEFAULT_AFFINITY));
-                applyStyle(desiredStyle);
-                rangeAroundNode->deref();
-            }
+        if (!node->inDocument())
+            continue;
 
-            currentStyle->deref();
+        // The desiredStyle declaration tells what style this node wants to be.
+        // Compare that to the style that it is right now in the document.
+        Position pos(node, 0);
+        CSSComputedStyleDeclarationImpl *currentStyle = pos.computedStyle();
+        currentStyle->ref();
+
+        // Check for the special "match nearest blockquote color" property and resolve to the correct
+        // color if necessary.
+        DOMString matchColorCheck = desiredStyle->getPropertyValue(CSS_PROP__KHTML_MATCH_NEAREST_MAIL_BLOCKQUOTE_COLOR);
+        if (matchColorCheck == matchNearestBlockquoteColorString()) {
+            NodeImpl *blockquote = nearestMailBlockquote(node);
+            Position pos(blockquote ? blockquote : node->getDocument()->documentElement(), 0);
+            CSSComputedStyleDeclarationImpl *style = pos.computedStyle();
+            DOMString desiredColor = desiredStyle->getPropertyValue(CSS_PROP_COLOR);
+            DOMString nearestColor = style->getPropertyValue(CSS_PROP_COLOR);
+            if (desiredColor != nearestColor)
+                desiredStyle->setProperty(CSS_PROP_COLOR, nearestColor);
         }
+        desiredStyle->removeProperty(CSS_PROP__KHTML_MATCH_NEAREST_MAIL_BLOCKQUOTE_COLOR);
 
-        node = next;
+        currentStyle->diff(desiredStyle);
+        
+        // Only add in block properties if the node is at the start of a 
+        // paragraph. This matches AppKit.
+        if (!isStartOfParagraph(VisiblePosition(pos, DOWNSTREAM)))
+            desiredStyle->removeBlockProperties();
+        
+        // If the desiredStyle is non-zero length, that means the current style differs
+        // from the desired by the styles remaining in the desiredStyle declaration.
+        if (desiredStyle->length() > 0) {
+            DOM::RangeImpl *rangeAroundNode = document()->createRange();
+            rangeAroundNode->ref();
+            int exceptionCode = 0;
+            rangeAroundNode->selectNode(node, exceptionCode);
+            ASSERT(exceptionCode == 0);
+            // affinity is not really important since this is a temp selection
+            // just for calling applyStyle
+            setEndingSelection(Selection(rangeAroundNode, SEL_DEFAULT_AFFINITY, SEL_DEFAULT_AFFINITY));
+            applyStyle(desiredStyle);
+            rangeAroundNode->deref();
+        }
+
+        currentStyle->deref();
     }
 }
 
+void mapDesiredStyleForNode(DOM::NodeImpl *node, QMap<NodeImpl *, CSSMutableStyleDeclarationImpl *> &map)
+{
+    if (!node || !node->inDocument())
+        return;
+        
+    CSSComputedStyleDeclarationImpl *computedStyle = Position(node, 0).computedStyle();
+    computedStyle->ref();
+    CSSMutableStyleDeclarationImpl *style = computedStyle->copyInheritableProperties();
+    node->ref();
+    style->ref();
+    map[node] = style;
+    computedStyle->deref();
+
+    // In either of the color-matching tests below, set the color to a pseudo-color that will
+    // make the content take on the color of the nearest-enclosing blockquote (if any) after
+    // being pasted in.
+    if (NodeImpl *blockquote = nearestMailBlockquote(node)) {
+        CSSComputedStyleDeclarationImpl *blockquoteStyle = Position(blockquote, 0).computedStyle();
+        if (blockquoteStyle->getPropertyValue(CSS_PROP_COLOR) == style->getPropertyValue(CSS_PROP_COLOR)) {
+            style->setProperty(CSS_PROP__KHTML_MATCH_NEAREST_MAIL_BLOCKQUOTE_COLOR, matchNearestBlockquoteColorString());
+            return;
+        }
+    }
+    NodeImpl *documentElement = node->getDocument() ? node->getDocument()->documentElement() : 0;
+    if (documentElement) {
+        CSSComputedStyleDeclarationImpl *documentStyle = Position(documentElement, 0).computedStyle();
+        if (documentStyle->getPropertyValue(CSS_PROP_COLOR) == style->getPropertyValue(CSS_PROP_COLOR)) {
+            style->setProperty(CSS_PROP__KHTML_MATCH_NEAREST_MAIL_BLOCKQUOTE_COLOR, matchNearestBlockquoteColorString());
+        }
+    }
+}
+
+void derefNodesAndStylesInMap(const QMap<NodeImpl *, CSSMutableStyleDeclarationImpl *> &map)
+{
+    QMapConstIterator<NodeImpl *, CSSMutableStyleDeclarationImpl *> it;
+    for (it = map.begin(); it != map.end(); ++it) {
+        it.key()->deref();
+        it.data()->deref();
+    }    
+}
+
 //------------------------------------------------------------------------------------------
 // SetNodeAttributeCommand
 
index 2969fad00f1dcec0db6baff5d96ceecbed0ce056..db762de60dc1a771c8416f900d3cdb1b2cbb0f02 100644 (file)
@@ -684,7 +684,7 @@ public:
 
     DOM::NodeImpl *mergeStartNode() const;
 
-    DOM::CSSMutableStyleDeclarationImpl *styleForNode(DOM::NodeImpl *node);
+    const QMap<DOM::NodeImpl *, DOM::CSSMutableStyleDeclarationImpl *> &desiredStyles() { return m_styles; }
         
     void pruneEmptyNodes();
 
@@ -710,7 +710,6 @@ private:
     void removeUnrenderedNodesUsingTestRendering(DOM::NodeImpl *);
     int countRenderedBlocks(DOM::NodeImpl *holder);
     void removeStyleNodes();
-    void removeBlockquoteColorsIfNeeded(DOM::NodeImpl *, DOM::CSSMutableStyleDeclarationImpl *);
 
     // A couple simple DOM helpers
     DOM::NodeImpl *enclosingBlock(DOM::NodeImpl *) const;
@@ -745,7 +744,7 @@ private:
     void insertNodeBeforeAndUpdateNodesInserted(DOM::NodeImpl *insertChild, DOM::NodeImpl *refChild);
 
     void updateNodesInserted(DOM::NodeImpl *);
-    void applyStyleToInsertedNodes();
+    void fixupNodeStyles(const QMap<DOM::NodeImpl *, DOM::CSSMutableStyleDeclarationImpl *> &);
 
     ReplacementFragment m_fragment;
     DOM::NodeImpl *m_firstNodeInserted;
@@ -756,6 +755,9 @@ private:
     bool m_matchStyle;
 };
 
+void mapDesiredStyleForNode(DOM::NodeImpl *, QMap<DOM::NodeImpl *, DOM::CSSMutableStyleDeclarationImpl *> &);
+void derefNodesAndStylesInMap(const QMap<DOM::NodeImpl *, DOM::CSSMutableStyleDeclarationImpl *> &);
+
 //------------------------------------------------------------------------------------------
 // SetNodeAttributeCommand