Bug 994559 - Style used fonts in rule view. r=pbro

devtools/client/inspector/rules/models/element-style.js

@@ -137,6 +137,26 @@ ElementStyle.prototype = {
     return this.populated;
   },
 
+  /**
+   * Get the font families in use by the element.
+   *
+   * Returns a promise that will be resolved to a list of CSS family
+   * names.  The list might have duplicates.
+   */
+  getUsedFontFamilies: function () {
+    return new Promise((resolve, reject) => {
+      this.ruleView.styleWindow.requestIdleCallback(async () => {
+        try {
+          let fonts = await this.pageStyle.getUsedFontFaces(
+            this.element, { includePreviews: false });
+          resolve(fonts.map(font => font.CSSFamilyName));
+        } catch (e) {
+          reject(e);
+        }
+      });
+    });
+  },
+
   /**
    * Put pseudo elements in front of others.
    */

devtools/client/inspector/rules/test/browser.ini

@@ -177,6 +177,7 @@ skip-if = (os == "win" && debug) # bug 963492: win.
 [browser_rules_grid-toggle_03.js]
 [browser_rules_grid-toggle_04.js]
 [browser_rules_guessIndentation.js]
+[browser_rules_highlight-used-fonts.js]
 [browser_rules_inherited-properties_01.js]
 [browser_rules_inherited-properties_02.js]
 [browser_rules_inherited-properties_03.js]

devtools/client/inspector/rules/test/browser_rules_highlight-used-fonts.js

@@ -0,0 +1,73 @@
+/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Tests that a used font-family is highlighted in the rule-view.
+
+const TEST_URI = `
+  <style type="text/css">
+    #id1 {
+      font-family: foo, bar, sans-serif;
+    }
+    #id2 {
+      font-family: serif;
+    }
+    #id3 {
+      font-family: foo, monospace, monospace, serif;
+    }
+    #id4 {
+      font-family: foo, bar;
+    }
+    #id5 {
+      font-family: "monospace";
+    }
+  </style>
+  <div id="id1">Text</div>
+  <div id="id2">Text</div>
+  <div id="id3">Text</div>
+  <div id="id4">Text</div>
+  <div id="id5">Text</div>
+`;
+
+// Tests that font-family properties in the rule-view correctly
+// indicates which font is in use.
+// Each entry in the test array should contain:
+// {
+//   selector: the rule-view selector to look for font-family in
+//   nb: the number of fonts this property should have
+//   used: the index of the font that should be highlighted, or
+//         -1 if none should be highlighted
+// }
+const TESTS = [
+  {selector: "#id1", nb: 3, used: 2}, // sans-serif
+  {selector: "#id2", nb: 1, used: 0}, // serif
+  {selector: "#id3", nb: 4, used: 1}, // monospace
+  {selector: "#id4", nb: 2, used: -1},
+  {selector: "#id5", nb: 1, used: 0}, // monospace
+];
+
+add_task(function* () {
+  yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  let {inspector, view} = yield openRuleView();
+
+  for (let {selector, nb, used} of TESTS) {
+    let onFontHighlighted = view.once("font-highlighted");
+    yield selectNode(selector, inspector);
+    yield onFontHighlighted;
+
+    info("Looking for fonts in font-family property in selector " + selector);
+
+    let prop = getRuleViewProperty(view, selector, "font-family").valueSpan;
+    let fonts = prop.querySelectorAll(".ruleview-font-family");
+
+    ok(fonts.length, "Fonts found in the property");
+    is(fonts.length, nb, "Correct number of fonts found in the property");
+
+    const highlighted = [...fonts].filter(span => span.classList.contains("used-font"));
+
+    ok(highlighted.length <= 1, "No more than one font highlighted");
+    is([...fonts].findIndex(f => f === highlighted[0]), used, "Correct font highlighted");
+  }
+});

devtools/client/inspector/rules/views/text-property-editor.js

