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.

Sign up for GitHub

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

fix: ensure :has selectors followed by other selectors match #13824

Merged
merged 1 commit into from
Oct 23, 2024
Merged

fix: ensure :has selectors followed by other selectors match #13824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/strong-feet-happen.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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