Reviewed by John
authorkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 3 Mar 2005 23:08:16 +0000 (23:08 +0000)
committerkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 3 Mar 2005 23:08:16 +0000 (23:08 +0000)
        Fix for this bug:

        <rdar://problem/4035198> Pasting text with different styles does not get reproducible results

        I had a good idea yesterday to improve the way we "fixup" styles after
        pasting, but i chose a poor data structure to do it, a map of
        nodes-to-styles. In the fixup step, I iterated over the map elements and
        did the fixup. However, since the order in which the items would come
        out of the map is indeterminate, we got unpredictable results.

        So, the concept was good, but the implementation was flawed. I have
        fixed this mapping to be a list instead, so the order that nodes are
        evaluated in the fixup step is document order. This works nicely.

        * khtml/editing/htmlediting.cpp:
        (khtml::ReplacementFragment::~ReplacementFragment): No longer need to explicity deref nodes and
        styles saved away for later fixup. This is now handled by the new NodeDesiredStyle class.
        (khtml::ReplacementFragment::computeStylesUsingTestRendering): Now calls computeAndStoreNodeDesiredStyle,
        function renamed from mapDesiredStyleForNode.
        Now accepts a QValueList<NodeDesiredStyle> in place of the old map.
        (khtml::NodeDesiredStyle::NodeDesiredStyle): New class that represents a node-to-style mapping.
        (khtml::NodeDesiredStyle::~NodeDesiredStyle): Ditto.
        (khtml::NodeDesiredStyle::operator=): Ditto.
        (khtml::ReplaceSelectionCommand::doApply): Now calls computeAndStoreNodeDesiredStyle,
        function renamed from mapDesiredStyleForNode.
        (khtml::ReplaceSelectionCommand::fixupNodeStyles): Now operates on a QValueList<NodeDesiredStyle> in
        place of the old map.
        (khtml::computeAndStoreNodeDesiredStyle): Renamed from mapDesiredStyleForNode.  Now operates on a
        QValueList<NodeDesiredStyle> in place of the old map.
        * khtml/editing/htmlediting.h:
        (khtml::NodeDesiredStyle): New class that represents a node-to-style mapping.
        (khtml::ReplacementFragment::desiredStyles): Now returns a QValueList<NodeDesiredStyle> in place of the old map.

        * layout-tests/editing/style/typing-style-003-expected.txt: Results changed in an acceptable way.

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

LayoutTests/editing/style/typing-style-003-expected.txt
WebCore/ChangeLog-2005-08-23
WebCore/khtml/editing/htmlediting.cpp
WebCore/khtml/editing/htmlediting.h

