Bug 1459898 - (Part 2) Add test for font-size unit conversion. r=gl draft
authorRazvan Caliman <rcaliman@mozilla.com>
Thu, 12 Jul 2018 12:34:34 +0200
changeset 817775 cafbd6ce313085ae06dccf0564724b82dfbc9954
parent 817322 2d21bd0f8bb48f7f2cacd6c87c5cbac55bb61dfa
push id116156
push userbmo:rcaliman@mozilla.com
push dateFri, 13 Jul 2018 12:22:52 +0000
reviewersgl
bugs1459898
milestone63.0a1
Bug 1459898 - (Part 2) Add test for font-size unit conversion. r=gl MozReview-Commit-ID: JZl7igm6h6A
devtools/client/inspector/fonts/fonts.js
devtools/client/inspector/fonts/test/browser.ini
devtools/client/inspector/fonts/test/browser_fontinspector_editor-font-size-conversion.js
devtools/client/inspector/fonts/test/browser_fontinspector_editor-keywords.js
devtools/client/inspector/fonts/test/head.js
--- a/devtools/client/inspector/fonts/fonts.js
+++ b/devtools/client/inspector/fonts/fonts.js
@@ -421,24 +421,29 @@ class FontInspector {
     }
 
     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.
    *
    * @param {String} name
    *        CSS property name
    * @param {String} value
    *        CSS property value
-   * @return {TextProperty}
+   * @return {TextProperty|null}
    */
   getTextProperty(name, value) {
+    if (!this.selectedRule) {
+      return null;
+    }
+
     let textProperty =
       this.selectedRule.textProps.find(prop => prop.name === name);
     if (!textProperty) {
       textProperty = this.selectedRule.editor.addProperty(name, value, "", true);
     }
 
     return textProperty;
   }
