From 7dbe812fc92ad87e30a91ddb3270261964531c54 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Mon, 4 Nov 2024 22:49:54 +0100 Subject: [PATCH] fix: ensure non-matching elements are scoped for `:not(...)` selector (#13999) If the contents of a `:not` selector don't match, then it's actually a match for `:not` because it's inverted. Therefore, we need to scope such elements. We're also making sure that contents of `:not` that never match actually count as a used (because the result is negated), and as such the contents of `:not` that always match are actually marked as unused. Fixes #13974 --- .changeset/dirty-parrots-smell.md | 5 ++ .../phases/2-analyze/css/css-prune.js | 89 ++++++++++++------- .../samples/not-selector-global/_config.js | 20 +++++ .../samples/not-selector-global/expected.css | 19 ++++ .../samples/not-selector-global/expected.html | 1 + .../samples/not-selector-global/input.svelte | 23 +++++ .../_config.js | 5 ++ .../expected.css | 4 + .../expected.html | 1 + .../input.svelte | 8 ++ .../tests/css/samples/not-selector/_config.js | 46 ++++------ .../css/samples/not-selector/expected.css | 21 ++--- .../css/samples/not-selector/expected.html | 1 + .../css/samples/not-selector/input.svelte | 19 ++-- 14 files changed, 172 insertions(+), 90 deletions(-) create mode 100644 .changeset/dirty-parrots-smell.md create mode 100644 packages/svelte/tests/css/samples/not-selector-global/_config.js create mode 100644 packages/svelte/tests/css/samples/not-selector-global/expected.css create mode 100644 packages/svelte/tests/css/samples/not-selector-global/expected.html create mode 100644 packages/svelte/tests/css/samples/not-selector-global/input.svelte create mode 100644 packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/_config.js create mode 100644 packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/expected.css create mode 100644 packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/expected.html create mode 100644 packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/input.svelte create mode 100644 packages/svelte/tests/css/samples/not-selector/expected.html diff --git a/.changeset/dirty-parrots-smell.md b/.changeset/dirty-parrots-smell.md new file mode 100644 index 0000000000..50864d20a4 --- /dev/null +++ b/.changeset/dirty-parrots-smell.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: ensure non-matching elements are scoped for `:not(...)` selector diff --git a/packages/svelte/src/compiler/phases/2-analyze/css/css-prune.js b/packages/svelte/src/compiler/phases/2-analyze/css/css-prune.js index f71a860958..fae89f11ed 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/css/css-prune.js +++ b/packages/svelte/src/compiler/phases/2-analyze/css/css-prune.js @@ -10,6 +10,7 @@ import { get_attribute_chunks, is_text_attribute } from '../../../utils/ast.js'; * stylesheet: Compiler.Css.StyleSheet; * element: Compiler.AST.RegularElement | Compiler.AST.SvelteElement; * from_render_tag: boolean; + * inside_not: boolean; * }} State */ /** @typedef {NODE_PROBABLY_EXISTS | NODE_DEFINITELY_EXISTS} NodeExistsValue */ @@ -61,9 +62,13 @@ export function prune(stylesheet, element) { const parent = get_element_parent(element); if (!parent) return; - walk(stylesheet, { stylesheet, element: parent, from_render_tag: true }, visitors); + walk( + stylesheet, + { stylesheet, element: parent, from_render_tag: true, inside_not: false }, + visitors + ); } else { - walk(stylesheet, { stylesheet, element, from_render_tag: false }, visitors); + walk(stylesheet, { stylesheet, element, from_render_tag: false, inside_not: false }, visitors); } } @@ -127,7 +132,7 @@ const visitors = { selectors_to_check, /** @type {Compiler.Css.Rule} */ (node.metadata.rule), element, - context.state.stylesheet + context.state ) ) { mark(inner, element); @@ -143,7 +148,7 @@ const visitors = { selectors, /** @type {Compiler.Css.Rule} */ (node.metadata.rule), context.state.element, - context.state.stylesheet + context.state ) ) { mark(inner, context.state.element); @@ -188,10 +193,10 @@ function truncate(node) { * @param {Compiler.Css.RelativeSelector[]} relative_selectors * @param {Compiler.Css.Rule} rule * @param {Compiler.AST.RegularElement | Compiler.AST.SvelteElement} element - * @param {Compiler.Css.StyleSheet} stylesheet + * @param {State} state * @returns {boolean} */ -function apply_selector(relative_selectors, rule, element, stylesheet) { +function apply_selector(relative_selectors, rule, element, state) { const parent_selectors = relative_selectors.slice(); const relative_selector = parent_selectors.pop(); @@ -201,7 +206,7 @@ function apply_selector(relative_selectors, rule, element, stylesheet) { relative_selector, rule, element, - stylesheet + state ); if (!possible_match) { @@ -215,14 +220,14 @@ function apply_selector(relative_selectors, rule, element, stylesheet) { parent_selectors, rule, element, - stylesheet + state ); } // if this is the left-most non-global selector, mark it — we want // `x y z {...}` to become `x.blah y z.blah {...}` const parent = parent_selectors[parent_selectors.length - 1]; - if (!parent || is_global(parent, rule)) { + if (!state.inside_not && (!parent || is_global(parent, rule))) { mark(relative_selector, element); } @@ -235,17 +240,10 @@ function apply_selector(relative_selectors, rule, element, stylesheet) { * @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 {State} state * @returns {boolean} */ -function apply_combinator( - combinator, - relative_selector, - parent_selectors, - rule, - element, - stylesheet -) { +function apply_combinator(combinator, relative_selector, parent_selectors, rule, element, state) { const name = combinator.name; switch (name) { @@ -269,9 +267,9 @@ function apply_combinator( } if (parent.type === 'RegularElement' || parent.type === 'SvelteElement') { - if (apply_selector(parent_selectors, rule, parent, stylesheet)) { + if (apply_selector(parent_selectors, rule, parent, state)) { // TODO the `name === ' '` causes false positives, but removing it causes false negatives... - if (name === ' ' || crossed_component_boundary) { + if (!state.inside_not && (name === ' ' || crossed_component_boundary)) { mark(parent_selectors[parent_selectors.length - 1], parent); } @@ -297,11 +295,15 @@ function apply_combinator( if (possible_sibling.type === 'RenderTag' || possible_sibling.type === 'SlotElement') { // `{@render foo()}

foo

` with `:global(.x) + p` is a match if (parent_selectors.length === 1 && parent_selectors[0].metadata.is_global) { - mark(relative_selector, element); + if (!state.inside_not) { + mark(relative_selector, element); + } sibling_matched = true; } - } else if (apply_selector(parent_selectors, rule, possible_sibling, stylesheet)) { - mark(relative_selector, element); + } else if (apply_selector(parent_selectors, rule, possible_sibling, state)) { + if (!state.inside_not) { + mark(relative_selector, element); + } sibling_matched = true; } } @@ -381,10 +383,10 @@ const regex_backslash_and_following_character = /\\(.)/g; * @param {Compiler.Css.RelativeSelector} relative_selector * @param {Compiler.Css.Rule} rule * @param {Compiler.AST.RegularElement | Compiler.AST.SvelteElement} element - * @param {Compiler.Css.StyleSheet} stylesheet + * @param {State} state * @returns {boolean } */ -function relative_selector_might_apply_to_node(relative_selector, rule, element, stylesheet) { +function relative_selector_might_apply_to_node(relative_selector, rule, element, state) { // Sort :has(...) selectors in one bucket and everything else into another const has_selectors = []; const other_selectors = []; @@ -458,7 +460,7 @@ function relative_selector_might_apply_to_node(relative_selector, rule, element, if ( selectors.length === 0 /* is :global(...) */ || (element.metadata.scoped && selector_matched) || - apply_selector(selectors, rule, element, stylesheet) + apply_selector(selectors, rule, element, state) ) { complex_selector.metadata.used = true; selector_matched = matched = true; @@ -504,7 +506,7 @@ function relative_selector_might_apply_to_node(relative_selector, rule, element, ) { const args = selector.args; const complex_selector = args.children[0]; - return apply_selector(complex_selector.children, rule, element, stylesheet); + return apply_selector(complex_selector.children, rule, element, state); } // We came across a :global, everything beyond it is global and therefore a potential match @@ -515,12 +517,37 @@ function relative_selector_might_apply_to_node(relative_selector, rule, element, for (const complex_selector of selector.args.children) { const relative = truncate(complex_selector); - if ( - relative.length === 0 /* is :global(...) */ || - apply_selector(relative, rule, element, stylesheet) + const is_global = relative.length === 0; + + if (is_global) { + complex_selector.metadata.used = true; + matched = true; + } else if (name !== 'not' && apply_selector(relative, rule, element, state)) { + complex_selector.metadata.used = true; + matched = true; + } else if ( + name === 'not' && + !apply_selector(relative, rule, element, { ...state, inside_not: true }) ) { + // For `:not(...)` we gotta do the inverse: If it did not match, mark the element and possibly + // everything above (if the selector is written is a such) as scoped (because they matched by not matching). + element.metadata.scoped = true; complex_selector.metadata.used = true; matched = true; + + for (const r of relative) { + r.metadata.scoped = true; + } + + // bar:not(foo bar) means that foo is an ancestor of bar + if (complex_selector.children.length > 1) { + /** @type {Compiler.AST.RegularElement | Compiler.AST.SvelteElement | null} */ + let el = get_element_parent(element); + while (el) { + el.metadata.scoped = true; + el = get_element_parent(el); + } + } } else if (complex_selector.children.length > 1 && (name == 'is' || name == 'where')) { // foo :is(bar baz) can also mean that bar is an ancestor of foo, and baz a descendant. // We can't fully check if that actually matches with our current algorithm, so we just assume it does. @@ -618,7 +645,7 @@ function relative_selector_might_apply_to_node(relative_selector, rule, element, 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)) { + if (apply_selector(truncate(complex_selector), parent, element, state)) { complex_selector.metadata.used = true; matched = true; } diff --git a/packages/svelte/tests/css/samples/not-selector-global/_config.js b/packages/svelte/tests/css/samples/not-selector-global/_config.js new file mode 100644 index 0000000000..2776ac227a --- /dev/null +++ b/packages/svelte/tests/css/samples/not-selector-global/_config.js @@ -0,0 +1,20 @@ +import { test } from '../../test'; + +export default test({ + warnings: [ + { + code: 'css_unused_selector', + message: 'Unused CSS selector ":global(.x) :not(p)"', + start: { + line: 14, + column: 1, + character: 197 + }, + end: { + line: 14, + column: 20, + character: 216 + } + } + ] +}); diff --git a/packages/svelte/tests/css/samples/not-selector-global/expected.css b/packages/svelte/tests/css/samples/not-selector-global/expected.css new file mode 100644 index 0000000000..f90edd0c88 --- /dev/null +++ b/packages/svelte/tests/css/samples/not-selector-global/expected.css @@ -0,0 +1,19 @@ + + .svelte-xyz:not(.foo) { + color: green; + } + .svelte-xyz:not(.foo:where(.svelte-xyz)):not(.unused) { + color: green; + } + .x:not(.foo.svelte-xyz) { + color: green; + } + /* (unused) :global(.x) :not(p) { + color: red; + }*/ + .x:not(p.svelte-xyz) { + color: red; /* TODO would be nice to prune this one day */ + } + .x .svelte-xyz:not(.unused:where(.svelte-xyz)) { + color: green; + } diff --git a/packages/svelte/tests/css/samples/not-selector-global/expected.html b/packages/svelte/tests/css/samples/not-selector-global/expected.html new file mode 100644 index 0000000000..b4834bb818 --- /dev/null +++ b/packages/svelte/tests/css/samples/not-selector-global/expected.html @@ -0,0 +1 @@ +

foo

bar

\ No newline at end of file diff --git a/packages/svelte/tests/css/samples/not-selector-global/input.svelte b/packages/svelte/tests/css/samples/not-selector-global/input.svelte new file mode 100644 index 0000000000..578052d943 --- /dev/null +++ b/packages/svelte/tests/css/samples/not-selector-global/input.svelte @@ -0,0 +1,23 @@ +

foo

+

bar

+ + \ No newline at end of file diff --git a/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/_config.js b/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/_config.js new file mode 100644 index 0000000000..292c6c49ac --- /dev/null +++ b/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/_config.js @@ -0,0 +1,5 @@ +import { test } from '../../test'; + +export default test({ + warnings: [] +}); diff --git a/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/expected.css b/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/expected.css new file mode 100644 index 0000000000..8fe09de6dc --- /dev/null +++ b/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/expected.css @@ -0,0 +1,4 @@ + + .svelte-xyz:not(.bar:where(.svelte-xyz)) { + color: green; + } diff --git a/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/expected.html b/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/expected.html new file mode 100644 index 0000000000..fa4ca95c60 --- /dev/null +++ b/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/expected.html @@ -0,0 +1 @@ +

foo

bar

\ No newline at end of file diff --git a/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/input.svelte b/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/input.svelte new file mode 100644 index 0000000000..6599a21ca6 --- /dev/null +++ b/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/input.svelte @@ -0,0 +1,8 @@ +

foo

+

bar

+ + \ No newline at end of file diff --git a/packages/svelte/tests/css/samples/not-selector/_config.js b/packages/svelte/tests/css/samples/not-selector/_config.js index 6ab49a8809..27b4f91a6a 100644 --- a/packages/svelte/tests/css/samples/not-selector/_config.js +++ b/packages/svelte/tests/css/samples/not-selector/_config.js @@ -4,58 +4,44 @@ export default test({ warnings: [ { code: 'css_unused_selector', - message: 'Unused CSS selector ":not(.unused)"', + message: 'Unused CSS selector ":not(p)"', start: { - line: 8, + line: 11, column: 1, - character: 89 + character: 125 }, end: { - line: 8, - column: 14, - character: 102 - } - }, - { - code: 'css_unused_selector', - message: 'Unused CSS selector ":not(.foo):not(.unused)"', - start: { - line: 18, - column: 1, - character: 221 - }, - end: { - line: 18, - column: 24, - character: 244 + line: 11, + column: 8, + character: 132 } }, { code: 'css_unused_selector', message: 'Unused CSS selector "p :not(.foo)"', start: { - line: 25, + line: 22, column: 1, - character: 300 + character: 235 }, end: { - line: 25, + line: 22, column: 13, - character: 312 + character: 247 } }, { code: 'css_unused_selector', - message: 'Unused CSS selector ":global(.x) :not(.unused)"', + message: 'Unused CSS selector "p :not(.unused)"', start: { - line: 34, + line: 25, column: 1, - character: 469 + character: 268 }, end: { - line: 34, - column: 26, - character: 494 + line: 25, + column: 16, + character: 283 } } ] diff --git a/packages/svelte/tests/css/samples/not-selector/expected.css b/packages/svelte/tests/css/samples/not-selector/expected.css index 45e9ad69c0..b4d51bf269 100644 --- a/packages/svelte/tests/css/samples/not-selector/expected.css +++ b/packages/svelte/tests/css/samples/not-selector/expected.css @@ -2,19 +2,16 @@ .svelte-xyz:not(.foo:where(.svelte-xyz)) { color: green; } - /* (unused) :not(.unused) { - color: red; - }*/ - .svelte-xyz:not(.foo) { + .svelte-xyz:not(.unused:where(.svelte-xyz)) { color: green; } + /* (unused) :not(p) { + color: red; + }*/ - .svelte-xyz:not(.foo:where(.svelte-xyz)):not(.unused) { + .svelte-xyz:not(.foo:where(.svelte-xyz)):not(.unused:where(.svelte-xyz)) { color: green; } - /* (unused) :not(.foo):not(.unused) { - color: red; - }*/ p.svelte-xyz:not(.foo:where(.svelte-xyz)) { color: green; @@ -22,12 +19,6 @@ /* (unused) p :not(.foo) { color: red; }*/ - .x:not(.foo.svelte-xyz) { - color: green; - } - .x:not(.unused.svelte-xyz) { - color: red; /* TODO would be nice to prune this one day */ - } - /* (unused) :global(.x) :not(.unused) { + /* (unused) p :not(.unused) { color: red; }*/ diff --git a/packages/svelte/tests/css/samples/not-selector/expected.html b/packages/svelte/tests/css/samples/not-selector/expected.html new file mode 100644 index 0000000000..b4834bb818 --- /dev/null +++ b/packages/svelte/tests/css/samples/not-selector/expected.html @@ -0,0 +1 @@ +

foo

bar

\ No newline at end of file diff --git a/packages/svelte/tests/css/samples/not-selector/input.svelte b/packages/svelte/tests/css/samples/not-selector/input.svelte index 57d062cf49..a0e72c7be8 100644 --- a/packages/svelte/tests/css/samples/not-selector/input.svelte +++ b/packages/svelte/tests/css/samples/not-selector/input.svelte @@ -6,17 +6,14 @@ color: green; } :not(.unused) { - color: red; - } - :not(:global(.foo)) { color: green; } - - :not(.foo):not(:global(.unused)) { - color: green; + :not(p) { + color: red; } + :not(.foo):not(.unused) { - color: red; + color: green; } p:not(.foo) { @@ -25,13 +22,7 @@ p :not(.foo) { color: red; } - :global(.x):not(.foo) { - color: green; - } - :global(.x):not(.unused) { - color: red; /* TODO would be nice to prune this one day */ - } - :global(.x) :not(.unused) { + p :not(.unused) { color: red; } \ No newline at end of file