Skip to content

Commit

Permalink
fix: ensure :has selectors followed by other selectors match (#13824)
Browse files Browse the repository at this point in the history 
I resisted this previously because it felt a bit wasteful, but I now think that there's really no way around this: Instead of only going upwards the tree while matching, for `:has` we go _down_ the tree to see what matches. More specifically, we're collecting the children of the current element and then check if one of those does match the selectors inside `:has`.

This makes the way the code works easier to reason about and also removes some boolean tracking we had to add for the previous approach.

Fixes #13779
  • Loading branch information
@dummdidumm
dummdidumm authored Oct 23, 2024
1 parent c603553 commit 5669b31
Show file tree
Hide file tree
Showing 5 changed files with 96 additions and 88 deletions.
5 changes: 5 additions & 0 deletions .changeset/strong-feet-happen.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: ensure `:has` selectors followed by other selectors match
100 changes: 44 additions & 56 deletions packages/svelte/src/compiler/phases/2-analyze/css/css-prune.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,7 @@ const visitors = {
selectors_to_check,
/** @type {Compiler.Css.Rule} */ (node.metadata.rule),
element,
context.state.stylesheet,
true
context.state.stylesheet
)
) {
mark(inner, element);
Expand All @@ -144,8 +143,7 @@ const visitors = {
selectors,
/** @type {Compiler.Css.Rule} */ (node.metadata.rule),
context.state.element,
context.state.stylesheet,
true
context.state.stylesheet
)
) {
mark(inner, context.state.element);
Expand Down Expand Up @@ -191,28 +189,21 @@ function truncate(node) {
* @param {Compiler.Css.Rule} rule
* @param {Compiler.AST.RegularElement | Compiler.AST.SvelteElement} element
* @param {Compiler.Css.StyleSheet} stylesheet
* @param {boolean} check_has Whether or not to check the `:has(...)` selectors
* @returns {boolean}
*/
function apply_selector(relative_selectors, rule, element, stylesheet, check_has) {
function apply_selector(relative_selectors, rule, element, stylesheet) {
const parent_selectors = relative_selectors.slice();
const relative_selector = parent_selectors.pop();

if (!relative_selector) return false;

const possible_match = relative_selector_might_apply_to_node(
relative_selector,
parent_selectors,
rule,
element,
stylesheet,
check_has
stylesheet
);

if (possible_match === 'definite_match') {
return true;
}

if (!possible_match) {
return false;
}
Expand All @@ -224,8 +215,7 @@ function apply_selector(relative_selectors, rule, element, stylesheet, check_has
parent_selectors,
rule,
element,
stylesheet,
check_has
stylesheet
);
}

Expand All @@ -246,7 +236,6 @@ function apply_selector(relative_selectors, rule, element, stylesheet, check_has
* @param {Compiler.Css.Rule} rule
* @param {Compiler.AST.RegularElement | Compiler.AST.SvelteElement} element
* @param {Compiler.Css.StyleSheet} stylesheet
* @param {boolean} check_has Whether or not to check the `:has(...)` selectors
* @returns {boolean}
*/
function apply_combinator(
Expand All @@ -255,8 +244,7 @@ function apply_combinator(
parent_selectors,
rule,
element,
stylesheet,
check_has
stylesheet
) {
const name = combinator.name;

Expand All @@ -281,7 +269,7 @@ function apply_combinator(
}

if (parent.type === 'RegularElement' || parent.type === 'SvelteElement') {
if (apply_selector(parent_selectors, rule, parent, stylesheet, check_has)) {
if (apply_selector(parent_selectors, rule, parent, stylesheet)) {
// TODO the `name === ' '` causes false positives, but removing it causes false negatives...
if (name === ' ' || crossed_component_boundary) {
mark(parent_selectors[parent_selectors.length - 1], parent);
Expand Down Expand Up @@ -312,9 +300,7 @@ function apply_combinator(
mark(relative_selector, element);
sibling_matched = true;
}
} else if (
apply_selector(parent_selectors, rule, possible_sibling, stylesheet, check_has)
) {
} else if (apply_selector(parent_selectors, rule, possible_sibling, stylesheet)) {
mark(relative_selector, element);
sibling_matched = true;
}
Expand Down Expand Up @@ -393,21 +379,12 @@ const regex_backslash_and_following_character = /\\(.)/g;
* Ensure that `element` satisfies each simple selector in `relative_selector`
*
* @param {Compiler.Css.RelativeSelector} relative_selector
* @param {Compiler.Css.RelativeSelector[]} parent_selectors
* @param {Compiler.Css.Rule} rule
* @param {Compiler.AST.RegularElement | Compiler.AST.SvelteElement} element
* @param {Compiler.Css.StyleSheet} stylesheet
* @param {boolean} check_has Whether or not to check the `:has(...)` selectors
* @returns {boolean | 'definite_match'}
* @returns {boolean }
*/
function relative_selector_might_apply_to_node(
relative_selector,
parent_selectors,
rule,
element,
stylesheet,
check_has
) {
function relative_selector_might_apply_to_node(relative_selector, rule, element, stylesheet) {
// Sort :has(...) selectors in one bucket and everything else into another
const has_selectors = [];
const other_selectors = [];
Expand All @@ -422,7 +399,26 @@ function relative_selector_might_apply_to_node(

// If we're called recursively from a :has(...) selector, we're on the way of checking if the other selectors match.
// In that case ignore this check (because we just came from this) to avoid an infinite loop.
if (check_has && has_selectors.length > 0) {
if (has_selectors.length > 0) {
/** @type {Array<Compiler.AST.RegularElement | Compiler.AST.SvelteElement>} */
const child_elements = [];
/** @type {Array<Compiler.AST.RegularElement | Compiler.AST.SvelteElement>} */
const descendant_elements = [];

/**
* @param {Compiler.SvelteNode} node
* @param {boolean} is_recursing
*/
function collect_child_elements(node, is_recursing) {
if (node.type === 'RegularElement' || node.type === 'SvelteElement') {
descendant_elements.push(node);
if (!is_recursing) child_elements.push(node);
node.fragment.nodes.forEach((node) => collect_child_elements(node, true));
}
}

element.fragment.nodes.forEach((node) => collect_child_elements(node, false));

// :has(...) is special in that it means "look downwards in the CSS tree". Since our matching algorithm goes
// upwards and back-to-front, we need to first check the selectors inside :has(...), then check the rest of the
// selector in a way that is similar to ancestor matching. In a sense, we're treating `.x:has(.y)` as `.x .y`.
Expand All @@ -443,25 +439,20 @@ function relative_selector_might_apply_to_node(
};
}

if (
selectors.length === 0 /* is :global(...) */ ||
apply_selector(selectors, rule, element, stylesheet, check_has)
) {
// Treat e.g. `.x:has(.y)` as `.x .y` with the .y part already being matched,
// and now looking upwards for the .x part.
const descendants =
left_most_combinator.name === '>' ? child_elements : descendant_elements;

let selector_matched = false;

// Iterate over all descendant elements and check if the selector inside :has matches
for (const element of descendants) {
if (
apply_combinator(
left_most_combinator,
selectors[0] ?? [],
[...parent_selectors, relative_selector],
rule,
element,
stylesheet,
false
)
selectors.length === 0 /* is :global(...) */ ||
(element.metadata.scoped && selector_matched) ||
apply_selector(selectors, rule, element, stylesheet)
) {
complex_selector.metadata.used = true;
matched = true;
selector_matched = matched = true;
}
}
}
Expand All @@ -484,9 +475,6 @@ function relative_selector_might_apply_to_node(
return false;
}
}

// We return this to signal the parent "don't bother checking the rest of the selectors, I already did that"
return 'definite_match';
}

for (const selector of other_selectors) {
Expand All @@ -507,7 +495,7 @@ function relative_selector_might_apply_to_node(
) {
const args = selector.args;
const complex_selector = args.children[0];
return apply_selector(complex_selector.children, rule, element, stylesheet, check_has);
return apply_selector(complex_selector.children, rule, element, stylesheet);
}

// We came across a :global, everything beyond it is global and therefore a potential match
Expand All @@ -520,7 +508,7 @@ function relative_selector_might_apply_to_node(
const relative = truncate(complex_selector);
if (
relative.length === 0 /* is :global(...) */ ||
apply_selector(relative, rule, element, stylesheet, check_has)
apply_selector(relative, rule, element, stylesheet)
) {
complex_selector.metadata.used = true;
matched = true;
Expand Down Expand Up @@ -621,7 +609,7 @@ function relative_selector_might_apply_to_node(
const parent = /** @type {Compiler.Css.Rule} */ (rule.metadata.parent_rule);

for (const complex_selector of parent.prelude.children) {
if (apply_selector(truncate(complex_selector), parent, element, stylesheet, check_has)) {
if (apply_selector(truncate(complex_selector), parent, element, stylesheet)) {
complex_selector.metadata.used = true;
matched = true;
}
Expand Down
64 changes: 32 additions & 32 deletions packages/svelte/tests/css/samples/has/_config.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,112 +6,112 @@ export default test({
code: 'css_unused_selector',
message: 'Unused CSS selector ".unused:has(y)"',
start: {
character: 269,
line: 28,
column: 1,
line: 27
character: 277
},
end: {
character: 283,
line: 28,
column: 15,
line: 27
character: 291
}
},
{
code: 'css_unused_selector',
message: 'Unused CSS selector ".unused:has(:global(y))"',
start: {
character: 304,
line: 31,
column: 1,
line: 30
character: 312
},
end: {
character: 327,
line: 31,
column: 24,
line: 30
character: 335
}
},
{
code: 'css_unused_selector',
message: 'Unused CSS selector "x:has(.unused)"',
start: {
character: 348,
line: 34,
column: 1,
line: 33
character: 356
},
end: {
character: 362,
line: 34,
column: 15,
line: 33
character: 370
}
},
{
code: 'css_unused_selector',
message: 'Unused CSS selector "x:has(y):has(.unused)"',
start: {
character: 517,
line: 47,
column: 1,
line: 46
character: 525
},
end: {
character: 538,
line: 47,
column: 22,
line: 46
character: 546
}
},
{
code: 'css_unused_selector',
message: 'Unused CSS selector ".unused"',
start: {
character: 743,
line: 66,
column: 2,
line: 65
character: 751
},
end: {
character: 750,
line: 66,
column: 9,
line: 65
character: 758
}
},
{
code: 'css_unused_selector',
message: 'Unused CSS selector ".unused x:has(y)"',
start: {
character: 897,
line: 82,
column: 1,
line: 81
character: 905
},
end: {
character: 913,
line: 82,
column: 17,
line: 81
character: 921
}
},
{
code: 'css_unused_selector',
message: 'Unused CSS selector ".unused:has(.unused)"',
start: {
line: 84,
line: 85,
column: 1,
character: 934
character: 942
},
end: {
line: 84,
line: 85,
column: 21,
character: 954
character: 962
}
},
{
code: 'css_unused_selector',
message: 'Unused CSS selector "x:has(> z)"',
start: {
line: 91,
line: 92,
column: 1,
character: 1021
character: 1029
},
end: {
line: 91,
line: 92,
column: 11,
character: 1031
character: 1039
}
}
]
Expand Down
7 changes: 7 additions & 0 deletions packages/svelte/tests/css/samples/has/expected.css
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,10 @@
x.svelte-xyz > y:where(.svelte-xyz):has(z:where(.svelte-xyz)) {
color: green;
}

x.svelte-xyz:has(y:where(.svelte-xyz)) z:where(.svelte-xyz) {
color: green;
}
x.svelte-xyz:has(y:where(.svelte-xyz)) + c:where(.svelte-xyz) {
color: green;
}
Loading

0 comments on commit 5669b31

Please sign in to comment.