Changeset 251285 in webkit


Ignore:
Timestamp:
Oct 18, 2019 9:20:17 AM (4 years ago)
Author:
Antti Koivisto
Message:

[CSS Shadow Parts] :part rules should be able to override style attribute
https://bugs.webkit.org/show_bug.cgi?id=202919

Reviewed by Zalan Bujtas.

LayoutTests/imported/w3c:

  • web-platform-tests/css/css-shadow-parts/simple-inline-expected.txt:

Source/WebCore:

Element inline style was simply appended to the matching declaration list and not sorted with the rest of the author style.
This used to work because before CSS Shadow Parts feature inline style would always win.

Fixing this involves refactoring the rule collection code to remove this assumption.

  • css/ElementRuleCollector.cpp:

(WebCore::ElementRuleCollector::addMatchedRule):

Both initialize and update ranges here.

(WebCore::ElementRuleCollector::clearMatchedRules):
(WebCore::ElementRuleCollector::addElementStyleProperties):

Both initialize and update ranges here.

(WebCore::ElementRuleCollector::sortAndTransferMatchedRules):

Split out transfering to a separate function.

(WebCore::ElementRuleCollector::transferMatchedRules):

Add a parameter to limit transfer to rules from a scope.

(WebCore::ElementRuleCollector::matchAuthorRules):
(WebCore::ElementRuleCollector::matchesAnyAuthorRules):

Replace hasMatchedRules() with a more specific function. This can use collectMatchingAuthorRules and avoids unnecessary sorting step.

(WebCore::ElementRuleCollector::collectMatchingAuthorRules):

Split out collecting the rules from matchAuthorRules. Like other collect functions, this doesn't do any sorting.

(WebCore::ElementRuleCollector::matchAllRules):

Add element inline style before transfering rules from the containing host scope.

(WebCore::ElementRuleCollector::addElementInlineStyleProperties):

Factor adding inline style into a function.

