Stop zero-value minification from producing invalid CSS

src/main/java/com/yahoo/platform/yui/compressor/CssCompressor.java

@@ -17,9 +17,48 @@
 import java.util.regex.Pattern;
 import java.util.regex.Matcher;
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
 
 public class CssCompressor {
 
+    /**
+     * Functions whose grammar rejects a bare {@code 0} where a
+     * {@code <percentage>} is written, so the "a zero value may drop its unit"
+     * rule must not fire inside them.
+     *
+     * <p>The colour functions take {@code <percentage>} arguments in their
+     * comma-separated legacy form ({@code hsl(27,0%,50%)}), and the math
+     * functions type-check their arguments against each other, so
+     * {@code min(0,10px)} compares a {@code <number>} with a {@code <length>}.
+     * In every one of those cases a browser drops the whole declaration, which
+     * costs the declaration to save one byte.
+     *
+     * <p>Matching is on the whole function name, so {@code minmax()} - where a
+     * zero really may lose its unit - is not caught by the {@code min} entry.
+     */
+    private static final Set<String> PERCENTAGE_REQUIRED_FUNCTIONS = Collections.unmodifiableSet(
+            new HashSet<String>(Arrays.asList("hsl", "hsla", "rgb", "rgba", "color-mix", "min", "max", "clamp")));
+
+    /**
+     * Properties whose value must not be collapsed from a run of zeroes to a
+     * single {@code 0}.
+     *
+     * <p>{@code margin:0 0 0 0} is the box-model shorthand, where the collapse is
+     * exact. {@code box-shadow} and {@code text-shadow} are not shorthands at all:
+     * a {@code <shadow>} needs both of its offsets, so {@code box-shadow:0} is
+     * invalid and the browser drops the declaration - and with it every other
+     * shadow in the same comma-separated list. {@code perspective-origin:0} means
+     * {@code 0 center}, not {@code 0 0}. {@code flex} was already excluded here
+     * for the same class of reason.
+     *
+     * <p>Names are matched with their vendor prefix removed.
+     */
+    private static final Set<String> ZERO_RUN_NOT_COLLAPSIBLE = Collections.unmodifiableSet(
+            new HashSet<String>(Arrays.asList("box-shadow", "text-shadow", "perspective-origin", "flex")));
+
     private StringBuffer srcsb = new StringBuffer();
 
     public CssCompressor(Reader in) throws IOException {
@@ -1100,21 +1139,52 @@ public void compress(Writer out, int linebreakpos)
           css = m.replaceAll("$1to{");
         } while (!(css.equals(oldCss)));
 
-        // Replace 0(px,em,%) with 0 inside groups (e.g. -MOZ-RADIAL-GRADIENT(CENTER 45DEG, CIRCLE CLOSEST-SIDE, ORANGE 0%, RED 100%))
-        p = Pattern.compile("(?i)\\( ?((?:[0-9a-z-.]+[ ,])*)?(?:0?\\.)?0(?:px|em|%|in|cm|mm|pc|pt|ex|deg|g?rad|m?s|k?hz)");
+        // Replace 0(px,em) with 0 inside groups (e.g. -MOZ-RADIAL-GRADIENT(CENTER 45DEG, CIRCLE CLOSEST-SIDE, ORANGE 0PX, RED 100%))
+        p = Pattern.compile("(?i)\\( ?((?:[0-9a-z-.]+[ ,])*)?(?:0?\\.)?0(?:px|em|in|cm|mm|pc|pt|ex|deg|g?rad|m?s|k?hz)");
         do {
           oldCss = css;
           m = p.matcher(css);
           css = m.replaceAll("($10");
         } while (!(css.equals(oldCss)));
 
+        // The same for "%", except inside a function that requires a real
+        // <percentage> there - see PERCENTAGE_REQUIRED_FUNCTIONS. The function
+        // name has to be captured rather than looked behind, so that "min" does
+        // not also match the tail of "minmax".
+        p = Pattern.compile("(?i)([-a-z0-9_]*)\\( ?((?:[0-9a-z-.]+[ ,])*)?(?:0?\\.)?0%");
+        do {
+          oldCss = css;
+          m = p.matcher(css);
+          sb = new StringBuffer();
+          while (m.find()) {
+            String function = m.group(1).toLowerCase();
+            String replacement = PERCENTAGE_REQUIRED_FUNCTIONS.contains(function)
+                    ? m.group(0)
+                    : m.group(1) + "(" + (m.group(2) == null ? "" : m.group(2)) + "0";
+            m.appendReplacement(sb, Matcher.quoteReplacement(replacement));
+          }
+          m.appendTail(sb);
+          css = sb.toString();
+        } while (!(css.equals(oldCss)));
+
         // Replace x.0(px,em,%) with x(px,em,%).
         css = css.replaceAll("([0-9])\\.0(px|em|%|in|cm|mm|pc|pt|ex|deg|m?s|g?rad|k?hz| |;)", "$1$2");
 
-        // Replace 0 0 0 0; with 0.
-        css = css.replaceAll(":0 0 0 0(;|})", ":0$1");
-        css = css.replaceAll(":0 0 0(;|})", ":0$1");
-        css = css.replaceAll("(?<!flex):0 0(;|})", ":0$1");
+        // Replace ":0 0 0 0" / ":0 0 0" / ":0 0" with ":0", for the properties where
+        // that is the box-model shorthand rather than a value with a fixed arity -
+        // see ZERO_RUN_NOT_COLLAPSIBLE.
+        p = Pattern.compile("(?i)([-a-z0-9_]+):0(?: 0){1,3}(;|})");
+        m = p.matcher(css);
+        sb = new StringBuffer();
+        while (m.find()) {
+            String property = m.group(1).toLowerCase().replaceFirst("^-(?:webkit|moz|ms|o)-", "");
+            String replacement = ZERO_RUN_NOT_COLLAPSIBLE.contains(property)
+                    ? m.group(0)
+                    : m.group(1) + ":0" + m.group(2);
+            m.appendReplacement(sb, Matcher.quoteReplacement(replacement));
+        }
+        m.appendTail(sb);
+        css = sb.toString();
 
 
         // Replace background-position:0; with background-position:0 0;

src/test/java/org/codelibs/yuicompressor/CssColorFunctionTest.java

@@ -0,0 +1,154 @@
+package org.codelibs.yuicompressor;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.io.StringReader;
+import java.io.StringWriter;
+
+import org.junit.jupiter.api.Test;
+
+import com.yahoo.platform.yui.compressor.CssCompressor;
+
+/**
+ * The zero-value unit stripping ("0px" -> "0") must not touch a "%" that its
+ * colour function requires.
+ *
+ * <p>hsl(), hsla(), rgb(), rgba() and color-mix() take {@code <percentage>}
+ * arguments in their comma-separated legacy form; a bare "0" there is not a value
+ * any browser accepts, so the declaration is dropped and the colour silently
+ * disappears from the page.
+ */
+class CssColorFunctionTest {
+
+    private String compress(String source) throws Exception {
+        StringWriter out = new StringWriter();
+        new CssCompressor(new StringReader(source)).compress(out, -1);
+        return out.toString().trim();
+    }
+
+    @Test
+    void hslaKeepsZeroPercentSaturation() throws Exception {
+        assertEquals("a{color:hsla(0,0%,100%,0.5)}", compress("a{color:hsla(0,0%,100%,0.5)}"));
+    }
+
+    @Test
+    void hslKeepsZeroPercentSaturation() throws Exception {
+        assertEquals("a{color:hsl(0,0%,100%)}", compress("a{color:hsl(0,0%,100%)}"));
+    }
+
+    @Test
+    void hslaKeepsBothZeroPercentArguments() throws Exception {
+        assertEquals("a{color:hsla(120,0%,0%,1)}", compress("a{color:hsla(120,0%,0%,1)}"));
+    }
+
+    @Test
+    void rgbKeepsZeroPercentChannel() throws Exception {
+        assertEquals("a{color:rgb(0%,50%,0%)}", compress("a{color:rgb(0%,50%,0%)}"));
+    }
+
+    @Test
+    void rgbaKeepsZeroPercentChannel() throws Exception {
+        assertEquals("a{color:rgba(0%,50%,50%,0.4)}", compress("a{color:rgba(0%,50%,50%,0.4)}"));
+    }
+
+    @Test
+    void colorMixKeepsZeroPercent() throws Exception {
+        assertEquals("a{color:color-mix(in srgb,red 0%,blue)}",
+                compress("a{color:color-mix(in srgb, red 0%, blue)}"));
+    }
+
+    /**
+     * A nested colour function keeps its percentages. The stop that follows it
+     * keeps its own "%" too - the argument-prefix part of the pattern cannot
+     * step over the nested parentheses to reach it - which is a missed byte,
+     * not a correctness problem.
+     */
+    @Test
+    void nestedColorFunctionKeepsItsPercentages() throws Exception {
+        assertEquals("a{background:linear-gradient(hsla(0,0%,50%,1) 0%,red 100%)}",
+                compress("a{background:linear-gradient(hsla(0,0%,50%,1) 0%, red 100%)}"));
+    }
+
+    @Test
+    void gradientColorStopStillLosesItsPercent() throws Exception {
+        assertEquals("a{background:linear-gradient(red 0,blue 100%)}",
+                compress("a{background:linear-gradient(red 0%, blue 100%)}"));
+    }
+
+    @Test
+    void minKeepsZeroPercentSoItsArgumentsStayTypeCompatible() throws Exception {
+        assertEquals("a{width:min(0%,10px)}", compress("a{width:min(0%, 10px)}"));
+    }
+
+    @Test
+    void maxKeepsZeroPercent() throws Exception {
+        assertEquals("a{width:max(0%,10px)}", compress("a{width:max(0%, 10px)}"));
+    }
+
+    @Test
+    void clampKeepsZeroPercent() throws Exception {
+        assertEquals("a{width:clamp(0%,50%,100%)}", compress("a{width:clamp(0%, 50%, 100%)}"));
+    }
+
+    /** "minmax" must not be caught by the "min" entry: a zero there may lose its unit. */
+    @Test
+    void nonColorFunctionStillLosesItsPercent() throws Exception {
+        assertEquals("a{grid-template-columns:minmax(0,1fr)}",
+                compress("a{grid-template-columns:minmax(0%,1fr)}"));
+    }
+
+    @Test
+    void lengthUnitsInsideGroupsAreStillStripped() throws Exception {
+        assertEquals("a{transform:translate(0,10px)}", compress("a{transform:translate(0px,10px)}"));
+    }
+
+    // ------------------------------------------------------------------
+    // A run of zeroes may only collapse to one where the property is a
+    // box-model shorthand.
+    // ------------------------------------------------------------------
+
+    @Test
+    void boxShadowKeepsBothOffsets() throws Exception {
+        assertEquals("a{box-shadow:0 0}", compress("a{box-shadow:0 0}"));
+    }
+
+    @Test
+    void boxShadowWithSpreadIsNotCollapsed() throws Exception {
+        assertEquals("a{box-shadow:0 0 0 0}", compress("a{box-shadow:0 0 0 0}"));
+    }
+
+    @Test
+    void vendorPrefixedBoxShadowIsNotCollapsed() throws Exception {
+        assertEquals("a{-webkit-box-shadow:0 0 0 0}", compress("a{-webkit-box-shadow:0 0 0 0}"));
+    }
+
+    @Test
+    void textShadowKeepsBothOffsets() throws Exception {
+        assertEquals("a{text-shadow:0 0 0}", compress("a{text-shadow:0 0 0}"));
+    }
+
+    /** "perspective-origin:0" means "0 center", which is not "0 0". */
+    @Test
+    void perspectiveOriginKeepsBothAxes() throws Exception {
+        assertEquals("a{perspective-origin:0 0}", compress("a{perspective-origin:0 0}"));
+    }
+
+    @Test
+    void flexIsStillNotCollapsed() throws Exception {
+        assertEquals("a{flex:0 0}", compress("a{flex:0 0}"));
+    }
+
+    @Test
+    void boxModelShorthandsStillCollapse() throws Exception {
+        assertEquals("a{margin:0}", compress("a{margin:0 0 0 0}"));
+        assertEquals("a{padding:0}", compress("a{padding:0 0}"));
+        assertEquals("a{border-radius:0}", compress("a{border-radius:0 0}"));
+        assertEquals("a{gap:0}", compress("a{gap:0 0}"));
+    }
+
+    @Test
+    void backgroundPositionIsStillNormalisedToTwoAxes() throws Exception {
+        assertEquals("a{background-position:0 0}", compress("a{background-position:0 0}"));
+        assertEquals("a{transform-origin:0 0}", compress("a{transform-origin:0 0}"));
+    }
+}

src/test/java/org/codelibs/yuicompressor/KnownCssLimitationsTest.java

@@ -35,63 +35,13 @@ private static String css(String source) throws IOException {
     }
 
     // ------------------------------------------------------------------
-    // A. Zero-percentage stripping produces invalid CSS.
+    // A. Zero-value unit stripping inside a group.
     //
-    // CssCompressor.java (identical on main):
-    //   Pattern.compile("(?i)\\( ?((?:[0-9a-z-.]+[ ,])*)?(?:0?\\.)?0(?:px|em|%|in|cm|mm|pc|pt|ex|deg|g?rad|m?s|k?hz)")
-    //
-    // The rule is "a zero <length> may drop its unit", which is true, and it is
-    // applied to "%" as well - but a <percentage> is a distinct type, not a
-    // length, and several grammars accept ONLY a percentage in the position
-    // this rewrites. The rule fires on anything inside a "(", so it reaches
-    // colour functions and math functions it was never meant for.
+    // The <percentage> half of this family is fixed: CssCompressor now skips
+    // the functions whose grammar requires a real percentage, and
+    // CssColorFunctionTest pins that. What is left is the <time> case below.
     // ------------------------------------------------------------------
 
-    /**
-     * {@code color-mix()} takes {@code <percentage [0,100]>}. A bare {@code 0}
-     * is not a percentage, so the whole declaration is dropped by a browser -
-     * the element loses its colour entirely. The cleanest case of the family:
-     * no legacy/modern grammar ambiguity to argue about.
-     */
-    @Test
-    void colorMixPercentageLosesItsUnit_knownDefect() {
-        assertDoesNotRoundTrip("a{color:color-mix(in srgb, red 0%, blue)}",
-                "a{color:color-mix(in srgb,red 0,blue)}");
-    }
-
-    /**
-     * Legacy comma-separated {@code rgb()} is
-     * {@code rgb(<percentage>#{3})} or {@code rgb(<number>#{3})} - the two may
-     * not be mixed. Stripping one unit produces exactly that mixture.
-     */
-    @Test
-    void rgbPercentageChannelLosesItsUnitAndMixesTypes_knownDefect() {
-        assertDoesNotRoundTrip("a{color:rgb(0%,50%,100%)}", "a{color:rgb(0,50%,100%)}");
-    }
-
-    /**
-     * Legacy comma-separated {@code hsl()}/{@code hsla()} require
-     * {@code <percentage>} for saturation and lightness.
-     */
-    @ParameterizedTest
-    @CsvSource(delimiter = '|', value = {
-            "a{color:hsl(27,0%,50%)}|a{color:hsl(27,0,50%)}",
-            "a{color:hsl(27, 0%, 50%)}|a{color:hsl(27,0,50%)}",
-            "a{color:hsla(27,0%,50%,0.5)}|a{color:hsla(27,0,50%,0.5)}" })
-    void hslSaturationLosesItsUnit_knownDefect(String source, String wrongOutput) throws Exception {
-        assertEquals(wrongOutput, css(source), "if this now keeps the unit, the defect is fixed - delete this row");
-    }
-
-    /**
-     * CSS math functions are type-checked: {@code min()} may not compare a
-     * {@code <number>} with a {@code <length>}. Stripping the "%" turns a legal
-     * comparison into an invalid one.
-     */
-    @Test
-    void aMathFunctionArgumentLosesItsPercentageAndBreaksTypeChecking_knownDefect() {
-        assertDoesNotRoundTrip("a{width:min(0%, 10px)}", "a{width:min(0,10px)}");
-    }
-
     /**
      * The same rule strips {@code <time>} units, but only inside a
      * parenthesised group. {@link ModernCssTest#zeroTimeValuesKeepTheirUnit}