CSS minification issues fix. Issue apache#1538 and apache#1572 fix.

ashishk-1 · Aug 11, 2017 · 4ca6514 · 4ca6514
1 parent 5fe092c
commit 4ca6514
Show file tree

Hide file tree

Showing 8 changed files with 118 additions and 6 deletions.
diff --git a/install/mod_pagespeed_test/css_minify_calc_function_value_zero.html b/install/mod_pagespeed_test/css_minify_calc_function_value_zero.html
@@ -0,0 +1,5 @@
+<html>
+  <head>
+     <style property="stylesheet">@font-face{width: calc(50ex - 0px)}</style>
+  </head>
+</html>
diff --git a/install/mod_pagespeed_test/css_minify_unicode_range_descriptor.html b/install/mod_pagespeed_test/css_minify_unicode_range_descriptor.html
@@ -0,0 +1,5 @@
+<html>
+  <head>
+     <style property="stylesheet">@font-face{unicode-range: U+0400-045F, U+0490-0491, U+04B0-04B1, U+2116;}</style>
+  </head>
+</html>
diff --git a/net/instaweb/rewriter/css_minify.cc b/net/instaweb/rewriter/css_minify.cc
@@ -90,7 +90,7 @@ bool CssMinify::Declarations(const Css::Declarations& declarations,
 
 CssMinify::CssMinify(Writer* writer, MessageHandler* handler)
     : writer_(writer), error_writer_(NULL), handler_(handler), ok_(true),
-      url_collector_(NULL) {
+      url_collector_(NULL), in_css_calc_function_(false) {
 }
 
 CssMinify::~CssMinify() {
@@ -354,18 +354,22 @@ bool IsLength(const GoogleString& unit) {
                             unit);
 }
 
-bool UnitsRequiredForValueZero(const GoogleString& unit) {
+}  // namespace
+
+bool CssMinify::UnitsRequiredForValueZero(const GoogleString& unit) {
   // https://github.com/pagespeed/mod_pagespeed/issues/1164 : Chrome does not
   // 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);
+  //
+  // https://github.com/pagespeed/mod_pagespeed/issues/1538
+  // retaining unit for zero value in calc function
+  return (unit == "%") || !IsLength(unit) ||
+          in_css_calc_function_;
 }
 
-}  // namespace
-
 void CssMinify::MinifyFont(const Css::Values& font_values) {
   CHECK_LE(5U, font_values.size());
 
@@ -423,7 +427,15 @@ void CssMinify::Minify(const Css::Declaration& declaration) {
         }
         break;
       default:
-        JoinMinify(*declaration.values(), " ");
+        // TODO(ashishk):unicode-range should get resolved to css property
+        // enum.
+        if (declaration.prop_text() == "unicode-range") {
+          // https://github.com/pagespeed/mod_pagespeed/issues/1572
+          // space separator should not be there in unicode range value
+          JoinMinify(*declaration.values(), "");
+        } else {
+          JoinMinify(*declaration.values(), " ");
+        }
         break;
     }
     if (declaration.IsImportant()) {
@@ -477,10 +489,14 @@ void CssMinify::Minify(const Css::Value& value) {
       Write(")");
       break;
     case Css::Value::FUNCTION:
+      if (Css::EscapeIdentifier(value.GetFunctionName()) == "calc") {
+        in_css_calc_function_ = true;
+      }
       Write(Css::EscapeIdentifier(value.GetFunctionName()));
       Write("(");
       Minify(*value.GetParametersWithSeparators());
       Write(")");
+      in_css_calc_function_ = false;
       break;
     case Css::Value::RECT:
       Write("rect(");

diff --git a/net/instaweb/rewriter/css_minify_test.cc b/net/instaweb/rewriter/css_minify_test.cc
@@ -255,5 +255,36 @@ TEST_F(CssMinifyTest, StraySingleQuote3) {
   EXPECT_STREQ(".view_all a{display:block;margin:1px}", minified);
 }
 
+// checking whether unit is retained for zero value in calc function
+TEST_F(CssMinifyTest, CalcFunctionWithZeroValueAndUnit) {
+  static const char kCss[] =
+      "@font-face {\n"
+      " width: calc(600px - 0px)"
+      "}";
+
+  GoogleString minified;
+  StringWriter writer(&minified);
+  CssMinify minify(&writer, &handler_);
+
+  EXPECT_TRUE(minify.ParseStylesheet(kCss));
+  EXPECT_HAS_SUBSTR("width:calc(600px - 0px)", minified);
+}
+
+// checking unicode-range descriptor minification
+TEST_F(CssMinifyTest, CssUnicodeRangeDescriptor) {
+  static const char kCss[] =
+      "@font-face {\n"
+      " unicode-range: U+0400-045F, U+0490-0491, U+04B0-04B1, U+2116;\n"
+      "}";
+
+  GoogleString minified;
+  StringWriter writer(&minified);
+  CssMinify minify(&writer, &handler_);
+
+  EXPECT_TRUE(minify.ParseStylesheet(kCss));
+  EXPECT_HAS_SUBSTR("unicode-range:U+0400-045F,U+0490-0491,U+04B0-04B1,U+2116",
+      minified);
+}
+
 }  // namespace
 }  // namespace net_instaweb
diff --git a/net/instaweb/rewriter/public/css_minify.h b/net/instaweb/rewriter/public/css_minify.h
@@ -123,12 +123,15 @@ class CssMinify {
   bool Equals(const Css::MediaExpression& a,
               const Css::MediaExpression& b) const;
 
+  bool UnitsRequiredForValueZero(const GoogleString& unit);
+
   Writer* writer_;
   Writer* error_writer_;
   MessageHandler* handler_;
   bool ok_;
 
   StringVector* url_collector_;
+  bool in_css_calc_function_;
 
   DISALLOW_COPY_AND_ASSIGN(CssMinify);
 };

diff --git a/pagespeed/system/system_test.sh b/pagespeed/system/system_test.sh
@@ -76,6 +76,8 @@ SYSTEM_TEST_DIR="$(dirname "${BASH_SOURCE[0]}")/system_tests/"
 run_test check_headers
 run_test aris
 run_test css_combining_authorization
+run_test css_minify_calc_function_value_zero
+run_test css_minify_unicode_range_descriptor
 run_test add_instrumentation
 run_test type_attribute_pedantic
 run_test cache_partial_html

diff --git a/pagespeed/system/system_tests/css_minify_calc_function_value_zero.sh b/pagespeed/system/system_tests/css_minify_calc_function_value_zero.sh
@@ -0,0 +1,25 @@
+#!/bin/bash
+#
+# Copyright 2017 Google Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+# Testing css minify for retained unit in calc function and with 0 value.
+
+start_test CSS minify with calc function and value 0.
+
+URL="$TEST_ROOT/css_minify_calc_function_value_zero.html?PageSpeedFilters=+inline_css"
+RESPONSE_OUT=$(http_proxy=$SECONDARY_HOSTNAME $WGET_DUMP $URL)
+
+# checking for minified css with unit retained for 0 value 
+MATCHES=$(echo "$RESPONSE_OUT" | fgrep -c "width:calc(50ex - 0px)")
+check [ $MATCHES -eq 1 ]
diff --git a/pagespeed/system/system_tests/css_minify_unicode_range_descriptor.sh b/pagespeed/system/system_tests/css_minify_unicode_range_descriptor.sh
@@ -0,0 +1,25 @@
+#!/bin/bash
+#
+# Copyright 2017 Google Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+# Testing css minification for unicode range descriptor in font-face
+
+start_test CSS minify for unicode-range descriptor.
+
+URL="$TEST_ROOT/css_minify_unicode_range_descriptor.html?PageSpeedFilters=+inline_css"
+RESPONSE_OUT=$(http_proxy=$SECONDARY_HOSTNAME $WGET_DUMP $URL)
+
+# checking for unicode range with valid construct 
+MATCHES=$(echo "$RESPONSE_OUT" | fgrep -c "unicode-range:U+0400-045F,U+0490-0491,U+04B0-04B1,U+2116")
+check [ $MATCHES -eq 1 ]