Bug 1248274 - Fix click on augmented value elements (swatch, url,…) on the RuleView when property's name is being edited. r=pbro draft
authorNicolas Chevobbe <chevobbe.nicolas@gmail.com>
Fri, 29 Apr 2016 13:29:49 +0200
changeset 375168 99e2feb702feae1faa0ee8309c698b6609b8cab5
parent 374485 fe57228e70aa503323bf177093e2cecb438a39cc
child 375169 085991aa8897489a789c7f5adb58d0e00a3b2335
push id20181
push userchevobbe.nicolas@gmail.com
push dateFri, 03 Jun 2016 16:03:15 +0000
reviewerspbro
bugs1248274
milestone49.0a1
Bug 1248274 - Fix click on augmented value elements (swatch, url,…) on the RuleView when property's name is being edited. r=pbro Do not update the value span if the property name has not been modified. Add tests to ensure the bug is fixed. MozReview-Commit-ID: 7AHnd4iFmYW
devtools/client/inspector/rules/test/browser.ini
devtools/client/inspector/rules/test/browser_rules_edit-property_10.js
devtools/client/inspector/rules/views/text-property-editor.js
devtools/client/shared/output-parser.js
--- a/devtools/client/inspector/rules/test/browser.ini
+++ b/devtools/client/inspector/rules/test/browser.ini
@@ -109,16 +109,17 @@ subsuite = clipboard
 [browser_rules_edit-property_02.js]
 [browser_rules_edit-property_03.js]
 [browser_rules_edit-property_04.js]
 [browser_rules_edit-property_05.js]
 [browser_rules_edit-property_06.js]
 [browser_rules_edit-property_07.js]
 [browser_rules_edit-property_08.js]
 [browser_rules_edit-property_09.js]
