Bug 994559 - Style used fonts in rule view. r=pbro
authorMichael Hoffmann <brennan.brisad@gmail.com>
Tue, 09 Jan 2018 08:56:00 -0500
changeset 452718 c809b916352b3d4056610be28e89dc048d363493
parent 452717 b96fa0adc2b3af934e8ca3098278ee2bc78b1bd2
child 452719 9fc7e71752fd1e9764e900eec7c0b7de43d5f8db
push id1648
push usermtabara@mozilla.com
push dateThu, 01 Mar 2018 12:45:47 +0000
treeherdermozilla-release@cbb9688c2eeb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro
bugs994559
milestone59.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 994559 - Style used fonts in rule view. r=pbro
devtools/client/inspector/rules/models/element-style.js
devtools/client/inspector/rules/test/browser.ini
devtools/client/inspector/rules/test/browser_rules_highlight-used-fonts.js
devtools/client/inspector/rules/views/text-property-editor.js
devtools/client/shared/output-parser.js
devtools/client/shared/test/browser_outputparser.js
devtools/client/themes/rules.css
--- a/devtools/client/inspector/rules/models/element-style.js
+++ b/devtools/client/inspector/rules/models/element-style.js
@@ -133,16 +133,36 @@ ElementStyle.prototype = {
       }
       return promiseWarn(e);
     });
     this.populated = populated;
     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.
    */
   _sortRulesForPseudoElement: function () {
     this.rules = this.rules.sort((a, b) => {
       return (a.pseudoElement || "z") > (b.pseudoElement || "z");
     });
   },
 
--- a/devtools/client/inspector/rules/test/browser.ini
+++ b/devtools/client/inspector/rules/test/browser.ini
@@ -172,16 +172,17 @@ skip-if = (os == "win" && debug) # bug 9
 [browser_rules_grid-highlighter-on-reload.js]
 [browser_rules_grid-highlighter-restored-after-reload.js]
 [browser_rules_grid-toggle_01.js]
 [browser_rules_grid-toggle_01b.js]
 [browser_rules_grid-toggle_02.js]
 [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]
 [browser_rules_inherited-properties_04.js]
 [browser_rules_inline-source-map.js]
 [browser_rules_invalid.js]
 [browser_rules_invalid-source-map.js]
 [browser_rules_keybindings.js]
