Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[:is/:where] Support :link/:visited #26139

Merged
merged 1 commit into from Oct 16, 2020

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Oct 15, 2020

For :link/:visited, the current behavior of SelectorChecker is that
both pseudo-classes will match any link (element.IsLink() == true),
regardless of their visited status. We then rely on the "static"
analysis of the selector (ComputeLinkMatchType()) to understand
apply-time whether to apply normal properties, -internal-visited
properties, or both. Hence, we are in a way delaying the "real"
match/no-match decision to apply-time.

This becomes complicated when :not() is involved, because we can no
longer simply invert the result of CheckOne: :link will match
any link, including visited links, so :not(:link) would (naively
implemented) not match visited links (which is should: it should
match anything except an unvisited link). There's an attempted
mitigation in place in CheckPseudoNot, where we check differently
for :link/:visited, but it doesn't work properly. There are still
cases where we don't match what we should (see the updated
test not-links.html).

When :is()/:where() is added to the mix, it becomes even more
complicated, as :not() can effectively nest, for example
:not(:is(:not(:visited))).

To deal with this complexity, this CL proposes an alternative
handling of visited links, in which we sometimes perform selector
matching twice. If we are inside a visited link, we first match the
selector as if we are not inside a visited link, and record the result,
and then match again with the real link status, and record that result
as well. This makes nested :not() scenarios work automatically, because
CheckPseudoClass is now actually checking the link status, rather than
just matching any link.

Obviously matching the same selector twice has performance implications.
To mitigate this as much as possible, we avoid the double matching
behavior for selectors which don't contain :visited or :link (even if
we're inside a visited link).

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

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2476214 branch 3 times, most recently from eb51e83 to fb77194 Compare October 16, 2020 12:20
For :link/:visited, the current behavior of SelectorChecker is that
both pseudo-classes will match any link (element.IsLink() == true),
regardless of their visited status. We then rely on the "static"
analysis of the selector (ComputeLinkMatchType()) to understand
apply-time whether to apply normal properties, -internal-visited
properties, or both. Hence, we are in a way delaying the "real"
match/no-match decision to apply-time.

This becomes complicated when :not() is involved, because we can no
longer simply invert the result of CheckOne: :link will match
*any* link, including visited links, so :not(:link) would (naively
implemented) *not* match visited links (which is should: it should
match anything *except* an unvisited link). There's an attempted
mitigation in place in CheckPseudoNot, where we check differently
for :link/:visited, but it doesn't work properly. There are still
cases where we don't match what we should (see the updated
test not-links.html).

When :is()/:where() is added to the mix, it becomes even more
complicated, as :not() can effectively nest, for example
:not(:is(:not(:visited))).

To deal with this complexity, this CL proposes an alternative
handling of visited links, in which we sometimes perform selector
matching twice. If we are inside a visited link, we first match the
selector as if we are *not* inside a visited link, and record the result,
and then match again with the real link status, and record that result
as well. This makes nested :not() scenarios work automatically, because
CheckPseudoClass is now actually checking the link status, rather than
just matching any link.

Obviously matching the same selector twice has performance implications.
To mitigate this as much as possible, we avoid the double matching
behavior for selectors which don't contain :visited or :link (even if
we're inside a visited link).

Bug: 568705
Change-Id: I08eeb225d52418b80535f996cecc888b05e23eec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2476214
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#817900}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants