Bug 1463055 - Font editor: read properties from computed style; write to inline style. r=pbro draft
authorRazvan Caliman <rcaliman@mozilla.com>
Mon, 21 May 2018 10:24:59 +0200
changeset 797789 7611eb6ef9511963bf7da25acd3cbbfff5a28c38
parent 797788 c5dd7e81b9e8d9c0f424727ee90f970cfe20b9f6
child 797790 c576ced8c70d28a2e58127e998eff3737803bd90
push id110568
push userbmo:rcaliman@mozilla.com
push dateMon, 21 May 2018 16:43:03 +0000
reviewerspbro
bugs1463055
milestone62.0a1
Bug 1463055 - Font editor: read properties from computed style; write to inline style. r=pbro MozReview-Commit-ID: 6sHeBgfbiDT
devtools/client/inspector/fonts/fonts.js
devtools/client/inspector/fonts/test/browser.ini
devtools/client/inspector/fonts/test/browser_fontinspector_text-node.js
--- a/devtools/client/inspector/fonts/fonts.js
+++ b/devtools/client/inspector/fonts/fonts.js
@@ -8,16 +8,17 @@
 
 const Services = require("Services");
 const { gDevTools } = require("devtools/client/framework/devtools");
 const { getColor } = require("devtools/client/shared/theme");
 const { createFactory, createElement } = require("devtools/client/shared/vendor/react");
 const { Provider } = require("devtools/client/shared/vendor/react-redux");
 const { throttle } = require("devtools/shared/throttle");
 const { debounce } = require("devtools/shared/debounce");
+const { ELEMENT_STYLE } = require("devtools/shared/specs/styles");
 
 const FontsApp = createFactory(require("./components/FontsApp"));
 
 const { LocalizationHelper } = require("devtools/shared/l10n");
 const INSPECTOR_L10N =
   new LocalizationHelper("devtools/client/locales/inspector.properties");
 
 const { getStr } = require("./utils/l10n");
