From aa23415179364d7f50f652c4e1a704e5a5d2ff16 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Wed, 6 Nov 2024 13:49:37 +0100 Subject: [PATCH] fix: remove scoping for most `:not` selectors (#14177) 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. The exception are `:not` selectors with descendant selectors, as that means "look up the tree" and we need to scope all ancestor elements in that case. --- .changeset/quick-eels-occur.md | 5 ++ packages/svelte/src/compiler/migrate/index.js | 6 +- .../phases/2-analyze/css/css-prune.js | 80 +++++++++---------- .../compiler/phases/3-transform/css/index.js | 43 +++++----- .../samples/not-selector-global/_config.js | 17 +--- .../samples/not-selector-global/expected.css | 26 ++++-- .../samples/not-selector-global/expected.html | 4 +- .../samples/not-selector-global/input.svelte | 20 ++++- .../_config.js | 5 -- .../expected.css | 4 - .../expected.html | 1 - .../input.svelte | 8 -- .../tests/css/samples/not-selector/_config.js | 38 +++------ .../css/samples/not-selector/expected.css | 25 +++--- .../css/samples/not-selector/expected.html | 4 +- .../css/samples/not-selector/input.svelte | 19 ++++- packages/svelte/tests/css/test.ts | 28 +++---- .../samples/is-not-where-has/output.svelte | 10 +-- 18 files changed, 172 insertions(+), 171 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 000000000000..e5eb153fbf5d --- /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 8e8670c8243e..387b6a5485e0 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 fae89f11edd8..4306d49d818f 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,7 +10,6 @@ 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 */ @@ -62,13 +61,9 @@ export function prune(stylesheet, element) { const parent = get_element_parent(element); if (!parent) return; - walk( - stylesheet, - { stylesheet, element: parent, from_render_tag: true, inside_not: false }, - visitors - ); + walk(stylesheet, { stylesheet, element: parent, from_render_tag: true }, visitors); } else { - walk(stylesheet, { stylesheet, element, from_render_tag: false, inside_not: false }, visitors); + walk(stylesheet, { stylesheet, element, from_render_tag: false }, visitors); } } @@ -179,9 +174,9 @@ function truncate(node) { selector.type === 'PseudoClassSelector' && selector.args !== null && (selector.name === 'has' || - selector.name === 'not' || selector.name === 'is' || - selector.name === 'where') + selector.name === 'where' || + selector.name === 'not') )) ); }); @@ -227,7 +222,7 @@ function apply_selector(relative_selectors, rule, element, 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 (!state.inside_not && (!parent || is_global(parent, rule))) { + if (!parent || is_global(parent, rule)) { mark(relative_selector, element); } @@ -269,7 +264,7 @@ function apply_combinator(combinator, relative_selector, parent_selectors, rule, if (parent.type === 'RegularElement' || parent.type === 'SvelteElement') { if (apply_selector(parent_selectors, rule, parent, state)) { // TODO the `name === ' '` causes false positives, but removing it causes false negatives... - if (!state.inside_not && (name === ' ' || crossed_component_boundary)) { + if (name === ' ' || crossed_component_boundary) { mark(parent_selectors[parent_selectors.length - 1], parent); } @@ -295,15 +290,11 @@ function apply_combinator(combinator, relative_selector, parent_selectors, rule, 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) { - if (!state.inside_not) { - mark(relative_selector, element); - } + mark(relative_selector, element); sibling_matched = true; } } else if (apply_selector(parent_selectors, rule, possible_sibling, state)) { - if (!state.inside_not) { - mark(relative_selector, element); - } + mark(relative_selector, element); sibling_matched = true; } } @@ -512,7 +503,35 @@ 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) { + // :not(...) 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. The exception is complex selectors + // with descendants, in which case we scope them all. + if (name === 'not' && selector.args) { + for (const complex_selector of selector.args.children) { + complex_selector.metadata.used = true; + const relative = truncate(complex_selector); + + if (complex_selector.children.length > 1) { + // foo:not(bar foo) means that bar is an ancestor of foo (side note: ending with foo is the only way the selector make sense). + // We can't fully check if that actually matches with our current algorithm, so we just assume it does. + // The result may not match a real element, so the only drawback is the missing prune. + for (const selector of relative) { + selector.metadata.scoped = true; + } + + /** @type {Compiler.AST.RegularElement | Compiler.AST.SvelteElement | null} */ + let el = element; + while (el) { + el.metadata.scoped = true; + el = get_element_parent(el); + } + } + } + + break; + } + + if ((name === 'is' || name === 'where') && selector.args) { let matched = false; for (const complex_selector of selector.args.children) { @@ -522,32 +541,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 62c520e99487..350eca35d8a6 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); } } @@ -380,6 +361,26 @@ const visitors = { } }; +/** + * @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 2776ac227af7..292c6c49ac9d 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 f90edd0c8801..ea9ff3948653 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,28 @@ .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) { - color: red; /* TODO would be nice to prune this one day */ + .x .svelte-xyz:not(p) { + color: green; + } + .x:not(p) { + color: green; + } + .x .svelte-xyz:not(.unused) { + color: green; + } + + span:not(p.svelte-xyz span:where(.svelte-xyz)) { + color: green; + } + span.svelte-xyz:not(p span) { + color: green; } - .x .svelte-xyz:not(.unused:where(.svelte-xyz)) { + span:not(p span) { 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 index b4834bb8189e..5eeb2be29429 100644 --- a/packages/svelte/tests/css/samples/not-selector-global/expected.html +++ b/packages/svelte/tests/css/samples/not-selector-global/expected.html @@ -1 +1,3 @@ -

foo

bar

\ No newline at end of file +

foo

+

bar baz

+buzz \ 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 index 578052d943ff..1f0e6cd6db1d 100644 --- a/packages/svelte/tests/css/samples/not-selector-global/input.svelte +++ b/packages/svelte/tests/css/samples/not-selector-global/input.svelte @@ -1,5 +1,9 @@

foo

-

bar

+

+ bar + baz +

+buzz \ 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 deleted file mode 100644 index 292c6c49ac9d..000000000000 --- 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 8fe09de6dc63..000000000000 --- 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 fa4ca95c605c..000000000000 --- 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 6599a21ca61f..000000000000 --- 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 27b4f91a6ac3..9c91a2a4c079 100644 --- a/packages/svelte/tests/css/samples/not-selector/_config.js +++ b/packages/svelte/tests/css/samples/not-selector/_config.js @@ -4,44 +4,30 @@ export default test({ warnings: [ { code: 'css_unused_selector', - message: 'Unused CSS selector ":not(p)"', + message: 'Unused CSS selector "span :not(.foo)"', start: { - line: 11, + line: 26, column: 1, - character: 125 + character: 276 }, 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 - }, - end: { - line: 22, - column: 13, - character: 247 + line: 26, + column: 16, + character: 291 } }, { code: 'css_unused_selector', - message: 'Unused CSS selector "p :not(.unused)"', + message: 'Unused CSS selector "span :not(.unused)"', start: { - line: 25, + line: 29, column: 1, - character: 268 + character: 312 }, end: { - line: 25, - column: 16, - character: 283 + line: 29, + column: 19, + character: 330 } } ] diff --git a/packages/svelte/tests/css/samples/not-selector/expected.css b/packages/svelte/tests/css/samples/not-selector/expected.css index b4d51bf269d8..ea0b3aacacd9 100644 --- a/packages/svelte/tests/css/samples/not-selector/expected.css +++ b/packages/svelte/tests/css/samples/not-selector/expected.css @@ -1,24 +1,31 @@ - .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; + } + .svelte-xyz:not(p) { color: green; } - /* (unused) :not(p) { - color: red; - }*/ - .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) { + /* (unused) span :not(.foo) { color: red; }*/ - /* (unused) p :not(.unused) { + /* (unused) span :not(.unused) { color: red; }*/ + + span.svelte-xyz:not(p:where(.svelte-xyz) span:where(.svelte-xyz)) { + color: green; + } + span.svelte-xyz:not(:focus) { + color: green; + } diff --git a/packages/svelte/tests/css/samples/not-selector/expected.html b/packages/svelte/tests/css/samples/not-selector/expected.html index b4834bb8189e..5eeb2be29429 100644 --- a/packages/svelte/tests/css/samples/not-selector/expected.html +++ b/packages/svelte/tests/css/samples/not-selector/expected.html @@ -1 +1,3 @@ -

foo

bar

\ No newline at end of file +

foo

+

bar baz

+buzz \ 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 a0e72c7be8e9..3a786530a2ee 100644 --- a/packages/svelte/tests/css/samples/not-selector/input.svelte +++ b/packages/svelte/tests/css/samples/not-selector/input.svelte @@ -1,5 +1,9 @@

foo

-

bar

+

+ bar + baz +

+buzz \ No newline at end of file diff --git a/packages/svelte/tests/css/test.ts b/packages/svelte/tests/css/test.ts index 30ad96369a1f..443ee0295809 100644 --- a/packages/svelte/tests/css/test.ts +++ b/packages/svelte/tests/css/test.ts @@ -32,30 +32,17 @@ const { test, run } = suite(async (config, cwd) => { await compile_directory(cwd, 'client', { cssHash: () => 'svelte-xyz', ...config.compileOptions }); await compile_directory(cwd, 'server', { cssHash: () => 'svelte-xyz', ...config.compileOptions }); - const dom_css = fs.readFileSync(`${cwd}/_output/client/input.svelte.css`, 'utf-8').trim(); - const ssr_css = fs.readFileSync(`${cwd}/_output/server/input.svelte.css`, 'utf-8').trim(); - - assert.equal(dom_css, ssr_css); - - const dom_warnings = load_warnings(`${cwd}/_output/client/input.svelte.warnings.json`); - const ssr_warnings = load_warnings(`${cwd}/_output/server/input.svelte.warnings.json`); - const expected_warnings = (config.warnings || []).map(normalize_warning); - assert.deepEqual(dom_warnings, ssr_warnings); - assert.deepEqual(dom_warnings.map(normalize_warning), expected_warnings); - const expected = { html: try_read_file(`${cwd}/expected.html`), css: try_read_file(`${cwd}/expected.css`) }; - assert.equal(dom_css.trim().replace(/\r\n/g, '\n'), (expected.css ?? '').trim()); - // we do this here, rather than in the expected.html !== null // block, to verify that valid code was generated const ClientComponent = (await import(`${cwd}/_output/client/input.svelte.js`)).default; const ServerComponent = (await import(`${cwd}/_output/server/input.svelte.js`)).default; - // verify that the right elements have scoping selectors + // verify that the right elements have scoping selectors (do this first to ensure all actual files are written to disk) if (expected.html !== null) { const target = window.document.createElement('main'); @@ -75,6 +62,19 @@ const { test, run } = suite(async (config, cwd) => { // const actual_ssr = ServerComponent.render(config.props).html; // assert_html_equal(actual_ssr, expected.html); } + + const dom_css = fs.readFileSync(`${cwd}/_output/client/input.svelte.css`, 'utf-8').trim(); + const ssr_css = fs.readFileSync(`${cwd}/_output/server/input.svelte.css`, 'utf-8').trim(); + + assert.equal(dom_css, ssr_css); + + const dom_warnings = load_warnings(`${cwd}/_output/client/input.svelte.warnings.json`); + const ssr_warnings = load_warnings(`${cwd}/_output/server/input.svelte.warnings.json`); + const expected_warnings = (config.warnings || []).map(normalize_warning); + assert.deepEqual(dom_warnings, ssr_warnings); + assert.deepEqual(dom_warnings.map(normalize_warning), expected_warnings); + + assert.equal(dom_css.trim().replace(/\r\n/g, '\n'), (expected.css ?? '').trim()); }); export { test }; 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 1e02e9bdfdc8..d8bb703ac53a 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?