Reviewed by Darin
authorkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 12 Aug 2004 17:34:56 +0000 (17:34 +0000)
committerkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 12 Aug 2004 17:34:56 +0000 (17:34 +0000)
        Fix for this bug:
        <rdar://problem/3751098> HTML email has one set of SPAN tags per character in the message

        Progress on this bug:
        <rdar://problem/3755562> Typing styles do not use same tag application conventions as font and color panel

        * khtml/editing/htmlediting_impl.cpp:
        (khtml::CompositeEditCommandImpl::applyTypingStyle): Name changed from createTypingStyleElement.
        Also, interface changed to take the node to which the typing style is to be applied.
        This makes it easier to apply what may be up to three levels of nested tags to get the
        desired style (<B>, <I>, and <SPAN STYLE="">).
        Also, Borrow some of the style change smarts from ApplyStyleCommandImpl to use bold and
        italic tags for applying styles when that is apprpriate. This creates on opportunity to
        factor the code to do this so that this function and the ApplyStyleCommandImpl class can
        share the implementation. I will follow up with a change to do that after landing this
        change. Some future code factoring could be done here to bring together some similar code
        into one place.
        (khtml::ApplyStyleCommandImpl::applyStyleIfNeeded): Add comment about code factoring work.
        (khtml::ApplyStyleCommandImpl::computeStyleChange): StyleChange struct no longer a member of the
        ApplyStyleCommandImpl class. CompositeEditCommandImpl needs it now in its applyTypingStyle()
        function.
        (khtml::InputNewlineCommandImpl::doApply): Pass along node to style to applyTypingStyle.
        (khtml::InputTextCommandImpl::prepareForTextInsertion): Ditto.
        * khtml/editing/htmlediting_impl.h:
        (khtml::StyleChange::StyleChange): Pull this struct out of ApplyStyleCommandImpl so
        CompositeEditCommandImpl can use it.
        * khtml/khtml_part.cpp:
        (KHTMLPart::notifySelectionChanged): Always clear typing style when the selection
        changes, not only when closing typing. This fixes 3751098.

        These three tests actually had results that treated the buggy behavior as correct!

        * layout-tests/editing/style/style-3681552-fix-001-expected.txt
        * layout-tests/editing/style/style-3681552-fix-002-expected.txt
        * layout-tests/editing/style/typing-style-002-expected.txt

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

LayoutTests/editing/style/style-3681552-fix-001-expected.txt
LayoutTests/editing/style/style-3681552-fix-002-expected.txt
LayoutTests/editing/style/typing-style-002-expected.txt
WebCore/ChangeLog-2005-08-23
WebCore/khtml/editing/htmlediting_impl.cpp
WebCore/khtml/editing/htmlediting_impl.h
WebCore/khtml/khtml_part.cpp

index d37d4047c9577245013d40930fc35886e186dada..8845796deddb1ceea252b4e093e6f35be31ffbe4 100644 (file)
@@ -7,22 +7,13 @@ layer at (0,0) size 800x600
         RenderInline {SPAN} at (0,0) size 157x28
           RenderText {TEXT} at (14,14) size 70x28
             text run at (14,14) width 70: "here is "
-          RenderInline {SPAN} at (0,0) size 44x28
-            RenderText {TEXT} at (84,14) size 11x28
-              text run at (84,14) width 11: "x"
-            RenderInline {SPAN} at (0,0) size 33x28
-              RenderText {TEXT} at (95,14) size 11x28
-                text run at (95,14) width 11: "x"
-              RenderInline {SPAN} at (0,0) size 22x28
-                RenderText {TEXT} at (106,14) size 11x28
-                  text run at (106,14) width 11: "x"
-                RenderInline {SPAN} at (0,0) size 11x28
-                  RenderText {TEXT} at (117,14) size 11x28
-                    text run at (117,14) width 11: "x"
+          RenderInline {I} at (0,0) size 44x28
+            RenderText {TEXT} at (84,14) size 44x28
+              text run at (84,14) width 44: "xxxx"
           RenderText {TEXT} at (128,14) size 43x28
             text run at (128,14) width 43: " text"
         RenderText {TEXT} at (0,0) size 0x0
 selection is CARET:
