From 4ca6514170079b1e937c8bf55ed616aec7b017ef Mon Sep 17 00:00:00 2001 From: Ashish Kulkarni Date: Fri, 11 Aug 2017 20:16:00 +0530 Subject: [PATCH] CSS minification issues fix. Issue #1538 and #1572 fix. --- .../css_minify_calc_function_value_zero.html | 5 +++ .../css_minify_unicode_range_descriptor.html | 5 +++ net/instaweb/rewriter/css_minify.cc | 28 +++++++++++++---- net/instaweb/rewriter/css_minify_test.cc | 31 +++++++++++++++++++ net/instaweb/rewriter/public/css_minify.h | 3 ++ pagespeed/system/system_test.sh | 2 ++ .../css_minify_calc_function_value_zero.sh | 25 +++++++++++++++ .../css_minify_unicode_range_descriptor.sh | 25 +++++++++++++++ 8 files changed, 118 insertions(+), 6 deletions(-) create mode 100644 install/mod_pagespeed_test/css_minify_calc_function_value_zero.html create mode 100644 install/mod_pagespeed_test/css_minify_unicode_range_descriptor.html create mode 100644 pagespeed/system/system_tests/css_minify_calc_function_value_zero.sh create mode 100644 pagespeed/system/system_tests/css_minify_unicode_range_descriptor.sh 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 new file mode 100644 index 0000000000..7d62cde10f --- /dev/null +++ b/install/mod_pagespeed_test/css_minify_calc_function_value_zero.html @@ -0,0 +1,5 @@ + + + + + diff --git a/install/mod_pagespeed_test/css_minify_unicode_range_descriptor.html b/install/mod_pagespeed_test/css_minify_unicode_range_descriptor.html new file mode 100644 index 0000000000..a357fcf4dc --- /dev/null +++ b/install/mod_pagespeed_test/css_minify_unicode_range_descriptor.html @@ -0,0 +1,5 @@ + + + + + diff --git a/net/instaweb/rewriter/css_minify.cc b/net/instaweb/rewriter/css_minify.cc index 44ff0f2163..ccc60bd019 100644 --- 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 index 7ec8370f53..4300f42078 100644 --- 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 index 9106d78341..74b2f5fbfb 100644 --- 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 index 1a4b84264e..e884cd7cbf 100755 --- 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 new file mode 100644 index 0000000000..6a654a8115 --- /dev/null +++ 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 new file mode 100644 index 0000000000..ea2789b711 --- /dev/null +++ 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 ]