index bdab017119951837dd496098527f2cf2449130ba..86b33fe6ba8ced5b500325c29dbe8d1f688a79f7 100644 (file)
@@ -4,23 +4,25 @@ 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 784x56 [border: (2px solid #FF0000)]
-        RenderInline {SPAN} at (0,0) size 0x0
-        RenderText {TEXT} at (0,0) size 0x0
-        RenderText {TEXT} at (14,14) size 36x28
-          text run at (14,14) width 36: "xxx"
-        RenderInline {B} at (0,0) size 141x28
-          RenderText {TEXT} at (50,14) size 36x28
-            text run at (50,14) width 36: "xxx"
-          RenderInline {I} at (0,0) size 105x28
-            RenderText {TEXT} at (86,14) size 36x28
-              text run at (86,14) width 36: "xxx"
-            RenderInline {SPAN} at (0,0) size 69x28
-              RenderText {TEXT} at (122,14) size 33x28
-                text run at (122,14) width 33: "xxx"
-              RenderInline {SPAN} at (0,0) size 36x28
-                RenderText {TEXT} at (155,14) size 36x28
-                  text run at (155,14) width 36: "xxx"
+        RenderInline {SPAN} at (0,0) size 177x28
+          RenderText {TEXT} at (0,0) size 0x0
+          RenderInline {SPAN} at (0,0) size 0x0
+          RenderText {TEXT} at (0,0) size 0x0
+          RenderText {TEXT} at (14,14) size 36x28
+            text run at (14,14) width 36: "xxx"
+          RenderInline {B} at (0,0) size 141x28
+            RenderText {TEXT} at (50,14) size 36x28
+              text run at (50,14) width 36: "xxx"
+            RenderInline {I} at (0,0) size 105x28
+              RenderText {TEXT} at (86,14) size 36x28
+                text run at (86,14) width 36: "xxx"
+              RenderInline {SPAN} at (0,0) size 69x28
+                RenderText {TEXT} at (122,14) size 33x28
+                  text run at (122,14) width 33: "xxx"
+                RenderInline {SPAN} at (0,0) size 36x28
+                  RenderText {TEXT} at (155,14) size 36x28
+                    text run at (155,14) width 36: "xxx"
 selection is CARET:
-start:      position 3 of child 1 {TEXT} of child 2 {SPAN} of child 2 {SPAN} of child 2 {I} of child 5 {B} of root {DIV}
-upstream:   position 3 of child 1 {TEXT} of child 2 {SPAN} of child 2 {SPAN} of child 2 {I} of child 5 {B} of root {DIV}
-downstream: position 3 of child 1 {TEXT} of child 2 {SPAN} of child 2 {SPAN} of child 2 {I} of child 5 {B} of root {DIV}
+start:      position 3 of child 1 {TEXT} of child 2 {SPAN} of child 2 {SPAN} of child 2 {I} of child 5 {B} of child 1 {SPAN} of root {DIV}
+upstream:   position 3 of child 1 {TEXT} of child 2 {SPAN} of child 2 {SPAN} of child 2 {I} of child 5 {B} of child 1 {SPAN} of root {DIV}
+downstream: position 3 of child 1 {TEXT} of child 2 {SPAN} of child 2 {SPAN} of child 2 {I} of child 5 {B} of child 1 {SPAN} of root {DIV}
index bcb22b55e1aadf9a406ad7ded2838e90af08f141..543b759657bb6c21f132141add41a37500a22611 100644 (file)
@@ -1,3 +1,42 @@
+2005-03-03  Ken Kocienda  <kocienda@apple.com>
+
+        Reviewed by John
+        
+        Fix for this bug:
+        
+        <rdar://problem/4035198> Pasting text with different styles does not get reproducible results
+
+        I had a good idea yesterday to improve the way we "fixup" styles after
+        pasting, but i chose a poor data structure to do it, a map of
+        nodes-to-styles. In the fixup step, I iterated over the map elements and
+        did the fixup. However, since the order in which the items would come
+        out of the map is indeterminate, we got unpredictable results.
+
+        So, the concept was good, but the implementation was flawed. I have
+        fixed this mapping to be a list instead, so the order that nodes are
+        evaluated in the fixup step is document order. This works nicely.
+
+        * khtml/editing/htmlediting.cpp:
+        (khtml::ReplacementFragment::~ReplacementFragment): No longer need to explicity deref nodes and
+        styles saved away for later fixup. This is now handled by the new NodeDesiredStyle class.
+        (khtml::ReplacementFragment::computeStylesUsingTestRendering): Now calls computeAndStoreNodeDesiredStyle,
+        function renamed from mapDesiredStyleForNode.
+        Now accepts a QValueList<NodeDesiredStyle> in place of the old map.
+        (khtml::NodeDesiredStyle::NodeDesiredStyle): New class that represents a node-to-style mapping.
+        (khtml::NodeDesiredStyle::~NodeDesiredStyle): Ditto.
+        (khtml::NodeDesiredStyle::operator=): Ditto.
+        (khtml::ReplaceSelectionCommand::doApply): Now calls computeAndStoreNodeDesiredStyle,
+        function renamed from mapDesiredStyleForNode.
+        (khtml::ReplaceSelectionCommand::fixupNodeStyles): Now operates on a QValueList<NodeDesiredStyle> in 
+        place of the old map.
+        (khtml::computeAndStoreNodeDesiredStyle): Renamed from mapDesiredStyleForNode.  Now operates on a 
+        QValueList<NodeDesiredStyle> in place of the old map.
+        * khtml/editing/htmlediting.h:
+        (khtml::NodeDesiredStyle): New class that represents a node-to-style mapping.
+        (khtml::ReplacementFragment::desiredStyles): Now returns a QValueList<NodeDesiredStyle> in place of the old map.
+
+        * layout-tests/editing/style/typing-style-003-expected.txt: Results changed in an acceptable way.
+
 2005-03-02  Darin Adler  <darin@apple.com>
 
         Reviewed by Maciej.
index 4becb392ae6dc152cf4b91811fcad9f86c0bf883..ae48531f62691cc473eb410e6f997dc61cd8c6bc 100644 (file)
@@ -4147,7 +4147,6 @@ ReplacementFragment::ReplacementFragment(DocumentImpl *document, DocumentFragmen
 
 ReplacementFragment::~ReplacementFragment()
 {
-    derefNodesAndStylesInMap(m_styles);
     if (m_document)
         m_document->deref();
     if (m_fragment)
@@ -4301,9 +4300,8 @@ void ReplacementFragment::computeStylesUsingTestRendering(NodeImpl *holder)
 
     m_document->updateLayout();
 
-    for (NodeImpl *node = holder->firstChild(); node; node = node->traverseNextNode(holder)) {
-        mapDesiredStyleForNode(node, m_styles);
-    }
+    for (NodeImpl *node = holder->firstChild(); node; node = node->traverseNextNode(holder))
+        computeAndStoreNodeDesiredStyle(node, m_styles);
 }
 
 void ReplacementFragment::removeUnrenderedNodesUsingTestRendering(NodeImpl *holder)
@@ -4385,6 +4383,53 @@ void ReplacementFragment::removeStyleNodes()
     }
 }
 
+NodeDesiredStyle::NodeDesiredStyle(NodeImpl *node, CSSMutableStyleDeclarationImpl *style) 
+    : m_node(node), m_style(style)
+{
+    if (m_node)
+        m_node->ref();
+    if (m_style)
+        m_style->ref();
+}
+
+NodeDesiredStyle::NodeDesiredStyle(const NodeDesiredStyle &other)
+    : m_node(other.node()), m_style(other.style())
+{
+    if (m_node)
+        m_node->ref();
+    if (m_style)
+        m_style->ref();
+}
+
+NodeDesiredStyle::~NodeDesiredStyle()
+{
+    if (m_node)
+        m_node->deref();
+    if (m_style)
+        m_style->deref();
+}
+
+NodeDesiredStyle &NodeDesiredStyle::operator=(const NodeDesiredStyle &other)
+{
+    NodeImpl *oldNode = m_node;
+    CSSMutableStyleDeclarationImpl *oldStyle = m_style;
+
+    m_node = other.node();
+    m_style = other.style();
+    
+    if (m_node)
+        m_node->ref();
+    if (m_style)
+        m_style->ref();
+    
+    if (oldNode)
+        oldNode->deref();
+    if (oldStyle)
+        oldStyle->deref();
+        
+    return *this;
+}
+
 ReplaceSelectionCommand::ReplaceSelectionCommand(DocumentImpl *document, DocumentFragmentImpl *fragment, bool selectReplacement, bool smartReplace, bool matchStyle) 
     : CompositeEditCommand(document), 
       m_fragment(document, fragment, matchStyle),
@@ -4653,14 +4698,14 @@ void ReplaceSelectionCommand::doApply()
 
         if (moveNodesAfterEnd) {
             document()->updateLayout();
-            QMap<NodeImpl *, CSSMutableStyleDeclarationImpl *> styles;
+            QValueList<NodeDesiredStyle> styles;
             QPtrList<NodeImpl> blocks;
             NodeImpl *node = beyondEndNode;
             NodeImpl *refNode = m_lastNodeInserted;
             while (node) {
                 NodeImpl *next = node->nextSibling();
                 blocks.append(node->enclosingBlockFlowElement());
-                mapDesiredStyleForNode(node, styles);
+                computeAndStoreNodeDesiredStyle(node, styles);
                 removeNode(node);
                 // No need to update inserted node variables.
                 insertNodeAfter(node, refNode);
@@ -4681,7 +4726,6 @@ void ReplaceSelectionCommand::doApply()
             }
 
             fixupNodeStyles(styles);
-            derefNodesAndStylesInMap(styles);
         }
     }
     