-start:      position 1 of child 1 {TEXT} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of root {DIV}
-upstream:   position 1 of child 1 {TEXT} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of root {DIV}
+start:      position 4 of child 1 {TEXT} of child 2 {I} of child 2 {SPAN} of root {DIV}
+upstream:   position 4 of child 1 {TEXT} of child 2 {I} of child 2 {SPAN} of root {DIV}
 downstream: position 0 of child 3 {TEXT} of child 2 {SPAN} of root {DIV}
index d37d4047c9577245013d40930fc35886e186dada..bd693cdea3db91a82c4a07d4586539cb09f78a8f 100644 (file)
@@ -4,25 +4,13 @@ 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 157x28
-          RenderText {TEXT} at (14,14) size 70x28
-            text run at (14,14) width 70: "here is "
-          RenderInline {SPAN} at (0,0) size 44x28
-            RenderText {TEXT} at (84,14) size 11x28
-              text run at (84,14) width 11: "x"
-            RenderInline {SPAN} at (0,0) size 33x28
-              RenderText {TEXT} at (95,14) size 11x28
-                text run at (95,14) width 11: "x"
-              RenderInline {SPAN} at (0,0) size 22x28
-                RenderText {TEXT} at (106,14) size 11x28
-                  text run at (106,14) width 11: "x"
-                RenderInline {SPAN} at (0,0) size 11x28
-                  RenderText {TEXT} at (117,14) size 11x28
-                    text run at (117,14) width 11: "x"
-          RenderText {TEXT} at (128,14) size 43x28
-            text run at (128,14) width 43: " text"
+        RenderInline {SPAN} at (0,0) size 161x28
+          RenderText {TEXT} at (14,14) size 118x28
+            text run at (14,14) width 118: "here is xxxx"
+          RenderText {TEXT} at (132,14) size 43x28
+            text run at (132,14) width 43: " text"
         RenderText {TEXT} at (0,0) size 0x0
 selection is CARET:
-start:      position 1 of child 1 {TEXT} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of root {DIV}
-upstream:   position 1 of child 1 {TEXT} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of root {DIV}
-downstream: position 0 of child 3 {TEXT} of child 2 {SPAN} of root {DIV}
+start:      position 12 of child 1 {TEXT} of child 2 {SPAN} of root {DIV}
+upstream:   position 12 of child 1 {TEXT} of child 2 {SPAN} of root {DIV}
+downstream: position 0 of child 2 {TEXT} of child 2 {SPAN} of root {DIV}
index 10fc38656c1574ba5f5d96c24421587ce545a90b..02081f130001adf3362f9d51b4eeba9f3dfb87bf 100644 (file)
@@ -8,24 +8,12 @@ layer at (0,0) size 800x600
           RenderText {TEXT} at (14,14) size 70x28
             text run at (14,14) width 70: "here is "
           RenderInline {I} at (0,0) size 53x28
-            RenderText {TEXT} at (84,14) size 9x28
-              text run at (84,14) width 9: "s"
-            RenderInline {SPAN} at (0,0) size 44x28
-              RenderText {TEXT} at (93,14) size 11x28
-                text run at (93,14) width 11: "x"
-              RenderInline {SPAN} at (0,0) size 33x28
-                RenderText {TEXT} at (104,14) size 11x28
-                  text run at (104,14) width 11: "x"
-                RenderInline {SPAN} at (0,0) size 22x28
-                  RenderText {TEXT} at (115,14) size 11x28
-                    text run at (115,14) width 11: "x"
-                  RenderInline {SPAN} at (0,0) size 11x28
-                    RenderText {TEXT} at (126,14) size 11x28
-                      text run at (126,14) width 11: "x"
+            RenderText {TEXT} at (84,14) size 53x28
+              text run at (84,14) width 53: "sxxxx"
           RenderText {TEXT} at (137,14) size 43x28
             text run at (137,14) width 43: " text"
         RenderText {TEXT} at (0,0) size 0x0
 selection is CARET:
