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 d37d404..8845796 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 d37d404..bd693cd 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 10fc386..02081f1 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 19a4dd3..59b68f5 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 88d89a7..625c79a 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 a339bbb..4d9014c 100644 (file)
@@ -55,6 +55,16 @@ namespace DOM {
 namespace khtml {
 
 //------------------------------------------------------------------------------------------
+// StyleChange
+
+struct StyleChange {
+    StyleChange() : applyBold(false), applyItalic(false) {}
+    DOM::DOMString cssStyle;
+    bool applyBold;
+    bool applyItalic;
+};
+
+//------------------------------------------------------------------------------------------
 // EditCommandImpl
 
 class EditCommandImpl : public SharedCommandImpl
@@ -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 32b313c..29b7cc2 100644 (file)
@@ -2406,10 +2406,10 @@ void KHTMLPart::notifySelectionChanged(bool closeTyping)
 {
     selectionLayoutChanged();
 
-    if (closeTyping) {
+    if (closeTyping)
         TypingCommand::closeTyping(lastEditCommand());
-        clearTypingStyle();
-    }
+
+    clearTypingStyle();
     
     emitSelectionChanged();