fix #3096: css transform bug with nested selectors

Author: evanw

Diff for: CHANGELOG.md

@@ -2,6 +2,43 @@
 
 ## Unreleased
 
+* Fix CSS transform bugs with nested selectors that start with a combinator ([#3096](https://github.com/evanw/esbuild/issues/3096))
+
+    This release fixes several bugs regarding transforming nested CSS into non-nested CSS for older browsers. The bugs were due to lack of test coverage for nested selectors with more than one compound selector where they all start with the same combinator. Here's what some problematic cases look like before and after these fixes:
+
+    ```css
+    /* Original code */
+    .foo {
+      > &a,
+      > &b {
+        color: red;
+      }
+    }
+    .bar {
+      > &a,
+      + &b {
+        color: green;
+      }
+    }
+
+    /* Old output (with --target=chrome90) */
+    .foo :is(> .fooa, > .foob) {
+      color: red;
+    }
+    .bar :is(> .bara, + .barb) {
+      color: green;
+    }
+
+    /* New output (with --target=chrome90) */
+    .foo > :is(a.foo, b.foo) {
+      color: red;
+    }
+    .bar > a.bar,
+    .bar + b.bar {
+      color: green;
+    }
+    ```
+
 * Avoid removing unrecognized directives from the directive prologue when minifying ([#3115](https://github.com/evanw/esbuild/issues/3115))
 
     The [directive prologue](https://262.ecma-international.org/6.0/#sec-directive-prologues-and-the-use-strict-directive) in JavaScript is a sequence of top-level string expressions that come before your code. The only directives that JavaScript engines currently recognize are `use strict` and sometimes `use asm`. However, the people behind React have made up their own directive for their own custom dialect of JavaScript. Previously esbuild only preserved the `use strict` directive when minifying, although you could still write React JavaScript with esbuild using something like `--banner:js="'your directive here';"`. With this release, you can now put arbitrary directives in the entry point and esbuild will preserve them in its minified output:

Diff for: internal/css_ast/css_ast.go

@@ -598,11 +598,14 @@ type ComplexSelector struct {
 	Selectors []CompoundSelector
 }
 
-func (s ComplexSelector) AppendToTokens(tokens []Token) []Token {
+func (s ComplexSelector) AppendToTokensWithoutLeadingCombinator(tokens []Token) []Token {
 	for i, sel := range s.Selectors {
 		if n := len(tokens); i > 0 && n > 0 {
 			tokens[n-1].Whitespace |= WhitespaceAfter
 		}
+		if i == 0 {
+			sel.Combinator = 0
+		}
 		tokens = sel.AppendToTokens(tokens)
 	}
 	return tokens
@@ -714,14 +717,16 @@ func (sel CompoundSelector) AppendToTokens(tokens []Token) []Token {
 		}
 	}
 
-	if sel.HasNestingSelector {
-		tokens = append(tokens, Token{Kind: css_lexer.TDelimAmpersand, Text: "&"})
-	}
-
 	if sel.TypeSelector != nil {
 		tokens = sel.TypeSelector.AppendToTokens(tokens)
 	}
 
+	// Put this after the type selector in case it's substituted for ":is()"
+	// ".foo { > &a, > &b {} }" => ".foo > :is(b.foo, c.foo) {}" (we don't want to get ".foo > :is(.foob, .fooc) {}" instead)
+	if sel.HasNestingSelector {
+		tokens = append(tokens, Token{Kind: css_lexer.TDelimAmpersand, Text: "&"})
+	}
+
 	for _, ss := range sel.SubclassSelectors {
 		tokens = ss.AppendToTokens(tokens)
 	}

Diff for: internal/css_parser/css_nesting.go

@@ -108,6 +108,7 @@ func lowerNestingInRuleWithContext(rule css_ast.Rule, context *lowerNestingConte
 		// Pass 1: Canonicalize and analyze our selectors
 		canUseGroupDescendantCombinator := true // Can we do "parent «space» :is(...selectors)"?
 		canUseGroupSubSelector := true          // Can we do "parent«nospace»:is(...selectors)"?
+		var commonLeadingCombinator uint8
 		for i := range r.Selectors {
 			sel := &r.Selectors[i]
 
@@ -119,6 +120,17 @@ func lowerNestingInRuleWithContext(rule css_ast.Rule, context *lowerNestingConte
 			// Are all children of the form "& «something»"?
 			if len(sel.Selectors) < 2 || !sel.Selectors[0].IsSingleAmpersand() {
 				canUseGroupDescendantCombinator = false
+			} else {
+				// If all children are of the form "& «COMBINATOR» «something»", is «COMBINATOR» the same in all cases?
+				var combinator uint8
+				if len(sel.Selectors) >= 2 {
+					combinator = sel.Selectors[1].Combinator
+				}
+				if i == 0 {
+					commonLeadingCombinator = combinator
+				} else if commonLeadingCombinator != combinator {
+					canUseGroupDescendantCombinator = false
+				}
 			}
 
 			// Are all children of the form "&«something»"?
@@ -130,6 +142,7 @@ func lowerNestingInRuleWithContext(rule css_ast.Rule, context *lowerNestingConte
 		// Try to apply simplifications for shorter output
 		if canUseGroupDescendantCombinator {
 			// "& a, & b {}" => "& :is(a, b) {}"
+			// "& > a, & > b {}" => "& > :is(a, b) {}"
 			for i := range r.Selectors {
 				sel := &r.Selectors[i]
 				sel.Selectors = sel.Selectors[1:]
@@ -139,6 +152,7 @@ func lowerNestingInRuleWithContext(rule css_ast.Rule, context *lowerNestingConte
 			r.Selectors = []css_ast.ComplexSelector{merged}
 		} else if canUseGroupSubSelector {
 			// "&a, &b {}" => "&:is(a, b) {}"
+			// "> &a, > &b {}" => "> &:is(a, b) {}"
 			for i := range r.Selectors {
 				sel := &r.Selectors[i]
 				sel.Selectors[0].HasNestingSelector = false
@@ -212,27 +226,29 @@ func substituteAmpersandsInCompoundSelector(sel css_ast.CompoundSelector, replac
 
 		// Convert the replacement to a single compound selector
 		var single css_ast.CompoundSelector
-		if len(replacement.Selectors) == 1 || len(results) == 0 {
+		if sel.Combinator == 0 && (len(replacement.Selectors) == 1 || len(results) == 0) {
 			// ".foo { :hover & {} }" => ":hover .foo {}"
 			// ".foo .bar { &:hover {} }" => ".foo .bar:hover {}"
 			last := len(replacement.Selectors) - 1
 			results = append(results, replacement.Selectors[:last]...)
 			single = replacement.Selectors[last]
+			sel.Combinator = single.Combinator
+		} else if len(replacement.Selectors) == 1 {
+			// ".foo { > &:hover {} }" => ".foo > .foo:hover {}"
+			single = replacement.Selectors[0]
 		} else {
 			// ".foo .bar { :hover & {} }" => ":hover :is(.foo .bar) {}"
+			// ".foo .bar { > &:hover {} }" => ".foo .bar > :is(.foo .bar):hover {}"
 			single = css_ast.CompoundSelector{
 				SubclassSelectors: []css_ast.SS{&css_ast.SSPseudoClass{
 					Name: "is",
-					Args: replacement.AppendToTokens(nil),
+					Args: replacement.AppendToTokensWithoutLeadingCombinator(nil),
 				}},
 			}
 		}
 
 		var subclassSelectorPrefix []css_ast.SS
 
-		// Copy over the combinator, if any
-		sel.Combinator = single.Combinator
-
 		// Insert the type selector
 		if single.TypeSelector != nil {
 			if sel.TypeSelector != nil {
@@ -281,7 +297,7 @@ func substituteAmpersandsInTokens(tokens []css_ast.Token, replacement css_ast.Co
 	}
 
 	var results []css_ast.Token
-	replacementTokens := replacement.AppendToTokens(nil)
+	replacementTokens := replacement.AppendToTokensWithoutLeadingCombinator(nil)
 	for _, t := range tokens {
 		if t.Kind != css_lexer.TDelimAmpersand {
 			results = append(results, t)
@@ -332,16 +348,21 @@ func multipleComplexSelectorsToSingleComplexSelector(selectors []css_ast.Complex
 
 	// This must be non-nil so we print ":is()" instead of ":is"
 	tokens := []css_ast.Token{}
+	var leadingCombinator uint8
 
 	for i, sel := range selectors {
 		if i > 0 {
 			tokens = append(tokens, css_ast.Token{Kind: css_lexer.TComma, Text: ",", Whitespace: css_ast.WhitespaceAfter})
 		}
-		tokens = sel.AppendToTokens(tokens)
+
+		// "> a, > b" => "> :is(a, b)" (the caller should have already checked that all leading combinators are the same)
+		leadingCombinator = sel.Selectors[0].Combinator
+		tokens = sel.AppendToTokensWithoutLeadingCombinator(tokens)
 	}
 
 	return css_ast.ComplexSelector{
 		Selectors: []css_ast.CompoundSelector{{
+			Combinator: leadingCombinator,
 			SubclassSelectors: []css_ast.SS{&css_ast.SSPseudoClass{
 				Name: "is",
 				Args: tokens,

Diff for: internal/css_parser/css_parser_test.go

@@ -907,6 +907,22 @@ func TestNestedSelector(t *testing.T) {
 	expectPrintedLower(t, "&, a { color: red }", ":scope,\na {\n  color: red;\n}\n")
 	expectPrintedLower(t, "&, a { .b { color: red } }", ":is(:scope, a) .b {\n  color: red;\n}\n")
 	expectPrintedLower(t, "&, a { .b { .c { color: red } } }", ":is(:scope, a) .b .c {\n  color: red;\n}\n")
+	expectPrintedLower(t, "a { > b, > c { color: red } }", "a > :is(b, c) {\n  color: red;\n}\n")
+	expectPrintedLower(t, "a { > b, + c { color: red } }", "a > b,\na + c {\n  color: red;\n}\n")
+	expectPrintedLower(t, "a { & > b, & > c { color: red } }", "a > :is(b, c) {\n  color: red;\n}\n")
+	expectPrintedLower(t, "a { & > b, & + c { color: red } }", "a > b,\na + c {\n  color: red;\n}\n")
+	expectPrintedLower(t, "a { > &b, > &c { color: red } }", "a > :is(b:is(a), c:is(a)) {\n  color: red;\n}\n")
+	expectPrintedLower(t, "a { > &b, + &c { color: red } }", "a > a:is(b),\na + a:is(c) {\n  color: red;\n}\n")
+	expectPrintedLower(t, "a { > &.b, > &.c { color: red } }", "a > :is(a.b, a.c) {\n  color: red;\n}\n")
+	expectPrintedLower(t, "a { > &.b, + &.c { color: red } }", "a > a.b,\na + a.c {\n  color: red;\n}\n")
+	expectPrintedLower(t, ".a { > &b, > &c { color: red } }", ".a > :is(b.a, c.a) {\n  color: red;\n}\n")
+	expectPrintedLower(t, ".a { > &b, + &c { color: red } }", ".a > b.a,\n.a + c.a {\n  color: red;\n}\n")
+	expectPrintedLower(t, ".a { > &.b, > &.c { color: red } }", ".a > :is(.a.b, .a.c) {\n  color: red;\n}\n")
+	expectPrintedLower(t, ".a { > &.b, + &.c { color: red } }", ".a > .a.b,\n.a + .a.c {\n  color: red;\n}\n")
+	expectPrintedLower(t, "~ .a { > &.b, > &.c { color: red } }", "~ .a > :is(.a.b, .a.c) {\n  color: red;\n}\n")
+	expectPrintedLower(t, "~ .a { > &.b, + &.c { color: red } }", "~ .a > .a.b,\n~ .a + .a.c {\n  color: red;\n}\n")
+	expectPrintedLower(t, ".foo .bar { > &.a, > &.b { color: red } }", ".foo .bar > :is(.foo .bar.a, .foo .bar.b) {\n  color: red;\n}\n")
+	expectPrintedLower(t, ".foo .bar { > &.a, + &.b { color: red } }", ".foo .bar > :is(.foo .bar).a,\n.foo .bar + :is(.foo .bar).b {\n  color: red;\n}\n")
 	expectPrintedLower(t, ".foo { @media screen {} }", "")
 	expectPrintedLower(t, ".foo { @media screen { color: red } }", "@media screen {\n  .foo {\n    color: red;\n  }\n}\n")
 	expectPrintedLower(t, ".foo { @media screen { &:hover { color: red } } }", "@media screen {\n  .foo:hover {\n    color: red;\n  }\n}\n")