Bug 1293616 - insert new rule in correct position in rule list; r?pbro draft
authorTom Tromey <tom@tromey.com>
Fri, 02 Sep 2016 10:42:00 -0600
changeset 424483 b1c8dd881074b5cd54d717dc0694f1b99a12b325
parent 424482 7e1ba3ec88a1709048caae08035f48fe4c8a3720
child 533687 9cff3fb5bc1d1c4dfbd5aa4f107051e571fd531a
push id32168
push userbmo:ttromey@mozilla.com
push dateWed, 12 Oct 2016 20:52:08 +0000
reviewerspbro
bugs1293616
milestone52.0a1
Bug 1293616 - insert new rule in correct position in rule list; r?pbro MozReview-Commit-ID: C4ib7ooI8BX
devtools/client/inspector/rules/test/browser.ini
devtools/client/inspector/rules/test/browser_rules_edit-selector_10.js
devtools/client/inspector/rules/test/browser_rules_edit-selector_11.js
devtools/client/inspector/rules/views/rule-editor.js
--- a/devtools/client/inspector/rules/test/browser.ini
+++ b/devtools/client/inspector/rules/test/browser.ini
@@ -123,16 +123,18 @@ skip-if = os == "mac" # Bug 1245996 : cl
 [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_edit-selector_10.js]
+[browser_rules_edit-selector_11.js]
 [browser_rules_edit-value-after-name_01.js]
 [browser_rules_edit-value-after-name_02.js]
 [browser_rules_edit-value-after-name_03.js]
 [browser_rules_edit-value-after-name_04.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]
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/rules/test/browser_rules_edit-selector_10.js
@@ -0,0 +1,64 @@
+/* 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";
+
+// Regression test for bug 1293616: make sure that editing a selector
+// keeps the rule in the proper position.
+
+const TEST_URI = `
+  <style type="text/css">
+    #testid span, #testid p {
+      background: aqua;
+    }
+    span {
+      background: fuchsia;
+    }
+  </style>
+  <div id="testid">
+    <span class="pickme">
+      Styled Node
+    </span>
+  </div>
+`;
+
+add_task(function* () {
+  yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  let {inspector, view} = yield openRuleView();
+  yield selectNode(".pickme", inspector);
+  yield testEditSelector(view);
+});
+
+function* testEditSelector(view) {
+  let ruleEditor = getRuleViewRuleEditor(view, 1);
+  let editor = yield focusEditableField(view, ruleEditor.selectorText);
+
+  editor.input.value = "#testid span";
+  let onRuleViewChanged = once(view, "ruleview-changed");
+  EventUtils.synthesizeKey("VK_RETURN", {});
+  yield onRuleViewChanged;
+
+  // Escape the new property editor after editing the selector
+  let onBlur = once(view.styleDocument.activeElement, "blur");
+  EventUtils.synthesizeKey("VK_ESCAPE", {}, view.styleWindow);
+  yield onBlur;
+
+  // Get the new rule editor that replaced the original
+  ruleEditor = getRuleViewRuleEditor(view, 1);
+
+  info("Check that the correct rules are visible");
+  is(view._elementStyle.rules.length, 3, "Should have 3 rules.");
+  is(ruleEditor.element.getAttribute("unmatched"), "false", "Rule editor is matched.");
+
+  let props = ruleEditor.rule.textProps;
+  is(props.length, 1, "Rule has correct number of properties");
+  is(props[0].name, "background", "Found background property");
+  ok(!props[0].overridden, "Background property is not overridden");
+
+  ruleEditor = getRuleViewRuleEditor(view, 2);
+  props = ruleEditor.rule.textProps;
+  is(props.length, 1, "Rule has correct number of properties");
+  is(props[0].name, "background", "Found background property");
+  ok(props[0].overridden, "Background property is overridden");
+}
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/rules/test/browser_rules_edit-selector_11.js
@@ -0,0 +1,69 @@
+/* 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";
+
+// Regression test for bug 1293616, where editing a selector should
+// change the relative priority of the rule.
+
+const TEST_URI = `
+  <style type="text/css">
+    #testid {
+      background: aqua;
+    }
+    .pickme {
+      background: seagreen;
+    }
+    span {
+      background: fuchsia;
+    }
+  </style>
+  <div>
+    <span id="testid" class="pickme">
+      Styled Node
+    </span>
+  </div>
+`;
+
+add_task(function* () {
+  yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  let {inspector, view} = yield openRuleView();
+  yield selectNode(".pickme", inspector);
+  yield testEditSelector(view);
+});
+
+function* testEditSelector(view) {
+  let ruleEditor = getRuleViewRuleEditor(view, 1);
+  let editor = yield focusEditableField(view, ruleEditor.selectorText);
+
+  editor.input.value = ".pickme";
+  let onRuleViewChanged = once(view, "ruleview-changed");
+  EventUtils.synthesizeKey("VK_RETURN", {});
+  yield onRuleViewChanged;
+
+  // Escape the new property editor after editing the selector
+  let onBlur = once(view.styleDocument.activeElement, "blur");
+  EventUtils.synthesizeKey("VK_ESCAPE", {}, view.styleWindow);
+  yield onBlur;
+
+  // Get the new rule editor that replaced the original
+  ruleEditor = getRuleViewRuleEditor(view, 1);
+
+  info("Check that the correct rules are visible");
+  is(view._elementStyle.rules.length, 4, "Should have 4 rules.");
+  is(ruleEditor.element.getAttribute("unmatched"), "false", "Rule editor is matched.");
+
+  let props = ruleEditor.rule.textProps;
+  is(props.length, 1, "Rule has correct number of properties");
+  is(props[0].name, "background", "Found background property");
+  is(props[0].value, "aqua", "Background property is aqua");
+  ok(props[0].overridden, "Background property is overridden");
+
+  ruleEditor = getRuleViewRuleEditor(view, 2);
+  props = ruleEditor.rule.textProps;
+  is(props.length, 1, "Rule has correct number of properties");
+  is(props[0].name, "background", "Found background property");
+  is(props[0].value, "seagreen", "Background property is seagreen");
+  ok(!props[0].overridden, "Background property is not overridden");
+}
--- a/devtools/client/inspector/rules/views/rule-editor.js
+++ b/devtools/client/inspector/rules/views/rule-editor.js
@@ -22,16 +22,17 @@ const {
   parsePseudoClassesAndAttributes,
   SELECTOR_ATTRIBUTE,
   SELECTOR_ELEMENT,
   SELECTOR_PSEUDO_CLASS
 } = require("devtools/shared/css/parsing-utils");
 const promise = require("promise");
 const Services = require("Services");
 const EventEmitter = require("devtools/shared/event-emitter");
+const {Task} = require("devtools/shared/task");
 
 const STYLE_INSPECTOR_PROPERTIES = "devtools-shared/locale/styleinspector.properties";
 const {LocalizationHelper} = require("devtools/shared/l10n");
 const STYLE_INSPECTOR_L10N = new LocalizationHelper(STYLE_INSPECTOR_PROPERTIES);
 
 const HTML_NS = "http://www.w3.org/1999/xhtml";
 const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
 
@@ -512,72 +513,98 @@ RuleEditor.prototype = {
    *
    * @param {String} value
    *        The value contained in the editor.
    * @param {Boolean} commit
    *        True if the change should be applied.
    * @param {Number} direction
    *        The move focus direction number.
    */
