Bug 1253869 - Don't assume that rule in the Rules view match the selected node when checking for overidden properties. r=gl draft
authorNicolas Chevobbe <chevobbe.nicolas@gmail.com>
Wed, 25 May 2016 18:16:26 +0200
changeset 372509 c4f17361ed34e68c568f2f6e4be4fb412ca7f50f
parent 372507 199230f44725b8e4919a5014b9649a9951383355
child 522181 6da6b66277d1a439df8476d413559018e4168f6d
push id19527
push userchevobbe.nicolas@gmail.com
push dateSat, 28 May 2016 17:30:01 +0000
reviewersgl
bugs1253869
milestone49.0a1
Bug 1253869 - Don't assume that rule in the Rules view match the selected node when checking for overidden properties. r=gl Unmatched rules created via the "Add New Rule" button was wrongfully make other rules look not applied. Fix returned isMatching boolean in PageStyleActor's function when the evaluated rule has multiple selectors. Add CSS rules to make look unmatched rules' properties unmatched, like the rule's selector. Add tests to make sur we handle unmatch rule selectors right. MozReview-Commit-ID: FPQ7XJoa7Ba
devtools/client/inspector/rules/models/element-style.js
devtools/client/inspector/rules/test/browser.ini
devtools/client/inspector/rules/test/browser_rules_edit-selector_05.js
devtools/client/inspector/rules/test/browser_rules_edit-selector_09.js
devtools/client/themes/rules.css
devtools/server/actors/styles.js
--- a/devtools/client/inspector/rules/models/element-style.js
+++ b/devtools/client/inspector/rules/models/element-style.js
@@ -5,16 +5,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
 const {Cc, Ci, Cu} = require("chrome");
 const promise = require("promise");
 const {Rule} = require("devtools/client/inspector/rules/models/rule");
 const {promiseWarn} = require("devtools/client/inspector/shared/utils");
