Changeset 87082 in webkit


Ignore:
Timestamp:
May 23, 2011 11:07:41 AM (13 years ago)
Author:
rniwa@webkit.org
Message:

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.
Location:
trunk
Files:
2 added
8 edited
2 moved

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r87081 r87082  
     12011-05-20  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Reviewed by Enrica Casucci.
     4
     5        REGRESSION(r84311): WebKit copies too much styles when copying
     6        https://bugs.webkit.org/show_bug.cgi?id=60914
     7
     8        Added a test to ensure WebKit does not clone hierarchy to preserve background color.
     9
     10        Also renamed editing/pasteboard/do-not-copy-body-color.html to do-no-clone-unnecessary-styles.html
     11        and updated the description to match new behavior. While this test ensures WebKit does not copy
     12        body's background color when it's not fully selected, this isn't a necessary requirement for us
     13        not to duplicate borders so new expected result is correct.
     14
     15        * editing/pasteboard/copy-text-with-backgroundcolor-expected.txt: Some spans became style spans.
     16        * editing/pasteboard/do-no-clone-unnecessary-styles-2-expected.txt: Added.
     17        * editing/pasteboard/do-no-clone-unnecessary-styles-2.html: Added.
     18        * editing/pasteboard/do-no-clone-unnecessary-styles-expected.txt: Renamed from
     19        LayoutTests/editing/pasteboard/do-not-copy-body-color-expected.txt.
     20        * editing/pasteboard/do-no-clone-unnecessary-styles.html: Renamed from
     21        LayoutTests/editing/pasteboard/do-not-copy-body-color.html.
     22
    1232011-05-23  Ryosuke Niwa  <rniwa@webkit.org>
    224

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

    r84311 r87082  
    5858|   "Red background"
    5959| <span>
     60|   class="Apple-style-span"
    6061|   style="background-color: rgb(255, 0, 0); "
    6162|   "Red background"
    … …  
    6667|     "Green background"
    6768|   <span>
    68 |     style="background-color: green; "
     69|     class="Apple-style-span"
     70|     style="background-color: rgb(0, 128, 0); "
    6971|     "Green background"
    7072| "

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

    r87008 r87082  
    11
    22<div>
    3 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.
     3This test verifies that WebKit does not erroneously clone nodes hierarchy when copying text with background color.
     4To 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.
    45<div style="border: 2px solid red">
    5 <div id="test">Select this text<br></div>
     6<div id="test"><span class="Apple-style-span" style="background-color: rgb(187, 187, 187); ">Select this text</span><br></div>
    67</div>
    78</div>
    … …  
    910
    1011
    11 

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

    r87008 r87082  
    1717    document.execCommand("Paste");
    1818    document.body.innerText = document.body.innerHTML;
    19    
    2019}
    2120</script>
    … …  
    2322<body onLoad="runTest();" contentEditable="true" style="background-color: #bbb;">
    2423<div>
    25 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.
     24This test verifies that WebKit does not erroneously clone nodes hierarchy when copying text with background color.
     25To 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.
    2626<div style="border: 2px solid red">
    2727    <div id="test">Select this text</div>
    … …  
    3030</body>
    3131</html>
    32 

  • trunk/Source/WebCore/ChangeLog

    r87079 r87082  
     12011-05-20  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Reviewed by Enrica Casucci.
     4
     5        REGRESSION(r84311): WebKit copies too much styles when copying
     6        https://bugs.webkit.org/show_bug.cgi?id=60914
     7
     8        The bug was caused by WebKit's cloning node hierarchy up until the node that has background color.
     9        Fixed the bug by not cloning background color and adding the effective background color to the wrapping
     10        style span.
     11
     12        Tests: editing/pasteboard/do-no-clone-unnecessary-styles-2.html
     13               editing/pasteboard/do-no-clone-unnecessary-styles.html
     14
     15        * editing/EditingStyle.cpp:
     16        (WebCore::cssValueToRGBA): Extracted from getRGBAFontColor.
     17        (WebCore::getRGBAFontColor): Moved.
     18        (WebCore::rgbaBackgroundColorInEffect): Added.
     19        (WebCore::EditingStyle::init): Added support for InheritablePropertiesAndBackgroundColorInEffect.
     20        (WebCore::EditingStyle::prepareToApplyAt): Include the effective background color at the given position.
     21        Also remove the background color property when the effective background color is equal to the background
     22        color property (in terms of RGBA value) of the editing style.
     23        (WebCore::hasTransparentBackgroundColor): Moved from Editor class.
     24        (WebCore::backgroundColorInEffect): Extracted from Editor::selectionStartCSSPropertyValue.
     25        * editing/EditingStyle.h: Added prototypes for hasTransparentBackgroundColor and backgroundColorInEffect.
     26        * editing/Editor.cpp:
     27        (WebCore::Editor::selectionStartCSSPropertyValue): Calls backgroundColorInEffect.
     28        * editing/Editor.h: Removed hasTransparentBackgroundColor.
     29        * editing/markup.cpp:
     30        (WebCore::isElementPresentational): Reverted r85090 and r84311.
     31        (WebCore::createMarkup): Include the background color in effect when computing the editing style.
     32
    1332011-05-23  Roland Steiner  <rolandsteiner@chromium.org>
    234

  • trunk/Source/WebCore/editing/EditingStyle.cpp

    r86983 r87082  
    9191
    9292static RefPtr<CSSMutableStyleDeclaration> getPropertiesNotIn(CSSStyleDeclaration* styleWithRedundantProperties, CSSStyleDeclaration* baseStyle);
    93 static RGBA32 getRGBAFontColor(CSSStyleDeclaration*);
    9493
    9594class HTMLElementEquivalent {
    … …  
    277276}
    278277
    279 EditingStyle::EditingStyle(const Position& position)
     278EditingStyle::EditingStyle(const Position& position, PropertiesToInclude propertiesToInclude)
    280279    : m_shouldUseFixedDefaultFontSize(false)
    281280    , m_fontSizeDelta(NoFontDelta)
    282281{
    283     init(position.deprecatedNode(), OnlyInheritableProperties);
     282    init(position.deprecatedNode(), propertiesToInclude);
    284283}
    285284
    … …  
    304303}
    305304
     305static RGBA32 cssValueToRGBA(CSSValue* colorValue)
     306{
     307    if (!colorValue || !colorValue->isPrimitiveValue())
     308        return Color::transparent;
     309   
     310    CSSPrimitiveValue* primitiveColor = static_cast<CSSPrimitiveValue*>(colorValue);
     311    if (primitiveColor->primitiveType() == CSSPrimitiveValue::CSS_RGBCOLOR)
     312        return primitiveColor->getRGBA32Value();
     313   
     314    RGBA32 rgba = 0;
     315    CSSParser::parseColor(rgba, colorValue->cssText());
     316    return rgba;
     317}
     318
     319static inline RGBA32 getRGBAFontColor(CSSStyleDeclaration* style)
     320{
     321    return cssValueToRGBA(style->getPropertyCSSValue(CSSPropertyColor).get());
     322}
     323
     324static inline RGBA32 rgbaBackgroundColorInEffect(Node* node)
     325{
     326    return cssValueToRGBA(backgroundColorInEffect(node).get());
     327}
     328
    306329void EditingStyle::init(Node* node, PropertiesToInclude propertiesToInclude)
    307330{
    … …  
    313336    RefPtr<CSSComputedStyleDeclaration> computedStyleAtPosition = computedStyle(node);
    314337    m_mutableStyle = propertiesToInclude == AllProperties && computedStyleAtPosition ? computedStyleAtPosition->copy() : editingStyleFromComputedStyle(computedStyleAtPosition);
     338
     339    if (propertiesToInclude == InheritablePropertiesAndBackgroundColorInEffect) {
     340        if (RefPtr<CSSValue> value = backgroundColorInEffect(node))
     341            m_mutableStyle->setProperty(CSSPropertyBackgroundColor, value->cssText());
     342    }
    315343
    316344    if (node && node->computedStyle()) {
    … …  
    533561TriState EditingStyle::triStateOfStyle(CSSStyleDeclaration* styleToCompare, ShouldIgnoreTextOnlyProperties shouldIgnoreTextOnlyProperties) const
    534562{
     563    // FIXME: take care of background-color in effect
    535564    RefPtr<CSSMutableStyleDeclaration> difference = getPropertiesNotIn(m_mutableStyle.get(), styleToCompare);
    536565
    … …  
    697726    // If this function was modified in the future to delete all redundant properties, then add a boolean value to indicate
    698727    // which one of editingStyleAtPosition or computedStyle is called.
    699     RefPtr<EditingStyle> style = EditingStyle::create(position);
     728    RefPtr<EditingStyle> style = EditingStyle::create(position, InheritablePropertiesAndBackgroundColorInEffect);
    700729
    701730    RefPtr<CSSValue> unicodeBidi;
    … …  
    710739        m_mutableStyle->removeProperty(CSSPropertyColor);
    711740
    712     // if alpha value is zero, we don't add the background color.
    713     RefPtr<CSSValue> backgroundColor = m_mutableStyle->getPropertyCSSValue(CSSPropertyBackgroundColor);
    714     if (backgroundColor && backgroundColor->isPrimitiveValue()
    715         && !alphaChannel(static_cast<CSSPrimitiveValue*>(backgroundColor.get())->getRGBA32Value())) {
    716         ExceptionCode ec;
    717         m_mutableStyle->removeProperty(CSSPropertyBackgroundColor, ec);
    718     }
     741    if (hasTransparentBackgroundColor(m_mutableStyle.get())
     742        || cssValueToRGBA(m_mutableStyle->getPropertyCSSValue(CSSPropertyBackgroundColor).get()) == rgbaBackgroundColorInEffect(position.containerNode()))
     743        m_mutableStyle->removeProperty(CSSPropertyBackgroundColor);
    719744
    720745    if (unicodeBidi && unicodeBidi->isPrimitiveValue()) {
    … …  
    811836
    812837    RefPtr<CSSComputedStyleDeclaration> computedStyle = position.computedStyle();
     838    // FIXME: take care of background-color in effect
    813839    RefPtr<CSSMutableStyleDeclaration> mutableStyle = getPropertiesNotIn(style->style(), computedStyle.get());
    814840
    … …  
    839865        style->removeProperty(propertyID);
    840866    }
    841 }
    842 
    843 static RGBA32 getRGBAFontColor(CSSStyleDeclaration* style)
    844 {
    845     RefPtr<CSSValue> colorValue = style->getPropertyCSSValue(CSSPropertyColor);
    846     if (!colorValue || !colorValue->isPrimitiveValue())
    847         return Color::transparent;
    848 
    849     CSSPrimitiveValue* primitiveColor = static_cast<CSSPrimitiveValue*>(colorValue.get());
    850     if (primitiveColor->primitiveType() == CSSPrimitiveValue::CSS_RGBCOLOR)
    851         return primitiveColor->getRGBA32Value();
    852 
    853     // Need to take care of named color such as green and black
    854     // This code should be removed after https://bugs.webkit.org/show_bug.cgi?id=28282 is fixed.
    855     RGBA32 rgba = 0;
    856     CSSParser::parseColor(rgba, colorValue->cssText());
    857     return rgba;
    858867}
    859868
    … …  
    9921001    ASSERT(baseStyle);
    9931002    RefPtr<CSSMutableStyleDeclaration> result = styleWithRedundantProperties->copy();
     1003
    9941004    baseStyle->diff(result.get());
    9951005
    … …  
    10051015
    10061016    if (getTextAlignment(result.get()) == getTextAlignment(baseStyle))
    1007         result->removeProperty(CSSPropertyTextAlign);       
     1017        result->removeProperty(CSSPropertyTextAlign);
    10081018
    10091019    return result;
    … …  
    10461056}
    10471057
    1048 }
     1058bool hasTransparentBackgroundColor(CSSStyleDeclaration* style)
     1059{
     1060    RefPtr<CSSValue> cssValue = style->getPropertyCSSValue(CSSPropertyBackgroundColor);
     1061    if (!cssValue)
     1062        return true;
     1063   
     1064    if (!cssValue->isPrimitiveValue())
     1065        return false;
     1066    CSSPrimitiveValue* value = static_cast<CSSPrimitiveValue*>(cssValue.get());
     1067   
     1068    if (value->primitiveType() == CSSPrimitiveValue::CSS_RGBCOLOR)
     1069        return !alphaChannel(value->getRGBA32Value());
     1070   
     1071    return value->getIdent() == CSSValueTransparent;
     1072}
     1073
     1074PassRefPtr<CSSValue> backgroundColorInEffect(Node* node)
     1075{
     1076    for (Node* ancestor = node; ancestor; ancestor = ancestor->parentNode()) {
     1077        RefPtr<CSSComputedStyleDeclaration> ancestorStyle = computedStyle(ancestor);
     1078        if (!hasTransparentBackgroundColor(ancestorStyle.get()))
     1079            return ancestorStyle->getPropertyCSSValue(CSSPropertyBackgroundColor);
     1080    }
     1081    return 0;
     1082}
     1083
     1084}

  • trunk/Source/WebCore/editing/EditingStyle.h

    r83618 r87082  
    4646class CSSMutableStyleDeclaration;
    4747class CSSPrimitiveValue;
     48class CSSValue;
    4849class Document;
    4950class HTMLElement;
    … …  
    5960public:
    6061
    61     enum PropertiesToInclude { AllProperties, OnlyInheritableProperties };
     62    enum PropertiesToInclude { AllProperties, OnlyInheritableProperties, InheritablePropertiesAndBackgroundColorInEffect };
    6263    enum ShouldPreserveWritingDirection { PreserveWritingDirection, DoNotPreserveWritingDirection };
    6364    enum ShouldExtractMatchingStyle { ExtractMatchingStyle, DoNotExtractMatchingStyle };
    … …  
    7475    }
    7576
    76     static PassRefPtr<EditingStyle> create(const Position& position)
    77     {
    78         return adoptRef(new EditingStyle(position));
     77    static PassRefPtr<EditingStyle> create(const Position& position, PropertiesToInclude propertiesToInclude = OnlyInheritableProperties)
     78    {
     79        return adoptRef(new EditingStyle(position, propertiesToInclude));
    7980    }
    8081
    … …  
    128129    EditingStyle();
    129130    EditingStyle(Node*, PropertiesToInclude);
    130     EditingStyle(const Position&);
     131    EditingStyle(const Position&, PropertiesToInclude);
    131132    EditingStyle(const CSSStyleDeclaration*);
    132133    EditingStyle(int propertyID, const String& value);
    … …  
    202203enum LegacyFontSizeMode { AlwaysUseLegacyFontSize, UseLegacyFontSizeOnlyIfPixelValuesMatch };
    203204int legacyFontSizeFromCSSValue(Document*, CSSPrimitiveValue*, bool shouldUseFixedFontDefaultSize, LegacyFontSizeMode);
     205bool hasTransparentBackgroundColor(CSSStyleDeclaration*);
     206PassRefPtr<CSSValue> backgroundColorInEffect(Node*);
    204207
    205208} // namespace WebCore

  • trunk/Source/WebCore/editing/Editor.cpp

    r87067 r87082  
    919919}
    920920
    921 bool Editor::hasTransparentBackgroundColor(CSSStyleDeclaration* style)
    922 {
    923     RefPtr<CSSValue> cssValue = style->getPropertyCSSValue(CSSPropertyBackgroundColor);
    924     if (!cssValue)
    925         return true;
    926 
    927     if (!cssValue->isPrimitiveValue())
    928         return false;
    929     CSSPrimitiveValue* value = static_cast<CSSPrimitiveValue*>(cssValue.get());
    930 
    931     if (value->primitiveType() == CSSPrimitiveValue::CSS_RGBCOLOR)
    932         return !alphaChannel(value->getRGBA32Value());
    933 
    934     return value->getIdent() == CSSValueTransparent;
    935 }
    936 
    937921String Editor::selectionStartCSSPropertyValue(int propertyID)
    938922{
    … …  
    949933        RefPtr<Range> range(m_frame->selection()->toNormalizedRange());
    950934        ExceptionCode ec = 0;
    951         for (Node* ancestor = range->commonAncestorContainer(ec); ancestor; ancestor = ancestor->parentNode()) {
    952             RefPtr<CSSComputedStyleDeclaration> ancestorStyle = computedStyle(ancestor);
    953             if (!hasTransparentBackgroundColor(ancestorStyle.get()))
    954                 return ancestorStyle->getPropertyValue(CSSPropertyBackgroundColor);
    955         }
     935        if (PassRefPtr<CSSValue> value = backgroundColorInEffect(range->commonAncestorContainer(ec)))
     936            return value->cssText();
    956937    }
    957938

  • trunk/Source/WebCore/editing/Editor.h

    r86520 r87082  
    197197    Command command(const String& commandName, EditorCommandSource);
    198198    static bool commandIsSupportedFromMenuOrKeyBinding(const String& commandName); // Works without a frame.
    199     static bool hasTransparentBackgroundColor(CSSStyleDeclaration*);
    200199
    201200    bool insertText(const String&, Event* triggeringEvent);

  • trunk/Source/WebCore/editing/markup.cpp

    r86983 r87082  
    469469    if (!style)
    470470        return false;
    471     return !propertyMissingOrEqualToNone(style.get(), CSSPropertyTextDecoration) || (!node->isBlockFlow() && !Editor::hasTransparentBackgroundColor(style.get()));
     471    return !propertyMissingOrEqualToNone(style.get(), CSSPropertyTextDecoration);
    472472}
    473473
    … …  
    634634    ContainerNode* parentOfLastClosed = lastClosed ? lastClosed->parentNode() : 0;
    635635    if (parentOfLastClosed && parentOfLastClosed->renderer()) {
    636         RefPtr<EditingStyle> style = EditingStyle::create(parentOfLastClosed);
     636        RefPtr<EditingStyle> style = EditingStyle::create(parentOfLastClosed, EditingStyle::InheritablePropertiesAndBackgroundColorInEffect);
    637637
    638638        // Styles that Mail blockquotes contribute should only be placed on the Mail blockquote, to help
Note: See TracChangeset for help on using the changeset viewer.