Perform kNotInsideLink-related adjustments in ElementRuleCollector

The link_match_type stored on the MatchedProperties object is only
relevant and correct if we're inside a link, hence we need to override
the effective link_match_type at some point.

Previously, we would do this apply-time, and not as part of the rule-
collection. This CL instead proposes that ElementRuleCollector take
the link-status into account, and perform the adjustment before storing
the link_match_type on MatchedProperties, such that no further
adjustments are required later.

This fixes a bug in the CSSCascade path, where we would incorrectly not
apply anything for selectors such as *:not(:link):not(:visited), even
for elements that are not inside links. (See AddLinkFilter in
cascade_expansion.cc. We would switch on the value 0, and then enter
the default case, causing a filter to be added on kProperty, which
causes all properties to be rejected. This is correct for elements
that are inside links, but not for elements that aren't).

Bug: 1055715
Change-Id: I2b63de41753fb0fa7b637a4be5bd009c2f165fcf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2072223Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#744631}

third_party/blink/renderer/core/css/element_rule_collector.cc

@@ -48,9 +48,21 @@
 
 namespace blink {
 
+namespace {
+
+unsigned AdjustLinkMatchType(EInsideLink inside_link,
+                             unsigned link_match_type) {
+  if (inside_link == EInsideLink::kNotInsideLink)
+    return CSSSelector::kMatchLink;
+  return link_match_type;
+}
+
+}  // namespace
+
 ElementRuleCollector::ElementRuleCollector(const ElementResolveContext& context,
                                            const SelectorFilter& filter,
-                                           ComputedStyle* style)
+                                           ComputedStyle* style,
+                                           EInsideLink inside_link)
     : context_(context),
       selector_filter_(filter),
       style_(style),
@@ -60,7 +72,8 @@ ElementRuleCollector::ElementRuleCollector(const ElementResolveContext& context,
           selector_filter_.ParentStackIsConsistent(context.ParentNode())),
       same_origin_only_(false),
       matching_ua_rules_(false),
-      include_empty_rules_(false) {}
+      include_empty_rules_(false),
+      inside_link_(inside_link) {}
 
 ElementRuleCollector::~ElementRuleCollector() = default;
 
@@ -99,7 +112,9 @@ void ElementRuleCollector::AddElementStyleProperties(
     bool is_cacheable) {
   if (!property_set)
     return;
-  result_.AddMatchedProperties(property_set);
+  auto link_match_type = static_cast<unsigned>(CSSSelector::kMatchAll);
+  result_.AddMatchedProperties(
+      property_set, AdjustLinkMatchType(inside_link_, link_match_type));
   if (!is_cacheable)
     result_.SetIsCacheable(false);
 }
@@ -326,7 +341,8 @@ void ElementRuleCollector::SortAndTransferMatchedRules() {
   for (unsigned i = 0; i < matched_rules_.size(); i++) {
     const RuleData* rule_data = matched_rules_[i].GetRuleData();
     result_.AddMatchedProperties(
-        &rule_data->Rule()->Properties(), rule_data->LinkMatchType(),
+        &rule_data->Rule()->Properties(),
+        AdjustLinkMatchType(inside_link_, rule_data->LinkMatchType()),
         rule_data->GetValidPropertyFilter(matching_ua_rules_));
   }
 }

third_party/blink/renderer/core/css/element_rule_collector.h

@@ -31,6 +31,7 @@
 #include "third_party/blink/renderer/core/css/resolver/match_request.h"
 #include "third_party/blink/renderer/core/css/resolver/match_result.h"
 #include "third_party/blink/renderer/core/css/selector_checker.h"
+#include "third_party/blink/renderer/core/style/computed_style_base_constants.h"
 #include "third_party/blink/renderer/platform/wtf/ref_counted.h"
 #include "third_party/blink/renderer/platform/wtf/vector.h"
 
@@ -109,7 +110,8 @@ class ElementRuleCollector {
  public:
   ElementRuleCollector(const ElementResolveContext&,
                        const SelectorFilter&,
-                       ComputedStyle* = nullptr);
+                       ComputedStyle*,
+                       EInsideLink);
   ~ElementRuleCollector();
 
   void SetMode(SelectorChecker::Mode mode) { mode_ = mode; }
@@ -188,6 +190,7 @@ class ElementRuleCollector {
   bool same_origin_only_;
   bool matching_ua_rules_;
   bool include_empty_rules_;
+  EInsideLink inside_link_;
 
   HeapVector<MatchedRule, 32> matched_rules_;
 

third_party/blink/renderer/core/css/resolver/style_resolver.cc

@@ -864,7 +864,7 @@ void StyleResolver::ApplyBaseComputedStyle(
     GetDocument().GetStyleEngine().EnsureUAStyleForElement(*element);
 
     ElementRuleCollector collector(state.ElementContext(), selector_filter_,
-                                   state.Style());
+                                   state.Style(), state.Style()->InsideLink());
 
     MatchAllRules(state, collector,
                   matching_behavior != kMatchAllRulesExcludingSMIL);
@@ -989,7 +989,7 @@ bool StyleResolver::PseudoStyleForElementInternal(
   if (!animation_base_computed_style) {
     // Check UA, user and author rules.
     ElementRuleCollector collector(state.ElementContext(), selector_filter_,
-                                   state.Style());
+                                   state.Style(), state.Style()->InsideLink());
     collector.SetPseudoElementStyleRequest(pseudo_style_request);
 
     // The UA sheet is supposed to set some styles to ::marker pseudo-elements,
@@ -1198,7 +1198,7 @@ StyleRuleList* StyleResolver::StyleRulesForElement(Element* element,
   DCHECK(element);
   StyleResolverState state(GetDocument(), *element);
   ElementRuleCollector collector(state.ElementContext(), selector_filter_,
-                                 state.Style());
+                                 nullptr, EInsideLink::kNotInsideLink);
   collector.SetMode(SelectorChecker::kCollectingStyleRules);
   CollectPseudoRulesForElement(*element, collector, kPseudoIdNone,
                                rules_to_include);
@@ -1212,7 +1212,7 @@ RuleIndexList* StyleResolver::PseudoCSSRulesForElement(
   DCHECK(element);
   StyleResolverState state(GetDocument(), *element);
   ElementRuleCollector collector(state.ElementContext(), selector_filter_,
-                                 state.Style());
+                                 nullptr, EInsideLink::kNotInsideLink);
   collector.SetMode(SelectorChecker::kCollectingCSSRules);
   CollectPseudoRulesForElement(*element, collector, pseudo_id,
                                rules_to_include);
@@ -1543,20 +1543,6 @@ void StyleResolver::ApplyProperties(StyleResolverState& state,
   }
 }
 
-static inline unsigned ComputeApplyMask(
-    StyleResolverState& state,
-    const MatchedProperties& matched_properties) {
-  if (state.Style()->InsideLink() == EInsideLink::kNotInsideLink)
-    return kApplyMaskRegular;
-  static_assert(static_cast<int>(kApplyMaskRegular) ==
-                    static_cast<int>(CSSSelector::kMatchLink),
-                "kApplyMaskRegular and kMatchLink must match");
-  static_assert(static_cast<int>(kApplyMaskVisited) ==
-                    static_cast<int>(CSSSelector::kMatchVisited),
-                "kApplyMaskVisited and kMatchVisited must match");
-  return matched_properties.types_.link_match_type;
-}
-
 template <CSSPropertyPriority priority,
           StyleResolver::ShouldUpdateNeedsApplyPass shouldUpdateNeedsApplyPass>
 void StyleResolver::ApplyMatchedProperties(StyleResolverState& state,
@@ -1575,7 +1561,13 @@ void StyleResolver::ApplyMatchedProperties(StyleResolverState& state,
     return;
 
   for (const auto& matched_properties : range) {
-    const unsigned apply_mask = ComputeApplyMask(state, matched_properties);
+    static_assert(static_cast<int>(kApplyMaskRegular) ==
+                      static_cast<int>(CSSSelector::kMatchLink),
+                  "kApplyMaskRegular and kMatchLink must match");
+    static_assert(static_cast<int>(kApplyMaskVisited) ==
+                      static_cast<int>(CSSSelector::kMatchVisited),
+                  "kApplyMaskVisited and kMatchVisited must match");
+    const unsigned apply_mask = matched_properties.types_.link_match_type;
     ApplyProperties<priority, shouldUpdateNeedsApplyPass>(
         state, matched_properties.properties.Get(), is_important,
         inherited_only, needs_apply_pass,
@@ -2046,9 +2038,6 @@ void StyleResolver::CascadeAndApplyMatchedProperties(
     return;
   }
 
-  if (state.Style()->InsideLink() == EInsideLink::kNotInsideLink)
-    filter = filter.Add(CSSProperty::kVisited, true);
-
   cascade.Analyze(result, filter);
 
   if (cache_success.ShouldApplyInheritedOnly()) {
@@ -2193,7 +2182,7 @@ void StyleResolver::ApplyCallbackSelectors(StyleResolverState& state) {
     return;
 
   ElementRuleCollector collector(state.ElementContext(), selector_filter_,
-                                 state.Style());
+                                 state.Style(), state.Style()->InsideLink());
   collector.SetMode(SelectorChecker::kCollectingStyleRules);
   collector.SetIncludeEmptyRules(true);
 

third_party/blink/web_tests/external/wpt/css/selectors/not-links-ref.html

+<!DOCTYPE html>
+<title>Test that *:not(:link):not(:visited) does not match links</title>
+<body>
+  <a href="#">Unvisited</a>
+  <a href="">Visited</a>
+  <span style="background-color: green">Green</span>
+  <p>
+    Only "Green" should have a green background.
+  </p>
+</body>

third_party/blink/web_tests/external/wpt/css/selectors/not-links.html

+<!DOCTYPE html>
+<title>Test that *:not(:link):not(:visited) does not match links</title>
+<link rel="match" href="not-links-ref.html">
+<link rel="help" href="https://drafts.csswg.org/selectors-4/#negation">
+<style>
+  body > *:not(:link):not(:visited) {
+    background-color: green;
+  }
+</style>
+<body>
+  <a href="#">Unvisited</a>
+  <a href="">Visited</a>
+  <span>Green</span>
+  <p style="background-color: initial">
+    Only "Green" should have a green background.
+  </p>
+</body>