Skip to content

Commit

Permalink
[@scope] Support :visited as scoping roots and limits
Browse files Browse the repository at this point in the history 
This is mostly fixed by taking the StyleScope into account during
DetermineLinkMatchType. But also:

 * The original match_visited flag must be used when calculating
   the scope activations. (Explanation in comments).
 * We can not use the cache (StyleScopeFrame) for visited matching,
   because it would occupy the same space as the unvisited
   counterpart. This means that using visited-dependent rules
   in @scope has a performance penalty.

Note that privacy issues re. :visited and @scope are already tested
by scope-visited-cssom.html.

Bug: 1459981
Change-Id: I7c3966cf826a86772fcdf20fbf207f063f97c8b0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4767964
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1185119}
  • Loading branch information
@andruud
andruud authored and Chromium LUCI CQ committed Aug 18, 2023
1 parent 8214c99 commit a20b5e3
Show file tree
Hide file tree
Showing 5 changed files with 409 additions and 24 deletions.
23 changes: 20 additions & 3 deletions third_party/blink/renderer/core/css/rule_set.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,25 @@ static inline ValidPropertyFilter DetermineValidPropertyFilter(
return ValidPropertyFilter::kNoFilter;
}

static bool SelectorListHasLinkOrVisited(const CSSSelector* selector_list) {
for (const CSSSelector* complex = selector_list; complex;
complex = CSSSelectorList::Next(*complex)) {
if (complex->HasLinkOrVisited()) {
return true;
}
}
return false;
}

static bool StyleScopeHasLinkOrVisited(const StyleScope* style_scope) {
return style_scope && (SelectorListHasLinkOrVisited(style_scope->From()) ||
SelectorListHasLinkOrVisited(style_scope->To()));
}