Location:
trunk
Files:
9 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/imported/w3c/ChangeLog

    r251279 r251285  
     12019-10-18  Antti Koivisto  <antti@apple.com>
     2
     3        [CSS Shadow Parts] :part rules should be able to override style attribute
     4        https://bugs.webkit.org/show_bug.cgi?id=202919
     5
     6        Reviewed by Zalan Bujtas.
     7
     8        * web-platform-tests/css/css-shadow-parts/simple-inline-expected.txt:
     9
    1102019-10-17  Wenson Hsieh  <wenson_hsieh@apple.com>
    211
  • trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-shadow-parts/simple-inline-expected.txt

    r251184 r251285  
    11The following text should be green:
    22
    3 FAIL Part in selected host is styled assert_equals: expected "rgb(0, 128, 0)" but got "rgb(255, 0, 0)"
     3PASS Part in selected host is styled
    44
  • trunk/Source/WebCore/ChangeLog

    r251284 r251285  
     12019-10-18  Antti Koivisto  <antti@apple.com>
     2
     3        [CSS Shadow Parts] :part rules should be able to override style attribute
     4        https://bugs.webkit.org/show_bug.cgi?id=202919
     5
     6        Reviewed by Zalan Bujtas.
     7
     8        Element inline style was simply appended to the matching declaration list and not sorted with the rest of the author style.
     9        This used to work because before CSS Shadow Parts feature inline style would always win.
     10
     11        Fixing this involves refactoring the rule collection code to remove this assumption.
     12
     13        * css/ElementRuleCollector.cpp:
     14        (WebCore::ElementRuleCollector::addMatchedRule):
     15
     16        Both initialize and update ranges here.
     17
     18        (WebCore::ElementRuleCollector::clearMatchedRules):
     19        (WebCore::ElementRuleCollector::addElementStyleProperties):
     20
     21        Both initialize and update ranges here.
     22
     23        (WebCore::ElementRuleCollector::sortAndTransferMatchedRules):
     24
     25        Split out transfering to a separate function.
     26
     27        (WebCore::ElementRuleCollector::transferMatchedRules):
     28
     29        Add a parameter to limit transfer to rules from a scope.
     30
     31        (WebCore::ElementRuleCollector::matchAuthorRules):
     32        (WebCore::ElementRuleCollector::matchesAnyAuthorRules):
     33
     34        Replace hasMatchedRules() with a more specific function. This can use collectMatchingAuthorRules and avoids unnecessary sorting step.
     35
     36        (WebCore::ElementRuleCollector::collectMatchingAuthorRules):
     37
     38        Split out collecting the rules from matchAuthorRules. Like other collect functions, this doesn't do any sorting.
     39
     40        (WebCore::ElementRuleCollector::matchAllRules):
     41
     42        Add element inline style before transfering rules from the containing host scope.
     43
     44        (WebCore::ElementRuleCollector::addElementInlineStyleProperties):
     45
     46        Factor adding inline style into a function.
     47
     48]        * css/ElementRuleCollector.h:
     49        (WebCore::ElementRuleCollector::hasMatchedRules const): Deleted.
     50        * css/StyleProperties.h:
     51        (WebCore::StylePropertiesBase::deref const):
     52        (WebCore::StylePropertiesBase::deref): Deleted.
     53
     54        Make const to allow RefPtr<const StyleProperties>
     55
     56        * css/StyleResolver.cpp:
     57        (WebCore::StyleResolver::CascadedProperties::addImportantMatches):
     58
     59        Sort !important properties taking into account that the host scope has lower priority.
     60
     61        * style/StyleInvalidator.cpp:
     62        (WebCore::Style::Invalidator::invalidateIfNeeded):
     63
    1642019-10-18  Zalan Bujtas  <zalan@apple.com>
    265
  • trunk/Source/WebCore/css/ElementRuleCollector.cpp

    r250817 r251285  
    113113{
    114114    // Update our first/last rule indices in the matched rules array.
    115     ++ruleRange.lastRuleIndex;
    116     if (ruleRange.firstRuleIndex == -1)
     115    if (ruleRange.lastRuleIndex != -1)
     116        ++ruleRange.lastRuleIndex;
     117    else {
     118        ruleRange.lastRuleIndex = m_result.matchedProperties().size();
    117119        ruleRange.firstRuleIndex = ruleRange.lastRuleIndex;
     120    }
    118121
    119122    m_matchedRules.append({ &ruleData, specificity, styleScopeOrdinal });
     
    124127    m_matchedRules.clear();
    125128    m_keepAliveSlottedPseudoElementRules.clear();
     129    m_matchedRuleTransferIndex = 0;
    126130}
    127131
     
    130134    if (!propertySet)
    131135        return;
    132     m_result.ranges.lastAuthorRule = m_result.matchedProperties().size();
    133     if (m_result.ranges.firstAuthorRule == -1)
     136
     137    if (m_result.ranges.lastAuthorRule != -1)
     138        ++m_result.ranges.lastAuthorRule;
     139    else {
     140        m_result.ranges.lastAuthorRule = m_result.matchedProperties().size();
    134141        m_result.ranges.firstAuthorRule = m_result.ranges.lastAuthorRule;
     142    }
     143
    135144    m_result.addMatchedProperties(*propertySet);
    136145    if (!isCacheable)
     
    172181    sortMatchedRules();
    173182
    174     if (m_mode == SelectorChecker::Mode::CollectingRules) {
    175         for (const MatchedRule& matchedRule : m_matchedRules)
     183    transferMatchedRules();
     184}
     185
     186void ElementRuleCollector::transferMatchedRules(Optional<Style::ScopeOrdinal> fromScope)
     187{
     188    for (; m_matchedRuleTransferIndex < m_matchedRules.size(); ++m_matchedRuleTransferIndex) {
     189        auto& matchedRule = m_matchedRules[m_matchedRuleTransferIndex];
     190        if (fromScope && matchedRule.styleScopeOrdinal < *fromScope)
     191            break;
     192
     193        if (m_mode == SelectorChecker::Mode::CollectingRules) {
    176194            m_matchedRuleList.append(matchedRule.ruleData->rule());
    177         return;
    178     }
    179 
    180     for (const MatchedRule& matchedRule : m_matchedRules) {
     195            continue;
     196        }
     197
    181198        m_result.addMatchedProperties(matchedRule.ruleData->rule()->properties(), matchedRule.ruleData->rule(), matchedRule.ruleData->linkMatchType(), matchedRule.ruleData->propertyWhitelistType(), matchedRule.styleScopeOrdinal);
    182199    }
     
    187204    clearMatchedRules();
    188205
    189     m_result.ranges.lastAuthorRule = m_result.matchedProperties().size() - 1;
     206    collectMatchingAuthorRules(includeEmptyRules);
     207
     208    sortAndTransferMatchedRules();
     209}
     210
     211bool ElementRuleCollector::matchesAnyAuthorRules()
     212{
     213    clearMatchedRules();
     214
     215    // FIXME: This should bail out on first match.
     216    collectMatchingAuthorRules(false);
     217
     218    return !m_matchedRules.isEmpty();
     219}
     220
     221void ElementRuleCollector::collectMatchingAuthorRules(bool includeEmptyRules)
     222{
    190223    StyleResolver::RuleRange ruleRange = m_result.ranges.authorRuleRange();
    191224
     
    206239        matchPartPseudoElementRules(includeEmptyRules, ruleRange);
    207240    }
    208 
    209     sortAndTransferMatchedRules();
    210241}
    211242
     
    341372    clearMatchedRules();
    342373
    343     m_result.ranges.lastUserRule = m_result.matchedProperties().size() - 1;
    344374    MatchRequest matchRequest(m_userStyle, includeEmptyRules);
    345375    StyleResolver::RuleRange ruleRange = m_result.ranges.userRuleRange();
     
    370400    clearMatchedRules();
    371401   
    372     m_result.ranges.lastUARule = m_result.matchedProperties().size() - 1;
    373402    StyleResolver::RuleRange ruleRange = m_result.ranges.UARuleRange();
    374403    collectMatchingRules(MatchRequest(&rules), ruleRange);
     
    558587    }
    559588   
    560     // Check the rules in author sheets next.
    561     if (matchAuthorAndUserStyles)
    562         matchAuthorRules(false);
    563 
    564     if (matchAuthorAndUserStyles && is<StyledElement>(element())) {
    565         auto& styledElement = downcast<StyledElement>(element());
    566         // Now check our inline style attribute.
    567         if (styledElement.inlineStyle()) {
    568             // Inline style is immutable as long as there is no CSSOM wrapper.
    569             // FIXME: Media control shadow trees seem to have problems with caching.
    570             bool isInlineStyleCacheable = !styledElement.inlineStyle()->isMutable() && !styledElement.isInShadowTree();
    571             // FIXME: Constify.
    572             addElementStyleProperties(styledElement.inlineStyle(), isInlineStyleCacheable);
    573         }
    574 
    575         // Now check SMIL animation override style.
    576         if (includeSMILProperties && is<SVGElement>(styledElement))
    577             addElementStyleProperties(downcast<SVGElement>(styledElement).animatedSMILStyleProperties(), false /* isCacheable */);
    578     }
     589    if (matchAuthorAndUserStyles) {
     590        clearMatchedRules();
     591
     592        collectMatchingAuthorRules(false);
     593        sortMatchedRules();
     594
     595        transferMatchedRules(Style::ScopeOrdinal::Element);
     596
     597        // Inline style behaves as if it has higher specificity than any rule.
     598        addElementInlineStyleProperties(includeSMILProperties);
     599
     600        // Rules from the host scope override inline style.
     601        transferMatchedRules(Style::ScopeOrdinal::ContainingHost);
     602    }
     603}
     604
     605void ElementRuleCollector::addElementInlineStyleProperties(bool includeSMILProperties)
     606{
     607    if (!is<StyledElement>(element()))
     608        return;
     609
     610    if (auto* inlineStyle = downcast<StyledElement>(element()).inlineStyle()) {
     611        // FIXME: Media control shadow trees seem to have problems with caching.
     612        bool isInlineStyleCacheable = !inlineStyle->isMutable() && !element().isInShadowTree();
     613        addElementStyleProperties(inlineStyle, isInlineStyleCacheable);
     614    }
     615
     616    if (includeSMILProperties && is<SVGElement>(element()))
     617        addElementStyleProperties(downcast<SVGElement>(element()).animatedSMILStyleProperties(), false /* isCacheable */);
    579618}
    580619
  • trunk/Source/WebCore/css/ElementRuleCollector.h

    r250817 r251285  
    5353    void matchUserRules(bool includeEmptyRules);
    5454
     55    bool matchesAnyAuthorRules();
     56
    5557    void setMode(SelectorChecker::Mode mode) { m_mode = mode; }
    5658    void setPseudoStyleRequest(const PseudoStyleRequest& request) { m_pseudoStyleRequest = request; }
     
    6264    const Vector<RefPtr<StyleRule>>& matchedRuleList() const;
    6365
    64     bool hasMatchedRules() const { return !m_matchedRules.isEmpty(); }
    6566    void clearMatchedRules();
    6667
     
    7374
    7475    void matchUARules(const RuleSet&);
     76
     77    void collectMatchingAuthorRules(bool includeEmptyRules);
     78    void addElementInlineStyleProperties(bool includeSMILProperties);
     79
    7580    void matchAuthorShadowPseudoElementRules(bool includeEmptyRules, StyleResolver::RuleRange&);
    7681    void matchHostPseudoClassRules(bool includeEmptyRules, StyleResolver::RuleRange&);
     
    8893    void sortMatchedRules();
    8994    void sortAndTransferMatchedRules();
     95    void transferMatchedRules(Optional<Style::ScopeOrdinal> forScope = { });
    9096
    9197    void addMatchedRule(const RuleData&, unsigned specificity, Style::ScopeOrdinal, StyleResolver::RuleRange&);
     
    108114
    109115    Vector<MatchedRule, 64> m_matchedRules;
     116    size_t m_matchedRuleTransferIndex { 0 };
    110117
    111118    // Output.
  • trunk/Source/WebCore/css/StyleProperties.h

    r245276 r251285  
    5151    // Override RefCounted's deref() to ensure operator delete is called on
    5252    // the appropriate subclass type.
    53     void deref();
     53    void deref() const;
    5454   
    5555    StylePropertiesType type() const { return static_cast<StylePropertiesType>(m_type); }
     
    306306}
    307307
    308 inline void StylePropertiesBase::deref()
     308inline void StylePropertiesBase::deref() const
    309309{
    310310    if (!derefBase())
  • trunk/Source/WebCore/css/StyleResolver.cpp

    r248966 r251285  
    23702370        Style::ScopeOrdinal ordinal;
    23712371    };
    2372     Vector<IndexAndOrdinal> shadowTreeMatches;
     2372    Vector<IndexAndOrdinal> importantMatches;
     2373    bool hasMatchesFromOtherScopes = false;
    23732374
    23742375    for (int i = startIndex; i <= endIndex; ++i) {
     
    23782379            continue;
    23792380
    2380         if (matchedProperties.styleScopeOrdinal != Style::ScopeOrdinal::Element) {
    2381             shadowTreeMatches.append({ i, matchedProperties.styleScopeOrdinal });
    2382             continue;
    2383         }
    2384 
    2385         addMatch(matchResult, i, true, inheritedOnly);
    2386     }
    2387 
    2388     if (shadowTreeMatches.isEmpty())
     2381        importantMatches.append({ i, matchedProperties.styleScopeOrdinal });
     2382
     2383        if (matchedProperties.styleScopeOrdinal != Style::ScopeOrdinal::Element)
     2384            hasMatchesFromOtherScopes = true;
     2385    }
     2386
     2387    if (importantMatches.isEmpty())
    23892388        return;
    23902389
    2391     // For !important properties a later shadow tree wins.
    2392     // Match results are sorted in reverse tree context order so this is not needed for normal properties.
    2393     std::stable_sort(shadowTreeMatches.begin(), shadowTreeMatches.end(), [] (const IndexAndOrdinal& a, const IndexAndOrdinal& b) {
    2394         return a.ordinal < b.ordinal;
    2395     });
    2396 
    2397     for (auto& match : shadowTreeMatches)
     2390    if (hasMatchesFromOtherScopes) {
     2391        // For !important properties a later shadow tree wins.
     2392        // Match results are sorted in reverse tree context order so this is not needed for normal properties.
     2393        std::stable_sort(importantMatches.begin(), importantMatches.end(), [] (const IndexAndOrdinal& a, const IndexAndOrdinal& b) {
     2394            return a.ordinal < b.ordinal;
     2395        });
     2396    }
     2397
     2398    for (auto& match : importantMatches)
    23982399        addMatch(matchResult, match.index, true, inheritedOnly);
    23992400}
  • trunk/Source/WebCore/style/StyleInvalidator.cpp

    r250861 r251285  
    116116        ElementRuleCollector ruleCollector(element, m_ruleSet, filter);
    117117        ruleCollector.setMode(SelectorChecker::Mode::CollectingRulesIgnoringVirtualPseudoElements);
    118         ruleCollector.matchAuthorRules(false);
    119 
    120         if (ruleCollector.hasMatchedRules())
     118
     119        if (ruleCollector.matchesAnyAuthorRules())
    121120            element.invalidateStyleInternal();
     121
    122122        return CheckDescendants::Yes;
    123123    }
  • trunk/Source/WebCore/style/StyleScope.h

    r239427 r251285  
    5656// Scopes are in tree-of-trees order. Styles from earlier scopes win over later ones (modulo !important).
    5757enum class ScopeOrdinal : int {
    58     ContainingHost = -1, // Author-exposed UA pseudo classes from the host tree scope.
     58    ContainingHost = -1, // ::part rules and author-exposed UA pseudo classes from the host tree scope.
    5959    Element = 0, // Normal rules in the same tree where the element is.
    6060    FirstSlot = 1, // ::slotted rules in the parent's shadow tree. Values greater than FirstSlot indicate subsequent slots in the chain.
Note: See TracChangeset for help on using the changeset viewer.