Bug 1526054 - Set target node in Font Editor according to selected node type. r=gl
☠☠ backed out by 6fe891a5de1a ☠ ☠
authorRazvan Caliman <rcaliman@mozilla.com>
Wed, 27 Feb 2019 17:40:49 +0000
changeset 519363 19b29b2e2f5e651518b4ca8e111c3c1dcf61b8c7
parent 519362 85c4d0c94938bca534fce92b1ed55a658fdb98dd
child 519364 31a79abb583d5a0c98be5bef9f20058908ee4d58
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgl
bugs1526054
milestone67.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 1526054 - Set target node in Font Editor according to selected node type. r=gl Refactors the logic so the target node on which the Font Editor operates can point to a parent node in case of text nodes without explicitly changing the node selection. The target node is assigned to `this.node`. When that is null, it means the node selection is not supported by the Font Editor. This removes the need for the `isSelectedNodeValid()` method. Differential Revision: https://phabricator.services.mozilla.com/D21387
devtools/client/inspector/fonts/fonts.js
devtools/client/inspector/fonts/test/browser_fontinspector_text-node.js
--- a/devtools/client/inspector/fonts/fonts.js
+++ b/devtools/client/inspector/fonts/fonts.js
@@ -57,16 +57,19 @@ const HISTOGRAM_FONT_TYPE_DISPLAYED = "D
 
 class FontInspector {
   constructor(inspector, window) {
     this.cssProperties = inspector.cssProperties;
     this.document = window.document;
     this.inspector = inspector;
     // Set of unique keyword values supported by designated font properties.
     this.keywordValues = new Set(this.getFontPropertyValueKeywords());
+    // Selected node in the markup view. For text nodes, this points to their parent node
+    // element. Font faces and font properties for this node will be shown in the editor.
+    this.node = null;
     this.nodeComputedStyle = {};
     this.pageStyle = this.inspector.pageStyle;
     this.ruleViewTool = this.inspector.getPanel("ruleview");
     this.ruleView = this.ruleViewTool.view;
     this.selectedRule = null;
     this.store = this.inspector.store;
     // Map CSS property names and variable font axis names to methods that write their
     // corresponding values to the appropriate TextProperty from the Rule view.
@@ -157,18 +160,18 @@ class FontInspector {
       value = await this.convertUnits(property, value, fromUnit, "px");
       fromUnit = "px";
     }
 
     // Whether the conversion is done from pixels.
     const fromPx = fromUnit === "px";
     // Determine the target CSS unit for conversion.
     const unit = toUnit === "px" ? fromUnit : toUnit;
-    // NodeFront instance of selected element.
-    const node = this.inspector.selection.nodeFront;
+    // NodeFront instance of selected/target element.
+    const node = this.node;
     // Reference node based on which to convert relative sizes like "em" and "%".
     const referenceNode = (property === "line-height") ? node : node.parentNode();
     // Default output value to input value for a 1-to-1 conversion as a guard against
     // unrecognized CSS units. It will not be correct, but it will also not break.
     let out = value;
     // Computed style for reference node used for conversion of "em", "rem", "%".
     let computedStyle;
 
@@ -291,16 +294,17 @@ class FontInspector {
   destroy() {
     this.inspector.selection.off("new-node-front", this.onNewNode);
     this.inspector.sidebar.off("fontinspector-selected", this.onNewNode);
     this.ruleView.off("property-value-updated", this.onRulePropertyUpdated);
     gDevTools.off("theme-switched", this.onThemeChanged);
 
     this.document = null;
     this.inspector = null;
+    this.node = null;
     this.nodeComputedStyle = {};
     this.pageStyle = null;
     this.ruleView = null;
     this.selectedRule = null;
     this.store = null;
     this.writers.clear();
     this.writers = null;
   }
@@ -535,27 +539,16 @@ class FontInspector {
    *
    * @return {Boolean}
    */
   isPanelVisible() {
     return this.inspector &&
            this.inspector.sidebar &&
            this.inspector.sidebar.getCurrentTabID() === "fontinspector";
   }
-  /**
-   * Check if a selected node exists and fonts can apply to it.
-   *
-   * @return {Boolean}
-   */
-  isSelectedNodeValid() {
-    return this.inspector &&
-           this.inspector.selection.nodeFront &&
-           this.inspector.selection.isConnected() &&
-           this.inspector.selection.isElementNode();
-  }
 
   /**
    * Upon a new node selection, log some interesting telemetry probes.
    */
   logTelemetryProbesOnNewNode() {
     const { fontEditor } = this.store.getState();
     const { telemetry } = this.inspector;
 
@@ -649,20 +642,41 @@ class FontInspector {
     let writer;
     values.map(obj => {
       writer = this.getWriterForProperty(obj.axis);
       writer(obj.value.toString());
     });
   }
 
   /**
-   * Selection 'new-node' event handler.
+   * Event handler for "new-node-front" event fired when a new node is selected in the
+   * markup view.
+   *
+   * Sets the selected node for which font faces and font properties will be
+   * shown in the font editor. If the selection is a text node, use its parent element.
+   *
+   * Triggers a refresh of the font editor and font overview if the panel is visible.
    */
   onNewNode() {
     this.ruleView.off("property-value-updated", this.onRulePropertyUpdated);
+    // First, reset the selected node.
+    this.node = null;
+    // Then attempt to assign a selected node according to its type.
+    const selection = this.inspector && this.inspector.selection;
+    if (selection && selection.isConnected()) {
+
+      if (selection.isElementNode()) {
+        this.node = selection.nodeFront;
+      }
+
+      if (selection.isTextNode()) {
+        this.node = selection.nodeFront.parentNode();
+      }
+    }
+
     if (this.isPanelVisible()) {
       Promise.all([this.update(), this.refreshFontEditor()]).then(() => {
         this.logTelemetryProbesOnNewNode();
       }).catch(e => console.error(e));
     }
   }
 
   /**
@@ -748,19 +762,17 @@ class FontInspector {
         // calls to the function.
         this.onToggleFontHighlight = () => {};
         return;
       }
     }
 
     try {
       if (show) {
-        const node = isForCurrentElement
-                   ? this.inspector.selection.nodeFront
-                   : this.inspector.walker.rootNode;
+        const node = isForCurrentElement ? this.node : this.inspector.walker.rootNode;
 
         await this.fontsHighlighter.show(node, {
           CSSFamilyName: font.CSSFamilyName,
           name: font.name,
         });
       } else {
         await this.fontsHighlighter.hide();
       }
@@ -780,46 +792,37 @@ class FontInspector {
   }
 
   /**
    * Update the state of the font editor with:
    * - the fonts which apply to the current node;
    * - the computed style CSS font properties of the current node.
    *
    * 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() {
-    if (!this.store || !this.isSelectedNodeValid()) {
-      // If the selection is a TextNode, switch selection to be its parent node.
-      if (this.inspector.selection.isTextNode()) {
-        const selection = this.inspector.selection;
-        selection.setNodeFront(selection.nodeFront.parentNode());
-        return;
-      }
-
+    if (!this.node) {
       this.store.dispatch(resetFontEditor());
       return;
     }
 
     const options = {};
     if (this.pageStyle.supportsFontVariations) {
       options.includeVariations = true;
     }
 
-    const node = this.inspector.selection.nodeFront;
-    const fonts = await this.getFontsForNode(node, options);
+    const fonts = await this.getFontsForNode(this.node, options);
 
     try {
       // Get computed styles for the selected node, but filter by CSS font properties.
-      this.nodeComputedStyle = await this.pageStyle.getComputed(node, {
+      this.nodeComputedStyle = await this.pageStyle.getComputed(this.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;
@@ -837,33 +840,32 @@ class FontInspector {
     // 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();
 
     // If the Rule panel is not visible, the selected element's rule models may not have
     // been created yet. For example, in 2-pane mode when Fonts is opened as the default
     // panel. Select the current node to force the Rule view to create the rule models.
     if (!this.ruleViewTool.isSidebarActive()) {
-      await this.ruleView.selectElement(node, false);
+      await this.ruleView.selectElement(this.node, false);
     }
 
     // Select the node's inline style as the rule where to write property value changes.
     this.selectedRule =
       this.ruleView.rules.find(rule => rule.domRule.type === ELEMENT_STYLE);
 
     const properties = await this.getFontProperties();
     // Assign writer methods to each axis defined in font-variation-settings.
     const axes = parseFontVariationAxes(properties["font-variation-settings"]);
     Object.keys(axes).map(axis => {
       this.writers.set(axis, this.getWriterForAxis(axis));
     });
 
-    this.store.dispatch(updateFontEditor(fonts, properties, node.actorID));
-    const isPseudo = this.inspector.selection.isPseudoElementNode();
-    this.store.dispatch(setEditorDisabled(isPseudo));
+    this.store.dispatch(updateFontEditor(fonts, properties, this.node.actorID));
+    this.store.dispatch(setEditorDisabled(this.node.isPseudoElement));
 
     this.inspector.emit("fonteditor-updated");
     // Listen to manual changes in the Rule view that could update the Font Editor state
     this.ruleView.on("property-value-updated", this.onRulePropertyUpdated);
   }
 
   /**
    * Capture the state of all variation axes. Allows the user to return to this state with
@@ -877,17 +879,17 @@ class FontInspector {
   async update() {
     // Stop refreshing if the inspector or store is already destroyed.
     if (!this.inspector || !this.store) {
       return;
     }
 
     let allFonts = [];
 
-    if (!this.isSelectedNodeValid()) {
+    if (!this.node) {
       this.store.dispatch(updateFonts(allFonts));
       return;
     }
 
     const { fontOptions } = this.store.getState();
     const { previewText } = fontOptions;
 
     const options = {
--- a/devtools/client/inspector/fonts/test/browser_fontinspector_text-node.js
+++ b/devtools/client/inspector/fonts/test/browser_fontinspector_text-node.js
@@ -11,16 +11,17 @@ const TEST_URI = URL_ROOT + "doc_browser
 add_task(async function() {
   const { inspector, view } = await openFontInspectorForURL(TEST_URI);
   const viewDoc = view.document;
 
   info("Select the first text node of <body>");
   const bodyNode = await getNodeFront("body", inspector);
   const { nodes } = await inspector.walker.children(bodyNode);
   const onInspectorUpdated = inspector.once("fontinspector-updated");
+  info("Select the text node");
   await selectNode(nodes[0], inspector);
 
   info("Waiting for font editor to render");
   await onInspectorUpdated;
 
   const textFonts = getUsedFontsEls(viewDoc);
 
   info("Select the <body> element");