Bug 1253869 - Don't assume that rule in the Rules view match the selected node when checking for overidden properties. r=gl
authorNicolas Chevobbe <chevobbe.nicolas@gmail.com>
Wed, 25 May 2016 18:16:26 +0200
changeset 338489 7275ea8867397514d89c360b7397b7dcabd0b4a2
parent 338488 26ff2bb48c6d02e123752862370a38639714fcff
child 338490 640f046e0e79d18aa0d4c0efac7e9f5d365f09a0
push id6249
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 13:59:36 +0000
treeherdermozilla-beta@bad9d4f5bf7e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgl
bugs1253869
milestone49.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 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 };
     });
   }
 });
 
 /**