Bug 1411664 - Check if property name is invalid in rule view and change warning icon title text accordingly. r=pbro
authorRazvan Caliman <rcaliman@mozilla.com>
Wed, 04 Apr 2018 15:26:44 +0200
changeset 411744 a867d0b8c58e
parent 411743 8b603bf3ea53
child 411745 c1aa3f6a5a23
push id33770
push usernerli@mozilla.com
push dateThu, 05 Apr 2018 10:01:08 +0000
treeherdermozilla-central@65e0fefdab51 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro
bugs1411664
milestone61.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 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.