Bug 1411664 - Check if property name is invalid in rule view and change warning icon title text accordingly. r=pbro draft
authorRazvan Caliman <rcaliman@mozilla.com>
Wed, 04 Apr 2018 15:26:44 +0200
changeset 777243 1419520f2e7ca7d41edc486e7a7f852867d253d4
parent 776018 c44f60c43432d468639b5fe078420e60c13fd3de
push id105127
push userbmo:rcaliman@mozilla.com
push dateWed, 04 Apr 2018 13:27:19 +0000
reviewerspbro
bugs1411664
milestone61.0a1
Bug 1411664 - Check if property name is invalid in rule view and change warning icon title text accordingly. r=pbro MozReview-Commit-ID: L64Ow4cLAg8
devtools/client/inspector/rules/models/text-property.js
devtools/client/inspector/rules/test/browser_rules_authored.js
devtools/client/inspector/rules/views/text-property-editor.js
devtools/server/actors/styles.js
devtools/shared/locales/en-US/styleinspector.properties
--- a/devtools/client/inspector/rules/models/text-property.js
+++ b/devtools/client/inspector/rules/models/text-property.js
@@ -182,34 +182,50 @@ TextProperty.prototype = {
   isKnownProperty: function() {
     return this.cssProperties.isKnown(this.name);
   },
 
   /**
    * Validate this property. Does it make sense for this value to be assigned
    * to this property name?
    *
-   * @return {Boolean} true if the property value is valid, false otherwise.
+   * @return {Boolean} true if the whole CSS declaration is valid, false otherwise.
    */
   isValid: function() {
-    // Starting with FF49, StyleRuleActors provide a list of parsed
-    // declarations, with data about their validity, but if we don't have this,
-    // compute validity locally (which might not be correct, but better than
-    // nothing).
-    if (!this.rule.domRule.declarations) {
-      return this.cssProperties.isValidOnClient(this.name, this.value, this.panelDoc);
-    }
-
     let selfIndex = this.rule.textProps.indexOf(this);
 
     // When adding a new property in the rule-view, the TextProperty object is
     // created right away before the rule gets updated on the server, so we're
     // not going to find the corresponding declaration object yet. Default to
     // true.
     if (!this.rule.domRule.declarations[selfIndex]) {
       return true;
     }
 
     return this.rule.domRule.declarations[selfIndex].isValid;
+  },
+
+  /**
+   * Validate the name of this property.
+   *
+   * @return {Boolean} true if the property name is valid, false otherwise.
+   */
+  isNameValid: function() {
+    let selfIndex = this.rule.textProps.indexOf(this);
+
+    // When adding a new property in the rule-view, the TextProperty object is
+    // created right away before the rule gets updated on the server, so we're
+    // not going to find the corresponding declaration object yet. Default to
+    // true.
+    if (!this.rule.domRule.declarations[selfIndex]) {
+      return true;
+    }
+
+    // Starting with FF61, StyleRuleActor provides an accessor to signal if the property
+    // name is valid. If we don't have this, assume the name is valid. In use, rely on
+    // isValid() as a guard against false positives.
+    return (this.rule.domRule.declarations[selfIndex].isNameValid !== undefined)
+      ? this.rule.domRule.declarations[selfIndex].isNameValid
+      : true;
   }
 };
 
 module.exports = TextProperty;
--- a/devtools/client/inspector/rules/test/browser_rules_authored.js
+++ b/devtools/client/inspector/rules/test/browser_rules_authored.js
@@ -27,23 +27,28 @@ add_task(async function() {
                                      // Override.
                                      "  background-color: blue;" +
                                      "  background-color: #f06;" +
                                      "} ");
 
   let elementStyle = view._elementStyle;
 
   let expected = [
-    {name: "something", overridden: true},
-    {name: "color", overridden: true},
-    {name: "background-color", overridden: true},
-    {name: "background-color", overridden: false}
+    {name: "something", overridden: true, isNameValid: false, isValid: false},
+    {name: "color", overridden: true, isNameValid: true, isValid: false},
+    {name: "background-color", overridden: true, isNameValid: true, isValid: true},
+    {name: "background-color", overridden: false, isNameValid: true, isValid: true}
   ];
 
   let rule = elementStyle.rules[1];
 
   for (let i = 0; i < expected.length; ++i) {
     let prop = rule.textProps[i];
-    is(prop.name, expected[i].name, "test name for prop " + i);
+    is(prop.name, expected[i].name,
+      "Check name for prop " + i);
     is(prop.overridden, expected[i].overridden,
-       "test overridden for prop " + i);
+      "Check overridden for prop " + i);
+    is(prop.isNameValid(), expected[i].isNameValid,
+      "Check if property name is valid for prop " + i);
+    is(prop.isValid(), expected[i].isValid,
+      "Check if whole declaration is valid for prop " + i);
   }
 });