@@ -28,6 +28,7 @@ const BEZIER_SWATCH_CLASS = "ruleview-bezierswatch";
 const FILTER_SWATCH_CLASS = "ruleview-filterswatch";
 const ANGLE_SWATCH_CLASS = "ruleview-angleswatch";
 const INSET_POINT_TYPES = ["top", "right", "bottom", "left"];
+const FONT_FAMILY_CLASS = "ruleview-font-family";
 
 /*
  * An actionable element is an element which on click triggers a specific action
@@ -41,6 +42,21 @@ const ACTIONABLE_ELEMENTS_SELECTORS = [
   "a"
 ];
 
+// In order to highlight the used fonts in font-family properties, we
+// retrieve the list of used fonts from the server. That always
+// returns the actually used font family name(s). If the property's
+// authored value is sans-serif for instance, the used font might be
+// arial instead.  So we need the list of all generic font family
+// names to underline those when we find them.
+const GENERIC_FONT_FAMILIES = [
+  "serif",
+  "sans-serif",
+  "cursive",
+  "fantasy",
+  "monospace",
+  "system-ui"
+];
+
 /**
  * TextPropertyEditor is responsible for the following:
  *   Owns a TextProperty object.
@@ -365,6 +381,7 @@ TextPropertyEditor.prototype = {
       shapeClass: "ruleview-shape",
       defaultColorType: !propDirty,
       urlClass: "theme-link",
+      fontFamilyClass: FONT_FAMILY_CLASS,
       baseURI: this.sheetHref,
       unmatchedVariableClass: "ruleview-unmatched-variable",
       matchedVariableClass: "ruleview-variable",
@@ -376,6 +393,39 @@ TextPropertyEditor.prototype = {
 
     this.ruleView.emit("property-value-updated", this.valueSpan);
 
+    // Highlight the currently used font in font-family properties.
+    // If we cannot find a match, highlight the first generic family instead.
+    let fontFamilySpans = this.valueSpan.querySelectorAll("." + FONT_FAMILY_CLASS);
+    if (fontFamilySpans.length && this.prop.enabled && !this.prop.overridden) {
+      this.rule.elementStyle.getUsedFontFamilies().then(families => {
+        const usedFontFamilies = families.map(font => font.toLowerCase());
+        let foundMatchingFamily = false;
+        let firstGenericSpan = null;
+
+        for (let span of fontFamilySpans) {
+          const authoredFont = span.textContent.toLowerCase();
+
+          if (!firstGenericSpan && GENERIC_FONT_FAMILIES.includes(authoredFont)) {
+            firstGenericSpan = span;
+          }
+
+          if (usedFontFamilies.includes(authoredFont)) {
+            span.classList.add("used-font");
+            foundMatchingFamily = true;
+            // We found the span to style, no need to continue with
+            // the remaining ones
+            break;
+          }
+        }
+
+        if (!foundMatchingFamily && firstGenericSpan) {
+          firstGenericSpan.classList.add("used-font");
+        }
+
+        this.ruleView.emit("font-highlighted", this.valueSpan);
+      }).catch(e => console.error("Could not get the list of font families", e));
+    }
+
     // Attach the color picker tooltip to the color swatches
     this._colorSwatchSpans =
       this.valueSpan.querySelectorAll("." + COLOR_SWATCH_CLASS);

devtools/client/shared/output-parser.js

@@ -93,6 +93,7 @@ OutputParser.prototype = {
     options.expectShape = name === "clip-path" ||
                           (name === "shape-outside"
                            && Services.prefs.getBoolPref(CSS_SHAPE_OUTSIDE_ENABLED_PREF));
+    options.expectFont = name === "font-family";
     options.supportsColor = this.supportsType(name, CSS_TYPES.COLOR) ||
                             this.supportsType(name, CSS_TYPES.GRADIENT);
 
@@ -285,6 +286,7 @@ OutputParser.prototype = {
   _doParse: function (text, options, tokenStream, stopAtCloseParen) {
     let parenDepth = stopAtCloseParen ? 1 : 0;
     let outerMostFunctionTakesColor = false;
+    let fontFamilyNameParts = [];
 
     let colorOK = function () {
       return options.supportsColor ||
@@ -302,6 +304,9 @@ OutputParser.prototype = {
     while (!done) {
       let token = tokenStream.nextToken();
       if (!token) {
+        if (options.expectFont && fontFamilyNameParts.length !== 0) {
+          this._appendFontFamily(fontFamilyNameParts.join(""), options);
+        }
         break;
       }
 
@@ -383,6 +388,8 @@ OutputParser.prototype = {
             this._appendColor(token.text, options);
           } else if (angleOK(token.text)) {
             this._appendAngle(token.text, options);
+          } else if (options.expectFont) {
+            fontFamilyNameParts.push(token.text);
           } else {
             this._appendTextNode(text.substring(token.startOffset,
                                                 token.endOffset));
@@ -418,6 +425,24 @@ OutputParser.prototype = {
                           token.text, options);
           break;
 
+        case "string":
+          if (options.expectFont) {
+            fontFamilyNameParts.push(text.substring(token.startOffset, token.endOffset));
+          } else {
+            this._appendTextNode(
+              text.substring(token.startOffset, token.endOffset));
+          }
+          break;
+
+        case "whitespace":
+          if (options.expectFont) {
+            fontFamilyNameParts.push(" ");
+          } else {
+            this._appendTextNode(
+              text.substring(token.startOffset, token.endOffset));
+          }
+          break;
+
         case "symbol":
           if (token.text === "(") {
             ++parenDepth;
@@ -432,6 +457,10 @@ OutputParser.prototype = {
             if (parenDepth === 0) {
               outerMostFunctionTakesColor = false;
             }
+          } else if (token.text === "," &&
+                     options.expectFont && fontFamilyNameParts.length !== 0) {
+            this._appendFontFamily(fontFamilyNameParts.join(""), options);
+            fontFamilyNameParts = [];
           }
           // falls through
         default:
@@ -1329,6 +1358,58 @@ OutputParser.prototype = {
     }
   },
 
+  /**
+   * Append a font family to the output.
+   *
+   * @param  {String} fontFamily
+   *         Font family to append
+   * @param  {Object} options
+   *         Options object. For valid options and default values see
+   *         _mergeOptions().
+   */
+  _appendFontFamily: function (fontFamily, options) {
+    let spanContents = fontFamily;
+    let quoteChar = null;
+    let trailingWhitespace = false;
+
+    // Before appending the actual font-family span, we need to trim
+    // down the actual contents by removing any whitespace before and
+    // after, and any quotation characters in the passed string.  Any
+    // such characters are preserved in the actual output, but just
+    // not inside the span element.
+
+    if (spanContents[0] === " ") {
+      this._appendTextNode(" ");
+      spanContents = spanContents.slice(1);
+    }
+
+    if (spanContents[spanContents.length - 1] === " ") {
+      spanContents = spanContents.slice(0, -1);
+      trailingWhitespace = true;
+    }
+
+    if (spanContents[0] === "'" || spanContents[0] === "\"") {
+      quoteChar = spanContents[0];
+    }
+
+    if (quoteChar) {
+      this._appendTextNode(quoteChar);
+      spanContents = spanContents.slice(1, -1);
+    }
+
+    this._appendNode("span", {
+      class: options.fontFamilyClass
+    }, spanContents);
+
+    if (quoteChar) {
+      this._appendTextNode(quoteChar);
+    }
+
+    if (trailingWhitespace) {
+      this._appendTextNode(" ");
+    }
+  },
+
   /**
    * Create a node.
    *
@@ -1440,6 +1521,7 @@ OutputParser.prototype = {
    *           - shapeClass: ""         // The class to use for the shape icon.
    *           - supportsColor: false   // Does the CSS property support colors?
    *           - urlClass: ""           // The class to be used for url() links.
+   *           - fontFamilyClass: ""    // The class to be used for font families.
    *           - baseURI: undefined     // A string used to resolve
    *                                    // relative links.
    *           - isVariableInUse        // A function taking a single
@@ -1468,6 +1550,7 @@ OutputParser.prototype = {
       shapeClass: "",
       supportsColor: false,
       urlClass: "",
+      fontFamilyClass: "",
       baseURI: undefined,
       isVariableInUse: null,
       unmatchedVariableClass: null,

devtools/client/shared/test/browser_outputparser.js

@@ -30,6 +30,7 @@ function* performTest() {
   testParseAngle(doc, parser);
   testParseShape(doc, parser);
   testParseVariable(doc, parser);
+  testParseFontFamily(doc, parser);
 
   host.destroy();
 }
@@ -81,7 +82,8 @@ function testParseCssProperty(doc, parser) {
        ")"]),
 
     // In "arial black", "black" is a font, not a color.
-    makeColorTest("font-family", "arial black", ["arial black"]),
+    // (The font-family parser creates a span)
+    makeColorTest("font-family", "arial black", ["<span>arial black</span>"]),
 
     makeColorTest("box-shadow", "0 0 1em red",
                   ["0 0 1em ", {name: "red"}]),
@@ -463,3 +465,87 @@ function testParseVariable(doc, parser) {
     target.innerHTML = "";
   }
 }
+
+function testParseFontFamily(doc, parser) {
+  info("Test font-family parsing");
+  const tests = [
+    {
+      desc: "No fonts",
+      definition: "",
+      families: []
+    },
+    {
+      desc: "List of fonts",
+      definition: "Arial,Helvetica,sans-serif",
+      families: ["Arial", "Helvetica", "sans-serif"]
+    },
+    {
+      desc: "Fonts with spaces",
+      definition: "Open Sans",
+      families: ["Open Sans"]
+    },
+    {
+      desc: "Quoted fonts",
+      definition: "\"Arial\",'Open Sans'",
+      families: ["Arial", "Open Sans"]
+    },
+    {
+      desc: "Fonts with extra whitespace",
+      definition: " Open  Sans  ",
+      families: ["Open Sans"]
+    }
+  ];
+
+  const textContentTests = [
+    {
+      desc: "No whitespace between fonts",
+      definition: "Arial,Helvetica,sans-serif",
+      output: "Arial,Helvetica,sans-serif",
+    },
+    {
+      desc: "Whitespace between fonts",
+      definition: "Arial ,  Helvetica,   sans-serif",
+      output: "Arial , Helvetica, sans-serif",
+    },
+    {
+      desc: "Whitespace before first font trimmed",
+      definition: "  Arial,Helvetica,sans-serif",
+      output: "Arial,Helvetica,sans-serif",
+    },
+    {
+      desc: "Whitespace after last font trimmed",
+      definition: "Arial,Helvetica,sans-serif  ",
+      output: "Arial,Helvetica,sans-serif",
+    },
+    {
+      desc: "Whitespace between quoted fonts",
+      definition: "'Arial' ,  \"Helvetica\" ",
+      output: "'Arial' , \"Helvetica\"",
+    },
+    {
+      desc: "Whitespace within font preserved",
+      definition: "'  Ari al '",
+      output: "'  Ari al '",
+    }
+  ];
+
+  for (let {desc, definition, families} of tests) {
+    info(desc);
+    let frag = parser.parseCssProperty("font-family", definition, {
+      fontFamilyClass: "ruleview-font-family"
+    });
+    let spans = frag.querySelectorAll(".ruleview-font-family");
+
+    is(spans.length, families.length, desc + " span count");
+    for (let i = 0; i < spans.length; i++) {
+      is(spans[i].textContent, families[i], desc + " span contents");
+    }
+  }
+
+  info("Test font-family text content");
+  for (let {desc, definition, output} of textContentTests) {
+    info(desc);
+    let frag = parser.parseCssProperty("font-family", definition, {});
+    is(frag.textContent, output, desc + " text content matches");
+  }
+}

devtools/client/themes/rules.css

@@ -541,6 +541,10 @@
   text-decoration-color: var(--theme-content-color3);
 }
 
+.ruleview-font-family.used-font {
+  text-decoration: underline;
+}
+
 .styleinspector-propertyeditor {
   border: 1px solid #CCC;
   padding: 0;