Bug 1464796 - (Part 2) Select declared and used fonts. Group font family names by used and not used. r=pbro draft
authorRazvan Caliman <rcaliman@mozilla.com>
Tue, 05 Jun 2018 12:22:52 +0200
changeset 808651 ba6d8d38cd897df2a4daf6deb5dcac13f4af3488
parent 808650 2eda1a2ab66b87a62ffc22c7750e7a323bb2adf3
child 808652 df376cd399136737e362c49b7beb6f7ac386b594
push id113452
push userbmo:rcaliman@mozilla.com
push dateWed, 20 Jun 2018 09:12:20 +0000
reviewerspbro
bugs1464796
milestone62.0a1
Bug 1464796 - (Part 2) Select declared and used fonts. Group font family names by used and not used. r=pbro MozReview-Commit-ID: 44RuWZBfzGX
devtools/client/inspector/fonts/actions/font-editor.js
devtools/client/inspector/fonts/components/FontEditor.js
devtools/client/inspector/fonts/fonts.js
devtools/client/inspector/fonts/reducers/font-editor.js
devtools/client/inspector/fonts/types.js
devtools/client/inspector/fonts/utils/font-utils.js
devtools/server/actors/styles.js
--- a/devtools/client/inspector/fonts/actions/font-editor.js
+++ b/devtools/client/inspector/fonts/actions/font-editor.js
@@ -38,20 +38,21 @@ module.exports = {
   updateAxis(axis, value) {
     return {
       type: UPDATE_AXIS_VALUE,
       axis,
       value,
     };
   },
 
-  updateFontEditor(fonts, properties = {}) {
+  updateFontEditor(fonts, families = { used: [], notUsed: [] }, properties = {}) {
     return {
       type: UPDATE_EDITOR_STATE,
       fonts,
+      families,
       properties,
     };
   },
 
   updateFontProperty(property, value) {
     return {
       type: UPDATE_PROPERTY_VALUE,
       property,
--- a/devtools/client/inspector/fonts/components/FontEditor.js
+++ b/devtools/client/inspector/fonts/components/FontEditor.js
@@ -178,22 +178,21 @@ class FontEditor extends PureComponent {
       ),
       instanceSelect
     );
   }
 
   render() {
     const { fontEditor, onToggleFontHighlight } = this.props;
     const { fonts, axes, instance, properties } = fontEditor;
-    const usedFonts = fonts.filter(font => font.used);
-    // If no used fonts were found, pick the first available font.
-    // Else, pick the first used font regardless of how many there are.
-    const font = usedFonts.length === 0 ? fonts[0] : usedFonts[0];
+    // Pick the first font to show editor controls regardless of how many fonts are used.
+    const font = fonts[0];
     const hasFontAxes = font && font.variationAxes;
-    const hasFontInstances = font && font.variationInstances.length > 0;
+    const hasFontInstances = font && font.variationInstances
+      && font.variationInstances.length > 0;
     const hasSlantOrItalicAxis = hasFontAxes && font.variationAxes.find(axis => {
       return axis.tag === "slnt" || axis.tag === "ital";
     });
     const hasWeightAxis = hasFontAxes && font.variationAxes.find(axis => {
       return axis.tag === "wght";
     });
 
     return dom.div(
--- a/devtools/client/inspector/fonts/fonts.js
+++ b/devtools/client/inspector/fonts/fonts.js
@@ -69,18 +69,18 @@ class FontInspector {
     this.writers = new Map();
 
     this.snapshotChanges = debounce(this.snapshotChanges, 100, this);
     this.syncChanges = debounce(this.syncChanges, 100, this);
     this.onInstanceChange = this.onInstanceChange.bind(this);
     this.onNewNode = this.onNewNode.bind(this);
     this.onPreviewFonts = this.onPreviewFonts.bind(this);
     this.onPropertyChange = this.onPropertyChange.bind(this);
+    this.onRuleUpdated = this.onRuleUpdated.bind(this);
     this.onToggleFontHighlight = this.onToggleFontHighlight.bind(this);
-    this.onRuleUpdated = this.onRuleUpdated.bind(this);
     this.onThemeChanged = this.onThemeChanged.bind(this);
     this.update = this.update.bind(this);
     this.updateFontVariationSettings = this.updateFontVariationSettings.bind(this);
 
     this.init();
   }
 
   init() {
@@ -103,17 +103,16 @@ class FontInspector {
     }, fontsApp);
 
     // Expose the provider to let inspector.js use it in setupSidebar.
     this.provider = provider;
 
     this.inspector.selection.on("new-node-front", this.onNewNode);
     // @see ToolSidebar.onSidebarTabSelected()
     this.inspector.sidebar.on("fontinspector-selected", this.onNewNode);
-    this.ruleView.on("property-value-updated", this.onRuleUpdated);
 
     // 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.
@@ -148,28 +147,76 @@ class FontInspector {
     this.ruleView = null;
     this.selectedRule = null;
     this.store = null;
     this.writers.clear();
     this.writers = null;
   }
 
   /**
+   * Get a subset of fonts used on a node whose font family names are found in the
+   * node's CSS font-family property value. The fonts will be sorted in the order their
+   * family names are declared in CSS font-family.
+   *
+   * Fonts returned by this.getFontsForNode() contain, among others, these attributes:
+   * - CSSFamilyName: a string of the font's family name (ex: "Times");
+   * - CSSGeneric: a string of the generic font family (ex: "serif", "sans-serif") if
+   * the font was resolved from a generic font family keyword, like serif, instead of
+   * an explicit font famly, like "Times". If the font is resolved from an
+   * explicit font family, CSSGeneric is null.
+   *
+   * For example:
+   * font-family: "Avenir", serif;
+   *
+   * If fonts from both families are used, it will yield:
+   * { CSSFamilyName: "Avenir", CSSGeneric: null, ... },
+   * { CSSFamilyName: "Times", CSSGeneric: "serif", ... },
+   *
+   * @param {Array} fonts
+   *        Fonts used on a node got from a call to this.getFontsForNode().
+   * @param {Array} fontFamilies
+   *        Strings of font families from a node's CSS font-family property value.
+   * @return {Array}
+   *         Subset of `fonts` whose font family names appear in `fontFamilies`.
+   */
+  filterFontsUsed(fonts = [], fontFamilies = []) {
+    return fontFamilies.reduce((acc, family) => {
+      const match = fonts.find(font => {
+        const generic = typeof font.CSSGeneric === "string"
+          ? font.CSSGeneric.toLowerCase()
+          : font.CSSGeneric;
+
+        return generic === family.toLowerCase()
+          || font.CSSFamilyName.toLowerCase() === family.toLowerCase();
+      });
+
+      if (match) {
+        acc.push(match);
+      }
+
+      return acc;
+    }, []);
+  }
+
+  /**
    * Get all expected CSS font properties and values from the node's matching rules and
    * fallback to computed style.
    *
    * @return {Object}
    */
   getFontProperties() {
     const KEYWORD_VALUES = ["initial", "inherit", "unset", "none"];
     const properties = {};
 
-    // First, get all expected font properties from computed styles.
+    // First, get all expected font properties from computed styles, if available.
     for (const prop of FONT_PROPERTIES) {
-      properties[prop] = this.nodeComputedStyle[prop].value;
+      properties[prop] =
+        (this.nodeComputedStyle[prop] && this.nodeComputedStyle[prop].value)
+          ? this.nodeComputedStyle[prop].value
+          : "";
     }
 
     // Then, replace with enabled font properties found on any of the rules that apply.
     for (const rule of this.ruleView.rules) {
       for (const textProp of rule.textProps) {
         if (FONT_PROPERTIES.includes(textProp.name) &&
             !KEYWORD_VALUES.includes(textProp.value) &&
             !textProp.value.includes("calc(") &&
@@ -338,31 +385,61 @@ class FontInspector {
     } else {
       this.writers.set(name, this.updateFontVariationSettings);
     }
 
     return this.writers.get(name);
   }
 
   /**
+   * Given a list of font families, return an object that groups them into sets of used
+   * and not used if they match families of fonts from the given list of fonts used on a
+   * node.
+   *
+   * @See this.filterFontsUsed() for an explanation of CSSFamilyName and CSSGeneric.
+   *
+   * @param {Array} fontsUsed
+   *        Fonts used on a node.
+   * @param {Array} fontFamilies
+   *        Strings of font families
+   * @return {Object}
+   */
+  groupFontFamilies(fontsUsed = [], fontFamilies = []) {
+    const families = {};
+    // Font family names declared and used.
+    families.used = fontsUsed.map(font =>
+      font.CSSGeneric ? font.CSSGeneric : font.CSSFamilyName
+    );
+    const familiesUsedLowercase = families.used.map(family => family.toLowerCase());
+    // Font family names declared but not used.
+    families.notUsed = fontFamilies
+      .map(family => family.toLowerCase())
+      .filter(family => !familiesUsedLowercase.includes(family));
+
+    return families;
+  }
+
+  /**
    * Check if the font inspector panel is visible.
    *
    * @return {Boolean}
    */
   isPanelVisible() {
-    return this.inspector.sidebar &&
+    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.selection.nodeFront &&
+    return this.inspector &&
+           this.inspector.selection.nodeFront &&
            this.inspector.selection.isConnected() &&
            this.inspector.selection.isElementNode();
   }
 
   /**
    * Sync the Rule view with the latest styles from the page. Called in a debounced way
    * (see constructor) after property changes are applied directly to the CSS style rule
    * on the page circumventing direct TextProperty.setValue() which triggers expensive DOM
@@ -572,45 +649,44 @@ class FontInspector {
       this.store.dispatch(resetFontEditor());
       return;
     }
 
     // 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 because the node's
+
+    if (!this.nodeComputedStyle) {
+      this.store.dispatch(resetFontEditor());
+      return;
+    }
+
+    // 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();
     // 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 fontEditor = this.store.getState().fontEditor;
+
     const properties = this.getFontProperties();
-    // Names of fonts declared in font-family property without quotes and space trimmed.
-    const declaredFontNames =
-      properties["font-family"].split(",").map(font => font.replace(/\"+/g, "").trim());
-
-    // Mark available fonts as used if their names appears in the font-family declaration.
-    // TODO: sort used fonts in order of font-family declaration.
-    for (const font of fonts) {
-      font.used = declaredFontNames.includes(font.CSSFamilyName);
-    }
-
+    const familiesDeclared =
+      properties["font-family"].split(",")
+      .map(font => font.replace(/["']+/g, "").trim());
+    // Subset of fonts used on the node whose family names exist in CSS font-family.
+    const fontsUsed = this.filterFontsUsed(fonts, familiesDeclared);
+    // Object with font families groupped by used and not used.
+    const families = this.groupFontFamilies(fontsUsed, familiesDeclared);
     // 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));
     });
 
-    // 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));
-    }
+    this.store.dispatch(updateFontEditor(fontsUsed, families, 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.
    * This method is debounced. See constructor.
    */
   snapshotChanges() {
--- a/devtools/client/inspector/fonts/reducers/font-editor.js
+++ b/devtools/client/inspector/fonts/reducers/font-editor.js
@@ -16,17 +16,25 @@ const {
   UPDATE_PROPERTY_VALUE,
 } = require("../actions/index");
 
 const INITIAL_STATE = {
   // Variable font axes.
   axes: {},
   // Copy of the most recent axes values. Used to revert from a named instance.
   customInstanceValues: [],
-  // Fonts applicable to selected element.
+  // Font families declared on the selected element
+  families: {
+    // Names of font families used
+    used: [],
+    // Names of font families declared but not used
+    notUsed: []
+  },
+  // Fonts whose family names are declared in CSS font-family and used
+  // on the selected element.
   fonts: [],
   // Current selected font variation instance.
   instance: {
     name: getStr("fontinspector.customInstanceName"),
     values: [],
   },
   // CSS font properties defined on the selected rule.
   properties: {},
@@ -65,17 +73,17 @@ const reducers = {
   [UPDATE_CUSTOM_INSTANCE](state) {
     const newState = { ...state };
     newState.customInstanceValues = Object.keys(state.axes).map(axis => {
       return { axis: [axis], value: state.axes[axis] };
     });
     return newState;
   },
 
-  [UPDATE_EDITOR_STATE](state, { fonts, properties }) {
+  [UPDATE_EDITOR_STATE](state, { fonts, families, properties }) {
     const axes = parseFontVariationAxes(properties["font-variation-settings"]);
 
     // If not defined in font-variation-settings, setup "wght" axis with the value of
     // "font-weight" if it is numeric and not a keyword.
     const weight = properties["font-weight"];
     if (axes.wght === undefined && parseFloat(weight).toString() === weight.toString()) {
       axes.wght = weight;
     }
@@ -85,17 +93,17 @@ const reducers = {
     const stretch = properties["font-stretch"];
     // Match the number part from values like: 10%, 10.55%, 0.2%
     // If there's a match, the number is the second item in the match array.
     const match = stretch.trim().match(/^(\d+(.\d+)?)%$/);
     if (axes.wdth === undefined && match && match[1]) {
       axes.wdth = match[1];
     }
 
-    return { ...state, axes, fonts, properties };
+    return { ...state, axes, fonts, families, properties };
   },
 
   [UPDATE_PROPERTY_VALUE](state, { property, value }) {
     const newState = { ...state };
     newState.properties[property] = value;
     return newState;
   }
 
--- a/devtools/client/inspector/fonts/types.js
+++ b/devtools/client/inspector/fonts/types.js
@@ -74,25 +74,36 @@ const font = exports.font = {
   variationInstances: PropTypes.arrayOf(PropTypes.shape(fontVariationInstance))
 };
 
 exports.fontOptions = {
   // The current preview text
   previewText: PropTypes.string,
 };
 
+const fontFamilies = {
+  // Font family names used on the selected element
+  used: PropTypes.arrayOf(PropTypes.string),
+
+  // Font family names declared but not used on the selected element
+  notUsed: PropTypes.arrayOf(PropTypes.string),
+};
+
 exports.fontEditor = {
   // Variable font axes and their values
   axes: PropTypes.object,
 
   // Axes values changed at runtime structured like the "values" property
   // of a fontVariationInstance
   customInstanceValues: PropTypes.array,
 
-  // Fonts applicable to selected element
+  // Font families declared on this element
+  families: PropTypes.shape(fontFamilies),
+
+  // Fonts used on the selected element whose family names are declared in CSS font-family
   fonts: PropTypes.arrayOf(PropTypes.shape(font)),
 
   // Font variation instance currently selected
   instance: PropTypes.shape(fontVariationInstance),
 
   // CSS font properties defined on the element
   properties: PropTypes.object,
 };
--- a/devtools/client/inspector/fonts/utils/font-utils.js
+++ b/devtools/client/inspector/fonts/utils/font-utils.js
@@ -51,27 +51,32 @@ module.exports = {
    *        Its contents are expected to be stable having been already parsed by the
    *        browser.
    * @return {Object}
    */
   parseFontVariationAxes(string) {
     let axes = {};
     const keywords = ["initial", "normal", "inherit", "unset"];
 
-    if (keywords.includes(string.trim())) {
+    if (!string || keywords.includes(string.trim())) {
       return axes;
     }
 
     // Parse font-variation-settings CSS declaration into an object
     // with axis tags as keys and axis values as values.
     axes = string
       .split(",")
       .reduce((acc, pair) => {
         // Tags are always in quotes. Split by quote and filter excessive whitespace.
         pair = pair.split(/["']/).filter(part => part.trim() !== "");
+        // Guard against malformed input that may have slipped through.
+        if (pair.length === 0) {
+          return acc;
+        }
+
         const tag = pair[0];
         const value = pair[1].trim();
         // Axis tags shorter or longer than 4 characters are invalid. Whitespace is valid.
         if (tag.length === 4) {
           acc[tag] = value;
         }
         return acc;
       }, {});
--- a/devtools/server/actors/styles.js
+++ b/devtools/server/actors/styles.js
@@ -295,16 +295,17 @@ var PageStyleActor = protocol.ActorClass
     const fonts = InspectorUtils.getUsedFontFaces(rng);
     const fontsArray = [];
 
     for (let i = 0; i < fonts.length; i++) {
       const font = fonts[i];
       const fontFace = {
         name: font.name,
         CSSFamilyName: font.CSSFamilyName,
+        CSSGeneric: font.CSSGeneric || null,
         srcIndex: font.srcIndex,
         URI: font.URI,
         format: font.format,
         localName: font.localName,
         metadata: font.metadata
       };
 
       // If this font comes from a @font-face rule