Bug 1217328 - let filter editor work on invalid values. r=pbrosset
authorTom Tromey <tromey@mozilla.com>
Mon, 26 Oct 2015 06:22:00 +0100
changeset 304898 150b4bb5d07bb86735e914e1a2afcde3fcf33a12
parent 304897 396ed517522291ed0af520e2f117c98a04217502
child 304899 10533587f045f2f10e19e12d147312f5afbd3b6a
push id1001
push userraliiev@mozilla.com
push dateMon, 18 Jan 2016 19:06:03 +0000
treeherdermozilla-release@8b89261f3ac4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbrosset
bugs1217328
milestone44.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1217328 - let filter editor work on invalid values. r=pbrosset
devtools/client/shared/test/browser_filter-editor-01.js
devtools/client/shared/test/browser_outputparser.js
devtools/client/shared/widgets/FilterWidget.js
devtools/shared/output-parser.js
--- a/devtools/client/shared/test/browser_filter-editor-01.js
+++ b/devtools/client/shared/test/browser_filter-editor-01.js
@@ -28,9 +28,35 @@ add_task(function *() {
      "setCssValue should work for spaced values");
 
   info("Test parsing of string-typed values");
   widget.setCssValue("drop-shadow( 2px  1px 5px black) url( example.svg#filter )");
 
   is(widget.getCssValue(),
      "drop-shadow(2px  1px 5px black) url(example.svg#filter)",
      "setCssValue should work for string-typed values");
+
+  info("Test parsing of mixed-case function names");
+  widget.setCssValue("BLUR(2px) Contrast(200%) Drop-Shadow(2px 1px 5px Black)");
+  is(widget.getCssValue(),
+     "BLUR(2px) Contrast(200%) Drop-Shadow(2px 1px 5px Black)",
+     "setCssValue should work for mixed-case function names");
+
+  info("Test parsing of invalid filter value");
+  widget.setCssValue("totallyinvalid");
+  is(widget.getCssValue(), "none",
+     "setCssValue should turn completely invalid value to 'none'");
+
+  info("Test parsing of invalid function argument");
+  widget.setCssValue("blur('hello')");
+  is(widget.getCssValue(), "blur(0px)",
+     "setCssValue should replace invalid function argument with default");
+
+  info("Test parsing of invalid function argument #2");
+  widget.setCssValue("drop-shadow(whatever)");
+  is(widget.getCssValue(), "drop-shadow()",
+     "setCssValue should replace invalid drop-shadow argument with empty string");
+
+  info("Test parsing of mixed invalid argument");
+  widget.setCssValue("contrast(5%) whatever invert('xxx')");
+  is(widget.getCssValue(), "contrast(5%) invert(0%)",
+     "setCssValue should handle multiple errors");
 });
