Bug 1463055 - Font editor: read properties from computed style; write to inline style. r=pbro
authorRazvan Caliman <rcaliman@mozilla.com>
Mon, 21 May 2018 10:24:59 +0200
changeset 419188 5bc4309db28c144e80444a326ad33c2517233f71
parent 419187 ed8005f5f297cf477b71677b469763aaf7e2b07c
child 419189 c13beb8635004b99e9b04dbe7e850d9b5086c682
push id34031
push usernbeleuzu@mozilla.com
push dateTue, 22 May 2018 09:47:31 +0000
treeherdermozilla-central@ac1c5c363e29 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro
bugs1463055
milestone62.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 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");
-});