@@ -4805,17 +4849,17 @@ void ReplaceSelectionCommand::updateNodesInserted(NodeImpl *node)
         old->deref();
 }
 
-void ReplaceSelectionCommand::fixupNodeStyles(const QMap<NodeImpl *, CSSMutableStyleDeclarationImpl *> &map)
+void ReplaceSelectionCommand::fixupNodeStyles(const QValueList<NodeDesiredStyle> &list)
 {
     // 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();
 
-    QMapConstIterator<NodeImpl *, CSSMutableStyleDeclarationImpl *> it;
-    for (it = map.begin(); it != map.end(); ++it) {
-        NodeImpl *node = it.key();
-        CSSMutableStyleDeclarationImpl *desiredStyle = it.data();
+    QValueListConstIterator<NodeDesiredStyle> it;
+    for (it = list.begin(); it != list.end(); ++it) {
+        NodeImpl *node = (*it).node();
+        CSSMutableStyleDeclarationImpl *desiredStyle = (*it).style();
         ASSERT(desiredStyle);
 
         if (!node->inDocument())
@@ -4867,7 +4911,7 @@ void ReplaceSelectionCommand::fixupNodeStyles(const QMap<NodeImpl *, CSSMutableS
     }
 }
 
-void mapDesiredStyleForNode(DOM::NodeImpl *node, QMap<NodeImpl *, CSSMutableStyleDeclarationImpl *> &map)
+void computeAndStoreNodeDesiredStyle(DOM::NodeImpl *node, QValueList<NodeDesiredStyle> &list)
 {
     if (!node || !node->inDocument())
         return;
@@ -4875,9 +4919,7 @@ void mapDesiredStyleForNode(DOM::NodeImpl *node, QMap<NodeImpl *, CSSMutableStyl
     CSSComputedStyleDeclarationImpl *computedStyle = Position(node, 0).computedStyle();
     computedStyle->ref();
     CSSMutableStyleDeclarationImpl *style = computedStyle->copyInheritableProperties();
-    node->ref();
-    style->ref();
-    map[node] = style;
+    list.append(NodeDesiredStyle(node, style));
     computedStyle->deref();
 
     // In either of the color-matching tests below, set the color to a pseudo-color that will
@@ -4899,15 +4941,6 @@ void mapDesiredStyleForNode(DOM::NodeImpl *node, QMap<NodeImpl *, CSSMutableStyl
     }
 }
 
-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 547cd94b80e218c7f2b127ca70ee77be11312d3a..abe457933acc804b232a9525f9b9d3630e8a93c0 100644 (file)
@@ -670,6 +670,25 @@ private:
 //------------------------------------------------------------------------------------------
 // ReplaceSelectionCommand
 
+// --- NodeDesiredStyle helper class
+
+class NodeDesiredStyle
+{
+public:
+    NodeDesiredStyle(DOM::NodeImpl *, DOM::CSSMutableStyleDeclarationImpl *);
+    NodeDesiredStyle(const NodeDesiredStyle &);
+    ~NodeDesiredStyle();
+    
+    DOM::NodeImpl *node() const { return m_node; }
+    DOM::CSSMutableStyleDeclarationImpl *style() const { return m_style; }
+
+    NodeDesiredStyle &operator=(const NodeDesiredStyle &);
+
+private:
+    DOM::NodeImpl *m_node;
+    DOM::CSSMutableStyleDeclarationImpl *m_style;
+};
+
 // --- ReplacementFragment helper class
 
 class ReplacementFragment
@@ -686,7 +705,7 @@ public:
 
     DOM::NodeImpl *mergeStartNode() const;
 
-    const QMap<DOM::NodeImpl *, DOM::CSSMutableStyleDeclarationImpl *> &desiredStyles() { return m_styles; }
+    const QValueList<NodeDesiredStyle> &desiredStyles() { return m_styles; }
         
     void pruneEmptyNodes();
 
@@ -722,7 +741,7 @@ private:
     EFragmentType m_type;
     DOM::DocumentImpl *m_document;
     DOM::DocumentFragmentImpl *m_fragment;
-    QMap<DOM::NodeImpl *, DOM::CSSMutableStyleDeclarationImpl *> m_styles;
+    QValueList<NodeDesiredStyle> m_styles;
     bool m_matchStyle;
     bool m_hasInterchangeNewline;
     bool m_hasMoreThanOneBlock;
@@ -745,7 +764,7 @@ private:
     void insertNodeBeforeAndUpdateNodesInserted(DOM::NodeImpl *insertChild, DOM::NodeImpl *refChild);
 
     void updateNodesInserted(DOM::NodeImpl *);
-    void fixupNodeStyles(const QMap<DOM::NodeImpl *, DOM::CSSMutableStyleDeclarationImpl *> &);
+    void fixupNodeStyles(const QValueList<NodeDesiredStyle> &);
 
     ReplacementFragment m_fragment;
     DOM::NodeImpl *m_firstNodeInserted;
@@ -757,8 +776,7 @@ private:
     bool m_matchStyle;
 };
 
-void mapDesiredStyleForNode(DOM::NodeImpl *, QMap<DOM::NodeImpl *, DOM::CSSMutableStyleDeclarationImpl *> &);
-void derefNodesAndStylesInMap(const QMap<DOM::NodeImpl *, DOM::CSSMutableStyleDeclarationImpl *> &);
+void computeAndStoreNodeDesiredStyle(DOM::NodeImpl *, QValueList<NodeDesiredStyle> &);
 
 //------------------------------------------------------------------------------------------
 // SetNodeAttributeCommand