-start:      position 1 of child 1 {TEXT} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of child 2 {I} of child 2 {SPAN} of root {DIV}
-upstream:   position 1 of child 1 {TEXT} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of child 2 {SPAN} of child 2 {I} of child 2 {SPAN} of root {DIV}
+start:      position 5 of child 1 {TEXT} of child 2 {I} of child 2 {SPAN} of root {DIV}
+upstream:   position 5 of child 1 {TEXT} of child 2 {I} of child 2 {SPAN} of root {DIV}
 downstream: position 0 of child 3 {TEXT} of child 2 {SPAN} of root {DIV}
index 19a4dd395dd6b33f2ba24598b0ce598ed8477b09..59b68f5beb86fd48cc3cd96c15204b4a5220866b 100644 (file)
@@ -1,3 +1,43 @@
+2004-08-12  Ken Kocienda  <kocienda@apple.com>
+
+        Reviewed by Darin
+
+        Fix for this bug:
+        <rdar://problem/3751098> HTML email has one set of SPAN tags per character in the message
+        
+        Progress on this bug:
+        <rdar://problem/3755562> Typing styles do not use same tag application conventions as font and color panel
+
+        * khtml/editing/htmlediting_impl.cpp:
+        (khtml::CompositeEditCommandImpl::applyTypingStyle): Name changed from createTypingStyleElement.
+        Also, interface changed to take the node to which the typing style is to be applied. 
+        This makes it easier to apply what may be up to three levels of nested tags to get the 
+        desired style (<B>, <I>, and <SPAN STYLE="">).
+        Also, Borrow some of the style change smarts from ApplyStyleCommandImpl to use bold and
+        italic tags for applying styles when that is apprpriate. This creates on opportunity to
+        factor the code to do this so that this function and the ApplyStyleCommandImpl class can
+        share the implementation. I will follow up with a change to do that after landing this
+        change. Some future code factoring could be done here to bring together some similar code
+        into one place.
+        (khtml::ApplyStyleCommandImpl::applyStyleIfNeeded): Add comment about code factoring work.
+        (khtml::ApplyStyleCommandImpl::computeStyleChange): StyleChange struct no longer a member of the
+        ApplyStyleCommandImpl class. CompositeEditCommandImpl needs it now in its applyTypingStyle()
+        function.
+        (khtml::InputNewlineCommandImpl::doApply): Pass along node to style to applyTypingStyle.
+        (khtml::InputTextCommandImpl::prepareForTextInsertion): Ditto.
+        * khtml/editing/htmlediting_impl.h:
+        (khtml::StyleChange::StyleChange): Pull this struct out of ApplyStyleCommandImpl so 
+        CompositeEditCommandImpl can use it.
+        * khtml/khtml_part.cpp:
+        (KHTMLPart::notifySelectionChanged): Always clear typing style when the selection
+        changes, not only when closing typing. This fixes 3751098.
+        
+        These three tests actually had results that treated the buggy behavior as correct!
+        
+        * layout-tests/editing/style/style-3681552-fix-001-expected.txt
+        * layout-tests/editing/style/style-3681552-fix-002-expected.txt
+        * layout-tests/editing/style/typing-style-002-expected.txt
+
 2004-08-12  Darin Adler  <darin@apple.com>
 
         Reviewed by Ken.
index 88d89a7f2dc3c9716155f3f4c237b93efb1baa0e..625c79a1908e0706ec2aa76d98af45ca91a8c913 100644 (file)
@@ -540,19 +540,72 @@ void CompositeEditCommandImpl::setNodeAttribute(ElementImpl *element, int attrib
     applyCommandToComposite(cmd);
 }
 
