2011-05-20 Ryosuke Niwa <rniwa@webkit.org>

        Reviewed by Enrica Casucci.

        REGRESSION(r84311): WebKit copies too much styles when copying
        https://bugs.webkit.org/show_bug.cgi?id=60914

        Added a test to ensure WebKit does not clone hierarchy to preserve background color.

        Also renamed editing/pasteboard/do-not-copy-body-color.html to do-no-clone-unnecessary-styles.html
        and updated the description to match new behavior. While this test ensures WebKit does not copy
        body's background color when it's not fully selected, this isn't a necessary requirement for us
        not to duplicate borders so new expected result is correct.

        * editing/pasteboard/copy-text-with-backgroundcolor-expected.txt: Some spans became style spans.
        * editing/pasteboard/do-no-clone-unnecessary-styles-2-expected.txt: Added.
        * editing/pasteboard/do-no-clone-unnecessary-styles-2.html: Added.
        * editing/pasteboard/do-no-clone-unnecessary-styles-expected.txt: Renamed from
        LayoutTests/editing/pasteboard/do-not-copy-body-color-expected.txt.
        * editing/pasteboard/do-no-clone-unnecessary-styles.html: Renamed from
        LayoutTests/editing/pasteboard/do-not-copy-body-color.html.
2011-05-20  Ryosuke Niwa  <rniwa@webkit.org>

        Reviewed by Enrica Casucci.

        REGRESSION(r84311): WebKit copies too much styles when copying
        https://bugs.webkit.org/show_bug.cgi?id=60914

        The bug was caused by WebKit's cloning node hierarchy up until the node that has background color.
        Fixed the bug by not cloning background color and adding the effective background color to the wrapping
        style span.

        Tests: editing/pasteboard/do-no-clone-unnecessary-styles-2.html
               editing/pasteboard/do-no-clone-unnecessary-styles.html

        * editing/EditingStyle.cpp:
        (WebCore::cssValueToRGBA): Extracted from getRGBAFontColor.
        (WebCore::getRGBAFontColor): Moved.
        (WebCore::rgbaBackgroundColorInEffect): Added.
        (WebCore::EditingStyle::init): Added support for InheritablePropertiesAndBackgroundColorInEffect.
        (WebCore::EditingStyle::prepareToApplyAt): Include the effective background color at the given position.
        Also remove the background color property when the effective background color is equal to the background
        color property (in terms of RGBA value) of the editing style.
        (WebCore::hasTransparentBackgroundColor): Moved from Editor class.
        (WebCore::backgroundColorInEffect): Extracted from Editor::selectionStartCSSPropertyValue.
        * editing/EditingStyle.h: Added prototypes for hasTransparentBackgroundColor and backgroundColorInEffect.
        * editing/Editor.cpp:
        (WebCore::Editor::selectionStartCSSPropertyValue): Calls backgroundColorInEffect.
        * editing/Editor.h: Removed hasTransparentBackgroundColor.
        * editing/markup.cpp:
        (WebCore::isElementPresentational): Reverted r85090 and r84311.
        (WebCore::createMarkup): Include the background color in effect when computing the editing style.


Canonical link: https://commits.webkit.org/76669@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@87082 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog

