msb-public
/
svelte
mirror of https://github.com/sveltejs/svelte
1
0
Code Issues Projects Releases Wiki Activity

fix: ensure `:has` selectors followed by other selectors match (#13824)

Browse Source 
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
pull/13839/head
Simon H 11 months ago committed by GitHub
parent c603553e89
commit 5669b31fe6
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 96 additions and 88 deletions
Show Stats Download Patch File Download Diff File

5
.changeset/strong-feet-happen.md
Unescape View File

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: ensure `:has` selectors followed by other selectors match

100
packages/svelte/src/compiler/phases/2-analyze/css/css-prune.js
Unescape View File

@ -127,8 +127,7 @@ const visitors = {
selectors_to_check, selectors_to_check,
/** @type {Compiler.Css.Rule} */ (node.metadata.rule), /** @type {Compiler.Css.Rule} */ (node.metadata.rule),
element, element,
context.state.stylesheet, context.state.stylesheet
true
) )
) { ) {
mark(inner, element); mark(inner, element);
@ -144,8 +143,7 @@ const visitors = {
selectors, selectors,
/** @type {Compiler.Css.Rule} */ (node.metadata.rule), /** @type {Compiler.Css.Rule} */ (node.metadata.rule),
context.state.element, context.state.element,
context.state.stylesheet, context.state.stylesheet
true
) )
) { ) {
mark(inner, context.state.element); mark(inner, context.state.element);
@ -191,10 +189,9 @@ function truncate(node) {
* @param {Compiler.Css.Rule} rule * @param {Compiler.Css.Rule} rule
* @param {Compiler.AST.RegularElement | Compiler.AST.SvelteElement} element * @param {Compiler.AST.RegularElement | Compiler.AST.SvelteElement} element
* @param {Compiler.Css.StyleSheet} stylesheet * @param {Compiler.Css.StyleSheet} stylesheet
* @param {boolean} check_has Whether or not to check the `:has(...)` selectors
* @returns {boolean} * @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 parent_selectors = relative_selectors.slice();
const relative_selector = parent_selectors.pop(); const relative_selector = parent_selectors.pop();
@ -202,17 +199,11 @@ function apply_selector(relative_selectors, rule, element, stylesheet, check_has
const possible_match = relative_selector_might_apply_to_node( const possible_match = relative_selector_might_apply_to_node(
relative_selector, relative_selector,
parent_selectors,
rule, rule,
element, element,
stylesheet, stylesheet
check_has
); );
if (possible_match === 'definite_match') {
return true;
}
if (!possible_match) { if (!possible_match) {
return false; return false;
} }
@ -224,8 +215,7 @@ function apply_selector(relative_selectors, rule, element, stylesheet, check_has
parent_selectors, parent_selectors,
rule, rule,
element, element,
stylesheet, stylesheet
check_has
); );
} }
@ -246,7 +236,6 @@ function apply_selector(relative_selectors, rule, element, stylesheet, check_has
* @param {Compiler.Css.Rule} rule * @param {Compiler.Css.Rule} rule
* @param {Compiler.AST.RegularElement | Compiler.AST.SvelteElement} element * @param {Compiler.AST.RegularElement | Compiler.AST.SvelteElement} element
* @param {Compiler.Css.StyleSheet} stylesheet * @param {Compiler.Css.StyleSheet} stylesheet
* @param {boolean} check_has Whether or not to check the `:has(...)` selectors
* @returns {boolean} * @returns {boolean}
*/ */
function apply_combinator( function apply_combinator(
@ -255,8 +244,7 @@ function apply_combinator(
parent_selectors, parent_selectors,
rule, rule,
element, element,
stylesheet, stylesheet
check_has
) { ) {
const name = combinator.name; const name = combinator.name;
@ -281,7 +269,7 @@ function apply_combinator(
} }
if (parent.type === 'RegularElement' || parent.type === 'SvelteElement') { 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... // TODO the `name === ' '` causes false positives, but removing it causes false negatives...
if (name === ' ' || crossed_component_boundary) { if (name === ' ' || crossed_component_boundary) {
mark(parent_selectors[parent_selectors.length - 1], parent); mark(parent_selectors[parent_selectors.length - 1], parent);
@ -312,9 +300,7 @@ function apply_combinator(
mark(relative_selector, element); mark(relative_selector, element);
sibling_matched = true; sibling_matched = true;
} }
} else if ( } else if (apply_selector(parent_selectors, rule, possible_sibling, stylesheet)) {
apply_selector(parent_selectors, rule, possible_sibling, stylesheet, check_has)
) {
mark(relative_selector, element); mark(relative_selector, element);
sibling_matched = true; sibling_matched = true;
} }
@ -393,21 +379,12 @@ const regex_backslash_and_following_character = /\\(.)/g;
* Ensure that `element` satisfies each simple selector in `relative_selector` * Ensure that `element` satisfies each simple selector in `relative_selector`
* *
* @param {Compiler.Css.RelativeSelector} relative_selector * @param {Compiler.Css.RelativeSelector} relative_selector
* @param {Compiler.Css.RelativeSelector[]} parent_selectors
* @param {Compiler.Css.Rule} rule * @param {Compiler.Css.Rule} rule
* @param {Compiler.AST.RegularElement | Compiler.AST.SvelteElement} element * @param {Compiler.AST.RegularElement | Compiler.AST.SvelteElement} element
* @param {Compiler.Css.StyleSheet} stylesheet * @param {Compiler.Css.StyleSheet} stylesheet
* @param {boolean} check_has Whether or not to check the `:has(...)` selectors * @returns {boolean }
* @returns {boolean | 'definite_match'}
*/ */
function relative_selector_might_apply_to_node( function relative_selector_might_apply_to_node(relative_selector, rule, element, stylesheet) {
relative_selector,
parent_selectors,
rule,
element,
stylesheet,
check_has
) {
// Sort :has(...) selectors in one bucket and everything else into another // Sort :has(...) selectors in one bucket and everything else into another
const has_selectors = []; const has_selectors = [];
const other_selectors = []; const other_selectors = [];
@ -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. // 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. // 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 // :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 // 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`. // selector in a way that is similar to ancestor matching. In a sense, we're treating `.x:has(.y)` as `.x .y`.
@ -443,25 +439,20 @@ function relative_selector_might_apply_to_node(
}; };
} }
if ( const descendants =
selectors.length === 0 /* is :global(...) */ || left_most_combinator.name === '>' ? child_elements : descendant_elements;
apply_selector(selectors, rule, element, stylesheet, check_has)
) { let selector_matched = false;
// Treat e.g. `.x:has(.y)` as `.x .y` with the .y part already being matched,
// and now looking upwards for the .x part. // Iterate over all descendant elements and check if the selector inside :has matches
for (const element of descendants) {
if ( if (
apply_combinator( selectors.length === 0 /* is :global(...) */ ||
left_most_combinator, (element.metadata.scoped && selector_matched) ||
selectors[0] ?? [], apply_selector(selectors, rule, element, stylesheet)
[...parent_selectors, relative_selector],
rule,
element,
stylesheet,
false
)
) { ) {
complex_selector.metadata.used = true; complex_selector.metadata.used = true;
matched = true; selector_matched = matched = true;
} }
} }
} }
@ -484,9 +475,6 @@ function relative_selector_might_apply_to_node(
return false; 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) { for (const selector of other_selectors) {
@ -507,7 +495,7 @@ function relative_selector_might_apply_to_node(
) { ) {
const args = selector.args; const args = selector.args;
const complex_selector = args.children[0]; 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 // We came across a :global, everything beyond it is global and therefore a potential match
@ -520,7 +508,7 @@ function relative_selector_might_apply_to_node(
const relative = truncate(complex_selector); const relative = truncate(complex_selector);
if ( if (
relative.length === 0 /* is :global(...) */ || relative.length === 0 /* is :global(...) */ ||
apply_selector(relative, rule, element, stylesheet, check_has) apply_selector(relative, rule, element, stylesheet)
) { ) {
complex_selector.metadata.used = true; complex_selector.metadata.used = true;
matched = true; matched = true;
@ -621,7 +609,7 @@ function relative_selector_might_apply_to_node(
const parent = /** @type {Compiler.Css.Rule} */ (rule.metadata.parent_rule); const parent = /** @type {Compiler.Css.Rule} */ (rule.metadata.parent_rule);
for (const complex_selector of parent.prelude.children) { 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; complex_selector.metadata.used = true;
matched = true; matched = true;
} }

64
packages/svelte/tests/css/samples/has/_config.js
Unescape View File

@ -6,112 +6,112 @@ export default test({
code: 'css_unused_selector', code: 'css_unused_selector',
message: 'Unused CSS selector ".unused:has(y)"', message: 'Unused CSS selector ".unused:has(y)"',
start: { start: {
character: 269, line: 28,
column: 1, column: 1,
line: 27 character: 277
}, },
end: { end: {
character: 283, line: 28,
column: 15, column: 15,
line: 27 character: 291
} }
}, },
{ {
code: 'css_unused_selector', code: 'css_unused_selector',
message: 'Unused CSS selector ".unused:has(:global(y))"', message: 'Unused CSS selector ".unused:has(:global(y))"',
start: { start: {
character: 304, line: 31,
column: 1, column: 1,
line: 30 character: 312
}, },
end: { end: {
character: 327, line: 31,
column: 24, column: 24,
line: 30 character: 335
} }
}, },
{ {
code: 'css_unused_selector', code: 'css_unused_selector',
message: 'Unused CSS selector "x:has(.unused)"', message: 'Unused CSS selector "x:has(.unused)"',
start: { start: {
character: 348, line: 34,
column: 1, column: 1,
line: 33 character: 356
}, },
end: { end: {
character: 362, line: 34,
column: 15, column: 15,
line: 33 character: 370
} }
}, },
{ {
code: 'css_unused_selector', code: 'css_unused_selector',
message: 'Unused CSS selector "x:has(y):has(.unused)"', message: 'Unused CSS selector "x:has(y):has(.unused)"',
start: { start: {
character: 517, line: 47,
column: 1, column: 1,
line: 46 character: 525
}, },
end: { end: {
character: 538, line: 47,
column: 22, column: 22,
line: 46 character: 546
} }
}, },
{ {
code: 'css_unused_selector', code: 'css_unused_selector',
message: 'Unused CSS selector ".unused"', message: 'Unused CSS selector ".unused"',
start: { start: {
character: 743, line: 66,
column: 2, column: 2,
line: 65 character: 751
}, },
end: { end: {
character: 750, line: 66,
column: 9, column: 9,
line: 65 character: 758
} }
}, },
{ {
code: 'css_unused_selector', code: 'css_unused_selector',
message: 'Unused CSS selector ".unused x:has(y)"', message: 'Unused CSS selector ".unused x:has(y)"',
start: { start: {
character: 897, line: 82,
column: 1, column: 1,
line: 81 character: 905
}, },
end: { end: {
character: 913, line: 82,
column: 17, column: 17,
line: 81 character: 921
} }
}, },
{ {
code: 'css_unused_selector', code: 'css_unused_selector',
message: 'Unused CSS selector ".unused:has(.unused)"', message: 'Unused CSS selector ".unused:has(.unused)"',
start: { start: {
line: 84, line: 85,
column: 1, column: 1,
character: 934 character: 942
}, },
end: { end: {
line: 84, line: 85,
column: 21, column: 21,
character: 954 character: 962
} }
}, },
{ {
code: 'css_unused_selector', code: 'css_unused_selector',
message: 'Unused CSS selector "x:has(> z)"', message: 'Unused CSS selector "x:has(> z)"',
start: { start: {
line: 91, line: 92,
column: 1, column: 1,
character: 1021 character: 1029
}, },
end: { end: {
line: 91, line: 92,
column: 11, column: 11,
character: 1031 character: 1039
} }
} }
] ]

7
packages/svelte/tests/css/samples/has/expected.css
Unescape View File

@ -88,3 +88,10 @@
x.svelte-xyz > y:where(.svelte-xyz):has(z:where(.svelte-xyz)) { x.svelte-xyz > y:where(.svelte-xyz):has(z:where(.svelte-xyz)) {
color: green; 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;
}

8
packages/svelte/tests/css/samples/has/input.svelte
Unescape View File

@ -3,6 +3,7 @@
<z></z> <z></z>
</y> </y>
</x> </x>
<c></c>
<style> <style>
x:has(y) { x:has(y) {
@ -94,4 +95,11 @@
x > y:has(z) { x > y:has(z) {
color: green; color: green;
} }
x:has(y) z {
color: green;
}
x:has(y) + c {
color: green;
}
</style> </style>

Write Preview
Loading…
Cancel
Save