Changeset 65019 in webkit

Timestamp:

Aug 9, 2010 5:24:18 PM (14 years ago)

Author:

rniwa@webkit.org

Message:

2010-08-09 Ryosuke Niwa <​rniwa@webkit.org>

Reviewed by Justin Garcia.

fontColorChangesComputedStyle, fontSizeChangesComputedStyle, and fontFaceChangesComputedStyle should be removed
​https://bugs.webkit.org/show_bug.cgi?id=43465

Removed fontColorChangesComputedStyle, fontSizeChangesComputedStyle, and fontFaceChangesComputedStyle
because StyleChange's applyFontColor, applyFontFace, and applyFontSize all return false
if there was no difference between the new font color, font size, font face and that of the computed style.

Also added a work-around for the bug 28282 in getPropertiesNotInComputedStyle with a test so that
the above change will not add a redundant font or span element.

Test: editing/style/fore-color-by-name.html

• editing/ApplyStyleCommand.cpp: (WebCore::getRGBAFontColor): Added. (WebCore::StyleChange::extractTextStyles): Calls getRGBAFontColor. (WebCore::getPropertiesNotInComputedStyle): Removes color property manually by checking the RGBA values. (WebCore::ApplyStyleCommand::addInlineStyleIfNeeded): See above.

2010-08-09 Ryosuke Niwa <​rniwa@webkit.org>

Reviewed by Justin Garcia.

fontColorChangesComputedStyle, fontSizeChangesComputedStyle, and fontFaceChangesComputedStyle should be removed
​https://bugs.webkit.org/show_bug.cgi?id=43465

Added a test for applying execCommand('foreColor') by color name. Because of the bug 28282, we have to manually
process color property in getPropertiesNotInComputedStyle to avoid adding a redundant font or span element.

• editing/style/fore-color-by-name-expected.txt: Added.
• editing/style/fore-color-by-name.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/editing/style/fore-color-by-name-expected.txt (added)
• LayoutTests/editing/style/fore-color-by-name.html (added)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/editing/ApplyStyleCommand.cpp (modified) (5 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2010-08-09  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Justin Garcia.
+
+        fontColorChangesComputedStyle, fontSizeChangesComputedStyle, and fontFaceChangesComputedStyle should be removed
+        https://bugs.webkit.org/show_bug.cgi?id=43465
+
+        Added a test for applying execCommand('foreColor') by color name. Because of the bug 28282, we have to manually
+        process color property in getPropertiesNotInComputedStyle to avoid adding a redundant font or span element.
+
+        * editing/style/fore-color-by-name-expected.txt: Added.
+        * editing/style/fore-color-by-name.html: Added.
+
 2010-08-09  Chris Fleizach  <cfleizach@apple.com>
 

trunk/WebCore/ChangeLog

@@ +1 @@
+2010-08-09  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Justin Garcia.
+
+        fontColorChangesComputedStyle, fontSizeChangesComputedStyle, and fontFaceChangesComputedStyle should be removed
+        https://bugs.webkit.org/show_bug.cgi?id=43465
+
+        Removed fontColorChangesComputedStyle, fontSizeChangesComputedStyle, and fontFaceChangesComputedStyle
+        because StyleChange's applyFontColor, applyFontFace, and applyFontSize all return false
+        if there was no difference between the new font color, font size, font face and that of the computed style.
+
+        Also added a work-around for the bug 28282 in getPropertiesNotInComputedStyle with a test so that
+        the above change will not add a redundant font or span element.
+
+        Test: editing/style/fore-color-by-name.html
+
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::getRGBAFontColor): Added.
+        (WebCore::StyleChange::extractTextStyles): Calls getRGBAFontColor.
+        (WebCore::getPropertiesNotInComputedStyle): Removes color property manually by checking the RGBA values.
+        (WebCore::ApplyStyleCommand::addInlineStyleIfNeeded): See above.
+
 2010-08-09  Kenneth Russell  <kbr@google.com>
 

trunk/WebCore/editing/ApplyStyleCommand.cpp

@@ -52 +52 @@
 using namespace HTMLNames;
 
+static RGBA32 getRGBAFontColor(CSSStyleDeclaration* style)
+{
+    RefPtr<CSSValue> colorValue = style->getPropertyCSSValue(CSSPropertyColor);
+    if (!colorValue)
+        return Color::transparent;
+
+    ASSERT(colorValue->isPrimitiveValue());
+
+    CSSPrimitiveValue* primitiveColor = static_cast<CSSPrimitiveValue*>(colorValue.get());
+    RGBA32 rgba = 0;
+    if (primitiveColor->primitiveType() != CSSPrimitiveValue::CSS_RGBCOLOR) {
+        CSSParser::parseColor(rgba, colorValue->cssText());
+        // Need to take care of named color such as green and black
+        // This code should be removed after https://bugs.webkit.org/show_bug.cgi?id=28282 is fixed.
+    } else
+        rgba = primitiveColor->getRGBA32Value();
+
+    return rgba;
+}
+
 class StyleChange {
 public:
@@ -210 +230 @@
     }
 