-  _onSelectorDone: function (value, commit, direction) {
+  _onSelectorDone: Task.async(function* (value, commit, direction) {
     if (!commit || this.isEditing || value === "" ||
         value === this.rule.selectorText) {
       return;
     }
 
     let ruleView = this.ruleView;
     let elementStyle = ruleView._elementStyle;
     let element = elementStyle.element;
     let supportsUnmatchedRules =
       this.rule.domRule.supportsModifySelectorUnmatched;
 
     this.isEditing = true;
 
-    this.rule.domRule.modifySelector(element, value).then(response => {
-      this.isEditing = false;
+    try {
+      let response = yield this.rule.domRule.modifySelector(element, value);
 
       if (!supportsUnmatchedRules) {
+        this.isEditing = false;
+
         if (response) {
           this.ruleView.refreshPanel();
         }
         return;
       }
 
+      // We recompute the list of applied styles, because editing a
+      // selector might cause this rule's position to change.
+      let applied = yield elementStyle.pageStyle.getApplied(element, {
+        inherited: true,
+        matchedSelectors: true,
+        filter: elementStyle.showUserAgentStyles ? "ua" : undefined
+      });
+
+      this.isEditing = false;
+
       let {ruleProps, isMatching} = response;
       if (!ruleProps) {
         // Notify for changes, even when nothing changes,
         // just to allow tests being able to track end of this request.
         ruleView.emit("ruleview-invalid-selector");
         return;
       }
 
       ruleProps.isUnmatched = !isMatching;
       let newRule = new Rule(elementStyle, ruleProps);
       let editor = new RuleEditor(ruleView, newRule);
       let rules = elementStyle.rules;
 
-      rules.splice(rules.indexOf(this.rule), 1);
-      rules.push(newRule);
+      let newRuleIndex = applied.findIndex((r) => r.rule == ruleProps.rule);
+      let oldIndex = rules.indexOf(this.rule);
+
+      // If the selector no longer matches, then we leave the rule in
+      // the same relative position.
+      if (newRuleIndex === -1) {
+        newRuleIndex = oldIndex;
+      }
+
+      // Remove the old rule and insert the new rule.
+      rules.splice(oldIndex, 1);
+      rules.splice(newRuleIndex, 0, newRule);
       elementStyle._changed();
       elementStyle.markOverriddenAll();
 
+      // We install the new editor in place of the old -- you might
+      // think we would replicate the list-modification logic above,
+      // but that is complicated due to the way the UI installs
+      // pseudo-element rules and the like.
       this.element.parentNode.replaceChild(editor.element, this.element);
 
       // Remove highlight for modified selector
       if (ruleView.highlightedSelector) {
         ruleView.toggleSelectorHighlighter(ruleView.lastSelectorIcon,
           ruleView.highlightedSelector);
       }
 
       editor._moveSelectorFocus(direction);
-    }).then(null, err => {
+    } catch (err) {
       this.isEditing = false;
       promiseWarn(err);
-    });
-  },
+    }
+  }),
 
   /**
    * Handle moving the focus change after a tab or return keypress in the
    * selector inplace editor.
    *
    * @param {Number} direction
    *        The move focus direction number.
    */