--- a/devtools/client/shared/test/browser_outputparser.js
+++ b/devtools/client/shared/test/browser_outputparser.js
@@ -17,16 +17,17 @@ add_task(function*() {
 function* performTest() {
   let [host, , doc] = yield createHost("bottom", "data:text/html," +
     "<h1>browser_outputParser.js</h1><div></div>");
 
   let parser = new OutputParser(doc);
   testParseCssProperty(doc, parser);
   testParseCssVar(doc, parser);
   testParseURL(doc, parser);
+  testParseFilter(doc, parser);
 
   host.destroy();
 }
 
 // Class name used in color swatch.
 var COLOR_TEST_CLASS = "test-class";
 
 // Create a new CSS color-parsing test.  |name| is the name of the CSS
@@ -244,8 +245,18 @@ function testParseURL(doc, parser) {
         "target=\"_blank\">something.jpg</a>" +
         expectedTrailer;
 
     is(target.innerHTML, expected, test.desc);
 
     target.innerHTML = "";
   }
 }
+
+function testParseFilter(doc, parser) {
+  let frag = parser.parseCssProperty("filter", "something invalid", {
+    filterSwatchClass: "test-filterswatch"
+  });
+
+  let swatchCount = frag.querySelectorAll(".test-filterswatch").length;
+  is(swatchCount, 1, "filter swatch was created");
+}
+
--- a/devtools/client/shared/widgets/FilterWidget.js
+++ b/devtools/client/shared/widgets/FilterWidget.js
@@ -5,27 +5,31 @@
 "use strict";
 
 /**
   * This is a CSS Filter Editor widget used
   * for Rule View's filter swatches
   */
 
 const EventEmitter = require("devtools/shared/event-emitter");
-const { Cu } = require("chrome");
+const { Cu, Cc, Ci } = require("chrome");
 const { ViewHelpers } =
       Cu.import("resource://devtools/client/shared/widgets/ViewHelpers.jsm",
                 {});
 const STRINGS_URI = "chrome://browser/locale/devtools/filterwidget.properties";
 const L10N = new ViewHelpers.L10N(STRINGS_URI);
 const {cssTokenizer} = require("devtools/client/shared/css-parsing-utils");
 
 loader.lazyGetter(this, "asyncStorage",
                   () => require("devtools/shared/async-storage"));
 
+loader.lazyGetter(this, "DOMUtils", () => {
+  return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
+});
+
 const DEFAULT_FILTER_TYPE = "length";
 const UNIT_MAPPING = {
   percentage: "%",
   length: "px",
   angle: "deg",
   string: ""
 };
 
@@ -386,29 +390,17 @@ CSSFilterEditorWidget.prototype = {
 
   _addButtonClick: function() {
     const select = this.filterSelect;
     if (!select.value) {
       return;
     }
 
     const key = select.value;
-    const def = this._definition(key);
-    // UNIT_MAPPING[string] is an empty string (falsy), so
-    // using || doesn't work here
-    const unitLabel = typeof UNIT_MAPPING[def.type] === "undefined" ?
-                             UNIT_MAPPING[DEFAULT_FILTER_TYPE] :
-                             UNIT_MAPPING[def.type];
-
-    // string-type filters have no default value but a placeholder instead
-    if (!unitLabel) {
-      this.add(key);
-    } else {
-      this.add(key, def.range[0] + unitLabel);
-    }
+    this.add(key, null);
 
     this.render();
   },
 
   _removeButtonClick: function(e) {
     const isRemoveButton = e.target.classList.contains("remove-button");
     if (!isRemoveButton) {
       return;
@@ -692,16 +684,17 @@ CSSFilterEditorWidget.prototype = {
     * returns definition of a filter as defined in filterList
     *
     * @param {String} name
     *        filter name (e.g. blur)
     * @return {Object}
     *        filter's definition
     */
   _definition: function(name) {
+    name = name.toLowerCase();
     return filterList.find(a => a.name === name);
   },
 
   /**
     * Parses the CSS value specified, updating widget's filters
     *
     * @param {String} cssValue
     *        css value to be parsed
@@ -715,39 +708,63 @@ CSSFilterEditorWidget.prototype = {
 
     if (cssValue === "none") {
       this.emit("updated", this.getCssValue());
       this.render();
       return;
     }
 
     for (let {name, value} of tokenizeFilterValue(cssValue)) {
+      // If the specified value is invalid, replace it with the
+      // default.
+      if (name !== "url") {
+        if (!DOMUtils.cssPropertyIsValid("filter", name + "(" + value + ")")) {
+          value = null;
+        }
+      }
+
       this.add(name, value);
     }
 
     this.emit("updated", this.getCssValue());
     this.render();
   },
 
   /**
     * Creates a new [name] filter record with value
     *
     * @param {String} name
     *        filter name (e.g. blur)
     * @param {String} value
     *        value of the filter (e.g. 30px, 20%)
+    *        If this is |null|, then a default value may be supplied.
     * @return {Number}
     *        The index of the new filter in the current list of filters
     */
-  add: function(name, value = "") {
+  add: function(name, value) {
     const def = this._definition(name);
     if (!def) {
       return false;
     }
 
+    if (value === null) {
+      // UNIT_MAPPING[string] is an empty string (falsy), so
+      // using || doesn't work here
+      const unitLabel = typeof UNIT_MAPPING[def.type] === "undefined" ?
+                               UNIT_MAPPING[DEFAULT_FILTER_TYPE] :
+                               UNIT_MAPPING[def.type];
+
+      // string-type filters have no default value but a placeholder instead
+      if (!unitLabel) {
+        value = "";
+      } else {
+        value = def.range[0] + unitLabel;
+      }
+    }
+
     let unit = def.type === "string"
                ? ""
                : (/[a-zA-Z%]+/.exec(value) || [])[0];
 
     if (def.type !== "string") {
       value = parseFloat(value);
 
       // You can omit percentage values' and use a value between 0..1
@@ -759,17 +776,17 @@ CSSFilterEditorWidget.prototype = {
       const [min, max] = def.range;
       if (value < min) {
         value = min;
       } else if (value > max) {
         value = max;
       }
     }
 
-    const index = this.filters.push({value, unit, name: def.name}) - 1;
+    const index = this.filters.push({value, unit, name}) - 1;
     this.emit("updated", this.getCssValue());
 
     return index;
   },
 
   /**
     * returns value + unit of the specified filter
     *
--- a/devtools/shared/output-parser.js
+++ b/devtools/shared/output-parser.js
@@ -72,17 +72,20 @@ OutputParser.prototype = {
 
     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)) {
+    // The filter property is special in that we want to show the
+    // swatch even if the value is invalid, because this way the user
+    // can easily use the editor to fix it.
+    if (options.expectFilter || this._cssPropertySupportsValue(name, value)) {
       return this._parse(value, options);
     }
     this._appendTextNode(value);
 
     return this._toDOM();
   },
 
   /**