Changeset 182130 in webkit


Ignore:
Timestamp:
Mar 29, 2015 8:10:48 PM (9 years ago)
Author:
benjamin@webkit.org
Message:

currentColor computes to the same colour on all elements, even if 'color' is inherited differently
​https://bugs.webkit.org/show_bug.cgi?id=133420

Reviewed by Darin Adler.

Source/WebCore:

When resolving a style with the help of the property cache, we were
completely ignoring currentColor.

Since you can set currentColor on properties that are not inherited,
those properties would just be copied from the cached style, which
may have a completely different inherited color.

This pacth fixes the issue by preventing any MatchResult from hitting
the cache if it contains any non-inherited property that would require
resolution by the cache:
-Using the inherit value.
-Using the currentColor value.

Tests: fast/css/currentColor-on-before-after-pseudo-elements.html

fast/css/currentColor-style-update-reftest.html
fast/css/currentColor-value-style-update.html

  • css/ElementRuleCollector.cpp:

(WebCore::ElementRuleCollector::addElementStyleProperties):
(WebCore::ElementRuleCollector::matchAuthorRules):
(WebCore::ElementRuleCollector::matchUserRules):
(WebCore::ElementRuleCollector::matchUARules):

  • css/StyleResolver.cpp:

(WebCore::StyleResolver::MatchResult::addMatchedProperties):
(WebCore::StyleResolver::styleForKeyframe):
(WebCore::StyleResolver::pseudoStyleForElement):
(WebCore::StyleResolver::styleForPage):
(WebCore::StyleResolver::findFromMatchedPropertiesCache):
(WebCore::StyleResolver::addToMatchedPropertiesCache):
(WebCore::extractDirectionAndWritingMode):
(WebCore::StyleResolver::applyMatchedProperties):
(WebCore::StyleResolver::CascadedProperties::addStyleProperties):
(WebCore::StyleResolver::CascadedProperties::addMatches):

  • css/StyleResolver.h:

(WebCore::StyleResolver::MatchResult::matchedProperties):

LayoutTests:

  • fast/css/currentColor-on-before-after-pseudo-elements-expected.html: Added.
  • fast/css/currentColor-on-before-after-pseudo-elements.html: Added.
  • fast/css/currentColor-style-update-reftest-expected.html: Added.
  • fast/css/currentColor-style-update-reftest.html: Added.
  • fast/css/currentColor-value-style-update-expected.txt: Added.
  • fast/css/currentColor-value-style-update.html: Added.