+const {ELEMENT_STYLE} = require("devtools/server/actors/styles");
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 loader.lazyGetter(this, "PSEUDO_ELEMENTS", () => {
   return domUtils.getCSSPseudoElementNames();
 });
 
 XPCOMUtils.defineLazyGetter(this, "domUtils", function () {
@@ -224,17 +225,19 @@ ElementStyle.prototype = {
   markOverridden: function (pseudo = "") {
     // Gather all the text properties applied by these rules, ordered
     // from more- to less-specific. Text properties from keyframes rule are
     // excluded from being marked as overridden since a number of criteria such
     // as time, and animation overlay are required to be check in order to
     // determine if the property is overridden.
     let textProps = [];
     for (let rule of this.rules) {
-      if (rule.pseudoElement === pseudo && !rule.keyframes) {
+      if ((rule.matchedSelectors.length > 0 ||
+           rule.domRule.type === ELEMENT_STYLE) &&
+          rule.pseudoElement === pseudo && !rule.keyframes) {
         for (let textProp of rule.textProps.slice(0).reverse()) {
           if (textProp.enabled) {
             textProps.push(textProp);
           }
         }
       }
     }
 
--- a/devtools/client/inspector/rules/test/browser.ini
+++ b/devtools/client/inspector/rules/test/browser.ini
@@ -121,16 +121,17 @@ skip-if = os == "mac" # Bug 1245996 : cl
 [browser_rules_edit-selector_01.js]
 [browser_rules_edit-selector_02.js]
 [browser_rules_edit-selector_03.js]
 [browser_rules_edit-selector_04.js]
 [browser_rules_edit-selector_05.js]
 [browser_rules_edit-selector_06.js]
 [browser_rules_edit-selector_07.js]
 [browser_rules_edit-selector_08.js]
+[browser_rules_edit-selector_09.js]
 [browser_rules_editable-field-focus_01.js]
 [browser_rules_editable-field-focus_02.js]
 [browser_rules_eyedropper.js]
 [browser_rules_filtereditor-appears-on-swatch-click.js]
 [browser_rules_filtereditor-commit-on-ENTER.js]
 [browser_rules_filtereditor-revert-on-ESC.js]
 skip-if = (os == "win" && debug) # bug 963492: win.
 [browser_rules_guessIndentation.js]
--- a/devtools/client/inspector/rules/test/browser_rules_edit-selector_05.js
+++ b/devtools/client/inspector/rules/test/browser_rules_edit-selector_05.js
@@ -69,10 +69,10 @@ function* checkModifiedElement(view, nam
   ok(getRuleViewRule(view, name), "Rule with " + name + " selector exists.");
 }
 
 function* testAddProperty(view) {
   info("Test creating a new property");
   let textProp = yield addProperty(view, 1, "text-align", "center");
 
   is(textProp.value, "center", "Text prop should have been changed.");
-  is(textProp.overridden, false, "Property should not be overridden");
+  ok(!textProp.overridden, "Property should not be overridden");
 }
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/rules/test/browser_rules_edit-selector_09.js
@@ -0,0 +1,110 @@
+/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Tests that editing a selector to an unmatched rule does set up the correct
+// property on the rule, and that settings property in said rule does not
+// lead to overriding properties from matched rules.
+// Test that having a rule with both matched and unmatched selectors does work
+// correctly.
+
+const TEST_URI = `
+  <style type="text/css">
+    #testid {
+      color: black;
+    }
+    .testclass {
+      background-color: white;
+    }
+  </style>
+  <div id="testid">Styled Node</div>
+  <span class="testclass">This is a span</span>
+`;
+
+add_task(function* () {
+  yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  let {inspector, view} = yield openRuleView();
+
+  yield selectNode("#testid", inspector);
+  yield testEditSelector(view, "span");
+  yield testAddImportantProperty(view);
+  yield testAddMatchedRule(view, "span, div");
+});
+
+function* testEditSelector(view, name) {
+  info("Test editing existing selector fields");
+
+  let ruleEditor = getRuleViewRuleEditor(view, 1);
+
+  info("Focusing an existing selector name in the rule-view");
+  let editor = yield focusEditableField(view, ruleEditor.selectorText);
+
+  is(inplaceEditor(ruleEditor.selectorText), editor,
+    "The selector editor got focused");
+
+  info("Entering a new selector name and committing");
+  editor.input.value = name;
+
+  info("Waiting for rule view to update");
+  let onRuleViewChanged = once(view, "ruleview-changed");
+
+  info("Entering the commit key");
+  EventUtils.synthesizeKey("VK_RETURN", {});
+  yield onRuleViewChanged;
+
+  ok(getRuleViewRule(view, name), "Rule with " + name + " selector exists.");
+  ok(getRuleViewRuleEditor(view, 1).element.getAttribute("unmatched"),
+    "Rule with " + name + " does not match the current element.");
+
+  // Escape the new property editor after editing the selector
+  let onBlur = once(view.styleDocument.activeElement, "blur");
+  EventUtils.synthesizeKey("VK_ESCAPE", {}, view.styleWindow);
+  yield onBlur;
+}
+
+function* testAddImportantProperty(view) {
+  info("Test creating a new property with !important");
+  let textProp = yield addProperty(view, 1, "color", "red !important");
+
+  is(textProp.value, "red", "Text prop should have been changed.");
+  is(textProp.priority, "important",
+    "Text prop has an \"important\" priority.");
+  ok(!textProp.overridden, "Property should not be overridden");
+
+  let ruleEditor = getRuleViewRuleEditor(view, 1);
+  let prop = ruleEditor.rule.textProps[0];
+  ok(!prop.overridden,
+    "Existing property on matched rule should not be overridden");
+}
+
+function* testAddMatchedRule(view, name) {
+  info("Test adding a matching selector");
+
+  let ruleEditor = getRuleViewRuleEditor(view, 1);
+
+  info("Focusing an existing selector name in the rule-view");
+  let editor = yield focusEditableField(view, ruleEditor.selectorText);
+
+  is(inplaceEditor(ruleEditor.selectorText), editor,
+    "The selector editor got focused");
+
+  info("Entering a new selector name and committing");
+  editor.input.value = name;
+
+  info("Waiting for rule view to update");
+  let onRuleViewChanged = once(view, "ruleview-changed");
+
+  info("Entering the commit key");
+  EventUtils.synthesizeKey("VK_RETURN", {});
+  yield onRuleViewChanged;
+
+  is(getRuleViewRuleEditor(view, 1).element.getAttribute("unmatched"), "false",
+    "Rule with " + name + " does match the current element.");
+
+  // Escape the new property editor after editing the selector
+  let onBlur = once(view.styleDocument.activeElement, "blur");
+  EventUtils.synthesizeKey("VK_ESCAPE", {}, view.styleWindow);
+  yield onBlur;
+}
--- a/devtools/client/themes/rules.css
+++ b/devtools/client/themes/rules.css
@@ -207,16 +207,20 @@
  * Display rules that don't match the current selected element and uneditable
  * user agent styles differently
  */
 .ruleview-rule[unmatched=true],
 .ruleview-rule[uneditable=true] {
   background: var(--theme-tab-toolbar-background);
 }
 
+.ruleview-rule[unmatched=true] {
+  opacity: 0.5;
+}
+
 .ruleview-rule[uneditable=true] :focus {
   outline: none;
 }
 
 .ruleview-rule[uneditable=true] .theme-link {
   color: var(--theme-highlight-bluegrey);
 }
 
@@ -443,17 +447,18 @@
   border-bottom-color: hsl(0,0%,50%);
 }
 
 .ruleview-selectorcontainer {
   word-wrap: break-word;
   cursor: text;
 }
 
-.ruleview-selector-separator, .ruleview-selector-unmatched {
+.ruleview-selector-separator,
+.ruleview-selector-unmatched {
   color: #888;
 }
 
 .ruleview-selector-matched > .ruleview-selector-attribute {
   /* TODO: Bug 1178535 Awaiting UX feedback on highlight colors */
 }
 
 .ruleview-selector-matched > .ruleview-selector-pseudo-class {
--- a/devtools/server/actors/styles.js
+++ b/devtools/server/actors/styles.js
@@ -1462,23 +1462,25 @@ var StyleRuleActor = protocol.ActorClass
 
     return selectorPromise.then((newCssRule) => {
       let ruleProps = null;
       let isMatching = false;
 
       if (newCssRule) {
         let ruleEntry = this.pageStyle.findEntryMatchingRule(node, newCssRule);
         if (ruleEntry.length === 1) {
-          isMatching = true;
           ruleProps =
             this.pageStyle.getAppliedProps(node, ruleEntry,
                                            { matchedSelectors: true });
         } else {
           ruleProps = this.pageStyle.getNewAppliedProps(node, newCssRule);
         }
+
+        isMatching = ruleProps.entries.some((ruleProp) =>
+          ruleProp.matchedSelectors.length > 0);
       }
 
       return { ruleProps, isMatching };
     });
   }
 });
 
 /**