fix #1702: invalid css transform of border-radius

evanw · Oct 23, 2021 · df08658 · df08658
1 parent e608c54
commit df08658
Show file tree

Hide file tree

Showing 6 changed files with 61 additions and 33 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,37 @@
 # Changelog
 
+## Unreleased
+
+* Fix invalid CSS minification of `border-radius` ([#1702](https://github.com/evanw/esbuild/issues/1702))
+
+    CSS minification does collapsing of `border-radius` related properties. For example:
+
+    ```css
+    /* Original CSS */
+    div {
+      border-radius: 1px;
+      border-top-left-radius: 5px;
+    }
+
+    /* Minified CSS */
+    div{border-radius:5px 1px 1px}
+    ```
+
+    However, this only works for numeric tokens, not identifiers. For example:
+
+    ```css
+    /* Original CSS */
+    div {
+      border-radius: 1px;
+      border-top-left-radius: inherit;
+    }
+
+    /* Minified CSS */
+    div{border-radius:1px;border-top-left-radius:inherit}
+    ```
+
+    Transforming this to `div{border-radius:inherit 1px 1px}`, as was done in previous releases of esbuild, is an invalid transformation and results in incorrect CSS. This release of esbuild fixes this CSS transformation bug.
+
 ## 0.13.8
 
 * Fix `super` inside arrow function inside lowered `async` function ([#1425](https://github.com/evanw/esbuild/issues/1425))

diff --git a/internal/css_lexer/css_lexer.go b/internal/css_lexer/css_lexer.go
@@ -104,10 +104,6 @@ func (t T) IsNumeric() bool {
 	return t == TNumber || t == TPercentage || t == TDimension
 }
 
-func (t T) IsNumericOrIdent() bool {
-	return t.IsNumeric() || t == TIdent
-}
-
 // This token struct is designed to be memory-efficient. It just references a
 // range in the input file instead of directly containing the substring of text
 // since a range takes up less memory than a string.

diff --git a/internal/css_parser/css_decls.go b/internal/css_parser/css_decls.go
@@ -16,15 +16,15 @@ func (p *parser) commaToken() css_ast.Token {
 	return t
 }
 
-func expandTokenQuad(tokens []css_ast.Token) (result [4]css_ast.Token, ok bool) {
+func expandTokenQuad(tokens []css_ast.Token, allowedIdent string) (result [4]css_ast.Token, ok bool) {
 	n := len(tokens)
 	if n < 1 || n > 4 {
 		return
 	}
 
 	// Don't do this if we encounter any unexpected tokens such as "var()"
 	for i := 0; i < n; i++ {
-		if !tokens[i].Kind.IsNumericOrIdent() {
+		if t := tokens[i]; !t.Kind.IsNumeric() && (t.Kind != css_lexer.TIdent || allowedIdent == "" || t.Text != allowedIdent) {
 			return
 		}
 	}

diff --git a/internal/css_parser/css_decls_border_radius.go b/internal/css_parser/css_decls_border_radius.go
@@ -57,8 +57,8 @@ func (borderRadius *borderRadiusTracker) mangleCorners(rules []css_ast.Rule, dec
 		}
 	}
 
-	firstRadii, firstRadiiOk := expandTokenQuad(tokens[:beforeSplit])
-	lastRadii, lastRadiiOk := expandTokenQuad(tokens[afterSplit:])
+	firstRadii, firstRadiiOk := expandTokenQuad(tokens[:beforeSplit], "")
+	lastRadii, lastRadiiOk := expandTokenQuad(tokens[afterSplit:], "")
 
 	// Stop now if the pattern wasn't matched
 	if !firstRadiiOk || (beforeSplit < afterSplit && !lastRadiiOk) {
@@ -95,8 +95,8 @@ func (borderRadius *borderRadiusTracker) mangleCorner(rules []css_ast.Rule, decl
 		borderRadius.important = decl.Important
 	}
 
-	if tokens := decl.Value; (len(tokens) == 1 && tokens[0].Kind.IsNumericOrIdent()) ||
-		(len(tokens) == 2 && tokens[0].Kind.IsNumericOrIdent() && tokens[1].Kind.IsNumericOrIdent()) {
+	if tokens := decl.Value; (len(tokens) == 1 && tokens[0].Kind.IsNumeric()) ||
+		(len(tokens) == 2 && tokens[0].Kind.IsNumeric() && tokens[1].Kind.IsNumeric()) {
 		firstToken := tokens[0]
 		if firstToken.TurnLengthIntoNumberIfZero() {
 			tokens[0] = firstToken

diff --git a/internal/css_parser/css_decls_box.go b/internal/css_parser/css_decls_box.go
@@ -38,11 +38,12 @@ func (box *boxTracker) mangleSides(rules []css_ast.Rule, decl *css_ast.RDeclarat
 		box.important = decl.Important
 	}
 
+	allowedIdent := ""
 	isMargin := decl.Key == css_ast.DMargin
-	if quad, ok := expandTokenQuad(decl.Value); ok &&
-		isNumericOrMarginAuto(&quad[0], isMargin) && isNumericOrMarginAuto(&quad[1], isMargin) &&
-		isNumericOrMarginAuto(&quad[2], isMargin) && isNumericOrMarginAuto(&quad[3], isMargin) {
-		isMargin := decl.Key == css_ast.DMargin
+	if isMargin {
+		allowedIdent = "auto"
+	}
+	if quad, ok := expandTokenQuad(decl.Value, allowedIdent); ok {
 		for side, t := range quad {
 			t.TurnLengthIntoNumberIfZero()
 			box.updateSide(rules, side, boxSide{token: t, index: uint32(index)})
@@ -65,16 +66,19 @@ func (box *boxTracker) mangleSide(rules []css_ast.Rule, decl *css_ast.RDeclarati
 	case css_ast.DMarginTop, css_ast.DMarginRight, css_ast.DMarginBottom, css_ast.DMarginLeft:
 		isMargin = true
 	}
-	if tokens := decl.Value; len(tokens) == 1 && isNumericOrMarginAuto(&tokens[0], isMargin) {
-		t := tokens[0]
-		if t.TurnLengthIntoNumberIfZero() {
-			tokens[0] = t
+
+	if tokens := decl.Value; len(tokens) == 1 {
+		if t := tokens[0]; t.Kind.IsNumeric() || (t.Kind == css_lexer.TIdent && isMargin && t.Text == "auto") {
+			if t.TurnLengthIntoNumberIfZero() {
+				tokens[0] = t
+			}
+			box.updateSide(rules, side, boxSide{token: t, index: uint32(index), single: true})
+			box.compactRules(rules, decl.KeyRange, removeWhitespace, isMargin)
+			return
 		}
-		box.updateSide(rules, side, boxSide{token: t, index: uint32(index), single: true})
-		box.compactRules(rules, decl.KeyRange, removeWhitespace, isMargin)
-	} else {
-		box.sides = [4]boxSide{}
 	}
+
+	box.sides = [4]boxSide{}
 }
 
 func (box *boxTracker) compactRules(rules []css_ast.Rule, keyRange logger.Range, removeWhitespace bool, isMargin bool) {
@@ -117,13 +121,3 @@ func (box *boxTracker) compactRules(rules []css_ast.Rule, keyRange logger.Range,
 		Important: box.important,
 	}
 }
-
-func isNumericOrMarginAuto(t *css_ast.Token, isMargin bool) bool {
-	if t.Kind.IsNumeric() {
-		return true
-	}
-	if isMargin && t.Kind == css_lexer.TIdent && t.Text == "auto" {
-		return true
-	}
-	return false
-}
diff --git a/internal/css_parser/css_parser_test.go b/internal/css_parser/css_parser_test.go
@@ -1016,7 +1016,6 @@ func TestBorderRadius(t *testing.T) {
 	expectPrintedMangle(t, "a { border-top-left-radius: 0 1 }", "a {\n  border-top-left-radius: 0 1;\n}\n")
 
 	expectPrintedMangle(t, "a { border-top-left-radius: 0; border-radius: 1 }", "a {\n  border-radius: 1;\n}\n")
-	expectPrintedMangle(t, "a { border-top-left-radius: 0; border-radius: inherit }", "a {\n  border-radius: inherit;\n}\n")
 
 	expectPrintedMangle(t, "a { border-radius: 1 2 3 4 }", "a {\n  border-radius: 1 2 3 4;\n}\n")
 	expectPrintedMangle(t, "a { border-radius: 1 2 1 3 }", "a {\n  border-radius: 1 2 1 3;\n}\n")
@@ -1063,6 +1062,13 @@ func TestBorderRadius(t *testing.T) {
 	// These should not be changed because "--x" and "--z" could be empty
 	expectPrintedMangle(t, "a { border-radius: var(--x) var(--y) var(--z) var(--y) }", "a {\n  border-radius: var(--x) var(--y) var(--z) var(--y);\n}\n")
 	expectPrintedMangle(t, "a { border-radius: 0 / var(--x) var(--y) var(--z) var(--y) }", "a {\n  border-radius: 0 / var(--x) var(--y) var(--z) var(--y);\n}\n")
+
+	// "inherit" should not be merged
+	expectPrintedMangle(t, "a { border-radius: 1px; border-top-left-radius: 0 }", "a {\n  border-radius: 0 1px 1px;\n}\n")
+	expectPrintedMangle(t, "a { border-radius: inherit; border-top-left-radius: 0 }", "a {\n  border-radius: inherit;\n  border-top-left-radius: 0;\n}\n")
+	expectPrintedMangle(t, "a { border-radius: 0; border-top-left-radius: inherit }", "a {\n  border-radius: 0;\n  border-top-left-radius: inherit;\n}\n")
+	expectPrintedMangle(t, "a { border-top-left-radius: 0; border-radius: inherit }", "a {\n  border-top-left-radius: 0;\n  border-radius: inherit;\n}\n")
+	expectPrintedMangle(t, "a { border-top-left-radius: inherit; border-radius: 0 }", "a {\n  border-top-left-radius: inherit;\n  border-radius: 0;\n}\n")
 }
 
 func TestBoxShadow(t *testing.T) {