-ElementImpl *CompositeEditCommandImpl::createTypingStyleElement() const
+ElementImpl *CompositeEditCommandImpl::applyTypingStyle(NodeImpl *child) const
 {
-    int exceptionCode = 0;
-    ElementImpl *styleElement = document()->createHTMLElement("SPAN", exceptionCode);
-    ASSERT(exceptionCode == 0);
+    // FIXME: This function should share code with ApplyStyleCommandImpl::applyStyleIfNeeded
+    // and ApplyStyleCommandImpl::computeStyleChange.
+    // Both function do similar work, and the common parts could be factored out.
+
+    CSSStyleDeclarationImpl *style = document()->part()->typingStyle();
+    StyleChange styleChange;
+
+    for (QPtrListIterator<CSSProperty> it(*(style->values())); it.current(); ++it) {
+        CSSProperty *property = it.current();
+        switch (property->id()) {
+            case CSS_PROP_FONT_WEIGHT:
+                if (strcasecmp(property->value()->cssText(), "bold") == 0)
+                    styleChange.applyBold = true;
+                else
+                    styleChange.cssStyle += property->cssText();
+                break;
+            case CSS_PROP_FONT_STYLE: {
+                    DOMString cssText(property->value()->cssText());
+                    if (strcasecmp(cssText, "italic") == 0 || strcasecmp(cssText, "oblique") == 0)
+                        styleChange.applyItalic = true;
+                    else
+                        styleChange.cssStyle += property->cssText();
+                }
+                break;
+            default:
+                styleChange.cssStyle += property->cssText();
+                break;
+        }
+    }
     
-    styleElement->setAttribute(ATTR_STYLE, document()->part()->typingStyle()->cssText().implementation(), exceptionCode);
-    ASSERT(exceptionCode == 0);
+    NodeImpl *childToAppend = child;
+    ElementImpl *element = 0;
+    int exceptionCode = 0;
 
-    styleElement->setAttribute(ATTR_CLASS, styleSpanClassString());
-    ASSERT(exceptionCode == 0);
+    if (styleChange.applyItalic) {
+        ElementImpl *italicElement = document()->createHTMLElement("I", exceptionCode);
+        ASSERT(exceptionCode == 0);
+        italicElement->appendChild(childToAppend, exceptionCode);
+        ASSERT(exceptionCode == 0);
+        element = italicElement;
+        childToAppend = italicElement;
+    }
+
+    if (styleChange.applyBold) {
+        ElementImpl *boldElement = document()->createHTMLElement("B", exceptionCode);
+        ASSERT(exceptionCode == 0);
+        boldElement->appendChild(childToAppend, exceptionCode);
+        ASSERT(exceptionCode == 0);
+        element = boldElement;
+        childToAppend = boldElement;
+    }
 
-    return styleElement;
+    if (styleChange.cssStyle.length() > 0) {
+        ElementImpl *styleElement = document()->createHTMLElement("SPAN", exceptionCode);
+        ASSERT(exceptionCode == 0);
+        styleElement->setAttribute(ATTR_STYLE, styleChange.cssStyle);
+        styleElement->setAttribute(ATTR_CLASS, styleSpanClassString());
+        styleElement->appendChild(childToAppend, exceptionCode);
+        ASSERT(exceptionCode == 0);
+        element = styleElement;
+        childToAppend = styleElement;
+    }
+
+    return element;
 }
 
 //==========================================================================================