@@ -610,17 +615,24 @@ class FontInspector {
    * @param  {String} name
    *         CSS property name
    * @param  {String} value
    *         CSS property value
    */
   syncChanges(name, value) {
     const textProperty = this.getTextProperty(name, value);
     if (textProperty) {
-      textProperty.setValue(value);
+      // This method may be called after the connection to the page style actor is closed.
+      // For example, during teardown of automated tests. Here, we catch any failure that
+      // may occur because of that. We're not interested in handling the error.
+      try {
+        textProperty.setValue(value);
+      } catch (e) {
+        // Silent error.
+      }
     }
 
     this.ruleView.on("property-value-updated", this.onRulePropertyUpdated);
   }
 
   /**
    * Handler for changes of a font axis value coming from the FontEditor.
    *
@@ -709,18 +721,22 @@ class FontInspector {
    * @param  {String|undefined} toUnit
    *         Optional CSS unit to convert to
    */
   async onPropertyChange(property, value, fromUnit, toUnit) {
     if (FONT_PROPERTIES.includes(property)) {
       let unit = fromUnit;
 
       if (toUnit && fromUnit) {
-        value = await this.convert(value, fromUnit, toUnit);
-        unit = toUnit;
+        try {
+          value = await this.convertUnits(value, fromUnit, toUnit);
+          unit = toUnit;
+        } catch (err) {
+          // Silent error
+        }
       }
 
       this.onFontPropertyUpdate(property, value, unit);
     } else {
       this.onAxisUpdate(property, value);
     }
   }
 
@@ -984,15 +1000,21 @@ class FontInspector {
     const textProperty = this.getTextProperty(name, value);
     if (!textProperty || 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.
-    textProperty.rule.previewPropertyValue(textProperty, value, "");
+
+    try {
+      textProperty.rule.previewPropertyValue(textProperty, value, "");
+    } catch (e) {
+      // Silent error
+    }
+
     // Sync Rule view with changes reflected on the page (debounced).
     this.syncChanges(name, value);
   }
 }
 
 module.exports = FontInspector;
--- a/devtools/client/inspector/fonts/test/browser.ini
+++ b/devtools/client/inspector/fonts/test/browser.ini
@@ -15,16 +15,17 @@ support-files =
   !/devtools/client/shared/test/test-actor.js
   !/devtools/client/shared/test/test-actor-registry.js
 
 [browser_fontinspector.js]
 [browser_fontinspector_copy-URL.js]
 skip-if = !e10s # too slow on !e10s, logging fully serialized actors (Bug 1446595)
 subsuite = clipboard
 [browser_fontinspector_edit-previews.js]
+[browser_fontinspector_editor-font-size-conversion.js]
 [browser_fontinspector_editor-values.js]
 [browser_fontinspector_editor-keywords.js]
 [browser_fontinspector_expand-css-code.js]
 [browser_fontinspector_family-unused.js]
 [browser_fontinspector_no-fonts.js]
 [browser_fontinspector_other-fonts.js]
 [browser_fontinspector_rendered-fonts.js]
 [browser_fontinspector_reveal-in-page.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/fonts/test/browser_fontinspector_editor-font-size-conversion.js
@@ -0,0 +1,71 @@
+/* vim: set ts=2 et sw=2 tw=80: */
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+/* global getPropertyValue waitFor */
+
+"use strict";
+
+// Test that changes to font-size units converts the value to the destination unit.
+// Check that converted values are applied to the inline style of the selected element.
+
+const TEST_URI = URL_ROOT + "browser_fontinspector.html";
+
+add_task(async function() {
+  await pushPref("devtools.inspector.fonteditor.enabled", true);
+  const { inspector, view, tab, testActor } = await openFontInspectorForURL(TEST_URI);
+  const viewDoc = view.document;
+  const property = "font-size";
+  const selector = "div";
+  const UNITS = {
+    "px": "36px",
+    "%": "100%",
+    "em": "1em",
+  };
+
+  await selectNode(selector, inspector);
+
+  info("Check that font editor shows font-size value in original units");
+  let fontSize = getPropertyValue(viewDoc, property);
+  is(fontSize.unit, "em", "Original unit for font size is em");
+  is(fontSize.value + fontSize.unit, "1em", "Original font size is 1em");
+
+  let prevValue = fontSize.value;
+  let prevUnit = fontSize.unit;
+
+  for (const unit in UNITS) {
+    const value = UNITS[unit];
+    const onEditorUpdated = inspector.once("fonteditor-updated");
+
+    info(`Convert font-size from ${prevValue}${prevUnit} to ${unit}`);
+    await view.onPropertyChange(property, prevValue, prevUnit, unit);
+    info("Waiting for font editor to re-render");
+    await onEditorUpdated;
+    info(`Waiting for font-size unit dropdown to re-render with selected value: ${unit}`);
+    await waitFor(() => {
+      const sel = `#font-editor .font-value-slider[name=${property}] ~ .font-unit-select`;
+      return viewDoc.querySelector(sel).value === unit;
+    });
+    info("Waiting for testactor reflow");
+    await testActor.reflow();
+
+    info(`Check that font editor font-size value is converted to ${unit}`);
+    fontSize = getPropertyValue(viewDoc, property);
+    is(fontSize.unit, unit, `Font size unit is converted to ${unit}`);
+    is(fontSize.value + fontSize.unit, value, `Font size in font editor is ${value}`);
+
+    info(`Check that inline style font-size value is converted to ${unit}`);
+    const inlineStyleValue = await getInlineStyleValue(tab, selector, property);
+    is(inlineStyleValue, value, `Font size on inline style is ${value}`);
+
+    // Store current unit and value to use in conversion on the next iteration.
+    prevUnit = fontSize.unit;
+    prevValue = fontSize.value;
+  }
+});
+
+async function getInlineStyleValue(tab, selector, property) {
+  return ContentTask.spawn(tab.linkedBrowser, { selector, property }, function(args) {
+    const el = content.document.querySelector(args.selector);
+    return el && el.style[args.property];
+  });
+}
--- a/devtools/client/inspector/fonts/test/browser_fontinspector_editor-keywords.js
+++ b/devtools/client/inspector/fonts/test/browser_fontinspector_editor-keywords.js
@@ -1,37 +1,28 @@
 /* vim: set ts=2 et sw=2 tw=80: */
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
+/* global getPropertyValue */
 
 "use strict";
 
 // Test that keyword values for font properties don't show up in the font editor,
 // but their computed style values show up instead.
 
 const TEST_URI = URL_ROOT + "browser_fontinspector.html";
 
 add_task(async function() {
   await pushPref("devtools.inspector.fonteditor.enabled", true);
   const { inspector, view } = await openFontInspectorForURL(TEST_URI);
   const viewDoc = view.document;
 
   await testKeywordValues(inspector, viewDoc);
 });
 
-function getPropertyValue(viewDoc, name) {
-  const selector = `#font-editor .font-value-slider[name=${name}]`;
-  return {
-    value: viewDoc.querySelector(selector).value,
-    // Ensure unit dropdown exists before querying its value
-    unit: viewDoc.querySelector(selector + ` ~ .font-unit-select`) &&
-          viewDoc.querySelector(selector + ` ~ .font-unit-select`).value
-  };
-}
-
 async function testKeywordValues(inspector, viewDoc) {
   await selectNode(".bold-text", inspector);
 
   info("Check font-weight shows its computed style instead of the bold keyword value.");
   const fontWeight = getPropertyValue(viewDoc, "font-weight");
   isnot(fontWeight.value, "bold", "Font weight is not shown as keyword");
   is(fontWeight.value, "700", "Font weight is shown as computed style");
 
--- a/devtools/client/inspector/fonts/test/head.js
+++ b/devtools/client/inspector/fonts/test/head.js
@@ -32,25 +32,26 @@ selectNode = async function(node, inspec
 
 /**
  * Adds a new tab with the given URL, opens the inspector and selects the
  * font-inspector tab.
  * @return {Promise} resolves to a {toolbox, inspector, view} object
  */
 var openFontInspectorForURL = async function(url) {
   const tab = await addTab(url);
-  const {toolbox, inspector} = await openInspector();
+  const {toolbox, inspector, testActor } = await openInspector();
 
   // Call selectNode again here to force a fontinspector update since we don't
   // know if the fontinspector-updated event has been sent while the inspector
   // was being opened or not.
   await selectNode("body", inspector);
 
   return {
     tab,
+    testActor,
     toolbox,
     inspector,
     view: inspector.fontinspector
   };
 };
 
 /**
  * Focus one of the preview inputs, clear it, type new text into it and wait for the
@@ -199,8 +200,50 @@ function getURL(fontEl) {
  * @param  {DOMNode} fontEl
  *         The font element.
  * @return {String}
  *         The name of the font family as shown in the UI.
  */
 function getFamilyName(fontEl) {
   return fontEl.querySelector(".font-family-name").textContent;
 }
+
+/**
+ * Get the value and unit of a CSS font property or font axis from the font editor.
+ *
+ * @param  {document} viewDoc
+ *         Host document of the font inspector panel.
+ * @param  {String} name
+ *         Font property name or axis tag
+ * @return {Object}
+ *         Object with the value and unit of the given font property or axis tag
+ *         from the corresponding input fron the font editor.
+ *         @Example:
+ *         {
+ *          value: {Number|String|null}
+ *          unit: {String|null}
+ *         }
+ */
+function getPropertyValue(viewDoc, name) {
+  const selector = `#font-editor .font-value-slider[name=${name}]`;
+  return {
+    // Ensure value input exists before querying its value
+    value: viewDoc.querySelector(selector) &&
+           parseFloat(viewDoc.querySelector(selector).value),
+    // Ensure unit dropdown exists before querying its value
+    unit: viewDoc.querySelector(selector + ` ~ .font-unit-select`) &&
+          viewDoc.querySelector(selector + ` ~ .font-unit-select`).value
+  };
+}
+
+/**
+ * Wait for a predicate to return a result.
+ *
+ * @param  {Function} condition
+ *         Invoked every 10ms for a maximum of 500 retries until it returns a truthy
+ *         value.
+ * @return {Promise}
+ *         A promise that is resolved with the result of the condition.
+ */
+async function waitFor(condition) {
+  await BrowserTestUtils.waitForCondition(condition, "waitFor", 10, 500);
+  return condition();
+}