Bug 1507749 - Account for changing TextProperty when editing inline styles. r=pbro
authorRazvan Caliman <rcaliman@mozilla.com>
Tue, 20 Nov 2018 15:19:47 +0000
changeset 503666 4b78e1686da9ba317be1796cf241db3231f9cb75
parent 503665 4b0554a10847e9a879ab193bf16a3bbdaa8e69d4
child 503667 c86b9e22b393d97c6d4e4dc0fc33bc6cd3c2172c
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro
bugs1507749, 1467076
milestone65.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 1507749 - Account for changing TextProperty when editing inline styles. r=pbro The patch for https://bugzilla.mozilla.org/show_bug.cgi?id=1467076 discards previous TextProperty instances for the element's inline style when the inline style is updated. On start, the Shape Path Editor kept a reference to the original TextProperty instance. This outdated instance caused the update mechanism of the Shape Path Editor to fail. This patch fixes the issue by keeping metadata (index and property name) about the TextProperty to re-identify the correct one when needed instead of relying on a potentially outdated reference. Differential Revision: https://phabricator.services.mozilla.com/D12157
devtools/client/shared/widgets/ShapesInContextEditor.js
--- a/devtools/client/shared/widgets/ShapesInContextEditor.js
+++ b/devtools/client/shared/widgets/ShapesInContextEditor.js
@@ -33,32 +33,52 @@ class ShapesInContextEditor {
     // when toggling the highlighter on the same node for different CSS properties.
     this.mode = null;
     // Reference to Rule view used to listen for changes
     this.ruleView = this.inspector.getPanel("ruleview").view;
     // Reference of |state| from HighlightersOverlay.
     this.state = state;
     // Reference to DOM node of the toggle icon for shapes highlighter.
     this.swatch = null;
-    // Reference to TextProperty where shape changes will be written.
-    this.textProperty = null;
 
     // Commit triggers expensive DOM changes in TextPropertyEditor.update()
     // so we debounce it.
     this.commit = debounce(this.commit, 200, this);
     this.onHighlighterEvent = this.onHighlighterEvent.bind(this);
     this.onNodeFrontChanged = this.onNodeFrontChanged.bind(this);
     this.onShapeValueUpdated = this.onShapeValueUpdated.bind(this);
     this.onRuleViewChanged = this.onRuleViewChanged.bind(this);
 
     this.highlighter.on("highlighter-event", this.onHighlighterEvent);
     this.ruleView.on("ruleview-changed", this.onRuleViewChanged);
   }
 
   /**
+   * Get the reference to the TextProperty where shape changes should be written.
+   *
+   * We can't rely on the TextProperty to be consistent while changing the value of an
+   * inline style because the fix for Bug 1467076 forces a full rebuild of TextProperties
+   * for the inline style's mock-CSS Rule in the Rule view.
+   *
+   * On |toggle()|, we store the target TextProperty index, property name and parent rule.
+   * Here, we use that index and property name to attempt to re-identify the correct
+   * TextProperty in the rule.
+   *
+   * @return {TextProperty|null}
+   */
+  get textProperty() {
+    if (!this.rule || !this.rule.textProps) {
+      return null;
+    }
+
+    const textProp = this.rule.textProps[this.textPropIndex];
+    return (textProp && textProp.name === this.textPropName) ? textProp : null;
+  }
+
+  /**
   * Called when the element style changes from the Rule view.
   * If the TextProperty we're acting on isn't enabled anymore or overridden,
   * turn off the shapes highlighter.
   */
   async onRuleViewChanged() {
     if (this.textProperty &&
       (!this.textProperty.enabled || this.textProperty.overridden)) {
       await this.hide();
@@ -85,17 +105,22 @@ class ShapesInContextEditor {
     }
 
     // Same target node, dfferent modes -> toggle between shape-outside and clip-path.
     // Hide highlighter for previous property, but continue and show for other property.
     if ((node == this.highlighterTargetNode) && (this.mode !== options.mode)) {
       await this.hide();
     }
 
-    this.textProperty = prop;
+    // Save the target TextProperty's parent rule, index and property name for later
+    // re-identification of the TextProperty. @see |get textProperty()|.
+    this.rule = prop.rule;
+    this.textPropIndex = this.rule.textProps.indexOf(prop);
+    this.textPropName = prop.name;
+
     this.findSwatch();
     await this.show(node, options);
   }
 
   /**
    * Show the shapes highlighter for the given element.
    *
    * @param {NodeFront} node
@@ -126,32 +151,38 @@ class ShapesInContextEditor {
     } catch (err) {
       // silent error
     }
 
     if (this.swatch) {
       this.swatch.classList.remove("active");
     }
     this.swatch = null;
-    this.textProperty = null;
+    this.rule = null;
+    this.textPropIndex = -1;
+    this.textPropName = null;
 
     this.emit("hide", { node: this.highlighterTargetNode });
     this.inspector.selection.off("detached-front", this.onNodeFrontChanged);
     this.inspector.selection.off("new-node-front", this.onNodeFrontChanged);
     this.ruleView.off("property-value-updated", this.onShapeValueUpdated);
     this.highlighterTargetNode = null;
   }
 
   /**
    * Identify the swatch (aka toggle icon) DOM node from the TextPropertyEditor of the
    * TextProperty we're working with. Whenever the TextPropertyEditor is updated (i.e.
    * when committing the shape value to the Rule view), it rebuilds its DOM and the old
    * swatch reference becomes invalid. Call this method to identify the current swatch.
    */
   findSwatch() {
+    if (!this.textProperty) {
+      return;
+    }
+
     const valueSpan = this.textProperty.editor.valueSpan;
     this.swatch = valueSpan.querySelector(".ruleview-shapeswatch");
     if (this.swatch) {
       this.swatch.classList.add("active");
     }
   }
 
   /**
@@ -244,21 +275,25 @@ class ShapesInContextEditor {
   }
 
   /**
   * Handler for "property-value-updated" event triggered by the Rule view.
   * Called after the shape value has been written to the element's style and the Rule
   * view updated. Emits an event on HighlightersOverlay that is expected by
   * tests in order to check if the shape value has been correctly applied.
   */
-  onShapeValueUpdated() {
-    // When TextPropertyEditor updates, it replaces the previous swatch DOM node.
-    // Find and store the new one.
-    this.findSwatch();
-    this.inspector.highlighters.emit("shapes-highlighter-changes-applied");
+  async onShapeValueUpdated() {
+    if (this.textProperty) {
+      // When TextPropertyEditor updates, it replaces the previous swatch DOM node.
+      // Find and store the new one.
+      this.findSwatch();
+      this.inspector.highlighters.emit("shapes-highlighter-changes-applied");
+    } else {
+      await this.hide();
+    }
   }
 
   /**
   * Preview a shape value on the element without committing the changes to the Rule view.
   *
   * @param {String} value
   *        The shape value to set the current property to
   */