@@ -824,6 +877,9 @@ void ApplyStyleCommandImpl::surroundNodeRangeWithElement(NodeImpl *startNode, No
 
 void ApplyStyleCommandImpl::applyStyleIfNeeded(NodeImpl *startNode, NodeImpl *endNode)
 {
+    // FIXME: This function should share code with CompositeEditCommandImpl::applyTypingStyle.
+    // Both function do similar work, and the common parts could be factored out.
+
     StyleChange styleChange = computeStyleChange(Position(startNode, 0), style());
     int exceptionCode = 0;
     
@@ -860,7 +916,7 @@ bool ApplyStyleCommandImpl::currentlyHasStyle(const Position &pos, const CSSProp
     return strcasecmp(value->cssText(), property->value()->cssText()) == 0;
 }
 
-ApplyStyleCommandImpl::StyleChange ApplyStyleCommandImpl::computeStyleChange(const Position &insertionPoint, CSSStyleDeclarationImpl *style)
+StyleChange ApplyStyleCommandImpl::computeStyleChange(const Position &insertionPoint, CSSStyleDeclarationImpl *style)
 {
     ASSERT(insertionPoint.notEmpty());
     ASSERT(style);
@@ -1562,12 +1618,8 @@ void InputNewlineCommandImpl::doApply()
     
     // Handle the case where there is a typing style.
     CSSStyleDeclarationImpl *typingStyle = document()->part()->typingStyle();
-    if (typingStyle && typingStyle->length() > 0) {
-        ElementImpl *styleElement = createTypingStyleElement();
-        styleElement->appendChild(breakNode, exceptionCode);
-        ASSERT(exceptionCode == 0);
-        nodeToInsert = styleElement;
-    }
+    if (typingStyle && typingStyle->length() > 0)
+        nodeToInsert = applyTypingStyle(breakNode);
     
     Position pos(selection.start().equivalentDownstreamPosition());
     bool atStart = pos.offset() <= pos.node()->caretMinOffset();
@@ -1672,13 +1724,8 @@ Position InputTextCommandImpl::prepareForTextInsertion(bool adjustDownstream)
 
         // Handle the case where there is a typing style.
         CSSStyleDeclarationImpl *typingStyle = document()->part()->typingStyle();
-        if (typingStyle && typingStyle->length() > 0) {
-            int exceptionCode = 0;
-            ElementImpl *styleElement = createTypingStyleElement();
-            styleElement->appendChild(textNode, exceptionCode);
-            ASSERT(exceptionCode == 0);
-            nodeToInsert = styleElement;
-        }
+        if (typingStyle && typingStyle->length() > 0)
+            nodeToInsert = applyTypingStyle(textNode);
         
         // Now insert the node in the right place
         if (pos.node()->isEditableBlock()) {
@@ -1714,15 +1761,9 @@ Position InputTextCommandImpl::prepareForTextInsertion(bool adjustDownstream)
                 setEndingSelection(Position(cmd.node(), 0));
             }
             
-            int exceptionCode = 0;
             TextImpl *editingTextNode = document()->createEditingTextNode("");
-
-            ElementImpl *styleElement = createTypingStyleElement();
-            styleElement->appendChild(editingTextNode, exceptionCode);
-            ASSERT(exceptionCode == 0);
-
             NodeImpl *node = endingSelection().start().equivalentUpstreamPosition().node();
-            insertNodeAfter(styleElement, node);
+            insertNodeAfter(applyTypingStyle(editingTextNode), node);
             pos = Position(editingTextNode, 0);
         }
     }
index a339bbbc1f0a78a5be0e5236a793b91bcc41417b..4d9014c65e2925e838bd628a76cf7864e220a76e 100644 (file)
@@ -54,6 +54,16 @@ namespace DOM {
 
 namespace khtml {
 
+//------------------------------------------------------------------------------------------
+// StyleChange
+
+struct StyleChange {
+    StyleChange() : applyBold(false), applyItalic(false) {}
+    DOM::DOMString cssStyle;
+    bool applyBold;
+    bool applyItalic;
+};
+
 //------------------------------------------------------------------------------------------
 // EditCommandImpl
 
@@ -141,7 +151,7 @@ protected:
     void setNodeAttribute(DOM::ElementImpl *, int attribute, const DOM::DOMString &);
     void splitTextNode(DOM::TextImpl *text, long offset);
 
-    DOM::ElementImpl *createTypingStyleElement() const;
+    DOM::ElementImpl *applyTypingStyle(DOM::NodeImpl *) const;
 
     QValueList<EditCommand> m_cmds;
 };
@@ -185,13 +195,6 @@ public:
 
     DOM::CSSStyleDeclarationImpl *style() const { return m_style; }
 
-    struct StyleChange {
-        StyleChange() : applyBold(false), applyItalic(false) {}
-        DOM::DOMString cssStyle;
-        bool applyBold:1;
-        bool applyItalic:1;
-    };
-
 private:
     // style-removal helpers
     bool isHTMLStyleNode(DOM::HTMLElementImpl *);
index 32b313c79e612d4e2bdf35c5fa2837f92d01e989..29b7cc268e0ff0bd8fe5e531759ef1bb76c65aab 100644 (file)
@@ -2406,10 +2406,10 @@ void KHTMLPart::notifySelectionChanged(bool closeTyping)
 {
     selectionLayoutChanged();
 
-    if (closeTyping) {
+    if (closeTyping)
         TypingCommand::closeTyping(lastEditCommand());
-        clearTypingStyle();
-    }
+
+    clearTypingStyle();
     
     emitSelectionChanged();