@@ -1,3 +1,25 @@
+2011-05-20  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Enrica Casucci.
+
+        REGRESSION(r84311): WebKit copies too much styles when copying
+        https://bugs.webkit.org/show_bug.cgi?id=60914
+
+        Added a test to ensure WebKit does not clone hierarchy to preserve background color.
+
+        Also renamed editing/pasteboard/do-not-copy-body-color.html to do-no-clone-unnecessary-styles.html
+        and updated the description to match new behavior. While this test ensures WebKit does not copy
+        body's background color when it's not fully selected, this isn't a necessary requirement for us
+        not to duplicate borders so new expected result is correct.
+
+        * editing/pasteboard/copy-text-with-backgroundcolor-expected.txt: Some spans became style spans.
+        * editing/pasteboard/do-no-clone-unnecessary-styles-2-expected.txt: Added.
+        * editing/pasteboard/do-no-clone-unnecessary-styles-2.html: Added.
+        * editing/pasteboard/do-no-clone-unnecessary-styles-expected.txt: Renamed from
+        LayoutTests/editing/pasteboard/do-not-copy-body-color-expected.txt.
+        * editing/pasteboard/do-no-clone-unnecessary-styles.html: Renamed from
+        LayoutTests/editing/pasteboard/do-not-copy-body-color.html.
+
 2011-05-23  Ryosuke Niwa  <rniwa@webkit.org>
 
         Reviewed by Tony Chang.

LayoutTests/editing/pasteboard/copy-text-with-backgroundcolor-expected.txt

@@ -57,6 +57,7 @@ Markup after:
 |   style="background-color: rgb(255, 0, 0);"
 |   "Red background"
 | <span>
+|   class="Apple-style-span"
 |   style="background-color: rgb(255, 0, 0); "
 |   "Red background"
 | <div>
@@ -65,7 +66,8 @@ Markup after:
 |     style="background-color: green;"
 |     "Green background"
 |   <span>
-|     style="background-color: green; "
+|     class="Apple-style-span"
+|     style="background-color: rgb(0, 128, 0); "
 |     "Green background"
 | "
 "

LayoutTests/editing/pasteboard/do-not-copy-unnecessary-styles-2-expected.txt

@@ -0,0 +1,8 @@
+This test verifies that WebKit does not erroneously clone nodes hierarchy when copying text with background color.
+"Hello" should be blue and has yellow background but you should not see red border.
+
+You should not see any borders:
+| <span>
+|   class="Apple-style-span"
+|   style="color: rgb(0, 0, 255); background-color: rgb(255, 255, 0); "
+|   "Hello<#selection-caret>"

LayoutTests/editing/pasteboard/do-not-copy-unnecessary-styles-2.html

@@ -0,0 +1,28 @@
+<!DOCTYPE html>
+<html> 
+<body>
+<p>This test verifies that WebKit does not erroneously clone nodes hierarchy when copying text with background color.
+"Hello" should be blue and has yellow background but you should not see red border.</p>
+<p>To manually this test, copy "Hello" and paste it on the black box below.</p>
+<div style="padding: 10px;"><span style="color: blue; background-color: yellow; padding: 5px;"><div><span style="border: 2px solid red; border-radius: 15px;">
+<span id="source">Hello</span>
+</span></div></span></div>
+<div id="destination" style="border: solid 1px black; padding: 20px;" contenteditable></div>
+<script src="../../resources/dump-as-markup.js"></script>
+<script>
+
+if (window.layoutTestController) {
+    window.getSelection().setBaseAndExtent(document.getElementById('source'), 0, document.getElementById('source'), 1);
+    document.execCommand("Copy");
+
+    window.getSelection().setPosition(document.getElementById('destination'), 0);
+    document.execCommand("Paste");
+
+    Markup.description(document.getElementsByTagName('p')[0].textContent);
+    Markup.dump('destination', 'You should not see any borders');
+} else
+    Markup.noAutoDump();
+
+</script>
+</body>
+</html>

LayoutTests/editing/pasteboard/do-not-copy-unnecessary-styles-expected.txt

@@ -0,0 +1,11 @@
+
+<div>
+This test verifies that WebKit does not erroneously clone nodes hierarchy when copying text with background color.
+To manually this test, select the text inside the div with the border, cut and paste it back. You should not see an additional red border.
+<div style="border: 2px solid red">
+<div id="test"><span class="Apple-style-span" style="background-color: rgb(187, 187, 187); ">Select this text</span><br></div>
+</div>
+</div>
+
+
+

LayoutTests/editing/pasteboard/do-not-copy-body-color.html → LayoutTests/editing/pasteboard/do-not-copy-unnecessary-styles.html