@@ -152,39 +153,27 @@ class FontInspector {
     this.store = null;
     this.textProperties.clear();
     this.textProperties = null;
     this.writers.clear();
     this.writers = null;
   }
 
   /**
-   * Collect all the expected CSS font properties and their values for the selected node
-   * from the node's computed style and from all the rules that apply to it.
+   * Get all expected CSS font properties and values from the node's computed style.
    *
    * @return {Object}
    */
   getFontProperties() {
     let properties = {};
 
-    // First, get all expected font properties from computed styles.
     for (let prop of FONT_PROPERTIES) {
       properties[prop] = this.nodeComputedStyle[prop].value;
     }
 
-    // Then, replace with enabled font properties found on any of the rules that apply.
-    for (let rule of this.ruleView.rules) {
-      for (let textProp of rule.textProps) {
-        if (FONT_PROPERTIES.includes(textProp.name) && textProp.enabled &&
-            !textProp.overridden) {
-          properties[textProp.name] = textProp.value;
-        }
-      }
-    }
-
     return properties;
   }
 
   async getFontsForNode(node, options) {
     // In case we've been destroyed in the meantime
     if (!this.document) {
       return [];
     }
@@ -338,17 +327,19 @@ class FontInspector {
     } else {
       this.writers.set(name, this.updateFontVariationSettings);
     }
 
     return this.writers.get(name);
   }
 
   /**
-   * Returns true if the font inspector panel is visible, and false otherwise.
+   * Check if the font inspector panel is visible.
+   *
+   * @return {Boolean}
    */
   isPanelVisible() {
     return this.inspector.sidebar &&
            this.inspector.sidebar.getCurrentTabID() === "fontinspector";
   }
   /**
    * Check if a selected node exists and fonts can apply to it.
    *
@@ -527,22 +518,24 @@ class FontInspector {
     if (frame === this.document.defaultView) {
       this.update();
     }
   }
 
   /**
    * Update the state of the font editor with:
    * - the fonts which apply to the current node;
-   * - the CSS font properties declared on the selected rule.
+   * - the computed style CSS font properties of the current node.
    *
-   * This method is called during initial setup and as a result of any property
-   * values change in the Rule view. For the latter case, we do a deep compare between the
-   * font properties on the selected rule and the ones already store to decide if to
-   * update the font edtior to reflect a new external state.
+   * This method is called:
+   * - during initial setup;
+   * - when a new node is selected;
+   * - when any property is changed in the Rule view.
+   * For the latter case, we compare between the latest computed style font properties
+   * and the ones already in the store to decide if to update the font editor state.
    */
   async refreshFontEditor() {
     // Early return if pref for font editor is not enabled.
     if (!Services.prefs.getBoolPref(PREF_FONT_EDITOR)) {
       return;
     }
 
     if (!this.inspector || !this.store || !this.isSelectedNodeValid()) {
@@ -552,25 +545,32 @@ 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 all computed styles for selected node; used for identifying inherited values.
+    // Get computed styles for the selected node, but filter by CSS font properties.
     this.nodeComputedStyle = await this.pageStyle.getComputed(node, {
       filterProperties: FONT_PROPERTIES
     });
+    // Clear any references to writer methods and CSS declarations because the node's
+    // styles may have changed since the last font editor refresh.
+    this.writers.clear();
+    this.textProperties.clear();
+    // Select the node's inline style as the rule where to write property value changes.
+    this.selectedRule =
+      this.ruleView.rules.find(rule => rule.style.type === ELEMENT_STYLE);
     const fontEditor = this.store.getState().fontEditor;
     const properties = this.getFontProperties();
 
-    // Update the font editor state only if property values in rule differ from store.
-    // This can happen when a user makes manual edits to the values in the rule view.
+    // Update the font editor state only if property values differ from the ones in store.
+    // This can happen when a user makes manual changes in the Rule view.
     if (JSON.stringify(properties) !== JSON.stringify(fontEditor.properties)) {
       this.store.dispatch(updateFontEditor(fonts, properties));
     }
   }
 
   /**
    * Capture the state of all variation axes. Allows the user to return to this state with
    * the "Custom" instance after they've selected a font-defined named variation instance.
@@ -589,24 +589,17 @@ class FontInspector {
     let node = this.inspector.selection.nodeFront;
 
     let fonts = [];
     let otherFonts = [];
 
     let { fontOptions } = this.store.getState();
     let { previewText } = fontOptions;
 
-    // Clear the list of fonts if the currently selected node is not connected or a text
-    // or element node unless all fonts are supposed to be shown.
-    let isElementOrTextNode = this.inspector.selection.isElementNode() ||
-                              this.inspector.selection.isTextNode();
-    if (!node ||
-        !this.isPanelVisible() ||
-        !this.inspector.selection.isConnected() ||
-        !isElementOrTextNode) {
+    if (!this.isSelectedNodeValid() || !this.isPanelVisible()) {
       this.store.dispatch(updateFonts(fonts, otherFonts));
       return;
     }
 
     let options = {
       includePreviews: true,
       previewText,
       previewFillStyle: getColor("body-color")
--- a/devtools/client/inspector/fonts/test/browser.ini
+++ b/devtools/client/inspector/fonts/test/browser.ini
@@ -18,10 +18,9 @@ support-files =
 [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_expand-css-code.js]
 [browser_fontinspector_other-fonts.js]
 [browser_fontinspector_reveal-in-page.js]
-[browser_fontinspector_text-node.js]
 [browser_fontinspector_theme-change.js]
deleted file mode 100644
--- a/devtools/client/inspector/fonts/test/browser_fontinspector_text-node.js
+++ /dev/null
@@ -1,22 +0,0 @@
-/* vim: set ts=2 et sw=2 tw=80: */
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-"use strict";
-
-// Test that the font-inspector also display fonts when a text node is selected.
-
-const TEST_URI = URL_ROOT + "browser_fontinspector.html";
-
-add_task(async function() {
-  let { inspector, view } = await openFontInspectorForURL(TEST_URI);
-  let viewDoc = view.document;
-
-  info("Selecting the first text-node first-child of <body>");
-  let bodyNode = await getNodeFront("body", inspector);
-  let { nodes } = await inspector.walker.children(bodyNode);
-  await selectNode(nodes[0], inspector);
-
-  let lis = getUsedFontsEls(viewDoc);
-  is(lis.length, 1, "Font inspector shows 1 font");
-});