fix: relax :global selector list validation (#15762)

We have to allow `:global x, :global y` selector lists because CSS preprocessors might generate that from `:global { x, y {...} }`

Author: dummdidumm

.changeset/green-starfishes-shave.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: relax `:global` selector list validation

documentation/docs/98-reference/.generated/compile-errors.md

@@ -235,7 +235,31 @@ A top-level `:global {...}` block can only contain rules, not declarations
 ### css_global_block_invalid_list
 
 ```
-A `:global` selector cannot be part of a selector list with more than one item
+A `:global` selector cannot be part of a selector list with entries that don't contain `:global`
+```
+
+The following CSS is invalid:
+
+```css
+:global, x {
+    y {
+        color: red;
+    }
+}
+```
+
+This is mixing a `:global` block, which means "everything in here is unscoped", with a scoped selector (`x` in this case). As a result it's not possible to transform the inner selector (`y` in this case) into something that satisfies both requirements. You therefore have to split this up into two selectors:
+
+```css
+:global {
+    y {
+        color: red;
+    }
+}
+
+x y {
+    color: red;
+}
 ```
 
 ### css_global_block_invalid_modifier

packages/svelte/messages/compile-errors/style.md

@@ -16,7 +16,31 @@
 
 ## css_global_block_invalid_list
 
-> A `:global` selector cannot be part of a selector list with more than one item
+> A `:global` selector cannot be part of a selector list with entries that don't contain `:global`
+
+The following CSS is invalid:
+
+```css
+:global, x {
+    y {
+        color: red;
+    }
+}
+```
+
+This is mixing a `:global` block, which means "everything in here is unscoped", with a scoped selector (`x` in this case). As a result it's not possible to transform the inner selector (`y` in this case) into something that satisfies both requirements. You therefore have to split this up into two selectors:
+
+```css
+:global {
+    y {
+        color: red;
+    }
+}
+
+x y {
+    color: red;
+}
+```
 
 ## css_global_block_invalid_modifier
 

packages/svelte/src/compiler/errors.js

@@ -555,12 +555,12 @@ export function css_global_block_invalid_declaration(node) {
 }
 
 /**
- * A `:global` selector cannot be part of a selector list with more than one item
+ * A `:global` selector cannot be part of a selector list with entries that don't contain `:global`
  * @param {null | number | NodeLike} node
  * @returns {never}
  */
 export function css_global_block_invalid_list(node) {
-	e(node, 'css_global_block_invalid_list', `A \`:global\` selector cannot be part of a selector list with more than one item\nhttps://svelte.dev/e/css_global_block_invalid_list`);
+	e(node, 'css_global_block_invalid_list', `A \`:global\` selector cannot be part of a selector list with entries that don't contain \`:global\`\nhttps://svelte.dev/e/css_global_block_invalid_list`);
 }
 
 /**

packages/svelte/src/compiler/phases/2-analyze/css/css-analyze.js

@@ -193,69 +193,69 @@ const css_visitors = {
 	Rule(node, context) {
 		node.metadata.parent_rule = context.state.rule;
 
-		node.metadata.is_global_block = node.prelude.children.some((selector) => {
+		// We gotta allow :global x, :global y because CSS preprocessors might generate that from :global { x, y {...} }
+		for (const complex_selector of node.prelude.children) {
 			let is_global_block = false;
 
-			for (const child of selector.children) {
+			for (let selector_idx = 0; selector_idx < complex_selector.children.length; selector_idx++) {
+				const child = complex_selector.children[selector_idx];
 				const idx = child.selectors.findIndex(is_global_block_selector);
 
 				if (is_global_block) {
 					// All selectors after :global are unscoped
 					child.metadata.is_global_like = true;
 				}
 
-				if (idx !== -1) {
-					is_global_block = true;
+				if (idx === 0) {
+					if (
+						child.selectors.length > 1 &&
+						selector_idx === 0 &&
+						node.metadata.parent_rule === null
+					) {
+						e.css_global_block_invalid_modifier_start(child.selectors[1]);
+					} else {
+						// `child` starts with `:global`
+						node.metadata.is_global_block = is_global_block = true;
+
+						for (let i = 1; i < child.selectors.length; i++) {
+							walk(/** @type {AST.CSS.Node} */ (child.selectors[i]), null, {
+								ComplexSelector(node) {
+									node.metadata.used = true;
+								}
+							});
+						}
 