@@ -16,17 +16,16 @@
     document.body.style.backgroundColor = "white";
     document.execCommand("Paste");
     document.body.innerText = document.body.innerHTML;
-
 }
 </script>
 </head>
 <body onLoad="runTest();" contentEditable="true" style="background-color: #bbb;">
 <div>
-This test verifies that, when the body element has a background color, we don't copy the color if the selection is not the entire content nor any of the intermediate elements. To manually this test, select the text inside the div with the border, cut and paste it back. You should not see an additional red border.
+This test verifies that WebKit does not erroneously clone nodes hierarchy when copying text with background color.
+To manually this test, select the text inside the div with the border, cut and paste it back. You should not see an additional red border.
 <div style="border: 2px solid red">
     <div id="test">Select this text</div>
 </div>
 </div>
 </body>
 </html>
-

Source/WebCore/ChangeLog

@@ -1,3 +1,35 @@
+2011-05-20  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Enrica Casucci.
+
+        REGRESSION(r84311): WebKit copies too much styles when copying
+        https://bugs.webkit.org/show_bug.cgi?id=60914
+
+        The bug was caused by WebKit's cloning node hierarchy up until the node that has background color.
+        Fixed the bug by not cloning background color and adding the effective background color to the wrapping
+        style span.
+
+        Tests: editing/pasteboard/do-no-clone-unnecessary-styles-2.html
+               editing/pasteboard/do-no-clone-unnecessary-styles.html
+
+        * editing/EditingStyle.cpp:
+        (WebCore::cssValueToRGBA): Extracted from getRGBAFontColor.
+        (WebCore::getRGBAFontColor): Moved.
+        (WebCore::rgbaBackgroundColorInEffect): Added.
+        (WebCore::EditingStyle::init): Added support for InheritablePropertiesAndBackgroundColorInEffect.
+        (WebCore::EditingStyle::prepareToApplyAt): Include the effective background color at the given position.
+        Also remove the background color property when the effective background color is equal to the background
+        color property (in terms of RGBA value) of the editing style.
+        (WebCore::hasTransparentBackgroundColor): Moved from Editor class.
+        (WebCore::backgroundColorInEffect): Extracted from Editor::selectionStartCSSPropertyValue.
+        * editing/EditingStyle.h: Added prototypes for hasTransparentBackgroundColor and backgroundColorInEffect.
+        * editing/Editor.cpp:
+        (WebCore::Editor::selectionStartCSSPropertyValue): Calls backgroundColorInEffect.
+        * editing/Editor.h: Removed hasTransparentBackgroundColor.
+        * editing/markup.cpp:
+        (WebCore::isElementPresentational): Reverted r85090 and r84311.
+        (WebCore::createMarkup): Include the background color in effect when computing the editing style.
+
 2011-05-23  Roland Steiner  <rolandsteiner@chromium.org>
 
         Reviewed by Dimitri Glazkov.

Source/WebCore/editing/EditingStyle.cpp

@@ -90,7 +90,6 @@ static PassRefPtr<CSSMutableStyleDeclaration> editingStyleFromComputedStyle(Pass
 }
 
 static RefPtr<CSSMutableStyleDeclaration> getPropertiesNotIn(CSSStyleDeclaration* styleWithRedundantProperties, CSSStyleDeclaration* baseStyle);
-static RGBA32 getRGBAFontColor(CSSStyleDeclaration*);
 
 class HTMLElementEquivalent {
 public:
@@ -276,11 +275,11 @@ EditingStyle::EditingStyle(Node* node, PropertiesToInclude propertiesToInclude)
     init(node, propertiesToInclude);
 }
 