--- a/devtools/client/inspector/rules/views/text-property-editor.js
+++ b/devtools/client/inspector/rules/views/text-property-editor.js
@@ -567,16 +567,20 @@ TextPropertyEditor.prototype = {
     if (this.prop.enabled) {
       this.enable.style.removeProperty("visibility");
       this.enable.setAttribute("checked", "");
     } else {
       this.enable.style.visibility = "visible";
       this.enable.removeAttribute("checked");
     }
 
+    this.warning.title = !this.isNameValid()
+      ? l10n("rule.warningName.title")
+      : l10n("rule.warning.title");
+
     this.warning.hidden = this.editing || this.isValid();
     this.filterProperty.hidden = this.editing ||
                                  !this.isValid() ||
                                  !this.prop.overridden ||
                                  this.ruleEditor.rule.isUnmatched;
 
     let showExpander = this.prop.computed.some(c => c.name !== this.prop.name);
     this.expander.style.display = showExpander ? "inline-block" : "none";
@@ -1003,23 +1007,31 @@ TextPropertyEditor.prototype = {
     this.ruleEditor.rule.previewPropertyValue(this.prop, val.value,
                                               val.priority);
   },
 
   /**
    * Validate this property. Does it make sense for this value to be assigned
    * to this property name? This does not apply the property value
    *
-   * @return {Boolean} true if the property value is valid, false otherwise.
+   * @return {Boolean} true if the property name + value pair is valid, false otherwise.
    */
   isValid: function() {
     return this.prop.isValid();
   },
 
   /**
+   * Validate the name of this property.
+   * @return {Boolean} true if the property name is valid, false otherwise.
+   */
+  isNameValid: function() {
+    return this.prop.isNameValid();
+  },
+
+  /**
    * Returns true if the property is a `display: [inline-]flex` declaration.
    *
    * @return {Boolean} true if the property is a `display: [inline-]flex` declaration.
    */
   isDisplayFlex: function() {
     return this.prop.name === "display" &&
       (this.prop.value === "flex" || this.prop.value === "inline-flex");
   },
--- a/devtools/server/actors/styles.js
+++ b/devtools/server/actors/styles.js
@@ -1124,16 +1124,18 @@ var StyleRuleActor = protocol.ActorClass
 
       // We need to grab CSS from the window, since calling supports() on the
       // one from the current global will fail due to not being an HTML global.
       let CSS = this.pageStyle.inspector.tabActor.window.CSS;
       form.declarations = declarations.map(decl => {
         // Use the 1-arg CSS.supports() call so that we also accept !important
         // in the value.
         decl.isValid = CSS.supports(`${decl.name}:${decl.value}`);
+        // Check property name. All valid CSS properties support "initial" as a value.
+        decl.isNameValid = CSS.supports(decl.name, "initial");
         return decl;
       });
     }
 
     return form;
   },
 
   /**
--- a/devtools/shared/locales/en-US/styleinspector.properties
+++ b/devtools/shared/locales/en-US/styleinspector.properties
@@ -53,16 +53,21 @@ rule.pseudoElement=Pseudo-elements
 # pseudo elements are present in the rule view.
 rule.selectedElement=This Element
 
 # LOCALIZATION NOTE (rule.warning.title): When an invalid property value is
 # entered into the rule view a warning icon is displayed. This text is used for
 # the title attribute of the warning icon.
 rule.warning.title=Invalid property value
 
+# LOCALIZATION NOTE (rule.warningName.title): When an invalid property name is
+# entered into the rule view a warning icon is displayed. This text is used for
+# the title attribute of the warning icon.
+rule.warningName.title=Invalid property name
+
 # LOCALIZATION NOTE (rule.filterProperty.title): Text displayed in the tooltip
 # of the search button that is shown next to a property that has been overridden
 # in the rule view.
 rule.filterProperty.title=Filter rules containing this property
 
 # LOCALIZATION NOTE (rule.empty): Text displayed when the highlighter is
 # first opened and there's no node selected in the rule view.
 rule.empty=No element selected.