height:0% should not be minified to height:0

Fixes #1261
apache · Feb 11, 2016 · 7c30b7e · 7c30b7e
1 parent 60f8018
commit 7c30b7e
Show file tree

Hide file tree

Showing 3 changed files with 10 additions and 5 deletions.
diff --git a/net/instaweb/rewriter/css_filter_test.cc b/net/instaweb/rewriter/css_filter_test.cc
@@ -1042,7 +1042,7 @@ TEST_F(CssFilterTest, ComplexCssTest) {
       "}\n"
       ".foo { color: rgba(1, 2, 3, 0.4); }\n",
 
-      "body{background-image:-webkit-gradient(linear,50% 0,50% 100%,"
+      "body{background-image:-webkit-gradient(linear,50% 0%,50% 100%,"
       "from(#e8edf0),to(#fcfcfd));color:red}.foo{color:rgba(1,2,3,.4)}" },
 
     // Counters

diff --git a/net/instaweb/rewriter/css_minify.cc b/net/instaweb/rewriter/css_minify.cc
@@ -356,8 +356,12 @@ bool IsLength(const GoogleString& unit) {
 
 bool UnitsRequiredForValueZero(const GoogleString& unit) {
   // https://github.com/pagespeed/mod_pagespeed/issues/1164 : Chrome does not
-  // allow abbreviating 0s as 0.  It only allows that abbreviation for lengths.
-  return !IsLength(unit) && (unit != "%");
+  // allow abbreviating 0s or 0% as 0.  It only allows that abbreviation for
+  // lengths.
+  //
+  // https://github.com/pagespeed/mod_pagespeed/issues/1261  See
+  // https://www.w3.org/TR/CSS2/visudet.html#the-height-property
+  return (unit == "%") || !IsLength(unit);
 }
 
 }  // namespace

diff --git a/net/instaweb/rewriter/css_minify_test.cc b/net/instaweb/rewriter/css_minify_test.cc
@@ -133,10 +133,11 @@ TEST_F(CssMinifyTest, DoNotFixBadColorsOrUnits) {
       minified);
 }
 
-TEST_F(CssMinifyTest, RemoveZeroLengthButNotTimeSuffix) {
+TEST_F(CssMinifyTest, RemoveZeroLengthButNotTimeOrPercentSuffix) {
   const char kCss[] =
       ".a {\n"
       "  width: 0px;\n"
+      "  height: 0%;\n"
       "  -moz-transition-delay: 0s, 0s;\n"
       "}";
   GoogleString minified;
@@ -145,7 +146,7 @@ TEST_F(CssMinifyTest, RemoveZeroLengthButNotTimeSuffix) {
   EXPECT_TRUE(minify.ParseStylesheet(kCss));
   // TODO(jmarantz): this CSS is not well minified.  We should strip
   // the spaces around the comma.
-  EXPECT_STREQ(".a{width:0;-moz-transition-delay:0s , 0s}", minified);
+  EXPECT_STREQ(".a{width:0;height:0%;-moz-transition-delay:0s , 0s}", minified);
 }
 
 }  // namespace