Bug 1483929 - (Part 1) Remove Rendered Fonts accordion; Add All Fonts accordion for Font Editor. r=gl
authorRazvan Caliman <rcaliman@mozilla.com>
Tue, 21 Aug 2018 18:20:50 +0000
changeset 433145 4a4b4a9b58d3d05942fcf843e5d4113f609261d6
parent 433028 77c50281ad2b074a52797cba560444a5585d339c
child 433146 9b918e1a1de5238d0f3b82f9d3a2dde4cbe2ffba
push id34500
push usertoros@mozilla.com
push dateFri, 24 Aug 2018 09:42:33 +0000
treeherdermozilla-central@043aff7fda61 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgl
bugs1483929
milestone63.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 1483929 - (Part 1) Remove Rendered Fonts accordion; Add All Fonts accordion for Font Editor. r=gl This patch replaces the Other Fonts and Rendered Fonts accordions with a single All Fonts accordion while the Font Editor pref is on. When thhe pref for the Font Editor is off, the Element Fonts and Other Fonts accordions are kept until we deprecate the old Font Inspector code. Differential Revision: https://phabricator.services.mozilla.com/D3797
devtools/client/inspector/fonts/actions/fonts.js
devtools/client/inspector/fonts/components/FontOverview.js
devtools/client/inspector/fonts/fonts.js
devtools/client/inspector/fonts/reducers/fonts.js
devtools/client/inspector/fonts/types.js
devtools/client/locales/en-US/font-inspector.properties
--- a/devtools/client/inspector/fonts/actions/fonts.js
+++ b/devtools/client/inspector/fonts/actions/fonts.js
@@ -8,17 +8,18 @@ const {
   UPDATE_FONTS,
 } = require("./index");
 
 module.exports = {
 
   /**
    * Update the list of fonts in the font inspector
    */
-  updateFonts(fonts, otherFonts) {
+  updateFonts(fonts, otherFonts, allFonts) {
     return {
       type: UPDATE_FONTS,
       fonts,
       otherFonts,
+      allFonts,
     };
   },
 
 };
--- a/devtools/client/inspector/fonts/components/FontOverview.js
+++ b/devtools/client/inspector/fonts/components/FontOverview.js
@@ -38,88 +38,73 @@ class FontOverview extends PureComponent
     const {
       fontData,
       fontOptions,
       onPreviewFonts,
       onToggleFontHighlight,
     } = this.props;
     const { fonts } = fontData;
 
-    // If the font editor is enabled, show the fonts in a collapsed accordion.
-    // The editor already displays fonts, in another way, rendering twice is not desired.
-    if (Services.prefs.getBoolPref(PREF_FONT_EDITOR)) {
-      return fonts.length ?
-        Accordion({
-          items: [
-            {
-              header: getStr("fontinspector.renderedFontsInPageHeader"),
-              component: FontList,
-              componentProps: {
-                fonts,
-                fontOptions,
-                onPreviewFonts,
-                onToggleFontHighlight,
-              },
-              opened: false
-            }
-          ]
-        })
-        :
-        null;
-    }
-
     return fonts.length ?
       FontList({
         fonts,
         fontOptions,
         onPreviewFonts,
         onToggleFontHighlight,
       })
       :
       dom.div(
         {
           className: "devtools-sidepanel-no-result"
         },
         getStr("fontinspector.noFontsOnSelectedElement")
       );
   }
 
