Commit a11d3c19 authored by Anders Hartvoll Ruud's avatar Anders Hartvoll Ruud Committed by Commit Bot

Handle :visited/:link in RuleSet

Instead of having SelectorChecker::Match match the same selector
twice, this CL proposes a way to deal with visited links via
RuleSet.

Whenever we add a rule to the RuleSet which contains :link or :visited,
we effectively split the rule into two non-overlapping rules;
one which applies to the regular style (kMatchLink) and one which
applies to the visited style (kMatchVisited). The rules marked with
kMatchVisited go in a separate list of visited dependent rules on the
RuleSet. Then, when collecting matching rules, we try to match from
this list only when kInsideVisitedLink.

This way we can avoid paying any extra cost on every call to
SelectorChecker::Match.

Fixed: 1139464
Change-Id: I66f59acfbc0eb5e3225ab2b5be9ae572542bb8ce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2485062Reviewed-by: default avatarRune Lillesveen <futhark@chromium.org>
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818901}
parent 9b60dd1e
......@@ -629,6 +629,7 @@ blink_core_tests_css = [
"cssom/prepopulated_computed_style_property_map_test.cc",
"cssom/style_property_map_test.cc",
"drag_update_test.cc",
"element_rule_collector_test.cc",
"font_face_cache_test.cc",
"font_size_functions_test.cc",
"font_update_invalidation_test.cc",
......@@ -681,7 +682,6 @@ blink_core_tests_css = [
"resolver/style_resolver_test.cc",
"rule_feature_set_test.cc",
"rule_set_test.cc",
"selector_checker_test.cc",
"selector_query_test.cc",
"style_element_test.cc",
"style_engine_test.cc",
......
......@@ -177,11 +177,10 @@ void ElementRuleCollector::CollectMatchingRulesForList(
SelectorChecker::MatchResult result;
context.selector = &rule_data->Selector();
// If the selector does not contain :link or :visited, we disable
// :visited matching as a performance optimization.
context.is_inside_visited_link =
(inside_link_ == EInsideLink::kInsideVisitedLink) &&
rule_data->HasLinkOrVisited();
rule_data->LinkMatchType() == CSSSelector::kMatchVisited;
DCHECK(!context.is_inside_visited_link ||
(inside_link_ == EInsideLink::kInsideVisitedLink));
if (!checker.Match(context, result)) {
rejected++;
continue;
......@@ -253,6 +252,10 @@ void ElementRuleCollector::CollectMatchingRules(
if (element.IsLink())
CollectMatchingRulesForList(match_request.rule_set->LinkPseudoClassRules(),
cascade_order, match_request);
if (inside_link_ == EInsideLink::kInsideVisitedLink) {
CollectMatchingRulesForList(match_request.rule_set->VisitedDependentRules(),
cascade_order, match_request);
}
if (SelectorChecker::MatchesFocusPseudoClass(element))
CollectMatchingRulesForList(match_request.rule_set->FocusPseudoClassRules(),
cascade_order, match_request);
......@@ -349,7 +352,7 @@ void ElementRuleCollector::SortAndTransferMatchedRules() {
const RuleData* rule_data = matched_rule.GetRuleData();
result_.AddMatchedProperties(
&rule_data->Rule()->Properties(),
AdjustLinkMatchType(inside_link_, matched_rule.GetLinkMatchType()),
AdjustLinkMatchType(inside_link_, rule_data->LinkMatchType()),
rule_data->GetValidPropertyFilter(matching_ua_rules_));
}
}
......@@ -378,7 +381,7 @@ void ElementRuleCollector::DidMatchRule(
style_->SetHasPseudoElementStyle(dynamic_pseudo);
} else {
matched_rules_.push_back(MatchedRule(
rule_data, result.specificity, result.link_match_type, cascade_order,
rule_data, result.specificity, cascade_order,
match_request.style_sheet_index, match_request.style_sheet));
}
}
......
......@@ -54,13 +54,11 @@ class MatchedRule {
public:
MatchedRule(const RuleData* rule_data,
unsigned specificity,
unsigned link_match_type,
ShadowV0CascadeOrder cascade_order,
unsigned style_sheet_index,
const CSSStyleSheet* parent_style_sheet)
: rule_data_(rule_data),
specificity_(specificity),
link_match_type_(link_match_type),
parent_style_sheet_(parent_style_sheet) {
DCHECK(rule_data_);
static const unsigned kBitsForPositionInRuleData = 18;
......@@ -76,7 +74,6 @@ class MatchedRule {
unsigned Specificity() const {
return GetRuleData()->Specificity() + specificity_;
}
unsigned GetLinkMatchType() const { return link_match_type_; }
const CSSStyleSheet* ParentStyleSheet() const { return parent_style_sheet_; }
void Trace(Visitor* visitor) const {
visitor->Trace(parent_style_sheet_);
......@@ -86,7 +83,6 @@ class MatchedRule {
private:
Member<const RuleData> rule_data_;
unsigned specificity_;
unsigned link_match_type_;
uint64_t position_;
Member<const CSSStyleSheet> parent_style_sheet_;
};
......
......@@ -2,65 +2,75 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "third_party/blink/renderer/core/css/selector_checker.h"
#include "third_party/blink/renderer/core/css/element_rule_collector.h"
#include "base/optional.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/renderer/core/css/css_test_helpers.h"
#include "third_party/blink/renderer/core/css/parser/css_parser.h"
#include "third_party/blink/renderer/core/css/parser/css_parser_context.h"
#include "third_party/blink/renderer/core/css/resolver/element_resolve_context.h"
#include "third_party/blink/renderer/core/css/selector_filter.h"
#include "third_party/blink/renderer/core/dom/document.h"
#include "third_party/blink/renderer/core/dom/element_traversal.h"
#include "third_party/blink/renderer/core/dom/flat_tree_traversal.h"
#include "third_party/blink/renderer/core/style/computed_style.h"
#include "third_party/blink/renderer/core/testing/page_test_base.h"
#include "testing/gtest/include/gtest/gtest.h"
#include <iostream>
namespace blink {
class SelectorCheckerTest : public PageTestBase {
class ElementRuleCollectorTest : public PageTestBase {
public:
bool IsInsideVisitedLink(Element* element) {
EInsideLink InsideLink(Element* element) {
if (!element)
return false;
return EInsideLink::kNotInsideLink;
if (element->IsLink()) {
ElementResolveContext context(*element);
return context.ElementLinkState() == EInsideLink::kInsideVisitedLink;
return context.ElementLinkState();
}
return IsInsideVisitedLink(
DynamicTo<Element>(FlatTreeTraversal::Parent(*element)));
}
// Returns the link_match_type upon successful match, or base::nullopt
// on failure.
base::Optional<unsigned> Match(
const SelectorChecker::SelectorCheckingContext& context,
const String& selector) {
CSSSelectorList selector_list =
css_test_helpers::ParseSelectorList(selector);
DCHECK(selector_list.First()) << "Invalid selector: " << selector;
SelectorChecker::Init init;
SelectorChecker checker(init);
SelectorChecker::SelectorCheckingContext local_context(context);
local_context.selector = selector_list.First();
local_context.is_inside_visited_link = IsInsideVisitedLink(context.element);
SelectorChecker::MatchResult result;
if (checker.Match(local_context, result))
return result.link_match_type;
return base::nullopt;
return InsideLink(DynamicTo<Element>(FlatTreeTraversal::Parent(*element)));
}
base::Optional<unsigned> Match(Element* element, const String& selector) {
SelectorChecker::SelectorCheckingContext context(element);
return Match(context, selector);
// Matches an element against a selector via ElementRuleCollector.
//
// Upon successful match, the combined CSSSelector::LinkMatchMask of
// of all matched rules is returned, or base::nullopt if no-match.
base::Optional<unsigned> Match(Element* element,
const String& selector,
const ContainerNode* scope = nullptr) {
ElementResolveContext context(*element);
SelectorFilter filter;
MatchResult result;
auto style = ComputedStyle::Create();
ElementRuleCollector collector(context, filter, result, style.get(),
InsideLink(element));
String rule = selector + " { color: green }";
auto* style_rule =
DynamicTo<StyleRule>(css_test_helpers::ParseRule(GetDocument(), rule));
if (!style_rule)
return base::nullopt;
RuleSet* rule_set = MakeGarbageCollected<RuleSet>();
rule_set->AddStyleRule(style_rule, kRuleHasNoSpecialState);
MatchRequest request(rule_set, scope);
collector.CollectMatchingRules(request);
collector.SortAndTransferMatchedRules();
const MatchedPropertiesVector& vector = result.GetMatchedProperties();
if (!vector.size())
return base::nullopt;
// Either the normal rules matched, the visited dependent rules matched,
// or both. There should be nothing else.
DCHECK(vector.size() == 1 || vector.size() == 2);
unsigned link_match_type = 0;
for (const auto& matched_propeties : vector)
link_match_type |= matched_propeties.types_.link_match_type;
return link_match_type;
}
};
TEST_F(SelectorCheckerTest, LinkMatchType) {
TEST_F(ElementRuleCollectorTest, LinkMatchType) {
SetBodyInnerHTML(R"HTML(
<div id=foo></div>
<a id=visited href="">
......@@ -84,33 +94,30 @@ TEST_F(SelectorCheckerTest, LinkMatchType) {
ASSERT_TRUE(unvisited_span);
ASSERT_TRUE(visited_span);
ASSERT_TRUE(IsInsideVisitedLink(visited));
ASSERT_TRUE(IsInsideVisitedLink(visited_span));
ASSERT_FALSE(IsInsideVisitedLink(foo));
ASSERT_FALSE(IsInsideVisitedLink(link));
ASSERT_FALSE(IsInsideVisitedLink(unvisited_span));
ASSERT_FALSE(IsInsideVisitedLink(bar));
ASSERT_EQ(EInsideLink::kInsideVisitedLink, InsideLink(visited));
ASSERT_EQ(EInsideLink::kInsideVisitedLink, InsideLink(visited_span));
ASSERT_EQ(EInsideLink::kNotInsideLink, InsideLink(foo));
ASSERT_EQ(EInsideLink::kInsideUnvisitedLink, InsideLink(link));
ASSERT_EQ(EInsideLink::kInsideUnvisitedLink, InsideLink(unvisited_span));
ASSERT_EQ(EInsideLink::kNotInsideLink, InsideLink(bar));
const auto kMatchLink = CSSSelector::kMatchLink;
const auto kMatchVisited = CSSSelector::kMatchVisited;
const auto kMatchAll = CSSSelector::kMatchAll;
ASSERT_TRUE(kMatchLink);
ASSERT_TRUE(kMatchVisited);
ASSERT_TRUE(kMatchAll);
EXPECT_EQ(Match(foo, "#bar"), base::nullopt);
EXPECT_EQ(Match(visited, "#foo"), base::nullopt);
EXPECT_EQ(Match(link, "#foo"), base::nullopt);
EXPECT_EQ(Match(foo, "#foo"), kMatchAll);
EXPECT_EQ(Match(foo, "#foo"), kMatchLink);
EXPECT_EQ(Match(link, ":visited"), base::nullopt);
// Note that |link| isn't a _visited_ link, hence it gets regular treatment by
// SelectorChecker::Match. And for regular treatments we always get kMatchAll
// if the selector matches.
EXPECT_EQ(Match(link, ":link"), kMatchAll);
EXPECT_EQ(Match(foo, ":not(:visited)"), kMatchAll);
EXPECT_EQ(Match(foo, ":not(:link)"), kMatchAll);
EXPECT_EQ(Match(foo, ":not(:link):not(:visited)"), kMatchAll);
EXPECT_EQ(Match(link, ":link"), kMatchLink);
// Note that for elements that are not inside links at all, we always
// expect kMatchLink, since kMatchLink represents the regular (non-visited)
// style.
EXPECT_EQ(Match(foo, ":not(:visited)"), kMatchLink);
EXPECT_EQ(Match(foo, ":not(:link)"), kMatchLink);
EXPECT_EQ(Match(foo, ":not(:link):not(:visited)"), kMatchLink);
EXPECT_EQ(Match(visited, ":link"), kMatchLink);
EXPECT_EQ(Match(visited, ":visited"), kMatchVisited);
......@@ -161,15 +168,15 @@ TEST_F(SelectorCheckerTest, LinkMatchType) {
// When using :link/:visited in a sibling selector, we expect special
// behavior for privacy reasons.
// https://developer.mozilla.org/en-US/docs/Web/CSS/Privacy_and_the_:visited_selector
EXPECT_EQ(Match(bar, ":link + #bar"), kMatchAll);
EXPECT_EQ(Match(bar, ":link + #bar"), kMatchLink);
EXPECT_EQ(Match(bar, ":visited + #bar"), base::nullopt);
EXPECT_EQ(Match(bar, ":is(:link + #bar)"), kMatchAll);
EXPECT_EQ(Match(bar, ":is(:link + #bar)"), kMatchLink);
EXPECT_EQ(Match(bar, ":is(:visited ~ #bar)"), base::nullopt);
EXPECT_EQ(Match(bar, ":not(:is(:link + #bar))"), base::nullopt);
EXPECT_EQ(Match(bar, ":not(:is(:visited ~ #bar))"), kMatchAll);
EXPECT_EQ(Match(bar, ":not(:is(:visited ~ #bar))"), kMatchLink);
}
TEST_F(SelectorCheckerTest, LinkMatchTypeHostContext) {
TEST_F(ElementRuleCollectorTest, LinkMatchTypeHostContext) {
SetBodyInnerHTML(R"HTML(
<a href=""><div id="visited_host"></div></a>
<a href="unvisited"><div id="unvisited_host"></div></a>
......@@ -211,29 +218,33 @@ TEST_F(SelectorCheckerTest, LinkMatchTypeHostContext) {
const auto kMatchAll = CSSSelector::kMatchAll;
{
SelectorChecker::SelectorCheckingContext context(visited_div);
context.scope = visited_style;
EXPECT_EQ(Match(context, ":host-context(a) div"), kMatchAll);
EXPECT_EQ(Match(context, ":host-context(:link) div"), kMatchLink);
EXPECT_EQ(Match(context, ":host-context(:visited) div"), kMatchVisited);
EXPECT_EQ(Match(context, ":host-context(:is(:visited, :link)) div"),
Element* element = visited_div;
const ContainerNode* scope = visited_style;
EXPECT_EQ(Match(element, ":host-context(a) div", scope), kMatchAll);
EXPECT_EQ(Match(element, ":host-context(:link) div", scope), kMatchLink);
EXPECT_EQ(Match(element, ":host-context(:visited) div", scope),
kMatchVisited);
EXPECT_EQ(Match(element, ":host-context(:is(:visited, :link)) div", scope),
kMatchAll);
// :host-context(:not(:visited/link)) matches the host itself.
EXPECT_EQ(Match(context, ":host-context(:not(:visited)) div"), kMatchAll);
EXPECT_EQ(Match(context, ":host-context(:not(:link)) div"), kMatchAll);
EXPECT_EQ(Match(element, ":host-context(:not(:visited)) div", scope),
kMatchAll);
EXPECT_EQ(Match(element, ":host-context(:not(:link)) div", scope),
kMatchAll);
}
{
SelectorChecker::SelectorCheckingContext context(unvisited_div);
context.scope = unvisited_style;
EXPECT_EQ(Match(context, ":host-context(a) div"), kMatchAll);
EXPECT_EQ(Match(context, ":host-context(:link) div"), kMatchAll);
EXPECT_EQ(Match(context, ":host-context(:visited) div"), base::nullopt);
EXPECT_EQ(Match(context, ":host-context(:is(:visited, :link)) div"),
kMatchAll);
Element* element = unvisited_div;
const ContainerNode* scope = unvisited_style;
EXPECT_EQ(Match(element, ":host-context(a) div", scope), kMatchAll);
EXPECT_EQ(Match(element, ":host-context(:link) div", scope), kMatchLink);
EXPECT_EQ(Match(element, ":host-context(:visited) div", scope),
base::nullopt);
EXPECT_EQ(Match(element, ":host-context(:is(:visited, :link)) div", scope),
kMatchLink);
}
}
......
......@@ -34,7 +34,7 @@ class ContainerNode;
// Encapsulates the context for matching against a single style sheet by
// ElementRuleCollector. Carries the RuleSet, scope (a ContainerNode) and
// CSSStyleSheet.
class MatchRequest {
class CORE_EXPORT MatchRequest {
STACK_ALLOCATED();
public:
......
......@@ -63,6 +63,16 @@ static inline ValidPropertyFilter DetermineValidPropertyFilter(
return ValidPropertyFilter::kNoFilter;
}
static unsigned DetermineLinkMatchType(const AddRuleFlags add_rule_flags,
const CSSSelector& selector) {
if (selector.HasLinkOrVisited()) {
return (add_rule_flags & kRuleIsVisitedDependent)
? CSSSelector::kMatchVisited
: CSSSelector::kMatchLink;
}
return CSSSelector::kMatchAll;
}
RuleData* RuleData::MaybeCreate(StyleRule* rule,
unsigned selector_index,
unsigned position,
......@@ -86,7 +96,7 @@ RuleData::RuleData(StyleRule* rule,
selector_index_(selector_index),
position_(position),
specificity_(Selector().Specificity()),
has_link_or_visited_(Selector().HasLinkOrVisited()),
link_match_type_(DetermineLinkMatchType(add_rule_flags, Selector())),
has_document_security_origin_(add_rule_flags &
kRuleHasDocumentSecurityOrigin),
valid_property_filter_(
......@@ -272,6 +282,19 @@ void RuleSet::AddRule(StyleRule* rule,
// rules.
universal_rules_.push_back(rule_data);
}
// If the rule has CSSSelector::kMatchLink, it means that there is a :visited
// or :link pseudo-class somewhere in the selector. In those cases, we
// effectively split the rule into two: one which covers the situation
// where we are in an unvisited link (kMatchLink), and another which covers
// the visited link case (kMatchVisited).
if (rule_data->LinkMatchType() == CSSSelector::kMatchLink) {
RuleData* visited_dependent = RuleData::MaybeCreate(
rule, rule_data->SelectorIndex(), rule_data->GetPosition(),
add_rule_flags | kRuleIsVisitedDependent);
DCHECK(visited_dependent);
visited_dependent_rules_.push_back(visited_dependent);
}
}
void RuleSet::AddPageRule(StyleRulePage* rule) {
......@@ -467,6 +490,7 @@ void RuleSet::Trace(Visitor* visitor) const {
visitor->Trace(scroll_timeline_rules_);
visitor->Trace(deep_combinator_or_shadow_pseudo_rules_);
visitor->Trace(part_pseudo_rules_);
visitor->Trace(visited_dependent_rules_);
visitor->Trace(content_pseudo_element_rules_);
visitor->Trace(slotted_pseudo_element_rules_);
visitor->Trace(pending_rules_);
......
......@@ -36,9 +36,12 @@
namespace blink {
enum AddRuleFlags {
using AddRuleFlags = unsigned;
enum AddRuleFlag {
kRuleHasNoSpecialState = 0,
kRuleHasDocumentSecurityOrigin = 1,
kRuleHasDocumentSecurityOrigin = 1 << 0,
kRuleIsVisitedDependent = 1 << 1,
};
// Some CSS properties do not apply to certain pseudo-elements, and need to be
......@@ -110,7 +113,7 @@ class CORE_EXPORT RuleData : public GarbageCollected<RuleData> {
return contains_uncommon_attribute_selector_;
}
unsigned Specificity() const { return specificity_; }
bool HasLinkOrVisited() const { return has_link_or_visited_; }
unsigned LinkMatchType() const { return link_match_type_; }
bool HasDocumentSecurityOrigin() const {
return has_document_security_origin_;
}
......@@ -146,10 +149,10 @@ class CORE_EXPORT RuleData : public GarbageCollected<RuleData> {
unsigned contains_uncommon_attribute_selector_ : 1;
// 32 bits above
unsigned specificity_ : 24;
unsigned has_link_or_visited_ : 1;
unsigned link_match_type_ : 2;
unsigned has_document_security_origin_ : 1;
unsigned valid_property_filter_ : 2;
// 28 bits above
// 29 bits above
// Use plain array instead of a Vector to minimize memory overhead.
unsigned descendant_selector_identifier_hashes_[kMaximumIdentifierCount];
};
......@@ -240,6 +243,10 @@ class CORE_EXPORT RuleSet final : public GarbageCollected<RuleSet> {
DCHECK(!pending_rules_);
return &part_pseudo_rules_;
}
const HeapVector<Member<const RuleData>>* VisitedDependentRules() const {
DCHECK(!pending_rules_);
return &visited_dependent_rules_;
}
const HeapVector<Member<StyleRulePage>>& PageRules() const {
DCHECK(!pending_rules_);
return page_rules_;
......@@ -351,6 +358,7 @@ class CORE_EXPORT RuleSet final : public GarbageCollected<RuleSet> {
HeapVector<Member<const RuleData>> universal_rules_;
HeapVector<Member<const RuleData>> shadow_host_rules_;
HeapVector<Member<const RuleData>> part_pseudo_rules_;
HeapVector<Member<const RuleData>> visited_dependent_rules_;
RuleFeatureSet features_;
HeapVector<Member<StyleRulePage>> page_rules_;
HeapVector<Member<StyleRuleFontFace>> font_face_rules_;
......
......@@ -238,8 +238,6 @@ bool SelectorChecker::Match(const SelectorCheckingContext& context,
if (context.selector->IsLastInTagHistory())
return false;
}
if (UNLIKELY(context.is_inside_visited_link))
return MatchForVisitedLink(context, result) == kSelectorMatches;
return MatchSelector(context, result) == kSelectorMatches;
}
......@@ -339,26 +337,6 @@ SelectorChecker::MatchStatus SelectorChecker::MatchForPseudoShadow(
return MatchSelector(context, result);
}
SelectorChecker::MatchStatus SelectorChecker::MatchForVisitedLink(
const SelectorCheckingContext& context,
MatchResult& result) const {
DCHECK(context.is_inside_visited_link);
SelectorCheckingContext unvisited(context);
unvisited.is_inside_visited_link = false;
unsigned link_match_type = 0;
if (MatchSelector(unvisited, result) == kSelectorMatches)
link_match_type |= CSSSelector::kMatchLink;
if (MatchSelector(context, result) == kSelectorMatches)
link_match_type |= CSSSelector::kMatchVisited;
result.link_match_type = link_match_type;
return link_match_type ? kSelectorMatches : kSelectorFailsCompletely;
}
static inline Element* ParentOrV0ShadowHostElement(const Element& element) {
if (auto* shadow_root = DynamicTo<ShadowRoot>(element.parentNode())) {
if (shadow_root->GetType() != ShadowRootType::V0)
......
......@@ -128,7 +128,6 @@ class CORE_EXPORT SelectorChecker {
public:
PseudoId dynamic_pseudo{kPseudoIdNone};
unsigned specificity{0};
unsigned link_match_type{CSSSelector::kMatchAll};
};
bool Match(const SelectorCheckingContext& context, MatchResult& result) const;
......@@ -183,8 +182,6 @@ class CORE_EXPORT SelectorChecker {
MatchStatus MatchForPseudoShadow(const SelectorCheckingContext&,
const ContainerNode*,
MatchResult&) const;
MatchStatus MatchForVisitedLink(const SelectorCheckingContext&,
MatchResult&) const;
bool CheckPseudoClass(const SelectorCheckingContext&, MatchResult&) const;
bool CheckPseudoElement(const SelectorCheckingContext&, MatchResult&) const;
bool CheckScrollbarPseudoClass(const SelectorCheckingContext&,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment