From f2e8d3798c6dd8829b812d5bd1ecd71b6f2c2ad7 Mon Sep 17 00:00:00 2001 From: Sam Hemelryk Date: Tue, 5 Jun 2012 10:02:48 +1200 Subject: [PATCH] MDL-33468 css_optimiser: Numerous fixes to the CSS optimiser and its tests --- lib/csslib.php | 131 ++++++++++++++++++++++++++++++++----- lib/tests/cssslib_test.php | 106 +++++++++++++++++++++++++++++- 2 files changed, 219 insertions(+), 18 deletions(-) diff --git a/lib/csslib.php b/lib/csslib.php index c5bc0f56d40..e9d80306339 100644 --- a/lib/csslib.php +++ b/lib/csslib.php @@ -352,7 +352,7 @@ function css_is_width($value) { if (in_array(strtolower($value), array('auto', 'inherit'))) { return true; } - if (preg_match('#^(\-\s*)?(\d*\.)?(\d+)\s*(em|px|pt|%|in|cm|mm|ex|pc)?$#i', $value)) { + if ((string)$value === '0' || preg_match('#^(\-\s*)?(\d*\.)?(\d+)\s*(em|px|pt|\%|in|cm|mm|ex|pc)$#i', $value)) { return true; } return false; @@ -501,10 +501,17 @@ class css_optimiser { public function process($css) { global $CFG; + $css = trim($css); + $this->reset_stats(); $this->timestart = microtime(true); $this->rawstrlen = strlen($css); + if ($this->rawstrlen === 0) { + $this->errors[] = 'Skipping file as it has no content.'; + return ''; + } + // First up we need to remove all line breaks - this allows us to instantly // reduce our processing requirements and as we will process everything // into a new structure there's really nothing lost. @@ -749,7 +756,7 @@ class css_optimiser { $this->optimisedrules += $media->count_rules(); $this->optimisedselectors += $media->count_selectors(); if ($media->has_errors()) { - $this->errors[] = $media->get_errors(); + $this->errors += $media->get_errors(); } $css .= $media->out(); } @@ -774,11 +781,20 @@ class css_optimiser { 'rawrules' => $this->rawrules, 'optimisedstrlen' => $this->optimisedstrlen, 'optimisedrules' => $this->optimisedrules, - 'optimisedselectors' => $this->optimisedselectors, - 'improvementstrlen' => round(100 - ($this->optimisedstrlen / $this->rawstrlen) * 100, 1).'%', - 'improvementrules' => round(100 - ($this->optimisedrules / $this->rawrules) * 100, 1).'%', - 'improvementselectors' => round(100 - ($this->optimisedselectors / $this->rawselectors) * 100, 1).'%', + 'optimisedselectors' => $this->optimisedselectors, + 'improvementstrlen' => '-', + 'improvementrules' => '-', + 'improvementselectors' => '-', ); + if ($this->rawstrlen > 0) { + $stats['improvementstrlen'] = round(100 - ($this->optimisedstrlen / $this->rawstrlen) * 100, 1).'%'; + } + if ($this->rawrules > 0) { + $stats['improvementrules'] = round(100 - ($this->optimisedrules / $this->rawrules) * 100, 1).'%'; + } + if ($this->rawselectors > 0) { + $stats['improvementselectors'] = round(100 - ($this->optimisedselectors / $this->rawselectors) * 100, 1).'%'; + } return $stats; } @@ -794,10 +810,16 @@ class css_optimiser { /** * Returns an array of errors that have occured * + * @param bool $clear If set to true the errors will be cleared after being returned. * @return array */ - public function get_errors() { - return $this->errors; + public function get_errors($clear = false) { + $errors = $this->errors; + if ($clear) { + // Reset the error array + $this->errors = array(); + } + return $errors; } /** @@ -821,15 +843,22 @@ class css_optimiser { * @return string */ public function output_stats_css() { - $stats = $this->get_stats(); - - $strlenimprovement = round(100 - ($this->optimisedstrlen / $this->rawstrlen) * 100, 1); - $ruleimprovement = round(100 - ($this->optimisedrules / $this->rawrules) * 100, 1); - $selectorimprovement = round(100 - ($this->optimisedselectors / $this->rawselectors) * 100, 1); - $timetaken = round($this->timecomplete - $this->timestart, 4); $computedcss = "/****************************************\n"; $computedcss .= " *------- CSS Optimisation stats --------\n"; + + if ($this->rawstrlen === 0) { + $computedcss .= " File not processed as it has no content /\n\n"; + $computedcss .= " ****************************************/\n\n"; + return $computedcss; + } else if ($this->rawrules === 0) { + $computedcss .= " File contained no rules to be processed /\n\n"; + $computedcss .= " ****************************************/\n\n"; + return $computedcss; + } + + $stats = $this->get_stats(); + $computedcss .= " * ".date('r')."\n"; $computedcss .= " * {$stats['commentsincss']} \t comments removed\n"; $computedcss .= " * Optimisation took {$stats['timetaken']} seconds\n"; @@ -1169,6 +1198,7 @@ abstract class css_writer { * @return string */ public static function style($name, $value, $important = false) { + $value = trim($value); if ($important && strpos($value, '!important') === false) { $value .= ' !important'; } @@ -1657,7 +1687,7 @@ class css_media { $errors[] = $rule->get_error_string(); } } - return join("\n", $errors); + return $errors; } } @@ -1745,6 +1775,7 @@ abstract class css_style { $important = preg_match('#(\!important\s*;?\s*)$#', $value, $matches); if ($important) { $value = substr($value, 0, -(strlen($matches[1]))); + $value = rtrim($value); } if (!$this->important || $important) { $this->value = $this->clean_value($value); @@ -2233,7 +2264,7 @@ class css_style_border extends css_style { $return = array(); if (count($bits) > 0) { $width = array_shift($bits); - if (!css_is_width($width)) { + if (!css_style_borderwidth::is_border_width($width)) { $width = '0'; } $return[] = new css_style_borderwidth('border-top-width', $width); @@ -2801,6 +2832,35 @@ class css_style_borderwidth extends css_style_width { public function consolidate_to() { return 'border'; } + + /** + * Checks if the width is valid + * @return bool + */ + public function is_valid() { + return self::is_border_width($this->value); + } + + /** + * Cleans the provided value + * + * @param mixed $value Cleans the provided value optimising it if possible + * @return string + */ + protected function clean_value($value) { + $isvalid = self::is_border_width($value); + if (!$isvalid) { + $this->set_error('Invalid width specified for '.$this->name); + } else if (preg_match('#^0\D+$#', $value)) { + return '0'; + } + return trim($value); + } + + public static function is_border_width($value) { + $altwidthvalues = array('thin', 'medium', 'thick'); + return css_is_width($value) || in_array($value, $altwidthvalues); + } } /** @@ -3679,3 +3739,42 @@ class css_style_paddingleft extends css_style_padding { return 'padding'; } } + +class css_style_cursor extends css_style_generic { + public static function init($value) { + return new css_style_cursor('cursor', $value); + } + protected function clean_value($value) { + $allowed = array('auto', 'crosshair', 'default', 'e-resize', 'help', 'move', 'n-resize', 'ne-resize', 'nw-resize', + 'pointer', 'progress', 's-resize', 'se-resize', 'sw-resize', 'text', 'w-resize', 'wait', 'inherit'); + if (!in_array($value, $allowed) && !preg_match('#\.[a-zA-Z0-9_\-]{1,5}$#', $value)) { + $this->set_error('Invalid or unexpected cursor value specified: '.$value); + } + return trim($value); + } +} + +class css_style_verticalalign extends css_style_generic { + public static function init($value) { + return new css_style_verticalalign('vertical-align', $value); + } + protected function clean_value($value) { + $allowed = array('baseline', 'sub', 'super', 'top', 'text-top', 'middle', 'bottom', 'text-bottom', 'inherit'); + if (!css_is_width($value) && !in_array($value, $allowed)) { + $this->set_error('Invalid vertical-align value specified: '.$value); + } + return trim($value); + } +} +class css_style_float extends css_style_generic { + public static function init($value) { + return new css_style_float('float', $value); + } + protected function clean_value($value) { + $allowed = array('left', 'right', 'none', 'inherit'); + if (!css_is_width($value) && !in_array($value, $allowed)) { + $this->set_error('Invalid float value specified: '.$value); + } + return trim($value); + } +} \ No newline at end of file diff --git a/lib/tests/cssslib_test.php b/lib/tests/cssslib_test.php index 2faf911590c..fea80f211b0 100644 --- a/lib/tests/cssslib_test.php +++ b/lib/tests/cssslib_test.php @@ -65,11 +65,14 @@ class css_optimiser_testcase extends advanced_testcase { $this->check_margins($optimiser); $this->check_padding($optimiser); $this->check_widths($optimiser); + $this->check_cursor($optimiser); + $this->check_vertical_align($optimiser); $this->try_broken_css_found_in_moodle($optimiser); $this->try_invalid_css_handling($optimiser); $this->try_bulk_processing($optimiser); $this->try_break_things($optimiser); + $this->try_advanced_css_animation($optimiser); } /** @@ -90,6 +93,11 @@ class css_optimiser_testcase extends advanced_testcase { $cssout = '.test{background:#123456 url(\'test.png\') no-repeat top left;}'; $this->assertEquals($cssout, $optimiser->process($cssin)); + // Check out this for madness, background position and background-repeat have been reversed + $cssin = '.test {background: #123456 url(\'test.png\') center no-repeat;}'; + $cssout = '.test{background:#123456 url(\'test.png\') no-repeat center;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + $cssin = '.test {background: url(\'test.png\') no-repeat top left;}.test{background-position: bottom right}.test {background-color:#123456;}'; $cssout = '.test{background:#123456 url(\'test.png\') no-repeat bottom right;}'; $this->assertEquals($cssout, $optimiser->process($cssin)); @@ -155,6 +163,14 @@ class css_optimiser_testcase extends advanced_testcase { $cssout = ".one, .two{border-width:0;}"; $this->assertEquals($cssout, $optimiser->process($cssin)); + $cssin = '.one, .two {border: thin;}'; + $cssout = ".one, .two{border-width:thin;}"; + $this->assertEquals($cssout, $optimiser->process($cssin)); + + $cssin = '.one, .two {border: thin solid black;}'; + $cssout = ".one, .two{border:thin solid #000000;}"; + $this->assertEquals($cssout, $optimiser->process($cssin)); + $cssin = '.one, .two {border-top: 5px solid white;}'; $cssout = ".one, .two{border-top:5px solid #FFFFFF;}"; $this->assertEquals($cssout, $optimiser->process($cssin)); @@ -270,6 +286,10 @@ class css_optimiser_testcase extends advanced_testcase { $cssout = '.one{color:#123 !important;}'; $this->assertEquals($cssout, $optimiser->process($cssin)); + $cssin = '.one {color:#123!important;} .one {color:#321;}'; + $cssout = '.one{color:#123 !important;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + $cssin = '.one {color:rgb(255, 128, 1)}'; $cssout = '.one{color:rgb(255, 128, 1);}'; $this->assertEquals($cssout, $optimiser->process($cssin)); @@ -395,6 +415,70 @@ class css_optimiser_testcase extends advanced_testcase { $this->assertEquals($cssout, $optimiser->process($cssin)); } + protected function check_cursor(css_optimiser $optimiser) { + // Valid cursor + $cssin = '.one {cursor: pointer;}'; + $cssout = '.one{cursor:pointer;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + + // Invalid cursor but tollerated + $cssin = '.one {cursor: hand;}'; + $cssout = '.one{cursor:hand;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + + // Valid cursor: url relative + $cssin = '.one {cursor: mycursor.png;}'; + $cssout = '.one{cursor:mycursor.png;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + + // Valid cursor: url absolute + $cssin = '.one {cursor: http://local.host/mycursor.png;}'; + $cssout = '.one{cursor:http://local.host/mycursor.png;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + } + + protected function check_vertical_align(css_optimiser $optimiser) { + // Valid vertical aligns + $cssin = '.one {vertical-align: baseline;}'; + $cssout = '.one{vertical-align:baseline;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + $cssin = '.one {vertical-align: middle;}'; + $cssout = '.one{vertical-align:middle;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + $cssin = '.one {vertical-align: 0.75em;}'; + $cssout = '.one{vertical-align:0.75em;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + $cssin = '.one {vertical-align: 50%;}'; + $cssout = '.one{vertical-align:50%;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + + // Invalid but tollerated + $cssin = '.one {vertical-align: center;}'; + $cssout = '.one{vertical-align:center;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + } + + protected function check_float(css_optimiser $optimiser) { + // Valid vertical aligns + $cssin = '.one {float: inherit;}'; + $cssout = '.one{float:inherit;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + $cssin = '.one {float: left;}'; + $cssout = '.one{float:left;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + $cssin = '.one {float: right;}'; + $cssout = '.one{float:right;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + $cssin = '.one {float: none;}'; + $cssout = '.one{float:none;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + + // Invalid but tollerated + $cssin = '.one {float: center;}'; + $cssout = '.one{float:center;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + } + /** * Test some totally invalid CSS optimisation * @@ -663,11 +747,15 @@ CSS; $this->assertTrue(css_is_width('199px')); $this->assertTrue(css_is_width('199em')); $this->assertTrue(css_is_width('199%')); - $this->assertTrue(css_is_width('-1')); $this->assertTrue(css_is_width('-1px')); $this->assertTrue(css_is_width('auto')); $this->assertTrue(css_is_width('inherit')); + // Valid widths but missing their unit specifier + $this->assertFalse(css_is_width('0.75')); + $this->assertFalse(css_is_width('3')); + $this->assertFalse(css_is_width('-1')); + // Totally invalid widths $this->assertFalse(css_is_width('-')); $this->assertFalse(css_is_width('bananas')); $this->assertFalse(css_is_width('')); @@ -718,4 +806,18 @@ CSS; $cssout = '.test{opacity:0.5;-ms-filter:"progid:DXImageTransform.Microsoft.Alpha(Opacity=50)";filter:alpha(opacity=50);}'; $this->assertEquals($cssout, $optimiser->process($cssin)); } -} + + public function try_advanced_css_animation(css_optimiser $optimiser) { + $css = '.dndupload-arrow{width:56px;height:47px;position:absolute;animation:mymove 5s infinite;-moz-animation:mymove 5s infinite;-webkit-animation:mymove 5s infinite;background:url(\'[[pix:theme|fp/dnd_arrow]]\') center no-repeat;margin-left:-28px;}'; + $css = '.dndupload-arrow{width:56px;height:47px;position:absolute;animation:mymove 5s infinite;-moz-animation:mymove 5s infinite;-webkit-animation:mymove 5s infinite;background:url(\'[[pix:theme|fp/dnd_arrow]]\') no-repeat center;margin-left:-28px;}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + + $cssin = '@keyframes mymove{0%{top:10px;}12%{top:40px;}30%{top:20px}65%{top:35px;}100%{top:9px;}}'; + $cssout = '@keyframes mymove{0%{top:10px;} 12%{top:40px;} 30%{top:20px} 65%{top:35px;} 100%{top:9px;}}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + + $cssin = '@keyframes mymove{0%{top:10px;} 12%{top:40px;} 30%{top:20px} 65%{top:35px;} 100%{top:9px;}} @-moz-keyframes mymove{0%{top:10px;}12%{top:40px;}30%{top:20px}65%{top:35px;}100%{top:9px;}} @-webkit-keyframes mymove{0%{top:10px;}12%{top:40px;}30%{top:20px}65%{top:35px;}100%{top:9px;}}'; + $cssout = '@keyframes mymove{0%{top:10px;} 12%{top:40px;} 30%{top:20px} 65%{top:35px;} 100%{top:9px;}} @-moz-keyframes mymove{0%{top:10px;}12%{top:40px;}30%{top:20px}65%{top:35px;}100%{top:9px;}} @-webkit-keyframes mymove{0%{top:10px;}12%{top:40px;}30%{top:20px}65%{top:35px;}100%{top:9px;}}'; + $this->assertEquals($cssout, $optimiser->process($cssin)); + } +} \ No newline at end of file