-					for (let i = idx + 1; i < child.selectors.length; i++) {
-						walk(/** @type {AST.CSS.Node} */ (child.selectors[i]), null, {
-							ComplexSelector(node) {
-								node.metadata.used = true;
-							}
-						});
-					}
-				}
-			}
+						if (child.combinator && child.combinator.name !== ' ') {
+							e.css_global_block_invalid_combinator(child, child.combinator.name);
+						}
 
-			return is_global_block;
-		});
+						const declaration = node.block.children.find((child) => child.type === 'Declaration');
+						const is_lone_global =
+							complex_selector.children.length === 1 &&
+							complex_selector.children[0].selectors.length === 1; // just `:global`, not e.g. `:global x`
 
-		if (node.metadata.is_global_block) {
-			if (node.prelude.children.length > 1) {
-				e.css_global_block_invalid_list(node.prelude);
-			}
+						if (is_lone_global && node.prelude.children.length > 1) {
+							// `:global, :global x { z { ... } }` would become `x { z { ... } }` which means `z` is always
+							// constrained by `x`, which is not what the user intended
+							e.css_global_block_invalid_list(node.prelude);
+						}
 
-			const complex_selector = node.prelude.children[0];
-			const global_selector = complex_selector.children.find((r, selector_idx) => {
-				const idx = r.selectors.findIndex(is_global_block_selector);
-				if (idx === 0) {
-					if (r.selectors.length > 1 && selector_idx === 0 && node.metadata.parent_rule === null) {
-						e.css_global_block_invalid_modifier_start(r.selectors[1]);
+						if (
+							declaration &&
+							// :global { color: red; } is invalid, but foo :global { color: red; } is valid
+							node.prelude.children.length === 1 &&
+							is_lone_global
+						) {
+							e.css_global_block_invalid_declaration(declaration);
+						}
 					}
-					return true;
 				} else if (idx !== -1) {
-					e.css_global_block_invalid_modifier(r.selectors[idx]);
+					e.css_global_block_invalid_modifier(child.selectors[idx]);
 				}
-			});
-
-			if (!global_selector) {
-				throw new Error('Internal error: global block without :global selector');
-			}
-
-			if (global_selector.combinator && global_selector.combinator.name !== ' ') {
-				e.css_global_block_invalid_combinator(global_selector, global_selector.combinator.name);
 			}
 
-			const declaration = node.block.children.find((child) => child.type === 'Declaration');
-
-			if (
-				declaration &&
-				// :global { color: red; } is invalid, but foo :global { color: red; } is valid
-				node.prelude.children.length === 1 &&
-				node.prelude.children[0].children.length === 1 &&
-				node.prelude.children[0].children[0].selectors.length === 1
-			) {
-				e.css_global_block_invalid_declaration(declaration);
+			if (node.metadata.is_global_block && !is_global_block) {
+				e.css_global_block_invalid_list(node.prelude);
 			}
 		}
 

packages/svelte/src/compiler/phases/3-transform/css/index.js

@@ -170,7 +170,11 @@ const visitors = {
 		if (node.metadata.is_global_block) {
 			const selector = node.prelude.children[0];
 
-			if (selector.children.length === 1 && selector.children[0].selectors.length === 1) {
+			if (
+				node.prelude.children.length === 1 &&
+				selector.children.length === 1 &&
+				selector.children[0].selectors.length === 1
+			) {
 				// `:global {...}`
 				if (state.minify) {
 					state.code.remove(node.start, node.block.start + 1);
@@ -194,7 +198,7 @@ const visitors = {
 	SelectorList(node, { state, next, path }) {
 		// Only add comments if we're not inside a complex selector that itself is unused or a global block
 		if (
-			!is_in_global_block(path) &&
+			(!is_in_global_block(path) || node.children.length > 1) &&
 			!path.find((n) => n.type === 'ComplexSelector' && !n.metadata.used)
 		) {
 			const children = node.children;
@@ -282,13 +286,24 @@ const visitors = {
 				const global = /** @type {AST.CSS.PseudoClassSelector} */ (relative_selector.selectors[0]);
 				remove_global_pseudo_class(global, relative_selector.combinator, context.state);
 
-				if (
-					node.metadata.rule?.metadata.parent_rule &&
-					global.args === null &&
-					relative_selector.combinator === null
-				) {
-					// div { :global.x { ... } } becomes div { &.x { ... } }
-					context.state.code.prependRight(global.start, '&');
+				const parent_rule = node.metadata.rule?.metadata.parent_rule;
+				if (parent_rule && global.args === null) {
+					if (relative_selector.combinator === null) {
+						// div { :global.x { ... } } becomes div { &.x { ... } }
+						context.state.code.prependRight(global.start, '&');
+					}
+
+					// In case of multiple :global selectors in a selector list we gotta delete the comma, too, but only if
+					// the next selector is used; if it's unused then the comma deletion happens as part of removal of that next selector
+					if (
+						parent_rule.prelude.children.length > 1 &&
+						node.children.length === node.children.findIndex((s) => s === relative_selector) - 1
+					) {
+						const next_selector = parent_rule.prelude.children.find((s) => s.start > global.end);
+						if (next_selector && next_selector.metadata.used) {
+							context.state.code.update(global.end, next_selector.start, '');
+						}
+					}
 				}
 				continue;
 			} else {
@@ -380,7 +395,9 @@ function remove_global_pseudo_class(selector, combinator, state) {
 			// div :global.x becomes div.x
 			while (/\s/.test(state.code.original[start - 1])) start--;
 		}
-		state.code.remove(start, selector.start + ':global'.length);
+
+		// update(...), not remove(...) because there could be a closing unused comment at the end
+		state.code.update(start, selector.start + ':global'.length, '');
 	} else {
 		state.code
 			.remove(selector.start, selector.start + ':global('.length)

packages/svelte/tests/compiler-errors/samples/css-global-block-multiple-1/_config.js

@@ -0,0 +1,10 @@
+import { test } from '../../test';
+
+export default test({
+	error: {
+		code: 'css_global_block_invalid_list',
+		message:
+			"A `:global` selector cannot be part of a selector list with entries that don't contain `:global`",
+		position: [232, 246]
+	}
+});

packages/svelte/tests/compiler-errors/samples/css-global-block-multiple-1/main.svelte

@@ -0,0 +1,9 @@
+<style>
+	/* valid */
+	/* We gotta allow `:global x, :global y` and the likes because CSS preprocessors might generate that from e.g. `:global { x, y {...} }` */
+	:global .x, :global .y {}
+	.x :global, .y :global {}
+
+	/* invalid */
+	.x :global, .y {}
+</style>

packages/svelte/tests/compiler-errors/samples/css-global-block-multiple-2/_config.js

@@ -0,0 +1,10 @@
+import { test } from '../../test';
+
+export default test({
+	error: {
+		code: 'css_global_block_invalid_list',
+		message:
+			"A `:global` selector cannot be part of a selector list with entries that don't contain `:global`",
+		position: [24, 43]
+	}
+});

packages/svelte/tests/compiler-errors/samples/css-global-block-multiple-2/main.svelte

@@ -0,0 +1,6 @@
+<style>
+	/* invalid */
+	:global, :global .y {
+		z { color: red }
+	}
+</style>