+[browser_rules_edit-property_10.js]
 [browser_rules_edit-selector-click.js]
 [browser_rules_edit-selector-click-on-scrollbar.js]
 skip-if = os == "mac" # Bug 1245996 : click on scrollbar not working on OSX
 [browser_rules_edit-selector-commit.js]
 [browser_rules_edit-selector_01.js]
 [browser_rules_edit-selector_02.js]
 [browser_rules_edit-selector_03.js]
 [browser_rules_edit-selector_04.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/rules/test/browser_rules_edit-property_10.js
@@ -0,0 +1,145 @@
+/* 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 clicking on swatch-preceeded value while editing the property name
+// will result in editing the property value. See also Bug 1248274.
+
+const TEST_URI = `
+  <style type="text/css">
+  #testid {
+    color: red;
+    background: linear-gradient(
+      90deg,
+      rgb(183,222,237),
+      rgb(33,180,226),
+      rgb(31,170,217),
+      rgba(200,170,140,0.5));
+  }
+  </style>
+  <div id="testid">Styled Node</div>
+`;
+
+add_task(function* () {
+  yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  let {inspector, view} = yield openRuleView();
+
+  yield selectNode("#testid", inspector);
+
+  let ruleEditor = getRuleViewRuleEditor(view, 1);
+  yield testColorValueSpanClick(ruleEditor, view);
+  yield testColorSwatchShiftClick(ruleEditor, view);
+  yield testColorSwatchClick(ruleEditor, view);
+});
+
+function* testColorValueSpanClick(ruleEditor, view) {
+  info("Test click on color span while editing property name");
+  let propEditor = ruleEditor.rule.textProps[0].editor;
+  let colorSpan = propEditor.valueSpan.querySelector(".ruleview-color");
+
+  info("Focus the color name span");
+  yield focusEditableField(view, propEditor.nameSpan);
+  let editor = inplaceEditor(propEditor.doc.activeElement);
+
+  info("Modify the property to border-color to trigger the " +
+    "property-value-updated event");
+  editor.input.value = "border-color";
+
+  let onPropertyValueUpdate = view.once("property-value-updated");
+
+  info("blur propEditor.nameSpan by clicking on the color span");
+  EventUtils.synthesizeMouse(colorSpan, 1, 1, {}, propEditor.doc.defaultView);
+
+  info("wait for the property value to be updated");
+  yield onPropertyValueUpdate;
+
+  editor = inplaceEditor(propEditor.doc.activeElement);
+  is(inplaceEditor(propEditor.valueSpan), editor,
+    "The property value editor got focused");
+
+  info("blur valueSpan editor to trigger ruleview-changed event and prevent " +
+    "having pending request");
+  let onRuleViewChanged = view.once("ruleview-changed");
+  editor.input.blur();
+  yield onRuleViewChanged;
+}
+
+function* testColorSwatchShiftClick(ruleEditor, view) {
+  info("Test shift + click on color swatch while editing property name");
+  let propEditor = ruleEditor.rule.textProps[1].editor;
+  let swatchSpan = propEditor.valueSpan.querySelectorAll(
+    ".ruleview-colorswatch")[2];
+
+  info("Focus the background name span");
+  yield focusEditableField(view, propEditor.nameSpan);
+  let editor = inplaceEditor(propEditor.doc.activeElement);
+
+  info("Modify the property to background-image to trigger the " +
+    "property-value-updated event");
+  editor.input.value = "background-image";
+
+  let onPropertyValueUpdate = view.once("property-value-updated");
+  let onSwatchUnitChange = swatchSpan.once("unit-change");
+  let onRuleViewChanged = view.once("ruleview-changed");
+
+  info("blur propEditor.nameSpan by clicking on the color swatch");
+  EventUtils.synthesizeMouseAtCenter(swatchSpan, {shiftKey: true},
+    propEditor.doc.defaultView);
+
+  info("wait for ruleview-changed event to be triggered to prevent pending " +
+    "requests");
+  yield onRuleViewChanged;
+
+  info("wait for the color unit to change");
+  yield onSwatchUnitChange;
+  ok(true, "the color unit was changed");
+
+  info("wait for the property value to be updated");
+  yield onPropertyValueUpdate;
+
+  ok(!inplaceEditor(propEditor.valueSpan), "The inplace editor wasn't shown " +
+    "as a result of the color swatch shift + click");
+}
+
+function* testColorSwatchClick(ruleEditor, view) {
+  info("Test click on color swatch while editing property name");
+  let propEditor = ruleEditor.rule.textProps[1].editor;
+  let swatchSpan = propEditor.valueSpan.querySelectorAll(
+    ".ruleview-colorswatch")[3];
+  let colorPicker = view.tooltips.colorPicker;
+
+  info("Focus the background-image name span");
+  yield focusEditableField(view, propEditor.nameSpan);
+  let editor = inplaceEditor(propEditor.doc.activeElement);
+
+  info("Modify the property to background to trigger the " +
+    "property-value-updated event");
+  editor.input.value = "background";
+
+  let onRuleViewChanged = view.once("ruleview-changed");
+  let onPropertyValueUpdate = view.once("property-value-updated");
+  let onShown = colorPicker.tooltip.once("shown");
+
+  info("blur propEditor.nameSpan by clicking on the color swatch");
+  EventUtils.synthesizeMouseAtCenter(swatchSpan, {},
+    propEditor.doc.defaultView);
+
+  info("wait for ruleview-changed event to be triggered to prevent pending " +
+    "requests");
+  yield onRuleViewChanged;
+
+  info("wait for the property value to be updated");
+  yield onPropertyValueUpdate;
+
+  info("wait for the color picker to be shown");
+  yield onShown;
+
+  ok(true, "The color picker was shown on click of the color swatch");
+  ok(!inplaceEditor(propEditor.valueSpan),
+    "The inplace editor wasn't shown as a result of the color swatch click");
+
+  let spectrum = yield colorPicker.spectrum;
+  is(spectrum.rgb, "200,170,140,0.5", "The correct color picker was shown");
+}
--- a/devtools/client/inspector/rules/views/text-property-editor.js
+++ b/devtools/client/inspector/rules/views/text-property-editor.js
@@ -18,16 +18,34 @@ const {
 } = require("devtools/client/inspector/shared/utils");
 const {
   parseDeclarations,
   parseSingleValue,
 } = require("devtools/shared/css-parsing-utils");
 
 const HTML_NS = "http://www.w3.org/1999/xhtml";
 
+const SHARED_SWATCH_CLASS = "ruleview-swatch";
+const COLOR_SWATCH_CLASS = "ruleview-colorswatch";
+const BEZIER_SWATCH_CLASS = "ruleview-bezierswatch";
+const FILTER_SWATCH_CLASS = "ruleview-filterswatch";
+const ANGLE_SWATCH_CLASS = "ruleview-angleswatch";
+
+/*
+ * An actionable element is an element which on click triggers a specific action
+ * (e.g. shows a color tooltip, opens a link, …).
+ */
+const ACTIONABLE_ELEMENTS_SELECTORS = [
+  `.${COLOR_SWATCH_CLASS}`,
+  `.${BEZIER_SWATCH_CLASS}`,
+  `.${FILTER_SWATCH_CLASS}`,
+  `.${ANGLE_SWATCH_CLASS}`,
+  "a"
+];
+
 /**
  * TextPropertyEditor is responsible for the following:
  *   Owns a TextProperty object.
  *   Manages changes to the TextProperty.
  *   Can be expanded to display computed properties.
  *   Can mark a property disabled or enabled.
  *
  * @param {RuleEditor} ruleEditor
@@ -39,30 +57,33 @@ function TextPropertyEditor(ruleEditor, 
   this.ruleEditor = ruleEditor;
   this.ruleView = this.ruleEditor.ruleView;
   this.doc = this.ruleEditor.doc;
   this.popup = this.ruleView.popup;
   this.prop = property;
   this.prop.editor = this;
   this.browserWindow = this.doc.defaultView.top;
   this._populatedComputed = false;
+  this._hasPendingClick = false;
+  this._clickedElementOptions = null;
 
   const toolbox = this.ruleView.inspector.toolbox;
   this.cssProperties = getCssProperties(toolbox);
 
   this._onEnableClicked = this._onEnableClicked.bind(this);
   this._onExpandClicked = this._onExpandClicked.bind(this);
   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);
+  this.updatePropertyState = this.updatePropertyState.bind(this);
 
   this._create();
   this.update();
 }
 
 TextPropertyEditor.prototype = {
   /**
    * Boolean indicating if the name or value is being currently edited.
@@ -190,17 +211,17 @@ TextPropertyEditor.prototype = {
           this.nameSpan.click();
         }
       }, false);
 
       editableField({
         start: this._onStartEditing,
         element: this.nameSpan,
         done: this._onNameDone,
-        destroy: this.update,
+        destroy: this.updatePropertyState,
         advanceChars: ":",
         contentType: InplaceEditor.CONTENT_TYPES.CSS_PROPERTY,
         popup: this.popup
       });
 
       // Auto blur name field on multiple CSS rules get pasted in.
       this.nameContainer.addEventListener("paste",
         blurOnMultipleProperties(this.cssProperties), false);
@@ -210,16 +231,47 @@ TextPropertyEditor.prototype = {
         event.stopPropagation();
 
         // Forward clicks on valueContainer to the editable valueSpan
         if (event.target === this.valueContainer) {
           this.valueSpan.click();
         }
       }, false);
 
+      // The mousedown event could trigger a blur event on nameContainer, which
+      // will trigger a call to the update function. The update function clears
+      // valueSpan's markup. Thus the regular click event does not bubble up, and
+      // listener's callbacks are not called.
+      // So we need to remember where the user clicks in order to re-trigger the click
+      // after the valueSpan's markup is re-populated. We only need to track this for
+      // valueSpan's child elements, because direct click on valueSpan will always
+      // trigger a click event.
+      this.valueSpan.addEventListener("mousedown", (event) => {
+        let clickedEl = event.target;
+        if (clickedEl === this.valueSpan) {
+          return;
+        }
+        this._hasPendingClick = true;
+
+        let matchedSelector = ACTIONABLE_ELEMENTS_SELECTORS.find(
+          (selector) => clickedEl.matches(selector));
+        if (matchedSelector) {
+          let similarElements = [...this.valueSpan.querySelectorAll(matchedSelector)];
+          this._clickedElementOptions = {
+            selector: matchedSelector,
+            index: similarElements.indexOf(clickedEl)
+          };
+        }
+      }, false);
+
+      this.valueSpan.addEventListener("mouseup", (event) => {
+        this._clickedElementOptions = null;
+        this._hasPendingClick = false;
+      }, false);
+
       this.valueSpan.addEventListener("click", (event) => {
         let target = event.target;
 
         if (target.nodeName === "a") {
           event.stopPropagation();
           event.preventDefault();
           this.browserWindow.openUILinkIn(target.href, "tab");
         }
@@ -258,37 +310,17 @@ TextPropertyEditor.prototype = {
   /**
    * Populate the span based on changes to the TextProperty.
    */
   update: function () {
     if (this.ruleView.isDestroyed) {
       return;
     }
 
-    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.hidden = this.editing || this.isValid();
-    this.filterProperty.hidden = this.editing ||
-                                 !this.isValid() ||
-                                 !this.prop.overridden ||
-                                 this.ruleEditor.rule.isUnmatched;
-
-    if (!this.editing &&
-        (this.prop.overridden || !this.prop.enabled ||
-         !this.prop.isKnownProperty())) {
-      this.element.classList.add("ruleview-overridden");
-    } else {
-      this.element.classList.remove("ruleview-overridden");
-    }
+    this.updatePropertyState();
 
     let name = this.prop.name;
     this.nameSpan.textContent = name;
 
     // Combine the property's value and priority into one string for
     // the value.
     let store = this.rule.elementStyle.store;
     let val = store.userProperties.getProperty(this.rule.style, name,
@@ -300,43 +332,39 @@ TextPropertyEditor.prototype = {
     let propDirty = store.userProperties.contains(this.rule.style, name);
 
     if (propDirty) {
       this.element.setAttribute("dirty", "");
     } else {
       this.element.removeAttribute("dirty");
     }
 
-    const sharedSwatchClass = "ruleview-swatch ";
-    const colorSwatchClass = "ruleview-colorswatch";
-    const bezierSwatchClass = "ruleview-bezierswatch";
-    const filterSwatchClass = "ruleview-filterswatch";
-    const angleSwatchClass = "ruleview-angleswatch";
-
     let outputParser = this.ruleView._outputParser;
     let parserOptions = {
-      colorSwatchClass: sharedSwatchClass + colorSwatchClass,
+      colorSwatchClass: SHARED_SWATCH_CLASS + " " + COLOR_SWATCH_CLASS,
       colorClass: "ruleview-color",
-      bezierSwatchClass: sharedSwatchClass + bezierSwatchClass,
+      bezierSwatchClass: SHARED_SWATCH_CLASS + " " + BEZIER_SWATCH_CLASS,
       bezierClass: "ruleview-bezier",
-      filterSwatchClass: sharedSwatchClass + filterSwatchClass,
+      filterSwatchClass: SHARED_SWATCH_CLASS + " " + FILTER_SWATCH_CLASS,
       filterClass: "ruleview-filter",
-      angleSwatchClass: sharedSwatchClass + angleSwatchClass,
+      angleSwatchClass: SHARED_SWATCH_CLASS + " " + ANGLE_SWATCH_CLASS,
       angleClass: "ruleview-angle",
       defaultColorType: !propDirty,
       urlClass: "theme-link",
       baseURI: this.sheetHref
     };
     let frag = outputParser.parseCssProperty(name, val, parserOptions);
     this.valueSpan.innerHTML = "";
     this.valueSpan.appendChild(frag);
 
+    this.ruleView.emit("property-value-updated", this.valueSpan);
+
     // Attach the color picker tooltip to the color swatches
     this._colorSwatchSpans =
-      this.valueSpan.querySelectorAll("." + colorSwatchClass);
+      this.valueSpan.querySelectorAll("." + COLOR_SWATCH_CLASS);
     if (this.ruleEditor.isEditable) {
       for (let span of this._colorSwatchSpans) {
         // Adding this swatch to the list of swatches our colorpicker
         // knows about
         this.ruleView.tooltips.colorPicker.addSwatch(span, {
           onShow: this._onStartEditing,
           onPreview: this._onSwatchPreview,
           onCommit: this._onSwatchCommit,
@@ -345,72 +373,120 @@ TextPropertyEditor.prototype = {
         span.on("unit-change", this._onSwatchCommit);
         let title = CssLogic.l10n("rule.colorSwatch.tooltip");
         span.setAttribute("title", title);
       }
     }
 
     // Attach the cubic-bezier tooltip to the bezier swatches
     this._bezierSwatchSpans =
-      this.valueSpan.querySelectorAll("." + bezierSwatchClass);
+      this.valueSpan.querySelectorAll("." + BEZIER_SWATCH_CLASS);
     if (this.ruleEditor.isEditable) {
       for (let span of this._bezierSwatchSpans) {
         // Adding this swatch to the list of swatches our colorpicker
         // knows about
         this.ruleView.tooltips.cubicBezier.addSwatch(span, {
           onShow: this._onStartEditing,
           onPreview: this._onSwatchPreview,
           onCommit: this._onSwatchCommit,
           onRevert: this._onSwatchRevert
         });
         let title = CssLogic.l10n("rule.bezierSwatch.tooltip");
         span.setAttribute("title", title);
       }
     }
 
     // Attach the filter editor tooltip to the filter swatch
-    let span = this.valueSpan.querySelector("." + filterSwatchClass);
+    let span = this.valueSpan.querySelector("." + FILTER_SWATCH_CLASS);
     if (this.ruleEditor.isEditable) {
       if (span) {
         parserOptions.filterSwatch = true;
 
         this.ruleView.tooltips.filterEditor.addSwatch(span, {
           onShow: this._onStartEditing,
           onPreview: this._onSwatchPreview,
           onCommit: this._onSwatchCommit,
           onRevert: this._onSwatchRevert
         }, outputParser, parserOptions);
         let title = CssLogic.l10n("rule.filterSwatch.tooltip");
         span.setAttribute("title", title);
       }
     }
 
     this.angleSwatchSpans =
-      this.valueSpan.querySelectorAll("." + angleSwatchClass);
+      this.valueSpan.querySelectorAll("." + ANGLE_SWATCH_CLASS);
     if (this.ruleEditor.isEditable) {
       for (let angleSpan of this.angleSwatchSpans) {
         angleSpan.on("unit-change", this._onSwatchCommit);
         let title = CssLogic.l10n("rule.angleSwatch.tooltip");
         angleSpan.setAttribute("title", title);
       }
     }
 
+    // Now that we have updated the property's value, we might have a pending
+    // click on the value container. If we do, we have to trigger a click event
+    // on the right element.
+    if (this._hasPendingClick) {
+      this._hasPendingClick = false;
+      let elToClick;
+
+      if (this._clickedElementOptions !== null) {
+        let {selector, index} = this._clickedElementOptions;
+        elToClick = this.valueSpan.querySelectorAll(selector)[index];
+
+        this._clickedElementOptions = null;
+      }
+
+      if (!elToClick) {
+        elToClick = this.valueSpan;
+      }
+      elToClick.click();
+    }
+
     // Populate the computed styles.
     this._updateComputed();
 
     // Update the rule property highlight.
     this.ruleView._updatePropertyHighlight(this);
   },
 
   _onStartEditing: function () {
     this.element.classList.remove("ruleview-overridden");
     this.enable.style.visibility = "hidden";
   },
 
   /**
+   * Update the visibility of the enable checkbox, the warning indicator and
+   * the filter property, as well as the overriden state of the property.
+   */
+  updatePropertyState: function () {
+    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.hidden = this.editing || this.isValid();
+    this.filterProperty.hidden = this.editing ||
+                                 !this.isValid() ||
+                                 !this.prop.overridden ||
+                                 this.ruleEditor.rule.isUnmatched;
+
+    if (!this.editing &&
+        (this.prop.overridden || !this.prop.enabled ||
+         !this.prop.isKnownProperty())) {
+      this.element.classList.add("ruleview-overridden");
+    } else {
+      this.element.classList.remove("ruleview-overridden");
+    }
+  },
+
+  /**
    * Update the indicator for computed styles. The computed styles themselves
    * are populated on demand, when they become visible.
    */
   _updateComputed: function () {
     this.computed.innerHTML = "";
 
     let showExpander = this.prop.computed.some(c => c.name !== this.prop.name);
     this.expander.style.visibility = showExpander ? "visible" : "hidden";
--- a/devtools/client/shared/output-parser.js
+++ b/devtools/client/shared/output-parser.js
@@ -449,39 +449,38 @@ OutputParser.prototype = {
     });
     value.appendChild(nodes);
     container.appendChild(value);
 
     return container;
   },
 
   _onColorSwatchMouseDown: function (event) {
-    // Prevent text selection in the case of shift-click or double-click.
-    event.preventDefault();
-
     if (!event.shiftKey) {
       return;
     }
 
+    // Prevent click event to be fired to not show the tooltip
+    event.stopPropagation();
+
     let swatch = event.target;
     let color = this.colorSwatches.get(swatch);
     let val = color.nextColorUnit();
 
     swatch.nextElementSibling.textContent = val;
     swatch.emit("unit-change", val);
   },
 
   _onAngleSwatchMouseDown: function (event) {
-    // Prevent text selection in the case of shift-click or double-click.
-    event.preventDefault();
-
     if (!event.shiftKey) {
       return;
     }
 
+    event.stopPropagation();
+
     let swatch = event.target;
     let angle = this.angleSwatches.get(swatch);
     let val = angle.nextAngleUnit();
 
     swatch.nextElementSibling.textContent = val;
     swatch.emit("unit-change", val);
   },