From 1089c7b157ecaf14bda3dd672836b190c82373c6 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Wed, 6 Nov 2024 11:53:58 +0100 Subject: [PATCH] fix: remove scoping for `:not` selectors fixes #14168 This reverts the whole "selectors inside `:not` are scoped" logic. Scoping is done so that styles don't bleed. But within `:not`,everything is reversed, which means scoping the selectors now means they are more likely to bleed. That is the opposite of what we want to achieve, therefore we should just leave those selectors alone. --- .changeset/quick-eels-occur.md | 5 ++ packages/svelte/src/compiler/migrate/index.js | 6 +- .../phases/2-analyze/css/css-prune.js | 41 ++++---------- .../compiler/phases/3-transform/css/index.js | 55 +++++++++++-------- .../samples/not-selector-global/_config.js | 17 +----- .../samples/not-selector-global/expected.css | 14 ++--- .../samples/not-selector-global/input.svelte | 2 +- .../_config.js | 5 -- .../expected.css | 4 -- .../expected.html | 1 - .../input.svelte | 8 --- .../tests/css/samples/not-selector/_config.js | 22 ++------ .../css/samples/not-selector/expected.css | 14 ++--- .../css/samples/not-selector/input.svelte | 2 +- .../samples/is-not-where-has/output.svelte | 10 ++-- 15 files changed, 77 insertions(+), 129 deletions(-) create mode 100644 .changeset/quick-eels-occur.md delete mode 100644 packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/_config.js delete mode 100644 packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/expected.css delete mode 100644 packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/expected.html delete mode 100644 packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/input.svelte diff --git a/.changeset/quick-eels-occur.md b/.changeset/quick-eels-occur.md new file mode 100644 index 0000000000..e5eb153fbf --- /dev/null +++ b/.changeset/quick-eels-occur.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: remove scoping for `:not` selectors diff --git a/packages/svelte/src/compiler/migrate/index.js b/packages/svelte/src/compiler/migrate/index.js index 8e8670c824..387b6a5485 100644 --- a/packages/svelte/src/compiler/migrate/index.js +++ b/packages/svelte/src/compiler/migrate/index.js @@ -50,9 +50,9 @@ function migrate_css(state) { while (code) { if ( code.startsWith(':has') || - code.startsWith(':not') || code.startsWith(':is') || - code.startsWith(':where') + code.startsWith(':where') || + code.startsWith(':not') ) { let start = code.indexOf('(') + 1; let is_global = false; @@ -74,7 +74,7 @@ function migrate_css(state) { char = code[end]; } if (start && end) { - if (!is_global) { + if (!is_global && !code.startsWith(':not')) { str.prependLeft(starting + start, ':global('); str.appendRight(starting + end - 1, ')'); } 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 fae89f11ed..bf088fe84c 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 @@ -162,7 +162,9 @@ const visitors = { }; /** - * Discard trailing `:global(...)` selectors without a `:has/is/where/not(...)` modifier, these are unused for scoping purposes + * Discard trailing `:global(...)` selectors without a `:has/is/where(...)` modifier, these are unused for scoping purposes + * `:not(...)` modifiers are not considered, because they should stay unscoped, because scoping them would achieve the + * opposite of what we want, because they are then _more_ likely to bleed out of the component. * @param {Compiler.Css.ComplexSelector} node */ function truncate(node) { @@ -172,16 +174,13 @@ function truncate(node) { // not after a :global selector !metadata.is_global_like && !(first.type === 'PseudoClassSelector' && first.name === 'global' && first.args === null) && - // not a :global(...) without a :has/is/where/not(...) modifier + // not a :global(...) without a :has/is/where(...) modifier (!metadata.is_global || selectors.some( (selector) => selector.type === 'PseudoClassSelector' && selector.args !== null && - (selector.name === 'has' || - selector.name === 'not' || - selector.name === 'is' || - selector.name === 'where') + (selector.name === 'has' || selector.name === 'is' || selector.name === 'where') )) ); }); @@ -512,7 +511,10 @@ function relative_selector_might_apply_to_node(relative_selector, rule, element, // We came across a :global, everything beyond it is global and therefore a potential match if (name === 'global' && selector.args === null) return true; - if ((name === 'is' || name === 'where' || name === 'not') && selector.args) { + // We ignore :not(...) because its contents should stay unscoped. Scoping them would achieve the + // opposite of what we want, because they are then _more_ likely to bleed out of the component, + // because there would be more chances of the inner selector not matching, which means `:not` matches. + if ((name === 'is' || name === 'where') && selector.args) { let matched = false; for (const complex_selector of selector.args.children) { @@ -522,32 +524,9 @@ function relative_selector_might_apply_to_node(relative_selector, rule, element, if (is_global) { complex_selector.metadata.used = true; matched = true; - } else if (name !== 'not' && apply_selector(relative, rule, element, state)) { + } else if (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. diff --git a/packages/svelte/src/compiler/phases/3-transform/css/index.js b/packages/svelte/src/compiler/phases/3-transform/css/index.js index 62c520e994..406448f606 100644 --- a/packages/svelte/src/compiler/phases/3-transform/css/index.js +++ b/packages/svelte/src/compiler/phases/3-transform/css/index.js @@ -278,29 +278,10 @@ const visitors = { ComplexSelector(node, context) { const before_bumped = context.state.specificity.bumped; - /** - * @param {Css.PseudoClassSelector} selector - * @param {Css.Combinator | null} combinator - */ - function remove_global_pseudo_class(selector, combinator) { - if (selector.args === null) { - let start = selector.start; - if (combinator?.name === ' ') { - // div :global.x becomes div.x - while (/\s/.test(context.state.code.original[start - 1])) start--; - } - context.state.code.remove(start, selector.start + ':global'.length); - } else { - context.state.code - .remove(selector.start, selector.start + ':global('.length) - .remove(selector.end - 1, selector.end); - } - } - for (const relative_selector of node.children) { if (relative_selector.metadata.is_global) { const global = /** @type {Css.PseudoClassSelector} */ (relative_selector.selectors[0]); - remove_global_pseudo_class(global, relative_selector.combinator); + remove_global_pseudo_class(global, relative_selector.combinator, context.state); if ( node.metadata.rule?.metadata.parent_rule && @@ -328,7 +309,7 @@ const visitors = { // for any :global() or :global at the middle of compound selector for (const selector of relative_selector.selectors) { if (selector.type === 'PseudoClassSelector' && selector.name === 'global') { - remove_global_pseudo_class(selector, null); + remove_global_pseudo_class(selector, null, context.state); } } @@ -374,12 +355,42 @@ const visitors = { context.state.specificity.bumped = before_bumped; }, PseudoClassSelector(node, context) { - if (node.name === 'is' || node.name === 'where' || node.name === 'has' || node.name === 'not') { + if (node.name === 'is' || node.name === 'where' || node.name === 'has') { context.next(); } + if (node.name === 'not' && node.args) { + for (const complex_selector of node.args.children) { + for (const relative_selector of complex_selector.children) { + if (relative_selector.metadata.is_global) { + const global = /** @type {Css.PseudoClassSelector} */ (relative_selector.selectors[0]); + remove_global_pseudo_class(global, relative_selector.combinator, context.state); + } + } + } + } } }; +/** + * @param {Css.PseudoClassSelector} selector + * @param {Css.Combinator | null} combinator + * @param {State} state + */ +function remove_global_pseudo_class(selector, combinator, state) { + if (selector.args === null) { + let start = selector.start; + if (combinator?.name === ' ') { + // div :global.x becomes div.x + while (/\s/.test(state.code.original[start - 1])) start--; + } + state.code.remove(start, selector.start + ':global'.length); + } else { + state.code + .remove(selector.start, selector.start + ':global('.length) + .remove(selector.end - 1, selector.end); + } +} + /** * Walk backwards until we find a non-whitespace character * @param {number} end diff --git a/packages/svelte/tests/css/samples/not-selector-global/_config.js b/packages/svelte/tests/css/samples/not-selector-global/_config.js index 2776ac227a..292c6c49ac 100644 --- a/packages/svelte/tests/css/samples/not-selector-global/_config.js +++ b/packages/svelte/tests/css/samples/not-selector-global/_config.js @@ -1,20 +1,5 @@ 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 - } - } - ] + warnings: [] }); diff --git a/packages/svelte/tests/css/samples/not-selector-global/expected.css b/packages/svelte/tests/css/samples/not-selector-global/expected.css index f90edd0c88..0c60de5bd0 100644 --- a/packages/svelte/tests/css/samples/not-selector-global/expected.css +++ b/packages/svelte/tests/css/samples/not-selector-global/expected.css @@ -2,18 +2,18 @@ .svelte-xyz:not(.foo) { color: green; } - .svelte-xyz:not(.foo:where(.svelte-xyz)):not(.unused) { + .svelte-xyz:not(.foo):not(.unused) { color: green; } - .x:not(.foo.svelte-xyz) { + .x:not(.foo) { color: green; } - /* (unused) :global(.x) :not(p) { - color: red; - }*/ - .x:not(p.svelte-xyz) { + .x .svelte-xyz:not(p) { color: red; /* TODO would be nice to prune this one day */ } - .x .svelte-xyz:not(.unused:where(.svelte-xyz)) { + .x:not(p) { + color: red; /* TODO would be nice to prune this one day */ + } + .x .svelte-xyz:not(.unused) { color: green; } diff --git a/packages/svelte/tests/css/samples/not-selector-global/input.svelte b/packages/svelte/tests/css/samples/not-selector-global/input.svelte index 578052d943..f6debbd9e6 100644 --- a/packages/svelte/tests/css/samples/not-selector-global/input.svelte +++ b/packages/svelte/tests/css/samples/not-selector-global/input.svelte @@ -12,7 +12,7 @@ color: green; } :global(.x) :not(p) { - color: red; + color: red; /* TODO would be nice to prune this one day */ } :global(.x):not(p) { color: red; /* TODO would be nice to prune this one day */ 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 deleted file mode 100644 index 292c6c49ac..0000000000 --- a/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/_config.js +++ /dev/null @@ -1,5 +0,0 @@ -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 deleted file mode 100644 index 8fe09de6dc..0000000000 --- a/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/expected.css +++ /dev/null @@ -1,4 +0,0 @@ - - .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 deleted file mode 100644 index fa4ca95c60..0000000000 --- a/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/expected.html +++ /dev/null @@ -1 +0,0 @@ -

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 deleted file mode 100644 index 6599a21ca6..0000000000 --- a/packages/svelte/tests/css/samples/not-selector-hash-on-right-elements/input.svelte +++ /dev/null @@ -1,8 +0,0 @@ -

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 27b4f91a6a..2a568a8e20 100644 --- a/packages/svelte/tests/css/samples/not-selector/_config.js +++ b/packages/svelte/tests/css/samples/not-selector/_config.js @@ -2,32 +2,18 @@ import { test } from '../../test'; export default test({ warnings: [ - { - code: 'css_unused_selector', - message: 'Unused CSS selector ":not(p)"', - start: { - line: 11, - column: 1, - character: 125 - }, - end: { - line: 11, - column: 8, - character: 132 - } - }, { code: 'css_unused_selector', message: 'Unused CSS selector "p :not(.foo)"', start: { line: 22, column: 1, - character: 235 + character: 291 }, end: { line: 22, column: 13, - character: 247 + character: 303 } }, { @@ -36,12 +22,12 @@ export default test({ start: { line: 25, column: 1, - character: 268 + character: 324 }, end: { line: 25, column: 16, - character: 283 + character: 339 } } ] diff --git a/packages/svelte/tests/css/samples/not-selector/expected.css b/packages/svelte/tests/css/samples/not-selector/expected.css index b4d51bf269..b0db7de3d9 100644 --- a/packages/svelte/tests/css/samples/not-selector/expected.css +++ b/packages/svelte/tests/css/samples/not-selector/expected.css @@ -1,19 +1,19 @@ - .svelte-xyz:not(.foo:where(.svelte-xyz)) { + .svelte-xyz:not(.foo) { color: green; } - .svelte-xyz:not(.unused:where(.svelte-xyz)) { + .svelte-xyz:not(.unused) { color: green; } - /* (unused) :not(p) { - color: red; - }*/ + .svelte-xyz:not(p) { + color: red; /* TODO would be nice to mark this as unused someday */ + } - .svelte-xyz:not(.foo:where(.svelte-xyz)):not(.unused:where(.svelte-xyz)) { + .svelte-xyz:not(.foo):not(.unused) { color: green; } - p.svelte-xyz:not(.foo:where(.svelte-xyz)) { + p.svelte-xyz:not(.foo) { color: green; } /* (unused) p :not(.foo) { diff --git a/packages/svelte/tests/css/samples/not-selector/input.svelte b/packages/svelte/tests/css/samples/not-selector/input.svelte index a0e72c7be8..993e9cca19 100644 --- a/packages/svelte/tests/css/samples/not-selector/input.svelte +++ b/packages/svelte/tests/css/samples/not-selector/input.svelte @@ -9,7 +9,7 @@ color: green; } :not(p) { - color: red; + color: red; /* TODO would be nice to mark this as unused someday */ } :not(.foo):not(.unused) { diff --git a/packages/svelte/tests/migrate/samples/is-not-where-has/output.svelte b/packages/svelte/tests/migrate/samples/is-not-where-has/output.svelte index 1e02e9bdfd..d8bb703ac5 100644 --- a/packages/svelte/tests/migrate/samples/is-not-where-has/output.svelte +++ b/packages/svelte/tests/migrate/samples/is-not-where-has/output.svelte @@ -21,22 +21,22 @@ what if i'm talking about `:has()` in my blog?