[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.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@251285 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
index af87797..45825f0 100644
--- a/LayoutTests/imported/w3c/ChangeLog
+++ b/LayoutTests/imported/w3c/ChangeLog
@@ -1,3 +1,12 @@
+2019-10-18 Antti Koivisto <antti@apple.com>
+
+ [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.
+
+ * web-platform-tests/css/css-shadow-parts/simple-inline-expected.txt:
+
2019-10-17 Wenson Hsieh <wenson_hsieh@apple.com>
[Clipboard API] Support navigator.clipboard.read()
diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-shadow-parts/simple-inline-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/css/css-shadow-parts/simple-inline-expected.txt
index f9704a7..c5710c7 100644
--- a/LayoutTests/imported/w3c/web-platform-tests/css/css-shadow-parts/simple-inline-expected.txt
+++ b/LayoutTests/imported/w3c/web-platform-tests/css/css-shadow-parts/simple-inline-expected.txt
@@ -1,4 +1,4 @@
The following text should be green:
-FAIL Part in selected host is styled assert_equals: expected "rgb(0, 128, 0)" but got "rgb(255, 0, 0)"
+PASS Part in selected host is styled
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index c548d34..7a091d1 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,66 @@
+2019-10-18 Antti Koivisto <antti@apple.com>
+
+ [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.
+
+ 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.
+
+] * css/ElementRuleCollector.h:
+ (WebCore::ElementRuleCollector::hasMatchedRules const): Deleted.
+ * css/StyleProperties.h:
+ (WebCore::StylePropertiesBase::deref const):
+ (WebCore::StylePropertiesBase::deref): Deleted.
+
+ Make const to allow RefPtr<const StyleProperties>
+
+ * css/StyleResolver.cpp:
+ (WebCore::StyleResolver::CascadedProperties::addImportantMatches):
+
+ Sort !important properties taking into account that the host scope has lower priority.
+
+ * style/StyleInvalidator.cpp:
+ (WebCore::Style::Invalidator::invalidateIfNeeded):
+
2019-10-18 Zalan Bujtas <zalan@apple.com>
[LFC][TFC] Include horizontal spacing when checking for the extra horizontal space
diff --git a/Source/WebCore/css/ElementRuleCollector.cpp b/Source/WebCore/css/ElementRuleCollector.cpp
index 8599f86..8a6c871 100644
--- a/Source/WebCore/css/ElementRuleCollector.cpp
+++ b/Source/WebCore/css/ElementRuleCollector.cpp
@@ -112,9 +112,12 @@
inline void ElementRuleCollector::addMatchedRule(const RuleData& ruleData, unsigned specificity, Style::ScopeOrdinal styleScopeOrdinal, StyleResolver::RuleRange& ruleRange)
{
// Update our first/last rule indices in the matched rules array.
- ++ruleRange.lastRuleIndex;
- if (ruleRange.firstRuleIndex == -1)
+ if (ruleRange.lastRuleIndex != -1)
+ ++ruleRange.lastRuleIndex;
+ else {
+ ruleRange.lastRuleIndex = m_result.matchedProperties().size();
ruleRange.firstRuleIndex = ruleRange.lastRuleIndex;
+ }
m_matchedRules.append({ &ruleData, specificity, styleScopeOrdinal });
}
@@ -123,15 +126,21 @@
{
m_matchedRules.clear();
m_keepAliveSlottedPseudoElementRules.clear();
+ m_matchedRuleTransferIndex = 0;
}
inline void ElementRuleCollector::addElementStyleProperties(const StyleProperties* propertySet, bool isCacheable)
{
if (!propertySet)
return;
- m_result.ranges.lastAuthorRule = m_result.matchedProperties().size();
- if (m_result.ranges.firstAuthorRule == -1)
+
+ if (m_result.ranges.lastAuthorRule != -1)
+ ++m_result.ranges.lastAuthorRule;
+ else {
+ m_result.ranges.lastAuthorRule = m_result.matchedProperties().size();
m_result.ranges.firstAuthorRule = m_result.ranges.lastAuthorRule;
+ }
+
m_result.addMatchedProperties(*propertySet);
if (!isCacheable)
m_result.isCacheable = false;
@@ -171,13 +180,21 @@
sortMatchedRules();
- if (m_mode == SelectorChecker::Mode::CollectingRules) {
- for (const MatchedRule& matchedRule : m_matchedRules)
- m_matchedRuleList.append(matchedRule.ruleData->rule());
- return;
- }
+ transferMatchedRules();
+}
- for (const MatchedRule& matchedRule : m_matchedRules) {
+void ElementRuleCollector::transferMatchedRules(Optional<Style::ScopeOrdinal> fromScope)
+{
+ for (; m_matchedRuleTransferIndex < m_matchedRules.size(); ++m_matchedRuleTransferIndex) {
+ auto& matchedRule = m_matchedRules[m_matchedRuleTransferIndex];
+ if (fromScope && matchedRule.styleScopeOrdinal < *fromScope)
+ break;
+
+ if (m_mode == SelectorChecker::Mode::CollectingRules) {
+ m_matchedRuleList.append(matchedRule.ruleData->rule());
+ continue;
+ }
+
m_result.addMatchedProperties(matchedRule.ruleData->rule()->properties(), matchedRule.ruleData->rule(), matchedRule.ruleData->linkMatchType(), matchedRule.ruleData->propertyWhitelistType(), matchedRule.styleScopeOrdinal);
}
}
@@ -186,7 +203,23 @@
{
clearMatchedRules();
- m_result.ranges.lastAuthorRule = m_result.matchedProperties().size() - 1;
+ collectMatchingAuthorRules(includeEmptyRules);
+
+ sortAndTransferMatchedRules();
+}
+
+bool ElementRuleCollector::matchesAnyAuthorRules()
+{
+ clearMatchedRules();
+
+ // FIXME: This should bail out on first match.
+ collectMatchingAuthorRules(false);
+
+ return !m_matchedRules.isEmpty();
+}
+
+void ElementRuleCollector::collectMatchingAuthorRules(bool includeEmptyRules)
+{
StyleResolver::RuleRange ruleRange = m_result.ranges.authorRuleRange();
{
@@ -205,8 +238,6 @@
matchAuthorShadowPseudoElementRules(includeEmptyRules, ruleRange);
matchPartPseudoElementRules(includeEmptyRules, ruleRange);
}
-
- sortAndTransferMatchedRules();
}
void ElementRuleCollector::matchAuthorShadowPseudoElementRules(bool includeEmptyRules, StyleResolver::RuleRange& ruleRange)
@@ -340,7 +371,6 @@
clearMatchedRules();
- m_result.ranges.lastUserRule = m_result.matchedProperties().size() - 1;
MatchRequest matchRequest(m_userStyle, includeEmptyRules);
StyleResolver::RuleRange ruleRange = m_result.ranges.userRuleRange();
collectMatchingRules(matchRequest, ruleRange);
@@ -369,7 +399,6 @@
{
clearMatchedRules();
- m_result.ranges.lastUARule = m_result.matchedProperties().size() - 1;
StyleResolver::RuleRange ruleRange = m_result.ranges.UARuleRange();
collectMatchingRules(MatchRequest(&rules), ruleRange);
@@ -557,27 +586,37 @@
}
}
- // Check the rules in author sheets next.
- if (matchAuthorAndUserStyles)
- matchAuthorRules(false);
+ if (matchAuthorAndUserStyles) {
+ clearMatchedRules();
- if (matchAuthorAndUserStyles && is<StyledElement>(element())) {
- auto& styledElement = downcast<StyledElement>(element());
- // Now check our inline style attribute.
- if (styledElement.inlineStyle()) {
- // Inline style is immutable as long as there is no CSSOM wrapper.
- // FIXME: Media control shadow trees seem to have problems with caching.
- bool isInlineStyleCacheable = !styledElement.inlineStyle()->isMutable() && !styledElement.isInShadowTree();
- // FIXME: Constify.
- addElementStyleProperties(styledElement.inlineStyle(), isInlineStyleCacheable);
- }
+ collectMatchingAuthorRules(false);
+ sortMatchedRules();
- // Now check SMIL animation override style.
- if (includeSMILProperties && is<SVGElement>(styledElement))
- addElementStyleProperties(downcast<SVGElement>(styledElement).animatedSMILStyleProperties(), false /* isCacheable */);
+ transferMatchedRules(Style::ScopeOrdinal::Element);
+
+ // Inline style behaves as if it has higher specificity than any rule.
+ addElementInlineStyleProperties(includeSMILProperties);
+
+ // Rules from the host scope override inline style.
+ transferMatchedRules(Style::ScopeOrdinal::ContainingHost);
}
}
+void ElementRuleCollector::addElementInlineStyleProperties(bool includeSMILProperties)
+{
+ if (!is<StyledElement>(element()))
+ return;
+
+ if (auto* inlineStyle = downcast<StyledElement>(element()).inlineStyle()) {
+ // FIXME: Media control shadow trees seem to have problems with caching.
+ bool isInlineStyleCacheable = !inlineStyle->isMutable() && !element().isInShadowTree();
+ addElementStyleProperties(inlineStyle, isInlineStyleCacheable);
+ }
+
+ if (includeSMILProperties && is<SVGElement>(element()))
+ addElementStyleProperties(downcast<SVGElement>(element()).animatedSMILStyleProperties(), false /* isCacheable */);
+}
+
bool ElementRuleCollector::hasAnyMatchingRules(const RuleSet* ruleSet)
{
clearMatchedRules();
diff --git a/Source/WebCore/css/ElementRuleCollector.h b/Source/WebCore/css/ElementRuleCollector.h
index 19829db..903fb89 100644
--- a/Source/WebCore/css/ElementRuleCollector.h
+++ b/Source/WebCore/css/ElementRuleCollector.h
@@ -52,6 +52,8 @@
void matchAuthorRules(bool includeEmptyRules);
void matchUserRules(bool includeEmptyRules);
+ bool matchesAnyAuthorRules();
+
void setMode(SelectorChecker::Mode mode) { m_mode = mode; }
void setPseudoStyleRequest(const PseudoStyleRequest& request) { m_pseudoStyleRequest = request; }
void setMedium(const MediaQueryEvaluator* medium) { m_isPrintStyle = medium->mediaTypeMatchSpecific("print"); }
@@ -61,7 +63,6 @@
StyleResolver::MatchResult& matchedResult();
const Vector<RefPtr<StyleRule>>& matchedRuleList() const;
- bool hasMatchedRules() const { return !m_matchedRules.isEmpty(); }
void clearMatchedRules();
const PseudoIdSet& matchedPseudoElementIds() const { return m_matchedPseudoElementIds; }
@@ -72,6 +73,10 @@
void addElementStyleProperties(const StyleProperties*, bool isCacheable = true);
void matchUARules(const RuleSet&);
+
+ void collectMatchingAuthorRules(bool includeEmptyRules);
+ void addElementInlineStyleProperties(bool includeSMILProperties);
+
void matchAuthorShadowPseudoElementRules(bool includeEmptyRules, StyleResolver::RuleRange&);
void matchHostPseudoClassRules(bool includeEmptyRules, StyleResolver::RuleRange&);
void matchSlottedPseudoElementRules(bool includeEmptyRules, StyleResolver::RuleRange&);
@@ -87,6 +92,7 @@
void sortMatchedRules();
void sortAndTransferMatchedRules();
+ void transferMatchedRules(Optional<Style::ScopeOrdinal> forScope = { });
void addMatchedRule(const RuleData&, unsigned specificity, Style::ScopeOrdinal, StyleResolver::RuleRange&);
@@ -107,6 +113,7 @@
Vector<std::unique_ptr<RuleSet::RuleDataVector>> m_keepAliveSlottedPseudoElementRules;
Vector<MatchedRule, 64> m_matchedRules;
+ size_t m_matchedRuleTransferIndex { 0 };
// Output.
Vector<RefPtr<StyleRule>> m_matchedRuleList;
diff --git a/Source/WebCore/css/StyleProperties.h b/Source/WebCore/css/StyleProperties.h
index 0ca7e613..3703671 100644
--- a/Source/WebCore/css/StyleProperties.h
+++ b/Source/WebCore/css/StyleProperties.h
@@ -50,7 +50,7 @@
public:
// Override RefCounted's deref() to ensure operator delete is called on
// the appropriate subclass type.
- void deref();
+ void deref() const;
StylePropertiesType type() const { return static_cast<StylePropertiesType>(m_type); }
@@ -305,7 +305,7 @@
return downcast<ImmutableStyleProperties>(*this).propertyCount();
}
-inline void StylePropertiesBase::deref()
+inline void StylePropertiesBase::deref() const
{
if (!derefBase())
return;
diff --git a/Source/WebCore/css/StyleResolver.cpp b/Source/WebCore/css/StyleResolver.cpp
index 20e7b2a..0c13c99 100644
--- a/Source/WebCore/css/StyleResolver.cpp
+++ b/Source/WebCore/css/StyleResolver.cpp
@@ -2369,7 +2369,8 @@
int index;
Style::ScopeOrdinal ordinal;
};
- Vector<IndexAndOrdinal> shadowTreeMatches;
+ Vector<IndexAndOrdinal> importantMatches;
+ bool hasMatchesFromOtherScopes = false;
for (int i = startIndex; i <= endIndex; ++i) {
const MatchedProperties& matchedProperties = matchResult.matchedProperties()[i];
@@ -2377,24 +2378,24 @@
if (!hasImportantProperties(*matchedProperties.properties))
continue;
- if (matchedProperties.styleScopeOrdinal != Style::ScopeOrdinal::Element) {
- shadowTreeMatches.append({ i, matchedProperties.styleScopeOrdinal });
- continue;
- }
+ importantMatches.append({ i, matchedProperties.styleScopeOrdinal });
- addMatch(matchResult, i, true, inheritedOnly);
+ if (matchedProperties.styleScopeOrdinal != Style::ScopeOrdinal::Element)
+ hasMatchesFromOtherScopes = true;
}
- if (shadowTreeMatches.isEmpty())
+ if (importantMatches.isEmpty())
return;
- // For !important properties a later shadow tree wins.
- // Match results are sorted in reverse tree context order so this is not needed for normal properties.
- std::stable_sort(shadowTreeMatches.begin(), shadowTreeMatches.end(), [] (const IndexAndOrdinal& a, const IndexAndOrdinal& b) {
- return a.ordinal < b.ordinal;
- });
+ if (hasMatchesFromOtherScopes) {
+ // For !important properties a later shadow tree wins.
+ // Match results are sorted in reverse tree context order so this is not needed for normal properties.
+ std::stable_sort(importantMatches.begin(), importantMatches.end(), [] (const IndexAndOrdinal& a, const IndexAndOrdinal& b) {
+ return a.ordinal < b.ordinal;
+ });
+ }
- for (auto& match : shadowTreeMatches)
+ for (auto& match : importantMatches)
addMatch(matchResult, match.index, true, inheritedOnly);
}
diff --git a/Source/WebCore/style/StyleInvalidator.cpp b/Source/WebCore/style/StyleInvalidator.cpp
index 1f23ebf..5842921 100644
--- a/Source/WebCore/style/StyleInvalidator.cpp
+++ b/Source/WebCore/style/StyleInvalidator.cpp
@@ -115,10 +115,10 @@
case Style::Validity::Valid: {
ElementRuleCollector ruleCollector(element, m_ruleSet, filter);
ruleCollector.setMode(SelectorChecker::Mode::CollectingRulesIgnoringVirtualPseudoElements);
- ruleCollector.matchAuthorRules(false);
- if (ruleCollector.hasMatchedRules())
+ if (ruleCollector.matchesAnyAuthorRules())
element.invalidateStyleInternal();
+
return CheckDescendants::Yes;
}
case Style::Validity::ElementInvalid:
diff --git a/Source/WebCore/style/StyleScope.h b/Source/WebCore/style/StyleScope.h
index 7f55005..a702f73 100644
--- a/Source/WebCore/style/StyleScope.h
+++ b/Source/WebCore/style/StyleScope.h
@@ -55,7 +55,7 @@
// This is used to identify style scopes that can affect an element.
// Scopes are in tree-of-trees order. Styles from earlier scopes win over later ones (modulo !important).
enum class ScopeOrdinal : int {
- ContainingHost = -1, // Author-exposed UA pseudo classes from the host tree scope.
+ ContainingHost = -1, // ::part rules and author-exposed UA pseudo classes from the host tree scope.
Element = 0, // Normal rules in the same tree where the element is.
FirstSlot = 1, // ::slotted rules in the parent's shadow tree. Values greater than FirstSlot indicate subsequent slots in the chain.
Shadow = std::numeric_limits<int>::max(), // :host rules in element's own shadow tree.
