Bug 1461328 - Handle !important for font-family in inspector correctly. r=pbro, a=RyanVM DEVEDITION_61_0b7_BUILD1 DEVEDITION_61_0b7_RELEASE FENNEC_61_0b7_BUILD1 FENNEC_61_0b7_RELEASE FIREFOX_61_0b7_BUILD1 FIREFOX_61_0b7_RELEASE
authorMichael Hoffmann <brennan.brisad@gmail.com>
Fri, 18 May 2018 04:21:00 +0300
changeset 470855 4268240fb6d9b8a56058477017a345f87cae799f
parent 470854 bbb2cb6e0d060048b665ebee80ce299d551087e0
child 470856 eef511c730d8723a4101eb28de8453c07a7ce0c2
push id9244
push userryanvm@gmail.com
push dateMon, 21 May 2018 11:09:57 +0000
treeherdermozilla-beta@4268240fb6d9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro, RyanVM
bugs1461328
milestone61.0
Bug 1461328 - Handle !important for font-family in inspector correctly. r=pbro, a=RyanVM
devtools/client/inspector/rules/test/browser.ini
devtools/client/inspector/rules/test/browser_rules_font-family-parsing.js
devtools/client/inspector/rules/test/browser_rules_highlight-used-fonts.js
devtools/client/shared/output-parser.js
--- a/devtools/client/inspector/rules/test/browser.ini
+++ b/devtools/client/inspector/rules/test/browser.ini
@@ -164,16 +164,17 @@ skip-if = (os == "win" && debug) # bug 9
 [browser_rules_flexbox-highlighter-on-navigate.js]
 [browser_rules_flexbox-highlighter-on-reload.js]
 [browser_rules_flexbox-highlighter-restored-after-reload.js]
 [browser_rules_flexbox-toggle_01.js]
 [browser_rules_flexbox-toggle_01b.js]
 [browser_rules_flexbox-toggle_02.js]
 [browser_rules_flexbox-toggle_03.js]
 [browser_rules_flexbox-toggle_04.js]
+[browser_rules_font-family-parsing.js]
 [browser_rules_grid-highlighter-on-mutation.js]
 [browser_rules_grid-highlighter-on-navigate.js]
 [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]
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/rules/test/browser_rules_font-family-parsing.js
@@ -0,0 +1,58 @@
+/* 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 the parsed font-family property value shown in the rules
+// pane is correct.
+
+const TEST_URI = `
+  <style type="text/css">
+    #id1 {
+      font-family: georgia, arial, sans-serif;
+    }
+    #id2 {
+      font-family: georgia,arial,sans-serif;
+    }
+    #id3 {
+      font-family: georgia ,arial ,sans-serif;
+    }
+    #id4 {
+      font-family: georgia , arial , sans-serif;
+    }
+    #id4 {
+      font-family:   arial,  georgia,   sans-serif  ;
+    }
+    #id5 {
+      font-family: helvetica !important;
+    }
+  </style>
+  <div id="id1">1</div>
+  <div id="id2">2</div>
+  <div id="id3">3</div>
+  <div id="id4">4</div>
+  <div id="id5">5</div>
+`;
+
+const TESTS = [
+  {selector: "#id1", expectedTextContent: "georgia, arial, sans-serif"},
+  {selector: "#id2", expectedTextContent: "georgia,arial,sans-serif"},
+  {selector: "#id3", expectedTextContent: "georgia ,arial ,sans-serif"},
+  {selector: "#id4", expectedTextContent: "arial, georgia, sans-serif"},
+  {selector: "#id5", expectedTextContent: "helvetica !important"},
+];
+
+add_task(async function() {
+  await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  let {inspector, view} = await openRuleView();
+
+  for (let {selector, expectedTextContent} of TESTS) {
+    await selectNode(selector, inspector);
+    info("Looking for font-family property value in selector " + selector);
+
+    let prop = getRuleViewProperty(view, selector, "font-family").valueSpan;
+    is(prop.textContent, expectedTextContent,
+       "The font-family property value is correct");
+  }
+});
--- a/devtools/client/inspector/rules/test/browser_rules_highlight-used-fonts.js
+++ b/devtools/client/inspector/rules/test/browser_rules_highlight-used-fonts.js
@@ -21,40 +21,50 @@ const TEST_URI = `
       font-family: foo, bar;
     }
     #id5 {
       font-family: "monospace";
     }
     #id6 {
       font-family: georgia, arial;
     }
+    #id7 {
+      font-family: foo, serif !important;
+    }
+    #id8 {
+      font-family: important;
+    }
   </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>
   <div id="id6">A &#586;</div>
+  <div id="id7">Text</div>
+  <div id="id8">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 indexes of all the fonts that should be highlighted, or null if none should
-//         be highlighter
+//         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: null},
   {selector: "#id5", nb: 1, used: [0]}, // monospace
+  {selector: "#id7", nb: 2, used: [1]}, // serif
+  {selector: "#id8", nb: 1, used: null},
 ];
 
 if (Services.appinfo.OS !== "Linux") {
   // Both georgia and arial are used because the second character can't be rendered with
   // georgia, so the browser falls back. Also, skip this on Linux which has neither of
   // these fonts.
   TESTS.push({selector: "#id6", nb: 2, used: [0, 1]});
 }
--- a/devtools/client/shared/output-parser.js
+++ b/devtools/client/shared/output-parser.js
@@ -282,16 +282,17 @@ OutputParser.prototype = {
    *         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 previousWasBang = false;
 
     let colorOK = function() {
       return options.supportsColor ||
         (options.expectFilter && parenDepth === 1 &&
          outerMostFunctionTakesColor);
     };
 
     let angleOK = function(angle) {
@@ -383,17 +384,20 @@ 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) {
+          } else if (options.expectFont && !previousWasBang) {
+            // We don't append the identifier if the previous token
+            // was equal to '!', since in that case we expect the
+            // identifier to be equal to 'important'.
             fontFamilyNameParts.push(token.text);
           } else {
             this._appendTextNode(text.substring(token.startOffset,
                                                 token.endOffset));
           }
           break;
 
         case "id":
@@ -452,17 +456,17 @@ OutputParser.prototype = {
             if (stopAtCloseParen && parenDepth === 0) {
               done = true;
               break;
             }
 
             if (parenDepth === 0) {
               outerMostFunctionTakesColor = false;
             }
-          } else if (token.text === "," &&
+          } else if ((token.text === "," || token.text === "!") &&
                      options.expectFont && fontFamilyNameParts.length !== 0) {
             this._appendFontFamily(fontFamilyNameParts.join(""), options);
             fontFamilyNameParts = [];
           }
           // falls through
         default:
           this._appendTextNode(
             text.substring(token.startOffset, token.endOffset));
@@ -470,16 +474,17 @@ OutputParser.prototype = {
       }
 
       // If this token might possibly introduce token pasting when
       // color-cycling, require a space.
       spaceNeeded = (token.tokenType === "ident" || token.tokenType === "at" ||
                      token.tokenType === "id" || token.tokenType === "hash" ||
                      token.tokenType === "number" || token.tokenType === "dimension" ||
                      token.tokenType === "percentage" || token.tokenType === "dimension");
+      previousWasBang = (token.tokenType === "symbol" && token.text === "!");
     }
 
     let result = this._toDOM();
 
     if (options.expectFilter && !options.filterSwatch) {
       result = this._wrapFilter(text, options, result);
     }