-EditingStyle::EditingStyle(const Position& position)
+EditingStyle::EditingStyle(const Position& position, PropertiesToInclude propertiesToInclude)
     : m_shouldUseFixedDefaultFontSize(false)
     , m_fontSizeDelta(NoFontDelta)
 {
-    init(position.deprecatedNode(), OnlyInheritableProperties);
+    init(position.deprecatedNode(), propertiesToInclude);
 }
 
 EditingStyle::EditingStyle(const CSSStyleDeclaration* style)
@@ -303,6 +302,30 @@ EditingStyle::~EditingStyle()
 {
 }
 
+static RGBA32 cssValueToRGBA(CSSValue* colorValue)
+{
+    if (!colorValue || !colorValue->isPrimitiveValue())
+        return Color::transparent;
+
+    CSSPrimitiveValue* primitiveColor = static_cast<CSSPrimitiveValue*>(colorValue);
+    if (primitiveColor->primitiveType() == CSSPrimitiveValue::CSS_RGBCOLOR)
+        return primitiveColor->getRGBA32Value();
+
+    RGBA32 rgba = 0;
+    CSSParser::parseColor(rgba, colorValue->cssText());
+    return rgba;
+}
+
+static inline RGBA32 getRGBAFontColor(CSSStyleDeclaration* style)
+{
+    return cssValueToRGBA(style->getPropertyCSSValue(CSSPropertyColor).get());
+}
+
+static inline RGBA32 rgbaBackgroundColorInEffect(Node* node)
+{
+    return cssValueToRGBA(backgroundColorInEffect(node).get());
+}
+
 void EditingStyle::init(Node* node, PropertiesToInclude propertiesToInclude)
 {
     if (isTabSpanTextNode(node))
@@ -313,6 +336,11 @@ void EditingStyle::init(Node* node, PropertiesToInclude propertiesToInclude)
     RefPtr<CSSComputedStyleDeclaration> computedStyleAtPosition = computedStyle(node);
     m_mutableStyle = propertiesToInclude == AllProperties && computedStyleAtPosition ? computedStyleAtPosition->copy() : editingStyleFromComputedStyle(computedStyleAtPosition);
 
+    if (propertiesToInclude == InheritablePropertiesAndBackgroundColorInEffect) {
+        if (RefPtr<CSSValue> value = backgroundColorInEffect(node))
+            m_mutableStyle->setProperty(CSSPropertyBackgroundColor, value->cssText());
+    }
+
     if (node && node->computedStyle()) {
         RenderStyle* renderStyle = node->computedStyle();
         removeTextFillAndStrokeColorsIfNeeded(renderStyle);
@@ -532,6 +560,7 @@ static const int textOnlyProperties[] = {
 
 TriState EditingStyle::triStateOfStyle(CSSStyleDeclaration* styleToCompare, ShouldIgnoreTextOnlyProperties shouldIgnoreTextOnlyProperties) const
 {
+    // FIXME: take care of background-color in effect
     RefPtr<CSSMutableStyleDeclaration> difference = getPropertiesNotIn(m_mutableStyle.get(), styleToCompare);
 
     if (shouldIgnoreTextOnlyProperties == IgnoreTextOnlyProperties)
@@ -696,7 +725,7 @@ void EditingStyle::prepareToApplyAt(const Position& position, ShouldPreserveWrit
     // ReplaceSelectionCommand::handleStyleSpans() requires that this function only removes the editing style.
     // If this function was modified in the future to delete all redundant properties, then add a boolean value to indicate
     // which one of editingStyleAtPosition or computedStyle is called.
-    RefPtr<EditingStyle> style = EditingStyle::create(position);
+    RefPtr<EditingStyle> style = EditingStyle::create(position, InheritablePropertiesAndBackgroundColorInEffect);
 
     RefPtr<CSSValue> unicodeBidi;
     RefPtr<CSSValue> direction;
@@ -709,13 +738,9 @@ void EditingStyle::prepareToApplyAt(const Position& position, ShouldPreserveWrit
     if (getRGBAFontColor(m_mutableStyle.get()) == getRGBAFontColor(style->m_mutableStyle.get()))
         m_mutableStyle->removeProperty(CSSPropertyColor);
 
-    // if alpha value is zero, we don't add the background color.
-    RefPtr<CSSValue> backgroundColor = m_mutableStyle->getPropertyCSSValue(CSSPropertyBackgroundColor);
-    if (backgroundColor && backgroundColor->isPrimitiveValue()
-        && !alphaChannel(static_cast<CSSPrimitiveValue*>(backgroundColor.get())->getRGBA32Value())) {
-        ExceptionCode ec;
-        m_mutableStyle->removeProperty(CSSPropertyBackgroundColor, ec);
-    }
+    if (hasTransparentBackgroundColor(m_mutableStyle.get())
+        || cssValueToRGBA(m_mutableStyle->getPropertyCSSValue(CSSPropertyBackgroundColor).get()) == rgbaBackgroundColorInEffect(position.containerNode()))
+        m_mutableStyle->removeProperty(CSSPropertyBackgroundColor);
 
     if (unicodeBidi && unicodeBidi->isPrimitiveValue()) {
         m_mutableStyle->setProperty(CSSPropertyUnicodeBidi, static_cast<CSSPrimitiveValue*>(unicodeBidi.get())->getIdent());
@@ -810,6 +835,7 @@ StyleChange::StyleChange(EditingStyle* style, const Position& position)
         return;
 
     RefPtr<CSSComputedStyleDeclaration> computedStyle = position.computedStyle();
+    // FIXME: take care of background-color in effect
     RefPtr<CSSMutableStyleDeclaration> mutableStyle = getPropertiesNotIn(style->style(), computedStyle.get());
 
     reconcileTextDecorationProperties(mutableStyle.get());
@@ -840,23 +866,6 @@ static void setTextDecorationProperty(CSSMutableStyleDeclaration* style, const C
     }
 }
 
-static RGBA32 getRGBAFontColor(CSSStyleDeclaration* style)
-{
-    RefPtr<CSSValue> colorValue = style->getPropertyCSSValue(CSSPropertyColor);
-    if (!colorValue || !colorValue->isPrimitiveValue())
-        return Color::transparent;
-
-    CSSPrimitiveValue* primitiveColor = static_cast<CSSPrimitiveValue*>(colorValue.get());
-    if (primitiveColor->primitiveType() == CSSPrimitiveValue::CSS_RGBCOLOR)
-        return primitiveColor->getRGBA32Value();
-
-    // 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.
-    RGBA32 rgba = 0;
-    CSSParser::parseColor(rgba, colorValue->cssText());
-    return rgba;
-}
-
 void StyleChange::extractTextStyles(Document* document, CSSMutableStyleDeclaration* style, bool shouldUseFixedFontDefaultSize)
 {
     ASSERT(style);
@@ -991,6 +1000,7 @@ RefPtr<CSSMutableStyleDeclaration> getPropertiesNotIn(CSSStyleDeclaration* style
     ASSERT(styleWithRedundantProperties);
     ASSERT(baseStyle);
     RefPtr<CSSMutableStyleDeclaration> result = styleWithRedundantProperties->copy();
+
     baseStyle->diff(result.get());
 
     RefPtr<CSSValue> baseTextDecorationsInEffect = baseStyle->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect);
@@ -1004,7 +1014,7 @@ RefPtr<CSSMutableStyleDeclaration> getPropertiesNotIn(CSSStyleDeclaration* style
         result->removeProperty(CSSPropertyColor);
 
     if (getTextAlignment(result.get()) == getTextAlignment(baseStyle))
-        result->removeProperty(CSSPropertyTextAlign);        
+        result->removeProperty(CSSPropertyTextAlign);
 
     return result;
 }
@@ -1045,4 +1055,30 @@ int legacyFontSizeFromCSSValue(Document* document, CSSPrimitiveValue* value, boo
     return 0;
 }
 
+bool hasTransparentBackgroundColor(CSSStyleDeclaration* style)
+{
+    RefPtr<CSSValue> cssValue = style->getPropertyCSSValue(CSSPropertyBackgroundColor);
+    if (!cssValue)
+        return true;
+
+    if (!cssValue->isPrimitiveValue())
+        return false;
+    CSSPrimitiveValue* value = static_cast<CSSPrimitiveValue*>(cssValue.get());
+
+    if (value->primitiveType() == CSSPrimitiveValue::CSS_RGBCOLOR)
+        return !alphaChannel(value->getRGBA32Value());
+
+    return value->getIdent() == CSSValueTransparent;
+}
+
+PassRefPtr<CSSValue> backgroundColorInEffect(Node* node)
+{
+    for (Node* ancestor = node; ancestor; ancestor = ancestor->parentNode()) {
+        RefPtr<CSSComputedStyleDeclaration> ancestorStyle = computedStyle(ancestor);
+        if (!hasTransparentBackgroundColor(ancestorStyle.get()))
+            return ancestorStyle->getPropertyCSSValue(CSSPropertyBackgroundColor);
+    }
+    return 0;
+}
+
 }

Source/WebCore/editing/EditingStyle.h

@@ -45,6 +45,7 @@ class CSSStyleDeclaration;
 class CSSComputedStyleDeclaration;
 class CSSMutableStyleDeclaration;
 class CSSPrimitiveValue;
+class CSSValue;
 class Document;
 class HTMLElement;
 class Node;
@@ -58,7 +59,7 @@ enum TriState { FalseTriState, TrueTriState, MixedTriState };
 class EditingStyle : public RefCounted<EditingStyle> {
 public:
 
-    enum PropertiesToInclude { AllProperties, OnlyInheritableProperties };
+    enum PropertiesToInclude { AllProperties, OnlyInheritableProperties, InheritablePropertiesAndBackgroundColorInEffect };
     enum ShouldPreserveWritingDirection { PreserveWritingDirection, DoNotPreserveWritingDirection };
     enum ShouldExtractMatchingStyle { ExtractMatchingStyle, DoNotExtractMatchingStyle };
     static float NoFontDelta;
@@ -73,9 +74,9 @@ class EditingStyle : public RefCounted<EditingStyle> {
         return adoptRef(new EditingStyle(node, propertiesToInclude));
     }
 
-    static PassRefPtr<EditingStyle> create(const Position& position)
+    static PassRefPtr<EditingStyle> create(const Position& position, PropertiesToInclude propertiesToInclude = OnlyInheritableProperties)
     {
-        return adoptRef(new EditingStyle(position));
+        return adoptRef(new EditingStyle(position, propertiesToInclude));
     }
 
     static PassRefPtr<EditingStyle> create(const CSSStyleDeclaration* style)
@@ -127,7 +128,7 @@ class EditingStyle : public RefCounted<EditingStyle> {
 private:
     EditingStyle();
     EditingStyle(Node*, PropertiesToInclude);
-    EditingStyle(const Position&);
+    EditingStyle(const Position&, PropertiesToInclude);
     EditingStyle(const CSSStyleDeclaration*);
     EditingStyle(int propertyID, const String& value);
     void init(Node*, PropertiesToInclude);
@@ -201,6 +202,8 @@ class StyleChange {
 int getIdentifierValue(CSSStyleDeclaration*, int propertyID);
 enum LegacyFontSizeMode { AlwaysUseLegacyFontSize, UseLegacyFontSizeOnlyIfPixelValuesMatch };
 int legacyFontSizeFromCSSValue(Document*, CSSPrimitiveValue*, bool shouldUseFixedFontDefaultSize, LegacyFontSizeMode);
+bool hasTransparentBackgroundColor(CSSStyleDeclaration*);
+PassRefPtr<CSSValue> backgroundColorInEffect(Node*);
 
 } // namespace WebCore