new file mode 100644
--- /dev/null
+++ b/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");
+  }
+});
--- a/devtools/client/inspector/rules/views/text-property-editor.js
+++ b/devtools/client/inspector/rules/views/text-property-editor.js
@@ -23,29 +23,45 @@ const Services = require("Services");
 const HTML_NS = "http://www.w3.org/1999/xhtml";
 
 const SHARED_SWATCH_CLASS = "ruleview-swatch";
 const COLOR_SWATCH_CLASS = "ruleview-colorswatch";
 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
  * (e.g. shows a color tooltip, opens a link, …).
  */
 const ACTIONABLE_ELEMENTS_SELECTORS = [
   `.${COLOR_SWATCH_CLASS}`,
   `.${BEZIER_SWATCH_CLASS}`,
   `.${FILTER_SWATCH_CLASS}`,
   `.${ANGLE_SWATCH_CLASS}`,
   "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.
  *   Manages changes to the TextProperty.
  *   Can be expanded to display computed properties.
  *   Can mark a property disabled or enabled.
  *
  * @param {RuleEditor} ruleEditor
@@ -360,27 +376,61 @@ TextPropertyEditor.prototype = {
       colorSwatchClass: SHARED_SWATCH_CLASS + " " + COLOR_SWATCH_CLASS,
       filterClass: "ruleview-filter",
       filterSwatchClass: SHARED_SWATCH_CLASS + " " + FILTER_SWATCH_CLASS,
       flexClass: "ruleview-flex",
       gridClass: "ruleview-grid",
       shapeClass: "ruleview-shape",
       defaultColorType: !propDirty,
       urlClass: "theme-link",
+      fontFamilyClass: FONT_FAMILY_CLASS,
       baseURI: this.sheetHref,
       unmatchedVariableClass: "ruleview-unmatched-variable",
       matchedVariableClass: "ruleview-variable",
       isVariableInUse: varName => this.rule.elementStyle.getVariable(varName),
     };
     let frag = outputParser.parseCssProperty(name, val, parserOptions);
     this.valueSpan.innerHTML = "";
     this.valueSpan.appendChild(frag);
 
     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);
     if (this.ruleEditor.isEditable) {
       for (let span of this._colorSwatchSpans) {
         // Adding this swatch to the list of swatches our colorpicker
         // knows about
         this.ruleView.tooltips.getTooltip("colorPicker").addSwatch(span, {
--- a/devtools/client/shared/output-parser.js
+++ b/devtools/client/shared/output-parser.js
@@ -88,16 +88,17 @@ OutputParser.prototype = {
     options = this._mergeOptions(options);
 
     options.expectCubicBezier = this.supportsType(name, CSS_TYPES.TIMING_FUNCTION);
     options.expectDisplay = name === "display";
     options.expectFilter = name === "filter";
     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);
 
     // The filter property is special in that we want to show the
     // swatch even if the value is invalid, because this way the user
     // can easily use the editor to fix it.
     if (options.expectFilter || this._cssPropertySupportsValue(name, value)) {
       return this._parse(value, options);
@@ -280,16 +281,17 @@ OutputParser.prototype = {
    * @param  {Boolean} stopAtCloseParen
    *         If true, stop at an umatched close paren.
    * @return {DocumentFragment}
    *         A document fragment.
    */
   _doParse: function (text, options, tokenStream, stopAtCloseParen) {
     let parenDepth = stopAtCloseParen ? 1 : 0;
     let outerMostFunctionTakesColor = false;
+    let fontFamilyNameParts = [];
 
     let colorOK = function () {
       return options.supportsColor ||
         (options.expectFilter && parenDepth === 1 &&
          outerMostFunctionTakesColor);
     };
 
     let angleOK = function (angle) {
@@ -297,16 +299,19 @@ OutputParser.prototype = {
     };
 
     let spaceNeeded = false;
     let done = false;
 
     while (!done) {
       let token = tokenStream.nextToken();
       if (!token) {
+        if (options.expectFont && fontFamilyNameParts.length !== 0) {
+          this._appendFontFamily(fontFamilyNameParts.join(""), options);
+        }
         break;
       }
 
       if (token.tokenType === "comment") {
         // This doesn't change spaceNeeded, because we didn't emit
         // anything to the output.
         continue;
       }
@@ -378,16 +383,18 @@ OutputParser.prototype = {
             this._appendHighlighterToggle(token.text, options.flexClass);
           } else if (this._isDisplayGrid(text, token, options)) {
             this._appendHighlighterToggle(token.text, options.gridClass);
           } else if (colorOK() &&
                      colorUtils.isValidCSSColor(token.text, this.cssColor4)) {
             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));
           }
           break;
 
         case "id":
         case "hash": {
@@ -413,30 +420,52 @@ OutputParser.prototype = {
           }
           break;
         case "url":
         case "bad_url":
           this._appendURL(text.substring(token.startOffset, token.endOffset),
                           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;
           } else if (token.text === ")") {
             --parenDepth;
 
             if (stopAtCloseParen && parenDepth === 0) {
               done = true;
               break;
             }
 
             if (parenDepth === 0) {
               outerMostFunctionTakesColor = false;
             }
+          } else if (token.text === "," &&
+                     options.expectFont && fontFamilyNameParts.length !== 0) {
+            this._appendFontFamily(fontFamilyNameParts.join(""), options);
+            fontFamilyNameParts = [];
           }
           // falls through
         default:
           this._appendTextNode(
             text.substring(token.startOffset, token.endOffset));
           break;
       }
 
@@ -1325,16 +1354,68 @@ OutputParser.prototype = {
 
       this._appendTextNode(trailer);
     } else {
       this._appendTextNode(match);
     }
   },
 
   /**
+   * 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.
    *
    * @param  {String} tagName
    *         Tag type e.g. "div"
    * @param  {Object} attributes
    *         e.g. {class: "someClass", style: "cursor:pointer"};
    * @param  {String} [value]
    *         If a value is included it will be appended as a text node inside
@@ -1435,16 +1516,17 @@ OutputParser.prototype = {
    *                                    // parser to skip the call to
    *                                    // _wrapFilter.  Used only for
    *                                    // previewing with the filter swatch.
    *           - flexClass: ""          // The class to use for the flex icon.
    *           - gridClass: ""          // The class to use for the grid icon.
    *           - 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
    *                                    // argument, the name of a variable.
    *                                    // This should return the variable's
    *                                    // value, if it is in use; or null.
    *           - unmatchedVariableClass: ""
    *                                    // The class to use for a component
@@ -1463,16 +1545,17 @@ OutputParser.prototype = {
       colorClass: "",
       colorSwatchClass: "",
       filterSwatch: false,
       flexClass: "",
       gridClass: "",
       shapeClass: "",
       supportsColor: false,
       urlClass: "",
+      fontFamilyClass: "",
       baseURI: undefined,
       isVariableInUse: null,
       unmatchedVariableClass: null,
     };
 
     for (let item in overrides) {
       defaults[item] = overrides[item];
     }
--- a/devtools/client/shared/test/browser_outputparser.js
+++ b/devtools/client/shared/test/browser_outputparser.js
@@ -25,16 +25,17 @@ function* performTest() {
   let parser = new OutputParser(doc, cssProperties);
   testParseCssProperty(doc, parser);
   testParseCssVar(doc, parser);
   testParseURL(doc, parser);
   testParseFilter(doc, parser);
   testParseAngle(doc, parser);
   testParseShape(doc, parser);
   testParseVariable(doc, parser);
+  testParseFontFamily(doc, parser);
 
   host.destroy();
 }
 
 // Class name used in color swatch.
 var COLOR_TEST_CLASS = "test-class";
 
 // Create a new CSS color-parsing test.  |name| is the name of the CSS
@@ -76,17 +77,18 @@ function testParseCssProperty(doc, parse
 
     makeColorTest("background-image",
       "linear-gradient(to right, #F60 10%, rgba(0,0,0,1))",
       ["linear-gradient(to right, ", {name: "#F60"},
        " 10%, ", {name: "rgba(0,0,0,1)"},
        ")"]),
 
     // 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"}]),
 
     makeColorTest("box-shadow",
       "0 0 1em red, 2px 2px 0 0 rgba(0,0,0,.5)",
       ["0 0 1em ", {name: "red"},
        ", 2px 2px 0 0 ",
@@ -458,8 +460,92 @@ function testParseVariable(doc, parser) 
 
     let target = doc.querySelector("div");
     target.appendChild(frag);
 
     is(target.innerHTML, test.expected, test.text);
     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");
+  }
+}
--- a/devtools/client/themes/rules.css
+++ b/devtools/client/themes/rules.css
@@ -536,16 +536,20 @@
 .ruleview-overridden {
   text-decoration: line-through;
 }
 
 .theme-light .ruleview-overridden {
   text-decoration-color: var(--theme-content-color3);
 }
 
+.ruleview-font-family.used-font {
+  text-decoration: underline;
+}
+
 .styleinspector-propertyeditor {
   border: 1px solid #CCC;
   padding: 0;
   margin: -1px -3px -1px -1px;
 }
 
 .theme-firebug .styleinspector-propertyeditor {
   border: 1px solid var(--theme-splitter-color);