fix #1657: invalid css transform of margin/padding

Author: evanw

CHANGELOG.md

@@ -26,6 +26,38 @@
 
     With this release, esbuild will also automatically rewrite `.mjs` to `.mts` and `.cjs` to `.cts` when resolving import paths to files on the file system. This should make it possible to bundle code written in this new style. In addition, the extensions `.mts` and `.cts` are now also considered valid TypeScript file extensions by default along with the `.ts` extension.
 
+* Fix invalid CSS minification of `margin` and `padding` ([#1657](https://github.com/evanw/esbuild/issues/1657))
+
+    CSS minification does collapsing of `margin` and `padding` related properties. For example:
+
+    ```css
+    /* Original CSS */
+    div {
+      margin: auto;
+      margin-top: 5px;
+      margin-left: 5px;
+    }
+
+    /* Minified CSS */
+    div{margin:5px auto auto 5px}
+    ```
+
+    However, while this works for the `auto` keyword, it doesn't work for other keywords. For example:
+
+    ```css
+    /* Original CSS */
+    div {
+      margin: inherit;
+      margin-top: 5px;
+      margin-left: 5px;
+    }
+
+    /* Minified CSS */
+    div{margin:inherit;margin-top:5px;margin-left:5px}
+    ```
+
+    Transforming this to `div{margin:5px inherit inherit 5px}`, 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.3
 
 * Support TypeScript type-only import/export specifiers ([#1637](https://github.com/evanw/esbuild/pull/1637))

internal/css_parser/css_decls_box.go

@@ -38,7 +38,10 @@ func (box *boxTracker) mangleSides(rules []css_ast.Rule, decl *css_ast.RDeclarat
 		box.important = decl.Important
 	}
 
-	if quad, ok := expandTokenQuad(decl.Value); ok {
+	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
 		for side, t := range quad {
 			t.TurnLengthIntoNumberIfZero()
@@ -57,12 +60,12 @@ func (box *boxTracker) mangleSide(rules []css_ast.Rule, decl *css_ast.RDeclarati
 		box.important = decl.Important
 	}
 
-	if tokens := decl.Value; len(tokens) == 1 && tokens[0].Kind.IsNumericOrIdent() {
-		isMargin := false
-		switch decl.Key {
-		case css_ast.DMarginTop, css_ast.DMarginRight, css_ast.DMarginBottom, css_ast.DMarginLeft:
-			isMargin = true
-		}
+	isMargin := false
+	switch decl.Key {
+	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
@@ -114,3 +117,13 @@ 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
+}

internal/css_parser/css_parser_test.go

@@ -980,7 +980,6 @@ func TestMarginAndPadding(t *testing.T) {
 		expectPrintedMangle(t, "a { "+x+"-top: 1; "+x+"-top: }", "a {\n  "+x+"-top: 1;\n  "+x+"-top:;\n}\n")
 		expectPrintedMangle(t, "a { "+x+"-top: 1; "+x+"-top: 2 3 }", "a {\n  "+x+"-top: 1;\n  "+x+"-top: 2 3;\n}\n")
 		expectPrintedMangle(t, "a { "+x+": 1 2 3 4; "+x+"-left: -4; "+x+"-right: -2 }", "a {\n  "+x+": 1 -2 3 -4;\n}\n")
-		expectPrintedMangle(t, "a { "+x+": 1 auto 3 4; "+x+"-left: auto }", "a {\n  "+x+": 1 auto 3;\n}\n")
 		expectPrintedMangle(t, "a { "+x+": 1 2; "+x+"-top: 5 }", "a {\n  "+x+": 5 2 1;\n}\n")
 		expectPrintedMangle(t, "a { "+x+": 1; "+x+"-top: 5 }", "a {\n  "+x+": 5 1 1;\n}\n")
 
@@ -999,6 +998,14 @@ func TestMarginAndPadding(t *testing.T) {
 		// This should not be changed because "--x" and "--z" could be empty
 		expectPrintedMangle(t, "a { "+x+": var(--x) var(--y) var(--z) var(--y) }", "a {\n  "+x+": var(--x) var(--y) var(--z) var(--y);\n}\n")
 	}
+
+	// "auto" is the only keyword allowed in a quad, and only for "margin" not for "padding"
+	expectPrintedMangle(t, "a { margin: 1 auto 3 4; margin-left: auto }", "a {\n  margin: 1 auto 3;\n}\n")
+	expectPrintedMangle(t, "a { padding: 1 auto 3 4; padding-left: auto }", "a {\n  padding: 1 auto 3 4;\n  padding-left: auto;\n}\n")
+	expectPrintedMangle(t, "a { margin: auto; margin-left: 1px }", "a {\n  margin: auto auto auto 1px;\n}\n")
+	expectPrintedMangle(t, "a { padding: auto; padding-left: 1px }", "a {\n  padding: auto;\n  padding-left: 1px;\n}\n")
+	expectPrintedMangle(t, "a { margin: inherit; margin-left: 1px }", "a {\n  margin: inherit;\n  margin-left: 1px;\n}\n")
+	expectPrintedMangle(t, "a { padding: inherit; padding-left: 1px }", "a {\n  padding: inherit;\n  padding-left: 1px;\n}\n")
 }
 
 func TestBorderRadius(t *testing.T) {