Bug 971234 - Only apply color swatch depending on cssPropertySupportsType. r=pbrosset, a=sledru
authorMichael Ratcliffe <mratcliffe@mozilla.com>
Mon, 11 May 2015 06:33:00 -0400
changeset 274734 e604816947a8cb60ccaf9625e9c9b7c84b00915c
parent 274733 3df9581c43e5178f97da7cf466e17ac901f67546
child 274735 41857031f2054b9d96bc453dba613f7b06154088
push id863
push userraliiev@mozilla.com
push dateMon, 03 Aug 2015 13:22:43 +0000
treeherdermozilla-release@f6321b14228d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbrosset, sledru
bugs971234
milestone40.0a2
Bug 971234 - Only apply color swatch depending on cssPropertySupportsType. r=pbrosset, a=sledru
browser/devtools/shared/test/browser_outputparser.js
toolkit/devtools/output-parser.js
--- a/browser/devtools/shared/test/browser_outputparser.js
+++ b/browser/devtools/shared/test/browser_outputparser.js
@@ -13,90 +13,121 @@ add_task(function*() {
 
 function* performTest() {
   let [host, win, doc] = yield createHost("bottom", "data:text/html," +
     "<h1>browser_outputParser.js</h1><div></div>");
 
   let parser = new OutputParser();
   testParseCssProperty(doc, parser);
   testParseCssVar(doc, parser);
-  testParseHTMLAttribute(doc, parser);
-  testParseNonCssHTMLAttribute(doc, parser);
 
   host.destroy();
 }
 
+// Class name used in color swatch.
+let COLOR_TEST_CLASS = 'test-class';
+
+// Create a new CSS color-parsing test.  |name| is the name of the CSS
+// property.  |value| is the CSS text to use.  |segments| is an array
+// describing the expected result.  If an element of |segments| is a
+// string, it is simply appended to the expected string.  Otherwise,
+// it must be an object with a |value| property and a |name| property.
+// These describe the color and are both used in the generated
+// expected output -- |name| is the color name as it appears in the
+// input (e.g., "red"); and |value| is the hash-style numeric value
+// for the color, which parseCssProperty emits in some spots (e.g.,
+// "#F00").
+//
+// This approach is taken to reduce boilerplate and to make it simpler
+// to modify the test when the parseCssProperty output changes.
+function makeColorTest(name, value, segments) {
+  let result = {
+    name,
+    value,
+    expected: ''
+  };
+
+  for (let segment of segments) {
+    if (typeof(segment) === 'string') {
+      result.expected += segment;
+    } else {
+      result.expected += '<span data-color="' + segment.value + '">' +
+        '<span style="background-color:' + segment.name +
+        '" class="' + COLOR_TEST_CLASS + '"></span><span>' +
+        segment.value + '</span></span>';
+    }
+  }
+
+  result.desc = "Testing " + name + ": " + value;
+
+  return result;
+}
+
 function testParseCssProperty(doc, parser) {
-  let frag = parser.parseCssProperty("border", "1px solid red", {
-    colorSwatchClass: "test-colorswatch"
-  });
+  let tests = [
+    makeColorTest("border", "1px solid red",
+                  ["1px solid ", {name: "red", value: "#F00"}]),
+
+    makeColorTest("background-image",
+                  "linear-gradient(to right, #F60 10%, rgba(0,0,0,1))",
+                  ["linear-gradient(to right, ", {name: "#F60", value: "#F60"},
+                   " 10%, ", {name: "rgba(0,0,0,1)", value: "#000"},
+                   ")"]),
+
+    // In "arial black", "black" is a font, not a color.
+    makeColorTest("font-family", "arial black", ["arial black"]),
+
+    makeColorTest("box-shadow", "0 0 1em red",
+                  ["0 0 1em ", {name: "red", value: "#F00"}]),
+
+    makeColorTest("box-shadow",
+                  "0 0 1em red, 2px 2px 0 0 rgba(0,0,0,.5)",
+                  ["0 0 1em ", {name: "red", value: "#F00"},
+                   ", 2px 2px 0 0 ",
+                   {name: "rgba(0,0,0,.5)", value: "rgba(0,0,0,.5)"}]),
+
+    makeColorTest("content", '"red"', ['"red"']),
+
+    // Invalid property names should not cause exceptions.
+    makeColorTest("hellothere", "'red'", ["'red'"]),
+
+    // This requires better parsing than we currently have available.
+    // See bug 1158288.
+    // makeColorTest("filter",
+    //               "blur(1px) drop-shadow(0 0 0 blue) url(red.svg#blue)",
+    //               ["blur(1px) drop-shadow(0 0 0 ",
+    //                {name: "blue", value: "#00F"},
+    //                ") url(red.svg#blue)"])
+
+  ];
 
   let target = doc.querySelector("div");
   ok(target, "captain, we have the div");
-  target.appendChild(frag);
+
+  for (let test of tests) {
+    info(test.desc);
 
-  is(target.innerHTML,
-     '1px solid <span data-color="#F00"><span style="background-color:red" class="test-colorswatch"></span><span>#F00</span></span>',
-     "CSS property correctly parsed");
-
-  target.innerHTML = "";
+    let frag = parser.parseCssProperty(test.name, test.value, {
+      colorSwatchClass: COLOR_TEST_CLASS
+    });
 
-  frag = parser.parseCssProperty("background-image", "linear-gradient(to right, #F60 10%, rgba(0,0,0,1))", {
-    colorSwatchClass: "test-colorswatch",
-    colorClass: "test-color"
-  });
-  target.appendChild(frag);
-  is(target.innerHTML,
-     'linear-gradient(to right, <span data-color="#F60"><span style="background-color:#F60" class="test-colorswatch"></span><span class="test-color">#F60</span></span> 10%, ' +
-     '<span data-color="#000"><span style="background-color:rgba(0,0,0,1)" class="test-colorswatch"></span><span class="test-color">#000</span></span>)',
-     "Gradient CSS property correctly parsed");
+    target.appendChild(frag);
 
-  target.innerHTML = "";
+    is(target.innerHTML, test.expected,
+       "CSS property correctly parsed for " + test.name + ": " + test.value);
+
+    target.innerHTML = "";
+  }
 }
 
 function testParseCssVar(doc, parser) {
   let frag = parser.parseCssProperty("color", "var(--some-kind-of-green)", {
     colorSwatchClass: "test-colorswatch"
   });
 
   let target = doc.querySelector("div");
   ok(target, "captain, we have the div");
   target.appendChild(frag);
 
   is(target.innerHTML, "var(--some-kind-of-green)", "CSS property correctly parsed");
 
   target.innerHTML = "";
 }
-
-function testParseHTMLAttribute(doc, parser) {
-  let attrib = "color:red; font-size: 12px; background-image: " +
-               "url(chrome://branding/content/about-logo.png)";
-  let frag = parser.parseHTMLAttribute(attrib, {
-    urlClass: "theme-link",
-    colorClass: "theme-color"
-  });
-
-  let target = doc.querySelector("div");
-  ok(target, "captain, we have the div");
-  target.appendChild(frag);
-
-  let expected = 'color:<span data-color="#F00"><span class="theme-color">#F00</span></span>; font-size: 12px; ' +
-                 'background-image: url("<a href="chrome://branding/content/about-logo.png" ' +
-                 'class="theme-link" ' +
-                 'target="_blank">chrome://branding/content/about-logo.png</a>")';
-
-  is(target.innerHTML, expected, "HTML Attribute correctly parsed");
-  target.innerHTML = "";
-}
-
-function testParseNonCssHTMLAttribute(doc, parser) {
-  let attrib = "someclass background someotherclass red";
-  let frag = parser.parseHTMLAttribute(attrib);
-
-  let target = doc.querySelector("div");
-  ok(target, "captain, we have the div");
-  target.appendChild(frag);
-
-  let expected = 'someclass background someotherclass red';
-
-  is(target.innerHTML, expected, "Non-CSS HTML Attribute correctly parsed");
-  target.innerHTML = "";
-}
--- a/toolkit/devtools/output-parser.js
+++ b/toolkit/devtools/output-parser.js
@@ -34,51 +34,29 @@ const REGEX_CSS_VAR = /^\bvar\(\s*--[-_a
  */
 const REGEX_ALL_COLORS = /^#[0-9a-fA-F]{3}\b|^#[0-9a-fA-F]{6}\b|^hsl\(.*?\)|^hsla\(.*?\)|^rgba?\(.*?\)|^[a-zA-Z-]+/;
 
 loader.lazyGetter(this, "DOMUtils", function () {
   return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
 });
 
 /**
- * This regular expression catches all css property names with their trailing
- * spaces and semicolon. This is used to ensure a value is valid for a property
- * name within style="" attributes.
- */
-loader.lazyGetter(this, "REGEX_ALL_CSS_PROPERTIES", function () {
-  let names = DOMUtils.getCSSPropertyNames();
-    let pattern = "^(";
-
-    for (let i = 0; i < names.length; i++) {
-      if (i > 0) {
-        pattern += "|";
-      }
-      pattern += names[i];
-    }
-    pattern += ")\\s*:\\s*";
-
-    return new RegExp(pattern);
-});
-
-/**
  * This module is used to process text for output by developer tools. This means
  * linking JS files with the debugger, CSS files with the style editor, JS
  * functions with the debugger, placing color swatches next to colors and
  * adding doorhanger previews where possible (images, angles, lengths,
  * border radius, cubic-bezier etc.).
  *
  * Usage:
  *   const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
  *   const {OutputParser} = devtools.require("devtools/output-parser");
  *
  *   let parser = new OutputParser();
  *
  *   parser.parseCssProperty("color", "red"); // Returns document fragment.
- *   parser.parseHTMLAttribute("color:red; font-size: 12px;"); // Returns document
- *                                                             // fragment.
  */
 function OutputParser() {
   this.parsed = [];
   this.colorSwatches = new WeakMap();
   this._onSwatchMouseDown = this._onSwatchMouseDown.bind(this);
 }
 
 exports.OutputParser = OutputParser;
@@ -95,50 +73,32 @@ OutputParser.prototype = {
    *         Options object. For valid options and default values see
    *         _mergeOptions().
    * @return {DocumentFragment}
    *         A document fragment containing color swatches etc.
    */
   parseCssProperty: function(name, value, options={}) {
     options = this._mergeOptions(options);
 
-    // XXX: This is a quick fix that should stay until bug 977063 gets fixed.
-    // It avoids parsing "linear" as a timing-function in "linear-gradient(...)"
-    options.expectCubicBezier = ["transition", "transition-timing-function",
-      "animation", "animation-timing-function"].indexOf(name) !== -1;
-
+    options.expectCubicBezier =
+      safeCssPropertySupportsType(name, DOMUtils.TYPE_TIMING_FUNCTION);
     options.expectFilter = name === "filter";
+    options.supportsColor =
+      safeCssPropertySupportsType(name, DOMUtils.TYPE_COLOR) ||
+      safeCssPropertySupportsType(name, DOMUtils.TYPE_GRADIENT);
 
     if (this._cssPropertySupportsValue(name, value)) {
       return this._parse(value, options);
     }
     this._appendTextNode(value);
 
     return this._toDOM();
   },
 
   /**
-   * Parse a string.
-   *
-   * @param  {String} value
-   *         Text to parse.
-   * @param  {Object} [options]
-   *         Options object. For valid options and default values see
-   *         _mergeOptions().
-   * @return {DocumentFragment}
-   *         A document fragment. Colors will not be parsed.
-   */
-  parseHTMLAttribute: function(value, options={}) {
-    options.isHTMLAttribute = true;
-    options = this._mergeOptions(options);
-
-    return this._parse(value, options);
-  },
-
-  /**
    * Matches the beginning of the provided string to a css background-image url
    * and return both the whole url(...) match and the url itself.
    * This isn't handled via a regular expression to make sure we can match urls
    * that contain parenthesis easily
    */
   _matchBackgroundUrl: function(text) {
     let startToken = "url(";
     if (text.indexOf(startToken) !== 0) {
@@ -235,30 +195,17 @@ OutputParser.prototype = {
           let match = matched[0];
           text = this._trimMatchFromStart(text, match);
 
           this._appendCubicBezier(match, options);
           continue;
         }
       }
 
-      matched = text.match(REGEX_ALL_CSS_PROPERTIES);
-      if (matched) {
-        let [match] = matched;
-
-        text = this._trimMatchFromStart(text, match);
-        this._appendTextNode(match);
-
-        if (options.isHTMLAttribute) {
-          [text] = this._appendColorOnMatch(text, options);
-        }
-        continue;
-      }
-
-      if (!options.isHTMLAttribute) {
+      if (options.supportsColor) {
         let dirty;
 
         [text, dirty] = this._appendColorOnMatch(text, options);
 
         if (dirty) {
           continue;
         }
       }
@@ -583,43 +530,54 @@ OutputParser.prototype = {
    *           - defaultColorType: true // Convert colors to the default type
    *                                    // selected in the options panel.
    *           - colorSwatchClass: ""   // The class to use for color swatches.
    *           - colorClass: ""         // The class to use for the color value
    *                                    // that follows the swatch.
    *           - bezierSwatchClass: ""  // The class to use for bezier swatches.
    *           - bezierClass: ""        // The class to use for the bezier value
    *                                    // that follows the swatch.
-   *           - isHTMLAttribute: false // This property indicates whether we
-   *                                    // are parsing an HTML attribute value.
-   *                                    // When the value is passed in from an
-   *                                    // HTML attribute we need to check that
-   *                                    // any CSS property values are supported
-   *                                    // by the property name before
-   *                                    // processing the property value.
+   *           - supportsColor: false   // Does the CSS property support colors?
    *           - urlClass: ""           // The class to be used for url() links.
    *           - baseURI: ""            // A string or nsIURI used to resolve
    *                                    // relative links.
    * @return {Object}
    *         Overridden options object
    */
   _mergeOptions: function(overrides) {
     let defaults = {
       defaultColorType: true,
       colorSwatchClass: "",
       colorClass: "",
       bezierSwatchClass: "",
       bezierClass: "",
-      isHTMLAttribute: false,
+      supportsColor: false,
       urlClass: "",
       baseURI: ""
     };
 
     if (typeof overrides.baseURI === "string") {
       overrides.baseURI = Services.io.newURI(overrides.baseURI, null, null);
     }
 
     for (let item in overrides) {
       defaults[item] = overrides[item];
     }
     return defaults;
   }
 };
+
+/**
+ * A wrapper for DOMUtils.cssPropertySupportsType that ignores invalid
+ * properties.
+ *
+ * @param {String} name The property name.
+ * @param {number} type The type tested for support.
+ * @return {Boolean} Whether the property supports the type.
+ *        If the property is unknown, false is returned.
+ */
+function safeCssPropertySupportsType(name, type) {
+  try {
+    return DOMUtils.cssPropertySupportsType(name, type);
+  } catch(e) {
+    return false;
+  }
+}