-  renderOtherFonts() {
+  renderFonts() {
     const {
       fontData,
       fontOptions,
       onPreviewFonts,
     } = this.props;
-    const { otherFonts } = fontData;
+
+    const header = Services.prefs.getBoolPref(PREF_FONT_EDITOR)
+      ? getStr("fontinspector.allFontsOnPageHeader")
+      : getStr("fontinspector.otherFontsInPageHeader");
 
-    if (!otherFonts.length) {
+    const fonts = Services.prefs.getBoolPref(PREF_FONT_EDITOR)
+      ? fontData.allFonts
+      : fontData.otherFonts;
+
+    if (!fonts.length) {
       return null;
     }
 
     return Accordion({
       items: [
         {
-          header: getStr("fontinspector.otherFontsInPageHeader"),
+          header,
           component: FontList,
           componentProps: {
             fontOptions,
-            fonts: otherFonts,
+            fonts,
             onPreviewFonts,
             onToggleFontHighlight: this.onToggleFontHighlightGlobal
           },
           opened: false
         }
       ]
     });
   }
 
   render() {
     return dom.div(
       {
         id: "font-container",
       },
-      this.renderElementFonts(),
-      this.renderOtherFonts()
+      // Render element fonts only when the Font Editor is not enabled.
+      !Services.prefs.getBoolPref(PREF_FONT_EDITOR) && this.renderElementFonts(),
+      this.renderFonts()
     );
   }
 }
 
 module.exports = FontOverview;
--- a/devtools/client/inspector/fonts/fonts.js
+++ b/devtools/client/inspector/fonts/fonts.js
@@ -121,33 +121,16 @@ class FontInspector {
     // @see ToolSidebar.onSidebarTabSelected()
     this.inspector.sidebar.on("fontinspector-selected", this.onNewNode);
 
     // Listen for theme changes as the color of the previews depend on the theme
     gDevTools.on("theme-switched", this.onThemeChanged);
   }
 
   /**
-   * Given all fonts on the page, and given the fonts used in given node, return all fonts
-   * not from the page not used in this node.
-   *
-   * @param  {Array} allFonts
-   *         All fonts used on the entire page
-   * @param  {Array} nodeFonts
-   *         Fonts used only in one of the nodes
-   * @return {Array}
-   *         All fonts, except the ones used in the current node
-   */
-  excludeNodeFonts(allFonts, nodeFonts) {
-    return allFonts.filter(font => {
-      return !nodeFonts.some(nodeFont => nodeFont.name === font.name);
-    });
-  }
-
-  /**
    * Convert a value for font-size between two CSS unit types.
    * Conversion is done via pixels. If neither of the two given unit types is "px",
    * recursively get the value in pixels, then convert that result to the desired unit.
    *
    * @param  {String} property
    *         Property name for the converted value.
    *         Assumed to be "font-size", but special case for "line-height".
    * @param  {Number} value
@@ -446,28 +429,28 @@ class FontInspector {
       await this.pageStyle.getUsedFontFaces(node, options).catch(console.error);
     if (!fonts) {
       return [];
     }
 
     return fonts;
   }
 
-  async getFontsNotInNode(nodeFonts, options) {
+  async getAllFonts(options) {
     // In case we've been destroyed in the meantime
     if (!this.document) {
       return [];
     }
 
     let allFonts = await this.pageStyle.getAllUsedFontFaces(options).catch(console.error);
     if (!allFonts) {
       allFonts = [];
     }
 
-    return this.excludeNodeFonts(allFonts, nodeFonts);
+    return allFonts;
   }
 
   /**
    * Get a reference to a TextProperty instance from the current selected rule for a
    * given property name.
    *
    * @param {String} name
    *        CSS property name
@@ -1000,21 +983,22 @@ class FontInspector {
   }
 
   async update() {
     // Stop refreshing if the inspector or store is already destroyed.
     if (!this.inspector || !this.store) {
       return;
     }
 
+    let allFonts = [];
     let fonts = [];
     let otherFonts = [];
 
     if (!this.isSelectedNodeValid()) {
-      this.store.dispatch(updateFonts(fonts, otherFonts));
+      this.store.dispatch(updateFonts(fonts, otherFonts, allFonts));
       return;
     }
 
     const { fontOptions } = this.store.getState();
     const { previewText } = fontOptions;
 
     const options = {
       includePreviews: true,
@@ -1024,36 +1008,40 @@ class FontInspector {
 
     // Add the includeVariations argument into the option to get font variation data.
     if (this.pageStyle.supportsFontVariations) {
       options.includeVariations = true;
     }
 
     const node = this.inspector.selection.nodeFront;
     fonts = await this.getFontsForNode(node, options);
-    otherFonts = await this.getFontsNotInNode(fonts, options);
+    allFonts = await this.getAllFonts(options);
+    // Get the subset of fonts from the page which are not used on the selected node.
+    otherFonts = allFonts.filter(font => {
+      return !fonts.some(nodeFont => nodeFont.name === font.name);
+    });
 
     if (!fonts.length && !otherFonts.length) {
       // No fonts to display. Clear the previously shown fonts.
       if (this.store) {
-        this.store.dispatch(updateFonts(fonts, otherFonts));
+        this.store.dispatch(updateFonts(fonts, otherFonts, allFonts));
       }
       return;
     }
 
-    for (const font of [...fonts, ...otherFonts]) {
+    for (const font of [...allFonts]) {
       font.previewUrl = await font.preview.data.string();
     }
 
     // in case we've been destroyed in the meantime
     if (!this.document) {
       return;
     }
 
-    this.store.dispatch(updateFonts(fonts, otherFonts));
+    this.store.dispatch(updateFonts(fonts, otherFonts, allFonts));
 
     this.inspector.emit("fontinspector-updated");
   }
 
   /**
    * Update the "font-variation-settings" CSS property with the state of all touched
    * font variation axes which shouldn't be written to other CSS font properties.
    */
--- a/devtools/client/inspector/fonts/reducers/fonts.js
+++ b/devtools/client/inspector/fonts/reducers/fonts.js
@@ -4,24 +4,28 @@
 
 "use strict";
 
 const {
   UPDATE_FONTS,
 } = require("../actions/index");
 
 const INITIAL_FONT_DATA = {
+  // All fonts on the current page.
+  allFonts: [],
+  // Fonts used on the selected element.
   fonts: [],
-  otherFonts: []
+  // Fonts on the current page not used on the selected element.
+  otherFonts: [],
 };
 
 const reducers = {
 
-  [UPDATE_FONTS](_, { fonts, otherFonts }) {
-    return { fonts, otherFonts };
+  [UPDATE_FONTS](_, { fonts, otherFonts, allFonts }) {
+    return { fonts, otherFonts, allFonts };
   },
 
 };
 
 module.exports = function(fontData = INITIAL_FONT_DATA, action) {
   const reducer = reducers[action.type];
   if (!reducer) {
     return fontData;
--- a/devtools/client/inspector/fonts/types.js
+++ b/devtools/client/inspector/fonts/types.js
@@ -110,14 +110,17 @@ exports.fontEditor = {
   // CSS font properties defined on the element
   properties: PropTypes.object,
 };
 
 /**
  * Font data.
  */
 exports.fontData = {
+  // All fonts on the current page.
+  allFonts: PropTypes.arrayOf(PropTypes.shape(font)),
+
   // The fonts used in the current element.
   fonts: PropTypes.arrayOf(PropTypes.shape(font)),
 
   // Fonts used elsewhere.
   otherFonts: PropTypes.arrayOf(PropTypes.shape(font)),
 };
--- a/devtools/client/locales/en-US/font-inspector.properties
+++ b/devtools/client/locales/en-US/font-inspector.properties
@@ -12,21 +12,16 @@ fontinspector.system=system
 # LOCALIZATION NOTE (fontinspector.noFontsOnSelectedElement): This label is shown when
 # no fonts found on the selected element.
 fontinspector.noFontsOnSelectedElement=No fonts were found for the current element.
 
 # LOCALIZATION NOTE (fontinspector.otherFontsInPageHeader): This is the text for the
 # header of a collapsible section containing other fonts used in the page.
 fontinspector.otherFontsInPageHeader=Other fonts in page
 
-# LOCALIZATION NOTE (fontinspector.renderedFontsInPageHeader): This is the text for the
-# header of a collapsible section containing the fonts used to render the content of the
-# selected element.
-fontinspector.renderedFontsInPageHeader=Rendered fonts
-
 # LOCALIZATION NOTE (fontinspector.editPreview): This is the text that appears in a
 # tooltip on hover of a font preview string. Clicking on the string opens a text input
 # where users can type to change the preview text.
 fontinspector.editPreview=Click to edit preview
 
 # LOCALIZATION NOTE (fontinspector.copyURL): This is the text that appears in a tooltip
 # displayed when the user hovers over the copy icon next to the font URL.
 # Clicking the copy icon copies the full font URL to the user's clipboard
@@ -72,8 +67,12 @@ fontinspector.showMore=Show more
 fontinspector.showLess=Show less
 
 # LOCALIZATION NOTE (fontinspector.noPseduoWarning): Warning message shown when the
 # selected element is a pseudo-element that the font editor cannot work with.
 fontinspector.noPseduoWarning=Pseudo-elements are not currently supported.
 
 # LOCALIZATION NOTE (fontinspector.lineHeightLabel): Label for the UI to change the line height in the font editor.
 fontinspector.lineHeightLabel=Line height
+
+# LOCALIZATION NOTE (fontinspector.allFontsOnPageHeader): Header for the section listing
+# all the fonts on the current page.
+fontinspector.allFontsOnPageHeader=All fonts on page