Bug 1190047 - [Rule View] Ensure changes to swatches are reverted to the original value on escape r=bgrins

browser/devtools/shared/widgets/Tooltip.js

@@ -1025,6 +1025,7 @@ SwatchBasedEditorTooltip.prototype = {
    * @param {object} callbacks
    *        Callbacks that will be executed when the editor wants to preview a
    *        value change, or revert a change, or commit a change.
+   *        - onShow: will be called when one of the swatch tooltip is shown
    *        - onPreview: will be called when one of the sub-classes calls
    *        preview
    *        - onRevert: will be called when the user ESCapes out of the tooltip
@@ -1032,6 +1033,9 @@ SwatchBasedEditorTooltip.prototype = {
    *        outside the tooltip.
    */
   addSwatch: function(swatchEl, callbacks={}) {
+    if (!callbacks.onShow) {
+      callbacks.onShow = function() {};
+    }
     if (!callbacks.onPreview) {
       callbacks.onPreview = function() {};
     }
@@ -1069,6 +1073,7 @@ SwatchBasedEditorTooltip.prototype = {
     if (swatch) {
       this.activeSwatch = event.target;
       this.show();
+      swatch.callbacks.onShow();
       event.stopPropagation();
     }
   },

browser/devtools/styleinspector/rule-view.js

@@ -2849,6 +2849,9 @@ function TextPropertyEditor(aRuleEditor, aProperty) {
   this._onStartEditing = this._onStartEditing.bind(this);
   this._onNameDone = this._onNameDone.bind(this);
   this._onValueDone = this._onValueDone.bind(this);
+  this._onSwatchCommit = this._onSwatchCommit.bind(this);
+  this._onSwatchPreview = this._onSwatchPreview.bind(this);
+  this._onSwatchRevert = this._onSwatchRevert.bind(this);
   this._onValidate = throttle(this._previewValue, 10, this);
   this.update = this.update.bind(this);
 
@@ -3075,7 +3078,7 @@ TextPropertyEditor.prototype = {
 
     this.warning.hidden = this.editing || this.isValid();
 
-    if ((this.prop.overridden || !this.prop.enabled) && !this.editing) {
+    if (this.prop.overridden || !this.prop.enabled) {
       this.element.classList.add("ruleview-overridden");
     } else {
       this.element.classList.remove("ruleview-overridden");
@@ -3130,9 +3133,10 @@ TextPropertyEditor.prototype = {
         // Adding this swatch to the list of swatches our colorpicker
         // knows about
         this.ruleView.tooltips.colorPicker.addSwatch(span, {
-          onPreview: () => this._previewValue(this.valueSpan.textContent),
-          onCommit: () => this._onValueDone(this.valueSpan.textContent, true),
-          onRevert: () => this._onValueDone(undefined, false)
+          onShow: this._onStartEditing,
+          onPreview: this._onSwatchPreview,
+          onCommit: this._onSwatchCommit,
+          onRevert: this._onSwatchRevert
         });
       }
     }
@@ -3145,9 +3149,10 @@ TextPropertyEditor.prototype = {
         // Adding this swatch to the list of swatches our colorpicker
         // knows about
         this.ruleView.tooltips.cubicBezier.addSwatch(span, {
-          onPreview: () => this._previewValue(this.valueSpan.textContent),
-          onCommit: () => this._onValueDone(this.valueSpan.textContent, true),
-          onRevert: () => this._onValueDone(undefined, false)
+          onShow: this._onStartEditing,
+          onPreview: this._onSwatchPreview,
+          onCommit: this._onSwatchCommit,
+          onRevert: this._onSwatchRevert
         });
       }
     }
@@ -3159,9 +3164,10 @@ TextPropertyEditor.prototype = {
         parserOptions.filterSwatch = true;
 
         this.ruleView.tooltips.filterEditor.addSwatch(span, {
-          onPreview: () => this._previewValue(this.valueSpan.textContent),
-          onCommit: () => this._onValueDone(this.valueSpan.textContent, true),
-          onRevert: () => this._onValueDone(undefined, false)
+          onShow: this._onStartEditing,
+          onPreview: this._onSwatchPreview,
+          onCommit: this._onSwatchCommit,
+          onRevert: this._onSwatchRevert
         }, outputParser, parserOptions);
       }
     }
@@ -3422,6 +3428,30 @@ TextPropertyEditor.prototype = {
     }
   },
 
+  /**
+   * Called when the swatch editor wants to commit a value change.
+   */
+  _onSwatchCommit: function() {
+    this._onValueDone(this.valueSpan.textContent, true);
+    this.update();
+  },
+
+  /**
+   * Called when the swatch editor wants to preview a value change.
+   */
+  _onSwatchPreview: function() {
+    this._previewValue(this.valueSpan.textContent);
+  },
+
+  /**
+   * Called when the swatch editor closes from an ESC. Revert to the original
+   * value of this property before editing.
+   */
+  _onSwatchRevert: function() {
+    this.rule.setPropertyEnabled(this.prop, this.prop.enabled);
+    this.update();
+  },
+
   /**
    * Parse a value string and break it into pieces, starting with the
    * first value, and into an array of additional properties (if any).

browser/devtools/styleinspector/test/browser_ruleview_colorpicker-revert-on-ESC.js

@@ -6,33 +6,32 @@
 
 // Test that a color change in the color picker is reverted when ESC is pressed
 
-const PAGE_CONTENT = [
-  '<style type="text/css">',
-  '  body {',
-  '    background-color: #ededed;',
-  '  }',
-  '</style>',
-  'Testing the color picker tooltip!'
+let TEST_URI = [
+  "<style type='text/css'>",
+  "  body {",
+  "    background-color: #EDEDED;",
+  "  }",
+  "</style>",
 ].join("\n");
 
 add_task(function*() {
-  yield addTab("data:text/html;charset=utf-8,rule view color picker tooltip test");
-  content.document.body.innerHTML = PAGE_CONTENT;
-  let {toolbox, inspector, view} = yield openRuleView();
-
-  let swatch = getRuleViewProperty(view, "body", "background-color").valueSpan
-    .querySelector(".ruleview-colorswatch");
-  yield testPressingEscapeRevertsChanges(swatch, view);
+  yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  let {view} = yield openRuleView();
+  yield testPressingEscapeRevertsChanges(view);
+  yield testPressingEscapeRevertsChangesAndDisables(view);
 });
 
-function* testPressingEscapeRevertsChanges(swatch, ruleView) {
-  let cPicker = ruleView.tooltips.colorPicker;
+function* testPressingEscapeRevertsChanges(view) {
+  let ruleEditor = getRuleViewRuleEditor(view, 1);
+  let propEditor = ruleEditor.rule.textProps[0].editor;
+  let swatch = propEditor.valueSpan.querySelector(".ruleview-colorswatch");
+  let cPicker = view.tooltips.colorPicker;
 
   let onShown = cPicker.tooltip.once("shown");
   swatch.click();
   yield onShown;
 
-  yield simulateColorPickerChange(ruleView, cPicker, [0, 0, 0, 1], {
+  yield simulateColorPickerChange(view, cPicker, [0, 0, 0, 1], {
     element: content.document.body,
     name: "backgroundColor",
     value: "rgb(0, 0, 0)"
@@ -40,17 +39,83 @@ function* testPressingEscapeRevertsChanges(swatch, ruleView) {
 
   is(swatch.style.backgroundColor, "rgb(0, 0, 0)",
     "The color swatch's background was updated");
-  is(getRuleViewProperty(ruleView, "body", "background-color").valueSpan.textContent,
-    "#000", "The text of the background-color css property was updated");
+  is(propEditor.valueSpan.textContent, "#000",
+    "The text of the background-color css property was updated");
 
   let spectrum = yield cPicker.spectrum;
 
-  // ESC out of the color picker
+  info("Pressing ESCAPE to close the tooltip");
   let onHidden = cPicker.tooltip.once("hidden");
   EventUtils.sendKey("ESCAPE", spectrum.element.ownerDocument.defaultView);
   yield onHidden;
+  yield ruleEditor.rule._applyingModifications;
 
-  yield waitForSuccess(() => {
-    return content.getComputedStyle(content.document.body).backgroundColor === "rgb(237, 237, 237)";
-  }, "The element's background-color was reverted");
+  yield waitForComputedStyleProperty("body", null, "background-color",
+    "rgb(237, 237, 237)");
+  is(propEditor.valueSpan.textContent, "#EDEDED",
+    "Got expected property value.");
+}
+
+function* testPressingEscapeRevertsChangesAndDisables(view) {
+  let ruleEditor = getRuleViewRuleEditor(view, 1);
+  let propEditor = ruleEditor.rule.textProps[0].editor;
+  let swatch = propEditor.valueSpan.querySelector(".ruleview-colorswatch");
+  let cPicker = view.tooltips.colorPicker;
+
+  info("Disabling background-color property");
+  propEditor.enable.click();
+  yield ruleEditor.rule._applyingModifications;
+
+  ok(propEditor.element.classList.contains("ruleview-overridden"),
+    "property is overridden.");
+  is(propEditor.enable.style.visibility, "visible",
+    "property enable checkbox is visible.");
+  ok(!propEditor.enable.getAttribute("checked"),
+    "property enable checkbox is not checked.");
+  ok(!propEditor.prop.enabled,
+    "background-color property is disabled.");
+  let newValue = yield getRulePropertyValue("background-color");
+  is(newValue, "", "background-color should have been unset.");
+
+  let onShown = cPicker.tooltip.once("shown");
+  swatch.click();
+  yield onShown;
+
+  ok(!propEditor.element.classList.contains("ruleview-overridden"),
+    "property overridden is not displayed.");
+  is(propEditor.enable.style.visibility, "hidden",
+    "property enable checkbox is hidden.");
+
+  let spectrum = yield cPicker.spectrum;
+  info("Simulating a color picker change in the widget");
+  spectrum.rgb = [0, 0, 0, 1];
+  yield ruleEditor.rule._applyingModifications;
+
+  info("Pressing ESCAPE to close the tooltip");
+  let onHidden = cPicker.tooltip.once("hidden");
+  EventUtils.sendKey("ESCAPE", spectrum.element.ownerDocument.defaultView);
+  yield onHidden;
+  yield ruleEditor.rule._applyingModifications;
+
+  ok(propEditor.element.classList.contains("ruleview-overridden"),
+    "property is overridden.");
+  is(propEditor.enable.style.visibility, "visible",
+    "property enable checkbox is visible.");
+  ok(!propEditor.enable.getAttribute("checked"),
+    "property enable checkbox is not checked.");
+  ok(!propEditor.prop.enabled,
+    "background-color property is disabled.");
+  newValue = yield getRulePropertyValue("background-color");
+  is(newValue, "", "background-color should have been unset.");
+  is(propEditor.valueSpan.textContent, "#EDEDED",
+    "Got expected property value.");
+}
+
+function* getRulePropertyValue(name) {
+  let propValue = yield executeInContent("Test:GetRulePropertyValue", {
+    styleSheetIndex: 0,
+    ruleIndex: 0,
+    name: name
+  });
+  return propValue;
 }

browser/devtools/styleinspector/test/browser_ruleview_cubicbezier-revert-on-ESC.js

@@ -4,30 +4,29 @@
 
 "use strict";
 
-// Test that changes made to the cubic-bezier timing-function in the cubic-bezier
-// tooltip are reverted when ESC is pressed
+// Test that changes made to the cubic-bezier timing-function in the
+// cubic-bezier tooltip are reverted when ESC is pressed
 
-const PAGE_CONTENT = [
-  '<style type="text/css">',
-  '  body {',
-  '    animation-timing-function: linear;',
-  '  }',
-  '</style>',
+let TEST_URI = [
+  "<style type='text/css'>",
+  "  body {",
+  "    animation-timing-function: linear;",
+  "  }",
+  "</style>",
 ].join("\n");
 
 add_task(function*() {
-  yield addTab("data:text/html;charset=utf-8,rule view cubic-bezier tooltip test");
-  content.document.body.innerHTML = PAGE_CONTENT;
-  let {toolbox, inspector, view} = yield openRuleView();
-
-  info("Getting the bezier swatch element");
-  let swatch = getRuleViewProperty(view, "body", "animation-timing-function").valueSpan
-    .querySelector(".ruleview-bezierswatch");
-  yield testPressingEscapeRevertsChanges(swatch, view);
+  yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  let {view} = yield openRuleView();
+  yield testPressingEscapeRevertsChanges(view);
+  yield testPressingEscapeRevertsChangesAndDisables(view);
 });
 
-function* testPressingEscapeRevertsChanges(swatch, ruleView) {
-  let bezierTooltip = ruleView.tooltips.cubicBezier;
+function* testPressingEscapeRevertsChanges(view) {
+  let ruleEditor = getRuleViewRuleEditor(view, 1);
+  let propEditor = ruleEditor.rule.textProps[0].editor;
+  let swatch = propEditor.valueSpan.querySelector(".ruleview-bezierswatch");
+  let bezierTooltip = view.tooltips.cubicBezier;
 
   let onShown = bezierTooltip.tooltip.once("shown");
   swatch.click();
@@ -36,18 +35,85 @@ function* testPressingEscapeRevertsChanges(swatch, ruleView) {
   let widget = yield bezierTooltip.widget;
   info("Simulating a change of curve in the widget");
   widget.coordinates = [0.1, 2, 0.9, -1];
-  let expected = "cubic-bezier(0.1, 2, 0.9, -1)";
+  yield ruleEditor.rule._applyingModifications;
 
-  yield waitForSuccess(() => {
-    return content.getComputedStyle(content.document.body).animationTimingFunction === expected;
-  }, "Waiting for the change to be previewed on the element");
+  yield waitForComputedStyleProperty("body", null, "animation-timing-function",
+    "cubic-bezier(0.1, 2, 0.9, -1)");
+  is(propEditor.valueSpan.textContent, "cubic-bezier(.1,2,.9,-1)",
+    "Got expected property value.");
 
   info("Pressing ESCAPE to close the tooltip");
   let onHidden = bezierTooltip.tooltip.once("hidden");
   EventUtils.sendKey("ESCAPE", widget.parent.ownerDocument.defaultView);
   yield onHidden;
+  yield ruleEditor.rule._applyingModifications;
 
-  yield waitForSuccess(() => {
-    return content.getComputedStyle(content.document.body).animationTimingFunction === "cubic-bezier(0, 0, 1, 1)";
-  }, "Waiting for the change to be reverted on the element");
+  yield waitForComputedStyleProperty("body", null, "animation-timing-function",
+    "cubic-bezier(0, 0, 1, 1)");
+  is(propEditor.valueSpan.textContent, "linear",
+    "Got expected property value.");
+}
+
+function* testPressingEscapeRevertsChangesAndDisables(view) {
+  let ruleEditor = getRuleViewRuleEditor(view, 1);
+  let propEditor = ruleEditor.rule.textProps[0].editor;
+  let swatch = propEditor.valueSpan.querySelector(".ruleview-bezierswatch");
+  let bezierTooltip = view.tooltips.cubicBezier;
+
+  info("Disabling animation-timing-function property");
+  propEditor.enable.click();
+  yield ruleEditor.rule._applyingModifications;
+
+  ok(propEditor.element.classList.contains("ruleview-overridden"),
+    "property is overridden.");
+  is(propEditor.enable.style.visibility, "visible",
+    "property enable checkbox is visible.");
+  ok(!propEditor.enable.getAttribute("checked"),
+    "property enable checkbox is not checked.");
+  ok(!propEditor.prop.enabled,
+    "animation-timing-function property is disabled.");
+  let newValue = yield getRulePropertyValue("animation-timing-function");
+  is(newValue, "", "animation-timing-function should have been unset.");
+
+  let onShown = bezierTooltip.tooltip.once("shown");
+  swatch.click();
+  yield onShown;
+
+  ok(!propEditor.element.classList.contains("ruleview-overridden"),
+    "property overridden is not displayed.");
+  is(propEditor.enable.style.visibility, "hidden",
+    "property enable checkbox is hidden.");
+
+  let widget = yield bezierTooltip.widget;
+  info("Simulating a change of curve in the widget");
+  widget.coordinates = [0.1, 2, 0.9, -1];
+  yield ruleEditor.rule._applyingModifications;
+
+  info("Pressing ESCAPE to close the tooltip");
+  let onHidden = bezierTooltip.tooltip.once("hidden");
+  EventUtils.sendKey("ESCAPE", widget.parent.ownerDocument.defaultView);
+  yield onHidden;
+  yield ruleEditor.rule._applyingModifications;
+
+  ok(propEditor.element.classList.contains("ruleview-overridden"),
+    "property is overridden.");
+  is(propEditor.enable.style.visibility, "visible",
+    "property enable checkbox is visible.");
+  ok(!propEditor.enable.getAttribute("checked"),
+    "property enable checkbox is not checked.");
+  ok(!propEditor.prop.enabled,
+    "animation-timing-function property is disabled.");
+  newValue = yield getRulePropertyValue("animation-timing-function");
+  is(newValue, "", "animation-timing-function should have been unset.");
+  is(propEditor.valueSpan.textContent, "linear",
+    "Got expected property value.");
+}
+
+function* getRulePropertyValue(name) {
+  let propValue = yield executeInContent("Test:GetRulePropertyValue", {
+    styleSheetIndex: 0,
+    ruleIndex: 0,
+    name: name
+  });
+  return propValue;
 }

browser/devtools/styleinspector/test/browser_ruleview_filtereditor-revert-on-ESC.js

@@ -3,36 +3,102 @@
 
 "use strict";
 
-// Tests the Filter Editor Tooltip reverting changes on ESC
+// Tests that changes made to the Filter Editor Tooltip are reverted when
+// ESC is pressed
 
 const TEST_URL = TEST_URL_ROOT + "doc_filter.html";
 
 add_task(function*() {
   yield addTab(TEST_URL);
+  let {view} = yield openRuleView();
+  yield testPressingEscapeRevertsChanges(view);
+  yield testPressingEscapeRevertsChangesAndDisables(view);
+});
 
-  let {toolbox, inspector, view} = yield openRuleView();
-
-  info("Getting the filter swatch element");
-  let swatch = getRuleViewProperty(view, "body", "filter").valueSpan
-    .querySelector(".ruleview-filterswatch");
-
+function* testPressingEscapeRevertsChanges(view) {
+  let ruleEditor = getRuleViewRuleEditor(view, 1);
+  let propEditor = ruleEditor.rule.textProps[0].editor;
+  let swatch = propEditor.valueSpan.querySelector(".ruleview-filterswatch");
   let filterTooltip = view.tooltips.filterEditor;
+
   let onShow = filterTooltip.tooltip.once("shown");
   swatch.click();
   yield onShow;
 
   let widget = yield filterTooltip.widget;
-
   widget.setCssValue("blur(2px)");
-  yield waitForComputedStyleProperty("body", null, "filter", "blur(2px)");
+  yield ruleEditor.rule._applyingModifications;
 
-  ok(true, "Changes previewed on the element");
+  yield waitForComputedStyleProperty("body", null, "filter", "blur(2px)");
+  is(propEditor.valueSpan.textContent, "blur(2px)",
+    "Got expected property value.");
 
   info("Pressing ESCAPE to close the tooltip");
   EventUtils.sendKey("ESCAPE", widget.styleWindow);
+  yield ruleEditor.rule._applyingModifications;
 
-  yield waitForSuccess(() => {
-    const computed = content.getComputedStyle(content.document.body);
-    return computed.filter === "blur(2px) contrast(2)";
-  }, "Waiting for the change to be reverted on the element");
-});
+  yield waitForComputedStyleProperty("body", null, "filter",
+    "blur(2px) contrast(2)");
+  is(propEditor.valueSpan.textContent, "blur(2px) contrast(2)",
+    "Got expected property value.");
+}
+
+function* testPressingEscapeRevertsChangesAndDisables(view) {
+  let ruleEditor = getRuleViewRuleEditor(view, 1);
+  let propEditor = ruleEditor.rule.textProps[0].editor;
+  let swatch = propEditor.valueSpan.querySelector(".ruleview-filterswatch");
+  let filterTooltip = view.tooltips.filterEditor;
+
+  info("Disabling filter property");
+  propEditor.enable.click();
+  yield ruleEditor.rule._applyingModifications;
+
+  ok(propEditor.element.classList.contains("ruleview-overridden"),
+    "property is overridden.");
+  is(propEditor.enable.style.visibility, "visible",
+    "property enable checkbox is visible.");
+  ok(!propEditor.enable.getAttribute("checked"),
+    "property enable checkbox is not checked.");
+  ok(!propEditor.prop.enabled,
+    "filter property is disabled.");
+  let newValue = yield getRulePropertyValue("filter");
+  is(newValue, "", "filter should have been unset.");
+
+  let onShow = filterTooltip.tooltip.once("shown");
+  swatch.click();
+  yield onShow;
+
+  ok(!propEditor.element.classList.contains("ruleview-overridden"),
+    "property overridden is not displayed.");
+  is(propEditor.enable.style.visibility, "hidden",
+    "property enable checkbox is hidden.");
+
+  let widget = yield filterTooltip.widget;
+  widget.setCssValue("blur(2px)");
+  yield ruleEditor.rule._applyingModifications;
+
+  info("Pressing ESCAPE to close the tooltip");
+  EventUtils.sendKey("ESCAPE", widget.styleWindow);
+  yield ruleEditor.rule._applyingModifications;
+
+  ok(propEditor.element.classList.contains("ruleview-overridden"),
+    "property is overridden.");
+  is(propEditor.enable.style.visibility, "visible",
+    "property enable checkbox is visible.");
+  ok(!propEditor.enable.getAttribute("checked"),
+    "property enable checkbox is not checked.");
+  ok(!propEditor.prop.enabled, "filter property is disabled.");
+  newValue = yield getRulePropertyValue("filter");
+  is(newValue, "", "filter should have been unset.");
+  is(propEditor.valueSpan.textContent, "blur(2px) contrast(2)",
+    "Got expected property value.");
+}
+
+function* getRulePropertyValue(name) {
+  let propValue = yield executeInContent("Test:GetRulePropertyValue", {
+    styleSheetIndex: 0,
+    ruleIndex: 0,
+    name: name
+  });
+  return propValue;
+}