MDL-33468 css_optimiser: Numerous fixes to the CSS optimiser and its tests

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);
+    }
+}

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));
+    }
+}