static unsigned DetermineLinkMatchType(const AddRuleFlags add_rule_flags,
const CSSSelector& selector) {
if (selector.HasLinkOrVisited()) {
const CSSSelector& selector,
const StyleScope* style_scope) {
if (selector.HasLinkOrVisited() || StyleScopeHasLinkOrVisited(style_scope)) {
return (add_rule_flags & kRuleIsVisitedDependent)
? CSSSelector::kMatchVisited
: CSSSelector::kMatchLink;
Expand All @@ -116,7 +132,8 @@ RuleData::RuleData(StyleRule* rule,
selector_index_(selector_index),
position_(position),
specificity_(Selector().Specificity()),
link_match_type_(DetermineLinkMatchType(add_rule_flags, Selector())),
link_match_type_(
DetermineLinkMatchType(add_rule_flags, Selector(), style_scope)),
valid_property_filter_(
static_cast<std::underlying_type_t<ValidPropertyFilter>>(
DetermineValidPropertyFilter(add_rule_flags, Selector()))),
Expand Down
80 changes: 62 additions & 18 deletions third_party/blink/renderer/core/css/selector_checker.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,9 +238,16 @@ static bool IsLastOfType(const Element& element, const QualifiedName& type) {
return !ElementTraversal::NextSibling(element, HasTagName(type));
}

static void DisallowMatchVisited(
SelectorChecker::SelectorCheckingContext& context) {
context.had_match_visited |= context.match_visited;
context.match_visited = false;
}

bool SelectorChecker::Match(const SelectorCheckingContext& context,
MatchResult& result) const {
DCHECK(context.selector);
DCHECK(!context.had_match_visited);
#if DCHECK_IS_ON()
DCHECK(!inside_match_) << "Do not re-enter Match: use MatchSelector instead";
base::AutoReset<bool> reset_inside_match(&inside_match_, true);
Expand Down Expand Up @@ -388,9 +395,11 @@ SelectorChecker::MatchStatus SelectorChecker::MatchForRelation(
// Disable :visited matching when we see the first link or try to match
// anything else than an ancestor.
if ((!context.is_sub_selector || context.in_nested_complex_selector) &&
(context.element->IsLink() || (relation != CSSSelector::kDescendant &&
relation != CSSSelector::kChild))) {
next_context.match_visited = false;
(context.element->IsLink() ||
(relation != CSSSelector::kScopeActivation &&
relation != CSSSelector::kDescendant &&
relation != CSSSelector::kChild))) {
DisallowMatchVisited(next_context);
}

next_context.in_rightmost_compound = false;
Expand Down Expand Up @@ -423,7 +432,7 @@ SelectorChecker::MatchStatus SelectorChecker::MatchForRelation(
return kSelectorFailsCompletely;
}
if (next_context.element->IsLink()) {
next_context.match_visited = false;
DisallowMatchVisited(next_context);
}
}
return kSelectorFailsCompletely;
Expand Down Expand Up @@ -547,6 +556,7 @@ SelectorChecker::MatchStatus SelectorChecker::MatchForRelation(
return kSelectorFailsCompletely;
}
for (const StyleScopeActivation& activation : activations) {
next_context.match_visited = context.match_visited;
next_context.style_scope = nullptr;
next_context.scope = activation.root;
if (MatchSelector(next_context, result) == kSelectorMatches) {
Expand Down Expand Up @@ -2394,9 +2404,31 @@ const StyleScopeActivations& SelectorChecker::EnsureActivations(
style_scope.Parent() ? &EnsureActivations(context, *style_scope.Parent())
: MakeGarbageCollected<StyleScopeActivations>(
1, DefaultActivation(context.scope));
const StyleScopeActivations* activations =
CalculateActivations(context.style_scope_frame->element_, style_scope,
*outer_activations, context.style_scope_frame);
// The `match_visited` flag may have been set to false e.g. due to a link
// having been encountered (see DisallowMatchVisited), but scope activations
// are calculated lazily when :scope is first seen in a compound selector,
// and the scoping limit needs to evaluate according to the original setting.
//
// Consider the following, which should not match, because the :visited link
// is a scoping limit:
//
// @scope (#foo) to (:visited) { :scope a:visited { ... } }
//
// In the above selector, we first match a:visited, and set match_visited to
// false since a link was encountered. Then we encounter a compound
// with :scope, which causes scopes to be activated (kScopeActivation). At
// this point we try to find the scoping limit (:visited), but it wouldn't
// match anything because match_visited is set to false, so the selector
// would incorrectly match. For this reason we need to evaluate the scoping
// root and limits with the original match_visited setting.
bool match_visited = context.match_visited || context.had_match_visited;
// We only use the cache when matching normal/non-visited rules. Otherwise
// we'd need to double up the cache.
StyleScopeFrame* style_scope_frame =
match_visited ? nullptr : context.style_scope_frame;
const StyleScopeActivations* activations = CalculateActivations(
context.style_scope_frame->element_, style_scope, *outer_activations,
style_scope_frame, match_visited);
DCHECK(activations);
return *activations;
}
Expand All @@ -2410,7 +2442,8 @@ const StyleScopeActivations* SelectorChecker::CalculateActivations(
Element& element,
const StyleScope& style_scope,
const StyleScopeActivations& outer_activations,
StyleScopeFrame* style_scope_frame) const {
StyleScopeFrame* style_scope_frame,
bool match_visited) const {
Member<const StyleScopeActivations>* cached_activations_entry = nullptr;
if (style_scope_frame) {
auto entry = style_scope_frame->data_.insert(&style_scope, nullptr);
Expand Down Expand Up @@ -2439,16 +2472,21 @@ const StyleScopeActivations* SelectorChecker::CalculateActivations(
StyleScopeFrame* parent_frame =
style_scope_frame ? style_scope_frame->GetParentFrameOrNull(*parent)
: nullptr;
parent_activations = CalculateActivations(
*parent, style_scope, outer_activations, parent_frame);
// Disable :visited matching when encountering the first link.
// This matches the behavior for regular child/descendant combinators.
bool parent_match_visited = match_visited && !element.IsLink();
parent_activations =
CalculateActivations(*parent, style_scope, outer_activations,
parent_frame, parent_match_visited);
}
}

// The activations of the parent element are still active for this element,
// unless this element is a scoping limit.
if (parent_activations) {
for (const StyleScopeActivation& activation : *parent_activations) {
if (!ElementIsScopingLimit(style_scope, activation, element)) {
if (!ElementIsScopingLimit(style_scope, activation, element,
match_visited)) {
activations->push_back(
StyleScopeActivation{activation.root, activation.proximity + 1});
}
Expand All @@ -2457,13 +2495,15 @@ const StyleScopeActivations* SelectorChecker::CalculateActivations(

// Check if we need to add a new activation for this element.
for (const StyleScopeActivation& outer_activation : outer_activations) {
if (style_scope.From() ? MatchesWithScope(element, *style_scope.From(),
outer_activation.root)
: style_scope.HasImplicitRoot(&element)) {
if (style_scope.From()
? MatchesWithScope(element, *style_scope.From(),
outer_activation.root, match_visited)
: style_scope.HasImplicitRoot(&element)) {
StyleScopeActivation activation{&element, 0};
// It's possible for a newly created activation to be immediately
// limited (e.g. @scope (.x) to (.x)).
if (!ElementIsScopingLimit(style_scope, activation, element)) {
if (!ElementIsScopingLimit(style_scope, activation, element,
match_visited)) {
activations->push_back(activation);
}
break;
Expand All @@ -2482,9 +2522,11 @@ const StyleScopeActivations* SelectorChecker::CalculateActivations(

bool SelectorChecker::MatchesWithScope(Element& element,
const CSSSelector& selector_list,
const ContainerNode* scope) const {
const ContainerNode* scope,
bool match_visited) const {
SelectorCheckingContext context(&element);
context.scope = scope;
context.match_visited = match_visited;
// We are matching this selector list with the intent of storing the result
// in a cache (StyleScopeFrame). The :scope pseudo-class which triggered
// this call to MatchesWithScope, is either part of the subject compound
Expand All @@ -2506,11 +2548,13 @@ bool SelectorChecker::MatchesWithScope(Element& element,
bool SelectorChecker::ElementIsScopingLimit(
const StyleScope& style_scope,
const StyleScopeActivation& activation,
Element& element) const {
Element& element,
bool match_visited) const {
if (!style_scope.To()) {
return false;
}
return MatchesWithScope(element, *style_scope.To(), activation.root.Get());
return MatchesWithScope(element, *style_scope.To(), activation.root.Get(),
match_visited);
}

} // namespace blink
16 changes: 13 additions & 3 deletions third_party/blink/renderer/core/css/selector_checker.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,13 @@ class CORE_EXPORT SelectorChecker {
// If true, elements that are links will match :visited. Otherwise,
// they will match :link.
bool match_visited = false;
// The `match_visited` flag can become false during selector matching
// for various reasons (see DisallowMatchVisited and its call sites).
// The `had_match_visited` flag tracks whether was initially true or not.
// This is needed by @scope (CalculateActivations), which needs to evaluate
// visited-dependent selectors according to the original `match_visited`
// setting.
bool had_match_visited = false;
bool pseudo_has_in_rightmost_compound = true;
bool is_inside_has_pseudo_class = false;
};
Expand Down Expand Up @@ -341,14 +348,17 @@ class CORE_EXPORT SelectorChecker {
Element&,
const StyleScope&,
const StyleScopeActivations& outer_activations,
StyleScopeFrame*) const;
StyleScopeFrame*,
bool match_visited) const;
bool MatchesWithScope(Element&,
const CSSSelector& selector_list,
const ContainerNode* scope) const;
const ContainerNode* scope,
bool match_visited) const;
// https://drafts.csswg.org/css-cascade-6/#scoping-limit
bool ElementIsScopingLimit(const StyleScope&,
const StyleScopeActivation&,
Element& element) const;
Element& element,
bool match_visited) const;

CustomScrollbar* scrollbar_;
PartNames* part_names_;
Expand Down
96 changes: 96 additions & 0 deletions third_party/blink/web_tests/external/wpt/css/css-cascade/scope-visited-ref.html
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
<!DOCTYPE html>
<title>@scope - :visited</title>
<link rel="help" href="https://drafts.csswg.org/css-cascade-6/#scoped-styles">
<link rel="help" href="https://drafts.csswg.org/selectors-4/#link">

<style>
/* The visited background-color magically gets the alpha of the
unvisited color, which by default is rgba(0, 0, 0, 0). Set alpha to
255 so that visited colors also gets this alpha. */
* { background-color: rgba(255, 255, 255, 255); }
</style>

<!-- :visited via :scope in subject -->
<div>
<a id=visited href="" style="background-color:coral">
Visited <span>Span</span>
</a>
</div>

<!-- :link via :scope in subject -->
<div>
<a id=unvisited href="x" style="background-color:skyblue">
Unvisited <span>Span</span>
</a>
</div>

<!-- :visited via :scope in non-subject -->
<div>
<a id=visited href="">
Visited <span style="background-color:coral">Span</span>
</a>
</div>

<!-- :link via :scope in non-subject -->
<div>
<a id=unvisited href="x">
Unvisited <span style="background-color:skyblue">Span</span>
</a>
</div>

<!-- :visited in scope-end -->
<div>
<main>
Main
<a id=visited href="">
Visited <span>Span</span>
</a>
</main>
</div>

<!-- :visited in scope-end, unvisited link -->
<div>
<main>
Main
<a id=unvisited href="x" style="background-color:skyblue">
Unvisited <span>Span</span>
</a>
</main>
</div>

<!-- :link in scope-end -->
<div>
<main>
Main
<a id=unvisited href="x">
Unvisited <span>Span</span>
</a>
</main>
</div>

<!-- :link in scope-end, visited link -->
<div>
<main>
Main
<a id=visited href="" style="background-color:coral">
Visited <span>Span</span>
</a>
</main>
</div>

<!-- :link in scope-end, visited link -->
<div>
<main>
Main
<a id=outer_visited href="">
Visited1
</a>
</main>
</div>
<script>
// Insert inner <a> with JS, since the parser can't produce this result.
let inner_a = document.createElement('a');
inner_a.setAttribute('href', '');
inner_a.textContent = 'Visited2';
outer_visited.append(inner_a);
</script>

0 comments on commit a20b5e3

Please sign in to comment.