-    if (RefPtr<CSSValue> colorValue = style->getPropertyCSSValue(CSSPropertyColor)) {
-        ASSERT(colorValue->isPrimitiveValue());
-        CSSPrimitiveValue* primitiveColor = static_cast<CSSPrimitiveValue*>(colorValue.get());
-        RGBA32 rgba = 0;
-        if (primitiveColor->primitiveType() != CSSPrimitiveValue::CSS_RGBCOLOR) {
-            CSSParser::parseColor(rgba, colorValue->cssText());
-            // Need to take care of named color such as green and black
-            // This code should be removed after https://bugs.webkit.org/show_bug.cgi?id=28282 is fixed.
-        } else
-            rgba = primitiveColor->getRGBA32Value();
-        m_applyFontColor = Color(rgba).name();
+    if (style->getPropertyCSSValue(CSSPropertyColor)) {
+        m_applyFontColor = Color(getRGBAFontColor(style)).name();
         style->removeProperty(CSSPropertyColor);
     }
@@ -382 +393 @@
     if (fontWeightIsBold(result.get()) == fontWeightIsBold(computedStyle))
         result->removeProperty(CSSPropertyFontWeight);
+
+    if (getRGBAFontColor(result.get()) == getRGBAFontColor(computedStyle))
+        result->removeProperty(CSSPropertyColor);
 
     return result;
@@ -1730 +1744 @@
 }
 
-static bool fontColorChangesComputedStyle(const Color& computedStyleColor, StyleChange styleChange)
-{
-    if (styleChange.applyFontColor()) {
-        if (Color(styleChange.fontColor()) != computedStyleColor)
-            return true;
-    }
-    return false;
-}
-
-static bool fontSizeChangesComputedStyle(RenderStyle* computedStyle, StyleChange styleChange)
-{
-    if (styleChange.applyFontSize()) {
-        if (styleChange.fontSize().toInt() != computedStyle->fontSize())
-            return true;
-    }
-    return false;
-}
-
-static bool fontFaceChangesComputedStyle(RenderStyle* computedStyle, StyleChange styleChange)
-{
-    if (styleChange.applyFontFace()) {
-        if (computedStyle->fontDescription().family().family().string() != styleChange.fontFace())
-            return true;
-    }
-    return false;
-}
-
 void ApplyStyleCommand::addInlineStyleIfNeeded(CSSMutableStyleDeclaration *style, Node *startNode, Node *endNode)
 {
@@ -1764 +1751 @@
     StyleChange styleChange(style, Position(startNode, 0));
 
-    //
     // Font tags need to go outside of CSS so that CSS font sizes override leagcy font sizes.
-    //
     if (styleChange.applyFontColor() || styleChange.applyFontFace() || styleChange.applyFontSize()) {
         RefPtr<Element> fontElement = createFontElement(document());
-        RenderStyle* computedStyle = startNode->computedStyle();
-
-        // We only want to insert a font element if it will end up changing the style of the
-        // text somehow. Otherwise it will be a garbage node that will create problems for us
-        // most notably when we apply a blockquote style for a message reply.
-        if (fontColorChangesComputedStyle(computedStyle->color(), styleChange)
-                || fontFaceChangesComputedStyle(computedStyle, styleChange)
-                || fontSizeChangesComputedStyle(computedStyle, styleChange)) {
-            if (styleChange.applyFontColor())
-                fontElement->setAttribute(colorAttr, styleChange.fontColor());
-            if (styleChange.applyFontFace())
-                fontElement->setAttribute(faceAttr, styleChange.fontFace());
-            if (styleChange.applyFontSize())
-                fontElement->setAttribute(sizeAttr, styleChange.fontSize());
-            surroundNodeRangeWithElement(startNode, endNode, fontElement.get());
-        }
+
+        if (styleChange.applyFontColor())
+            fontElement->setAttribute(colorAttr, styleChange.fontColor());
+        if (styleChange.applyFontFace())
+            fontElement->setAttribute(faceAttr, styleChange.fontFace());
+        if (styleChange.applyFontSize())
+            fontElement->setAttribute(sizeAttr, styleChange.fontSize());
+
+        surroundNodeRangeWithElement(startNode, endNode, fontElement.get());
     }