Fix ':has()' matching and invalidation error with nesting parent
1. Fix incorrect cache hit for ':has()' selector matching:
In case of having multiple style rules with same ':has()' selector
expression but different nesting parent, the ':has()' selector
matching cache (CheckPseudoCacheScope) returned incorrect cache hit
result because it used ':has()' argument selector text as the cache
key for the ':has()' match result.
To fix this bug, this CL added a 'CSSSelector' method that returns
selector text with replacing pseudo parent expressions ('&') with
equivalent pseudo ':is' expression (':is(<nesting parent>)'), and let
the cache uses the replaced selector string as its cache key.
2. Fix ':has()' invalidation error with complex nesting parent:
To build invalidation set for a ':has()' that contains a logical
combinations containing a complex selector, 'RuleFeatureSet' checks
a flag in the ':has()' selector:
- ContainsComplexLogicalCombinationsInsideHasPseudoClass
'CSSSelectorParser' need to set the flag while parsing ':has()'
argument selector, but the flag was not set in case that the ':has()'
contains complex nesting parent.
This CL sets the flag when a ':has()' contains nesting parent selector
so that 'RuleFeatureSet' successfully build invalidation sets for the
complex nesting parent inside ':has()'.
Low-Coverage-Reason: COVERAGE_UNDERREPORTED - The unit tests and wpt tests in this CL cover the changes, and most of the lines detected in css_selector.cc are existing code that are not directly related to this change and should have been tested previously.
Bug: 350946979
Change-Id: Ie3b457df2ac66e0e5884beb0e29f3877b436e2a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5680906
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Byungwoo Lee <blee@igalia.com>
Cr-Commit-Position: refs/heads/main@{#1335644}
diff --git a/third_party/blink/renderer/core/css/affected_by_pseudo_test.cc b/third_party/blink/renderer/core/css/affected_by_pseudo_test.cc
index c94caa8..b2650e8 100644
--- a/third_party/blink/renderer/core/css/affected_by_pseudo_test.cc
+++ b/third_party/blink/renderer/core/css/affected_by_pseudo_test.cc
@@ -5456,4 +5456,70 @@
UpdateAllLifecyclePhasesForTest();
}
+TEST_F(AffectedByPseudoTest, AffectedByPseudoInHasWithNestingComplexParent) {
+ SetHtmlInnerHTML(R"HTML(
+ <style>
+ .b .c {
+ .a:has(> &) { background-color: green; }
+ }
+ </style>
+ <div id=div1></div>
+ <div id=div2>
+ <div id=div3></div>
+ <div id=div4 class='a'>
+ <div id=div5 class='c'></div>
+ </div>
+ </div>
+ )HTML");
+
+ UpdateAllLifecyclePhasesForTest();
+ CheckAffectedByFlagsForHas(
+ "div1", {{kAffectedBySubjectHas, false},
+ {kAffectedByPseudoInHas, false},
+ {kAncestorsOrAncestorSiblingsAffectedByHas, false},
+ {kAffectedByLogicalCombinationsInHas, false}});
+ CheckAffectedByFlagsForHas(
+ "div2", {{kAffectedBySubjectHas, false},
+ {kAffectedByPseudoInHas, false},
+ {kAncestorsOrAncestorSiblingsAffectedByHas, false},
+ {kAffectedByLogicalCombinationsInHas, false}});
+ CheckAffectedByFlagsForHas(
+ "div3", {{kAffectedBySubjectHas, false},
+ {kAffectedByPseudoInHas, false},
+ {kAncestorsOrAncestorSiblingsAffectedByHas, false},
+ {kAffectedByLogicalCombinationsInHas, false}});
+ CheckAffectedByFlagsForHas("div4",
+ {{kAffectedBySubjectHas, true},
+ {kAffectedByPseudoInHas, true},
+ {kAncestorsOrAncestorSiblingsAffectedByHas, true},
+ {kAffectedByLogicalCombinationsInHas, true}});
+ CheckAffectedByFlagsForHas("div5",
+ {{kAffectedBySubjectHas, false},
+ {kAffectedByPseudoInHas, false},
+ {kAncestorsOrAncestorSiblingsAffectedByHas, true},
+ {kAffectedByLogicalCombinationsInHas, false}});
+
+ unsigned start_count = GetStyleEngine().StyleForElementCount();
+ GetElementById("div1")->setAttribute(html_names::kClassAttr,
+ AtomicString("b"));
+ UpdateAllLifecyclePhasesForTest();
+ unsigned element_count =
+ GetStyleEngine().StyleForElementCount() - start_count;
+ ASSERT_EQ(0U, element_count);
+
+ start_count = GetStyleEngine().StyleForElementCount();
+ GetElementById("div3")->setAttribute(html_names::kClassAttr,
+ AtomicString("b"));
+ UpdateAllLifecyclePhasesForTest();
+ element_count = GetStyleEngine().StyleForElementCount() - start_count;
+ ASSERT_EQ(0U, element_count);
+
+ start_count = GetStyleEngine().StyleForElementCount();
+ GetElementById("div2")->setAttribute(html_names::kClassAttr,
+ AtomicString("b"));
+ UpdateAllLifecyclePhasesForTest();
+ element_count = GetStyleEngine().StyleForElementCount() - start_count;
+ ASSERT_EQ(2U, element_count);
+}
+
} // namespace blink
diff --git a/third_party/blink/renderer/core/css/check_pseudo_has_cache_scope.cc b/third_party/blink/renderer/core/css/check_pseudo_has_cache_scope.cc
index 4a630c3..01e5fbd 100644
--- a/third_party/blink/renderer/core/css/check_pseudo_has_cache_scope.cc
+++ b/third_party/blink/renderer/core/css/check_pseudo_has_cache_scope.cc
@@ -48,7 +48,7 @@
// To increase the cache hit ratio, we need to have a same cache key
// for multiple selector instances those are actually has a same selector.
// TODO(blee@igalia.com) Find a way to get hash key without serialization.
- String selector_text = selector->SelectorText();
+ String selector_text = selector->SelectorTextExpandingPseudoParent();
DCHECK(document);
DCHECK(document->GetCheckPseudoHasCacheScope());
diff --git a/third_party/blink/renderer/core/css/css_selector.cc b/third_party/blink/renderer/core/css/css_selector.cc
index ae55bd1..f174c00 100644
--- a/third_party/blink/renderer/core/css/css_selector.cc
+++ b/third_party/blink/renderer/core/css/css_selector.cc
@@ -60,6 +60,8 @@
namespace {
+constexpr bool kExpandPseudoParent = true;
+
unsigned MaximumSpecificity(const CSSSelectorList* list) {
if (!list) {
return 0;
@@ -1030,18 +1032,29 @@
builder.Append('|');
}
-static void SerializeSelectorList(const CSSSelectorList* selector_list,
- StringBuilder& builder) {
+// static
+template <bool expand_pseudo_parent>
+void CSSSelector::SerializeSelectorList(const CSSSelectorList* selector_list,
+ StringBuilder& builder) {
const CSSSelector* first_sub_selector = selector_list->First();
for (const CSSSelector* sub_selector = first_sub_selector; sub_selector;
sub_selector = CSSSelectorList::Next(*sub_selector)) {
if (sub_selector != first_sub_selector) {
builder.Append(", ");
}
- builder.Append(sub_selector->SelectorText());
+ builder.Append(sub_selector->SelectorTextInternal<expand_pseudo_parent>());
}
}
+String CSSSelector::SelectorText() const {
+ return SelectorTextInternal<!kExpandPseudoParent>();
+}
+
+String CSSSelector::SelectorTextExpandingPseudoParent() const {
+ return SelectorTextInternal<kExpandPseudoParent>();
+}
+
+template <bool expand_pseudo_parent>
bool CSSSelector::SerializeSimpleSelector(StringBuilder& builder) const {
bool suppress_selector_list = false;
if (Match() == kId) {
@@ -1091,7 +1104,8 @@
// Only relevant for :nth-child, not :nth-of-type.
if (data_.rare_data_->selector_list_ != nullptr) {
builder.Append(" of ");
- SerializeSelectorList(data_.rare_data_->selector_list_, builder);
+ SerializeSelectorList<expand_pseudo_parent>(
+ data_.rare_data_->selector_list_, builder);
suppress_selector_list = true;
}
@@ -1120,7 +1134,17 @@
case kPseudoWhere:
break;
case kPseudoParent:
- builder.Append('&');
+ if constexpr (expand_pseudo_parent) {
+ // Replace parent pseudo with equivalent :is() pseudo.
+ builder.Append(":is");
+ if (auto* parent = SelectorListOrParent()) {
+ builder.Append('(');
+ builder.Append(parent->SelectorTextExpandingPseudoParent());
+ builder.Append(')');
+ }
+ } else {
+ builder.Append('&');
+ }
break;
case kPseudoRelativeAnchor:
NOTREACHED_IN_MIGRATION();
@@ -1232,12 +1256,13 @@
if (SelectorList() && !suppress_selector_list) {
builder.Append('(');
- SerializeSelectorList(SelectorList(), builder);
+ SerializeSelectorList<expand_pseudo_parent>(SelectorList(), builder);
builder.Append(')');
}
return true;
}
+template <bool expand_pseudo_parent>
const CSSSelector* CSSSelector::SerializeCompound(
StringBuilder& builder) const {
if (Match() == kTag && !IsImplicit()) {
@@ -1249,7 +1274,8 @@
for (const CSSSelector* simple_selector = this; simple_selector;
simple_selector = simple_selector->NextSimpleSelector()) {
- if (!simple_selector->SerializeSimpleSelector(builder)) {
+ if (!simple_selector->SerializeSimpleSelector<expand_pseudo_parent>(
+ builder)) {
return nullptr;
}
if (simple_selector->Relation() != kSubSelector &&
@@ -1260,12 +1286,13 @@
return nullptr;
}
-String CSSSelector::SelectorText() const {
+template <bool expand_pseudo_parent>
+String CSSSelector::SelectorTextInternal() const {
String result;
for (const CSSSelector* compound = this; compound;
compound = compound->NextSimpleSelector()) {
StringBuilder builder;
- compound = compound->SerializeCompound(builder);
+ compound = compound->SerializeCompound<expand_pseudo_parent>(builder);
if (!compound) {
return builder.ReleaseString() + result;
}
@@ -1335,7 +1362,7 @@
SerializeIdentifierOrAny(TagQName().LocalName(), UniversalSelectorAtom(),
builder);
} else {
- SerializeSimpleSelector(builder);
+ SerializeSimpleSelector<!kExpandPseudoParent>(builder);
}
return builder.ToString();
}
diff --git a/third_party/blink/renderer/core/css/css_selector.h b/third_party/blink/renderer/core/css/css_selector.h
index ab3eb7b..a86023ce 100644
--- a/third_party/blink/renderer/core/css/css_selector.h
+++ b/third_party/blink/renderer/core/css/css_selector.h
@@ -156,6 +156,11 @@
~CSSSelector();
String SelectorText() const;
+ // Like `SelectorText`, but replaces any '&' selectors
+ // with ':is(<parent rule selector list>)'. This is needed
+ // by the :has() cache, because it uses the serialization of
+ // the selector as a key.
+ String SelectorTextExpandingPseudoParent() const;
String SimpleSelectorTextForDebug() const;
CSSSelector& operator=(const CSSSelector&) = delete;
@@ -712,9 +717,20 @@
unsigned SpecificityForOneSelector() const;
unsigned SpecificityForPage() const;
+
+ template <bool expand_pseudo_parent>
bool SerializeSimpleSelector(WTF::StringBuilder& builder) const;
+
+ template <bool expand_pseudo_parent>
const CSSSelector* SerializeCompound(WTF::StringBuilder&) const;
+ template <bool expand_pseudo_parent>
+ static void SerializeSelectorList(const CSSSelectorList* selector_list,
+ WTF::StringBuilder& builder);
+
+ template <bool expand_pseudo_parent>
+ String SelectorTextInternal() const;
+
struct RareData : public GarbageCollected<RareData> {
explicit RareData(const AtomicString& value);
~RareData();
diff --git a/third_party/blink/renderer/core/css/css_selector_test.cc b/third_party/blink/renderer/core/css/css_selector_test.cc
index ba67a7b..5fef426 100644
--- a/third_party/blink/renderer/core/css/css_selector_test.cc
+++ b/third_party/blink/renderer/core/css/css_selector_test.cc
@@ -347,4 +347,51 @@
selector[0].Specificity());
}
+TEST(CSSSelector, CheckSelectorTextExpandingPseudoParent) {
+ test::TaskEnvironment task_environment;
+
+ css_test_helpers::TestStyleSheet sheet;
+ sheet.AddCSSRules(
+ ".a { .b { .c, &.c, .c:has(&) {} } }"
+ ".d .e { .f:has(> &) {} }");
+ RuleSet& rule_set = sheet.GetRuleSet();
+
+ base::span<const RuleData> rules = rule_set.ClassRules(AtomicString("a"));
+ ASSERT_EQ(1u, rules.size());
+ const CSSSelector* selector = &rules[0].Selector();
+ EXPECT_EQ(".a", selector->SelectorText());
+
+ rules = rule_set.ClassRules(AtomicString("b"));
+ ASSERT_EQ(1u, rules.size());
+ selector = &rules[0].Selector();
+ EXPECT_EQ("& .b", selector->SelectorText());
+ EXPECT_EQ(":is(.a) .b", selector->SelectorTextExpandingPseudoParent());
+
+ rules = rule_set.ClassRules(AtomicString("c"));
+ ASSERT_EQ(3u, rules.size());
+ selector = &rules[0].Selector();
+ EXPECT_EQ("& .c", selector->SelectorText());
+ EXPECT_EQ(":is(:is(.a) .b) .c",
+ selector->SelectorTextExpandingPseudoParent());
+ selector = &rules[1].Selector();
+ EXPECT_EQ("&.c", selector->SelectorText());
+ EXPECT_EQ(":is(:is(.a) .b).c", selector->SelectorTextExpandingPseudoParent());
+ selector = &rules[2].Selector();
+ EXPECT_EQ(".c:has(&)", selector->SelectorText());
+ EXPECT_EQ(".c:has(:is(:is(.a) .b))",
+ selector->SelectorTextExpandingPseudoParent());
+
+ rules = rule_set.ClassRules(AtomicString("e"));
+ ASSERT_EQ(1u, rules.size());
+ selector = &rules[0].Selector();
+ EXPECT_EQ(".d .e", selector->SelectorText());
+
+ rules = rule_set.ClassRules(AtomicString("f"));
+ ASSERT_EQ(1u, rules.size());
+ selector = &rules[0].Selector();
+ EXPECT_EQ(".f:has(> &)", selector->SelectorText());
+ EXPECT_EQ(".f:has(> :is(.d .e))",
+ selector->SelectorTextExpandingPseudoParent());
+}
+
} // namespace blink
diff --git a/third_party/blink/renderer/core/css/parser/css_selector_parser.cc b/third_party/blink/renderer/core/css/parser/css_selector_parser.cc
index 2e255a8a..42e2f252e 100644
--- a/third_party/blink/renderer/core/css/parser/css_selector_parser.cc
+++ b/third_party/blink/renderer/core/css/parser/css_selector_parser.cc
@@ -1916,13 +1916,15 @@
if (is_inside_has_argument_) {
// In case that a nesting parent selector is inside a :has() pseudo class,
- // mark the :has() containing a pseudo selector so that the StyleEngine can
- // invalidate the anchor element of the :has() for a pseudo state change
- // in the parent selector. (crbug.com/1517866)
- // This ignores whether the nesting parent actually contains a pseudo to
- // avoid nesting parent lookup overhead and the complexity caused by
- // reparenting style rules.
+ // mark the :has() containing a pseudo selector and a complex selector
+ // so that the StyleEngine can invalidate the anchor element of the :has()
+ // for a pseudo state change (crbug.com/1517866) or a complex selector
+ // state change (crbug.com/350946979) in the parent selector.
+ // These ignore whether the nesting parent actually contains a pseudo or
+ // complex selector to avoid nesting parent lookup overhead and the
+ // complexity caused by reparenting style rules.
found_pseudo_in_has_argument_ = true;
+ found_complex_logical_combinations_in_has_argument_ = true;
}
return true;
diff --git a/third_party/blink/web_tests/external/wpt/css/selectors/invalidation/has-with-nesting-parent-containing-complex.html b/third_party/blink/web_tests/external/wpt/css/selectors/invalidation/has-with-nesting-parent-containing-complex.html
new file mode 100644
index 0000000..4ed6111
--- /dev/null
+++ b/third_party/blink/web_tests/external/wpt/css/selectors/invalidation/has-with-nesting-parent-containing-complex.html
@@ -0,0 +1,158 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>CSS Selector Invalidation: :has() with nesting parent containing complex selector</title>
+<link rel="help" href="https://drafts.csswg.org/selectors/#relational">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/resources/testdriver.js"></script>
+<script src="/resources/testdriver-actions.js"></script>
+<script src="/resources/testdriver-vendor.js"></script>
+<style>
+ .anchor { background-color: white; }
+
+ .ancestor .descendant {
+ .anchor:has(&) { background-color: blue; }
+ }
+
+ .ancestor .child {
+ .anchor:has(> &) { background-color: lightblue; }
+ }
+
+ .ancestor_prev ~ div .descendant {
+ .anchor:has(&) { background-color: yellow; }
+ }
+
+ .ancestor_prev ~ div.ancestor .descendant {
+ .anchor:has(&) { background-color: yellowgreen; }
+ }
+
+ .prev ~ .indirect_next {
+ .anchor:has(~ &) { background-color: green; }
+ }
+
+ .prev ~ .direct_next {
+ .anchor:has(+ &) { background-color: lightgreen; }
+ }
+</style>
+<div><div id="grand_parent1">
+ <div id="parent1">
+ <div id="anchor1" class="anchor">
+ <div><div class="descendant"></div></div>
+ </div>
+ </div>
+</div></div>
+<div><div id="grand_parent2">
+ <div id="parent2">
+ <div id="anchor2" class="anchor">
+ <div class="child"></div>
+ </div>
+ </div>
+</div></div>
+<div><div id="grand_parent_indirect_prev3"></div>
+ <div id="grand_parent_direct_prev3"></div>
+ <div id="grand_parent3">
+ <div id="parent_indirect_prev3"></div>
+ <div id="parent_direct_prev3"></div>
+ <div id="parent3">
+ <div id="anchor_indirect_prev3"></div>
+ <div id="anchor_direct_prev3"></div>
+ <div id="anchor3" class="anchor">
+ <div><div class="descendant"></div></div>
+ </div>
+ </div>
+</div></div>
+<div><div id="indirect_prev4"></div>
+ <div id="direct_prev4"></div>
+ <div id="anchor4" class="anchor"></div>
+ <div></div><div class="indirect_next">
+</div></div>
+<div><div id="indirect_prev5"></div>
+ <div id="direct_prev5"></div>
+ <div id="anchor5" class="anchor"></div>
+ <div class="direct_next">
+</div></div>
+<script>
+ const white = "rgb(255, 255, 255)";
+ const blue = "rgb(0, 0, 255)";
+ const lightblue = "rgb(173, 216, 230)";
+ const yellow = "rgb(255, 255, 0)";
+ const yellowgreen = "rgb(154, 205, 50)";
+ const green = "rgb(0, 128, 0)";
+ const lightgreen = "rgb(144, 238, 144)";
+
+ function bg_color(element, color, message) {
+ promise_test(async () => {
+ assert_equals(getComputedStyle(element)['background-color'], color);
+ }, message);
+ }
+
+ function add_class_and_check_bg_color(
+ element_to_add, class_name, has_anchor, color) {
+ promise_test(async () => {
+ element_to_add.classList.add(class_name);
+ assert_equals(getComputedStyle(has_anchor)['background-color'], color);
+ }, `#${has_anchor.id} becomes ${color} after adding .${class_name} to #${element_to_add.id}`);
+ }
+
+ function remove_class_and_check_bg_color(
+ element_to_remove, class_name, has_anchor, color) {
+ promise_test(async () => {
+ element_to_remove.classList.remove(class_name);
+ assert_equals(getComputedStyle(has_anchor)['background-color'], color);
+ }, `#${has_anchor.id} becomes ${color} after removing .${class_name} from #${element_to_remove.id}`);
+ }
+
+ bg_color(anchor1, white, "#anchor1 initially white");
+ add_class_and_check_bg_color(grand_parent1, "ancestor", anchor1, blue);
+ remove_class_and_check_bg_color(grand_parent1, "ancestor", anchor1, white);
+ add_class_and_check_bg_color(parent1, "ancestor", anchor1, blue);
+ remove_class_and_check_bg_color(parent1, "ancestor", anchor1, white);
+
+ bg_color(anchor2, white, "#anchor2 initially white");
+ add_class_and_check_bg_color(grand_parent2, "ancestor", anchor2, lightblue);
+ remove_class_and_check_bg_color(grand_parent2, "ancestor", anchor2, white);
+ add_class_and_check_bg_color(parent2, "ancestor", anchor2, lightblue);
+ remove_class_and_check_bg_color(parent2, "ancestor", anchor2, white);
+
+ bg_color(anchor3, white, "#anchor3 initially white");
+ add_class_and_check_bg_color(grand_parent_indirect_prev3, "ancestor_prev",
+ anchor3, yellow);
+ add_class_and_check_bg_color(grand_parent3, "ancestor", anchor3, yellowgreen);
+ remove_class_and_check_bg_color(grand_parent3, "ancestor", anchor3, yellow);
+ remove_class_and_check_bg_color(grand_parent_indirect_prev3, "ancestor_prev",
+ anchor3, white);
+ add_class_and_check_bg_color(grand_parent_direct_prev3, "ancestor_prev",
+ anchor3, yellow);
+ remove_class_and_check_bg_color(grand_parent_direct_prev3, "ancestor_prev",
+ anchor3, white);
+ add_class_and_check_bg_color(parent_indirect_prev3, "ancestor_prev",
+ anchor3, yellow);
+ add_class_and_check_bg_color(parent3, "ancestor", anchor3, yellowgreen);
+ remove_class_and_check_bg_color(parent3, "ancestor", anchor3, yellow);
+ remove_class_and_check_bg_color(parent_indirect_prev3, "ancestor_prev",
+ anchor3, white);
+ add_class_and_check_bg_color(parent_direct_prev3, "ancestor_prev",
+ anchor3, yellow);
+ remove_class_and_check_bg_color(parent_direct_prev3, "ancestor_prev",
+ anchor3, white);
+ add_class_and_check_bg_color(anchor_indirect_prev3, "ancestor_prev",
+ anchor3, yellow);
+ remove_class_and_check_bg_color(anchor_indirect_prev3, "ancestor_prev",
+ anchor3, white);
+ add_class_and_check_bg_color(anchor_direct_prev3, "ancestor_prev",
+ anchor3, yellow);
+ remove_class_and_check_bg_color(anchor_direct_prev3, "ancestor_prev",
+ anchor3, white);
+
+ bg_color(anchor4, white, "#anchor4 initially white");
+ add_class_and_check_bg_color(indirect_prev4, "prev", anchor4, green);
+ remove_class_and_check_bg_color(indirect_prev4, "prev", anchor4, white);
+ add_class_and_check_bg_color(direct_prev4, "prev", anchor4, green);
+ remove_class_and_check_bg_color(direct_prev4, "prev", anchor4, white);
+
+ bg_color(anchor5, white, "#anchor5 initially white");
+ add_class_and_check_bg_color(indirect_prev5, "prev", anchor5, lightgreen);
+ remove_class_and_check_bg_color(indirect_prev5, "prev", anchor5, white);
+ add_class_and_check_bg_color(direct_prev5, "prev", anchor5, lightgreen);
+ remove_class_and_check_bg_color(direct_prev5, "prev", anchor5, white);
+</script>
\ No newline at end of file
