Bug 1475782 - Fix for intermittent test failures on Font Editor unit conversion. r=pbro draft
authorRazvan Caliman <rcaliman@mozilla.com>
Tue, 17 Jul 2018 12:21:40 +0200
changeset 823406 349b62c2da79c5ec58bd5465d396e907a52646b8
parent 823333 8f2f847b2f9dc5643eeb9935d603a3b30686f972
child 823407 773672745f6890a064d5d6a862c46e5246b0cfef
push id117676
push userbmo:rcaliman@mozilla.com
push dateFri, 27 Jul 2018 09:57:14 +0000
reviewerspbro
bugs1475782
milestone63.0a1
Bug 1475782 - Fix for intermittent test failures on Font Editor unit conversion. r=pbro - Catch getComputed errors from async call - Explicitly create new TextProperty when none exists MozReview-Commit-ID: DokLJHIO6Cr
devtools/client/inspector/fonts/fonts.js
devtools/client/inspector/rules/views/text-property-editor.js
--- a/devtools/client/inspector/fonts/fonts.js
+++ b/devtools/client/inspector/fonts/fonts.js
@@ -209,32 +209,49 @@ class FontInspector {
 
     if (unit === "pc") {
       out = fromPx
         ? value * 0.0625
         : value / 0.0625;
     }
 
     if (unit === "%") {
-      computedStyle = await this.pageStyle.getComputed(node.parentNode());
+      computedStyle =
+        await this.pageStyle.getComputed(node.parentNode()).catch(console.error);
+
+      if (!computedStyle) {
+        return value;
+      }
+
       out = fromPx
         ? value * 100 / parseFloat(computedStyle["font-size"].value)
         : value / 100 * parseFloat(computedStyle["font-size"].value);
     }
 
     if (unit === "em") {
-      computedStyle = await this.pageStyle.getComputed(node.parentNode());
+      computedStyle =
+        await this.pageStyle.getComputed(node.parentNode()).catch(console.error);
+
+      if (!computedStyle) {
+        return value;
+      }
+
       out = fromPx
         ? value / parseFloat(computedStyle["font-size"].value)
         : value * parseFloat(computedStyle["font-size"].value);
     }
 
     if (unit === "rem") {
       const document = await this.inspector.walker.documentElement();
-      computedStyle = await this.pageStyle.getComputed(document);
+      computedStyle = await this.pageStyle.getComputed(document).catch(console.error);
+
+      if (!computedStyle) {
+        return value;
+      }
+
       out = fromPx
         ? value / parseFloat(computedStyle["font-size"].value)
         : value * parseFloat(computedStyle["font-size"].value);
     }
 
     if (unit === "vh" || unit === "vw" || unit === "vmin" || unit === "vmax") {
       const dim = await node.getOwnerGlobalDimensions();
 
@@ -418,39 +435,30 @@ class FontInspector {
       allFonts = [];
     }
 
     return this.excludeNodeFonts(allFonts, nodeFonts);
   }
 
   /**
    * Get a reference to a TextProperty instance from the current selected rule for a
-   * given property name. If one doesn't exist, create one with the given value.
-   * If the selected rule no longer exists (ex: during test teardown), return null.
+   * given property name.
    *
    * @param {String} name
    *        CSS property name
-   * @param {String} value
-   *        CSS property value
    * @return {TextProperty|null}
    */
-  getTextProperty(name, value) {
+  getTextProperty(name) {
     if (!this.selectedRule) {
       return null;
     }
 
-    let textProperty =
-      this.selectedRule.textProps.find(prop =>
-        prop.name === name && prop.enabled && !prop.overridden
-      );
-    if (!textProperty) {
-      textProperty = this.selectedRule.editor.addProperty(name, value, "", true);
-    }
-
-    return textProperty;
+    return this.selectedRule.textProps.find(prop =>
+      prop.name === name && prop.enabled && !prop.overridden
+    );
   }
 
   /**
    * Given the axis name of a registered axis, return a method which updates the
    * corresponding CSS font property when called with a value.
    *
    * All variable font axes can be written in the value of the "font-variation-settings"
    * CSS font property. In CSS Fonts Level 4, registered axes values can be used as
@@ -766,21 +774,23 @@ class FontInspector {
     }
   }
 
   /**
    * Handler for "property-value-updated" event emitted from the rule view whenever a
    * property value changes. Ignore changes to properties unrelated to the font editor.
    *
    * @param {Object} eventData
-   *        Object with the property name and value.
-   *        Example: { name: "font-size", value: "1em" }
+   *        Object with the property name and value and origin rule.
+   *        Example: { name: "font-size", value: "1em", rule: Object }
    */
   async onRulePropertyUpdated(eventData) {
-    if (!FONT_PROPERTIES.includes(eventData.property)) {
+    if (!this.selectedRule ||
+        !this.selectedRule.matches({ rule: eventData.rule.domRule }) ||
+        !FONT_PROPERTIES.includes(eventData.property)) {
       return;
     }
 
     if (this.isPanelVisible()) {
       await this.refreshFontEditor();
     }
   }
 
@@ -877,20 +887,31 @@ class FontInspector {
     const options = {};
     if (this.pageStyle.supportsFontVariations) {
       options.includeVariations = true;
     }
 
     const node = this.inspector.selection.nodeFront;
     const fonts = await this.getFontsForNode(node, options);
 
-    // Get computed styles for the selected node, but filter by CSS font properties.
-    this.nodeComputedStyle = await this.pageStyle.getComputed(node, {
-      filterProperties: FONT_PROPERTIES
-    });
+    try {
+      // Get computed styles for the selected node, but filter by CSS font properties.
+      this.nodeComputedStyle = await this.pageStyle.getComputed(node, {
+        filterProperties: FONT_PROPERTIES
+      });
+    } catch (e) {
+      // Because getComputed is async, there is a chance the font editor was
+      // destroyed while we were waiting. If that happened, just bail out
+      // silently.
+      if (!this.document) {
+        return;
+      }
+
+      throw e;
+    }
 
     if (!this.nodeComputedStyle || !fonts.length) {
       this.store.dispatch(resetFontEditor());
       this.inspector.emit("fonteditor-updated");
       return;
     }
 
     // Clear any references to writer methods and CSS declarations because the node's
@@ -1032,29 +1053,30 @@ class FontInspector {
    * as "changed" in the Rule view with a green indicator.
    *
    * @param {String} name
    *        CSS property name
    * @param {String}value
    *        CSS property value
    */
   updatePropertyValue(name, value) {
-    const textProperty = this.getTextProperty(name, value);
-    if (!textProperty || textProperty.value === value) {
+    const textProperty = this.getTextProperty(name);
+
+    if (!textProperty) {
+      this.selectedRule.createProperty(name, value, "", true);
+      return;
+    }
+
+    if (textProperty.value === value) {
       return;
     }
 
     // Prevent reacting to changes we caused.
     this.ruleView.off("property-value-updated", this.onRulePropertyUpdated);
     // Live preview font property changes on the page.
-
-    try {
-      textProperty.rule.previewPropertyValue(textProperty, value, "");
-    } catch (e) {
-      // Silent error
-    }
+    textProperty.rule.previewPropertyValue(textProperty, value, "");
 
     // Sync Rule view with changes reflected on the page (debounced).
     this.syncChanges(name, value);
   }
 }
 
 module.exports = FontInspector;
--- a/devtools/client/inspector/rules/views/text-property-editor.js
+++ b/devtools/client/inspector/rules/views/text-property-editor.js
@@ -418,17 +418,23 @@ TextPropertyEditor.prototype = {
         value: frag.textContent,
         priority: this.prop.priority,
       };
     }
 
     this.valueSpan.innerHTML = "";
     this.valueSpan.appendChild(frag);
 
-    this.ruleView.emit("property-value-updated", { property: name, value: val });
+    this.ruleView.emit("property-value-updated",
+      {
+        rule: this.prop.rule,
+        property: name,
+        value: val,
+      }
+    );
 
     // Highlight the currently used font in font-family properties.
     // If we cannot find a match, highlight the first generic family instead.
     const fontFamilySpans = this.valueSpan.querySelectorAll("." + FONT_FAMILY_CLASS);
     if (fontFamilySpans.length && this.prop.enabled && !this.prop.overridden) {
       this.rule.elementStyle.getUsedFontFamilies().then(families => {
         const usedFontFamilies = families.map(font => font.toLowerCase());
         let foundMatchingFamily = false;