Location:
trunk
Files:
6 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r182125 r182130  
     12015-03-29  Benjamin Poulain  <benjamin@webkit.org>
     2
     3        `currentColor` computes to the same colour on all elements, even if 'color' is inherited differently
     4        https://bugs.webkit.org/show_bug.cgi?id=133420
     5
     6        Reviewed by Darin Adler.
     7
     8        * fast/css/currentColor-on-before-after-pseudo-elements-expected.html: Added.
     9        * fast/css/currentColor-on-before-after-pseudo-elements.html: Added.
     10        * fast/css/currentColor-style-update-reftest-expected.html: Added.
     11        * fast/css/currentColor-style-update-reftest.html: Added.
     12        * fast/css/currentColor-value-style-update-expected.txt: Added.
     13        * fast/css/currentColor-value-style-update.html: Added.
     14
    1152015-03-29  Darin Adler  <darin@apple.com>
    216

  • trunk/Source/WebCore/ChangeLog

    r182129 r182130  
     12015-03-29  Benjamin Poulain  <benjamin@webkit.org>
     2
     3        `currentColor` computes to the same colour on all elements, even if 'color' is inherited differently
     4        https://bugs.webkit.org/show_bug.cgi?id=133420
     5
     6        Reviewed by Darin Adler.
     7
     8        When resolving a style with the help of the property cache, we were
     9        completely ignoring currentColor.
     10
     11        Since you can set currentColor on properties that are not inherited,
     12        those properties would just be copied from the cached style, which
     13        may have a completely different inherited color.
     14
     15        This pacth fixes the issue by preventing any MatchResult from hitting
     16        the cache if it contains any non-inherited property that would require
     17        resolution by the cache:
     18        -Using the inherit value.
     19        -Using the currentColor value.
     20
     21        Tests: fast/css/currentColor-on-before-after-pseudo-elements.html
     22               fast/css/currentColor-style-update-reftest.html
     23               fast/css/currentColor-value-style-update.html
     24
     25        * css/ElementRuleCollector.cpp:
     26        (WebCore::ElementRuleCollector::addElementStyleProperties):
     27        (WebCore::ElementRuleCollector::matchAuthorRules):
     28        (WebCore::ElementRuleCollector::matchUserRules):
     29        (WebCore::ElementRuleCollector::matchUARules):
     30        * css/StyleResolver.cpp:
     31        (WebCore::StyleResolver::MatchResult::addMatchedProperties):
     32        (WebCore::StyleResolver::styleForKeyframe):
     33        (WebCore::StyleResolver::pseudoStyleForElement):
     34        (WebCore::StyleResolver::styleForPage):
     35        (WebCore::StyleResolver::findFromMatchedPropertiesCache):
     36        (WebCore::StyleResolver::addToMatchedPropertiesCache):
     37        (WebCore::extractDirectionAndWritingMode):
     38        (WebCore::StyleResolver::applyMatchedProperties):
     39        (WebCore::StyleResolver::CascadedProperties::addStyleProperties):
     40        (WebCore::StyleResolver::CascadedProperties::addMatches):
     41        * css/StyleResolver.h:
     42        (WebCore::StyleResolver::MatchResult::matchedProperties):
     43
    1442015-03-29  Benjamin Poulain  <benjamin@webkit.org>
    245

  • trunk/Source/WebCore/css/ElementRuleCollector.cpp

    r179132 r182130  
    105105    if (!propertySet)
    106106        return;
    107     m_result.ranges.lastAuthorRule = m_result.matchedProperties.size();
     107    m_result.ranges.lastAuthorRule = m_result.matchedProperties().size();
    108108    if (m_result.ranges.firstAuthorRule == -1)
    109109        m_result.ranges.firstAuthorRule = m_result.ranges.lastAuthorRule;
    … …  
    221221{
    222222    clearMatchedRules();
    223     m_result.ranges.lastAuthorRule = m_result.matchedProperties.size() - 1;
     223    m_result.ranges.lastAuthorRule = m_result.matchedProperties().size() - 1;
    224224
    225225    // Match global author rules.
    … …  
    239239    clearMatchedRules();
    240240
    241     m_result.ranges.lastUserRule = m_result.matchedProperties.size() - 1;
     241    m_result.ranges.lastUserRule = m_result.matchedProperties().size() - 1;
    242242    MatchRequest matchRequest(m_ruleSets.userStyle(), includeEmptyRules);
    243243    StyleResolver::RuleRange ruleRange = m_result.ranges.userRuleRange();
    … …  
    268268    clearMatchedRules();
    269269   
    270     m_result.ranges.lastUARule = m_result.matchedProperties.size() - 1;
     270    m_result.ranges.lastUARule = m_result.matchedProperties().size() - 1;
    271271    StyleResolver::RuleRange ruleRange = m_result.ranges.UARuleRange();
    272272    collectMatchingRules(MatchRequest(rules), ruleRange);

  • trunk/Source/WebCore/css/StyleResolver.cpp

    r182120 r182130  
    185185    bool hasProperty(CSSPropertyID id) const;
    186186    Property& property(CSSPropertyID);
    187     bool addMatches(const MatchResult&, bool important, int startIndex, int endIndex, bool inheritedOnly = false);
     187    void addMatches(const MatchResult&, bool important, int startIndex, int endIndex, bool inheritedOnly = false);
    188188
    189189    void set(CSSPropertyID, CSSValue&, unsigned linkMatchType);
    … …  
    193193
    194194private:
    195     bool addStyleProperties(const StyleProperties&, StyleRule&, bool isImportant, bool inheritedOnly, PropertyWhitelistType, unsigned linkMatchType);
     195    void addStyleProperties(const StyleProperties&, StyleRule&, bool isImportant, bool inheritedOnly, PropertyWhitelistType, unsigned linkMatchType);
    196196    static void setPropertyInternal(Property&, CSSPropertyID, CSSValue&, unsigned linkMatchType);
    197197
    … …  
    246246void StyleResolver::MatchResult::addMatchedProperties(const StyleProperties& properties, StyleRule* rule, unsigned linkMatchType, PropertyWhitelistType propertyWhitelistType)
    247247{
    248     matchedProperties.grow(matchedProperties.size() + 1);
    249     StyleResolver::MatchedProperties& newProperties = matchedProperties.last();
     248    m_matchedProperties.grow(m_matchedProperties.size() + 1);
     249    StyleResolver::MatchedProperties& newProperties = m_matchedProperties.last();
    250250    newProperties.properties = const_cast<StyleProperties*>(&properties);
    251251    newProperties.linkMatchType = linkMatchType;
    252252    newProperties.whitelistType = propertyWhitelistType;
    253253    matchedRules.append(rule);
     254
     255    if (isCacheable) {
     256        for (unsigned i = 0, count = properties.propertyCount(); i < count; ++i) {
     257            // Currently the property cache only copy the non-inherited values and resolve
     258            // the inherited ones.
     259            // Here we define some exception were we have to resolve some properties that are not inherited
     260            // by default. If those exceptions become too common on the web, it should be possible
     261            // to build a list of exception to resolve instead of completely disabling the cache.
     262
     263            StyleProperties::PropertyReference current = properties.propertyAt(i);
     264            if (!current.isInherited()) {
     265                // If the property value is explicitly inherited, we need to apply further non-inherited properties
     266                // as they might override the value inherited here. For this reason we don't allow declarations with
     267                // explicitly inherited properties to be cached.
     268                const CSSValue& value = *current.value();
     269                if (value.isInheritedValue()) {
     270                    isCacheable = false;
     271                    break;
     272                }
     273
     274                // The value currentColor has implicitely the same side effect. It depends on the value of color,
     275                // which is an inherited value, making the non-inherited property implicitely inherited.
     276                if (is<CSSPrimitiveValue>(value) && downcast<CSSPrimitiveValue>(value).getValueID() == CSSValueCurrentcolor) {
     277                    isCacheable = false;
     278                    break;
     279                }
     280            }
     281        }
     282    }
    254283}
    255284
    … …  
    836865    // decl, there's nothing to override. So just add the first properties.
    837866    CascadedProperties cascade(direction, writingMode);
    838     cascade.addMatches(result, false, 0, result.matchedProperties.size() - 1);
     867    cascade.addMatches(result, false, 0, result.matchedProperties().size() - 1);
    839868
    840869    applyCascadedProperties(cascade, firstCSSProperty, lastHighPriorityProperty);
    … …  
    963992    }
    964993
    965     if (collector.matchedResult().matchedProperties.isEmpty())
     994    if (collector.matchedResult().matchedProperties().isEmpty())
    966995        return 0;
    967996
    … …  
    10021031
    10031032    CascadedProperties cascade(direction, writingMode);
    1004     cascade.addMatches(result, false, 0, result.matchedProperties.size() - 1);
     1033    cascade.addMatches(result, false, 0, result.matchedProperties().size() - 1);
    10051034
    10061035    applyCascadedProperties(cascade, firstCSSProperty, lastHighPriorityProperty);
    … …  
    16111640    MatchedPropertiesCacheItem& cacheItem = it->value;
    16121641
    1613     size_t size = matchResult.matchedProperties.size();
     1642    size_t size = matchResult.matchedProperties().size();
    16141643    if (size != cacheItem.matchedProperties.size())
    16151644        return 0;
    16161645    for (size_t i = 0; i < size; ++i) {
    1617         if (matchResult.matchedProperties[i] != cacheItem.matchedProperties[i])
     1646        if (matchResult.matchedProperties()[i] != cacheItem.matchedProperties[i])
    16181647            return 0;
    16191648    }
    … …  
    16341663    ASSERT(hash);
    16351664    MatchedPropertiesCacheItem cacheItem;
    1636     cacheItem.matchedProperties.appendVector(matchResult.matchedProperties);
     1665    cacheItem.matchedProperties.appendVector(matchResult.matchedProperties());
    16371666    cacheItem.ranges = matchResult.ranges;
    16381667    // Note that we don't cache the original RenderStyle instance. It may be further modified.
    … …  
    16861715    bool hadImportantDirection = false;
    16871716
    1688     for (auto& matchedProperties : matchResult.matchedProperties) {
     1717    for (const auto& matchedProperties : matchResult.matchedProperties()) {
    16891718        for (unsigned i = 0, count = matchedProperties.properties->propertyCount(); i < count; ++i) {
    16901719            auto property = matchedProperties.properties->propertyAt(i);
    … …  
    17151744    ASSERT(element);
    17161745    State& state = m_state;
    1717     unsigned cacheHash = shouldUseMatchedPropertiesCache && matchResult.isCacheable ? computeMatchedPropertiesHash(matchResult.matchedProperties.data(), matchResult.matchedProperties.size()) : 0;
     1746    unsigned cacheHash = shouldUseMatchedPropertiesCache && matchResult.isCacheable ? computeMatchedPropertiesHash(matchResult.matchedProperties().data(), matchResult.matchedProperties().size()) : 0;
    17181747    bool applyInheritedOnly = false;
    17191748    const MatchedPropertiesCacheItem* cacheItem = 0;
    … …  
    17491778        // can look at them later to figure out if this is a styled form control or not.
    17501779        CascadedProperties cascade(direction, writingMode);
    1751         if (!cascade.addMatches(matchResult, false, matchResult.ranges.firstUARule, matchResult.ranges.lastUARule, applyInheritedOnly)
    1752             || !cascade.addMatches(matchResult, true, matchResult.ranges.firstUARule, matchResult.ranges.lastUARule, applyInheritedOnly))
    1753             return applyMatchedProperties(matchResult, element, DoNotUseMatchedPropertiesCache);
     1780        cascade.addMatches(matchResult, false, matchResult.ranges.firstUARule, matchResult.ranges.lastUARule, applyInheritedOnly);
     1781        cascade.addMatches(matchResult, true, matchResult.ranges.firstUARule, matchResult.ranges.lastUARule, applyInheritedOnly);
    17541782
    17551783        applyCascadedProperties(cascade, CSSPropertyWebkitRubyPosition, CSSPropertyWebkitRubyPosition);
    … …  
    17661794
    17671795    CascadedProperties cascade(direction, writingMode);
    1768     if (!cascade.addMatches(matchResult, false, 0, matchResult.matchedProperties.size() - 1, applyInheritedOnly)
    1769         || !cascade.addMatches(matchResult, true, matchResult.ranges.firstAuthorRule, matchResult.ranges.lastAuthorRule, applyInheritedOnly)
    1770         || !cascade.addMatches(matchResult, true, matchResult.ranges.firstUserRule, matchResult.ranges.lastUserRule, applyInheritedOnly)
    1771         || !cascade.addMatches(matchResult, true, matchResult.ranges.firstUARule, matchResult.ranges.lastUARule, applyInheritedOnly))
    1772         return applyMatchedProperties(matchResult, element, DoNotUseMatchedPropertiesCache);
     1796    cascade.addMatches(matchResult, false, 0, matchResult.matchedProperties().size() - 1, applyInheritedOnly);
     1797    cascade.addMatches(matchResult, true, matchResult.ranges.firstAuthorRule, matchResult.ranges.lastAuthorRule, applyInheritedOnly);
     1798    cascade.addMatches(matchResult, true, matchResult.ranges.firstUserRule, matchResult.ranges.lastUserRule, applyInheritedOnly);
     1799    cascade.addMatches(matchResult, true, matchResult.ranges.firstUARule, matchResult.ranges.lastUARule, applyInheritedOnly);
    17731800
    17741801    applyCascadedProperties(cascade, CSSPropertyWebkitRubyPosition, CSSPropertyWebkitRubyPosition);
    … …  
    26242651}
    26252652
    2626 bool StyleResolver::CascadedProperties::addStyleProperties(const StyleProperties& properties, StyleRule&, bool isImportant, bool inheritedOnly, PropertyWhitelistType propertyWhitelistType, unsigned linkMatchType)
     2653void StyleResolver::CascadedProperties::addStyleProperties(const StyleProperties& properties, StyleRule&, bool isImportant, bool inheritedOnly, PropertyWhitelistType propertyWhitelistType, unsigned linkMatchType)
    26272654{
    26282655    for (unsigned i = 0, count = properties.propertyCount(); i < count; ++i) {
    … …  
    26312658            continue;
    26322659        if (inheritedOnly && !current.isInherited()) {
    2633             // If the property value is explicitly inherited, we need to apply further non-inherited properties
    2634             // as they might override the value inherited here. For this reason we don't allow declarations with
    2635             // explicitly inherited properties to be cached.
    2636             if (current.value()->isInheritedValue())
    2637                 return false;
     2660            // We apply the inherited properties only when using the property cache.
     2661            // A match with a value that is explicitely inherited should never have been cached.
     2662            ASSERT(!current.value()->isInheritedValue());
    26382663            continue;
    26392664        }
    … …  
    26522677            set(propertyID, *current.value(), linkMatchType);
    26532678    }
    2654     return true;
    2655 }
    2656 
    2657 bool StyleResolver::CascadedProperties::addMatches(const MatchResult& matchResult, bool important, int startIndex, int endIndex, bool inheritedOnly)
     2679}
     2680
     2681void StyleResolver::CascadedProperties::addMatches(const MatchResult& matchResult, bool important, int startIndex, int endIndex, bool inheritedOnly)
    26582682{
    26592683    if (startIndex == -1)
    2660         return true;
     2684        return;
    26612685
    26622686    for (int i = startIndex; i <= endIndex; ++i) {
    2663         const MatchedProperties& matchedProperties = matchResult.matchedProperties[i];
    2664         if (!addStyleProperties(*matchedProperties.properties, *matchResult.matchedRules[i], important, inheritedOnly, static_cast<PropertyWhitelistType>(matchedProperties.whitelistType), matchedProperties.linkMatchType))
    2665             return false;
    2666     }
    2667     return true;
     2687        const MatchedProperties& matchedProperties = matchResult.matchedProperties()[i];
     2688        addStyleProperties(*matchedProperties.properties, *matchResult.matchedRules[i], important, inheritedOnly, static_cast<PropertyWhitelistType>(matchedProperties.whitelistType), matchedProperties.linkMatchType);
     2689    }
    26682690}
    26692691

  • trunk/Source/WebCore/css/StyleResolver.h

    r180051 r182130  
    278278    struct MatchResult {
    279279        MatchResult() : isCacheable(true) { }
    280         Vector<MatchedProperties, 64> matchedProperties;
    281280        Vector<StyleRule*, 64> matchedRules;
    282281        MatchRanges ranges;
    283282        bool isCacheable;
    284283
    285         void addMatchedProperties(const StyleProperties&, StyleRule* = 0, unsigned linkMatchType = SelectorChecker::MatchAll, PropertyWhitelistType = PropertyWhitelistNone);
     284        const Vector<MatchedProperties, 64>& matchedProperties() const { return m_matchedProperties; }
     285
     286        void addMatchedProperties(const StyleProperties&, StyleRule* = nullptr, unsigned linkMatchType = SelectorChecker::MatchAll, PropertyWhitelistType = PropertyWhitelistNone);
     287    private:
     288        Vector<MatchedProperties, 64> m_matchedProperties;
    286289    };
    287290
Note: See TracChangeset for help on using the changeset viewer.