Bug 1217328 - let filter editor work on invalid values. r=pbrosset

devtools/client/shared/test/browser_filter-editor-01.js

@@ -33,4 +33,30 @@ add_task(function *() {
   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");
 });

devtools/client/shared/test/browser_outputparser.js

@@ -22,6 +22,7 @@ function* performTest() {
   testParseCssProperty(doc, parser);
   testParseCssVar(doc, parser);
   testParseURL(doc, parser);
+  testParseFilter(doc, parser);
 
   host.destroy();
 }
@@ -249,3 +250,13 @@ function testParseURL(doc, parser) {
     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");
+}
+

devtools/client/shared/widgets/FilterWidget.js

@@ -10,7 +10,7 @@
   */
 
 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",
                 {});
@@ -21,6 +21,10 @@ 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: "%",
@@ -391,19 +395,7 @@ CSSFilterEditorWidget.prototype = {
     }
 
     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();
   },
@@ -697,6 +689,7 @@ CSSFilterEditorWidget.prototype = {
     *        filter's definition
     */
   _definition: function(name) {
+    name = name.toLowerCase();
     return filterList.find(a => a.name === name);
   },
 
@@ -720,6 +713,14 @@ CSSFilterEditorWidget.prototype = {
     }
 
     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);
     }
 
@@ -734,15 +735,31 @@ CSSFilterEditorWidget.prototype = {
     *        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];
@@ -764,7 +781,7 @@ CSSFilterEditorWidget.prototype = {
       }
     }
 
-    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;

devtools/shared/output-parser.js

@@ -77,7 +77,10 @@ OutputParser.prototype = {
       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);