Bug 1462553 - fix !important parsing in devtools; r?pbro draft
authorTom Tromey <tom@tromey.com>
Fri, 18 May 2018 11:39:43 -0600
changeset 798346 013f8ead226ee22f66a6bdcdf8357dd9deb9eefd
parent 798287 6adc58aae9925bae8688b8f9dfe6a8b23afa3565
push id110724
push userbmo:ttromey@mozilla.com
push dateTue, 22 May 2018 17:53:35 +0000
reviewerspbro
bugs1462553
milestone62.0a1
Bug 1462553 - fix !important parsing in devtools; r?pbro Bug 1462553 points out that the CSS-parsing code in parsing-utils was not correctly handling "!important"; in particular, it was allowing this to appear in the middle of a declaration, rather than only at the end. This patch fixes the parser. MozReview-Commit-ID: 9efv60gX6nV
devtools/client/shared/test/unit/test_parseDeclarations.js
devtools/client/shared/test/unit/test_parseSingleValue.js
devtools/shared/css/parsing-utils.js
--- a/devtools/client/shared/test/unit/test_parseDeclarations.js
+++ b/devtools/client/shared/test/unit/test_parseDeclarations.js
@@ -66,23 +66,70 @@ const TEST_DATA = [
   // Test simple priority
   {
     input: "p1: v1 !  important; p2: v2 ! important;",
     expected: [
       {name: "p1", value: "v1", priority: "important", offsets: [0, 20]},
       {name: "p2", value: "v2", priority: "important", offsets: [21, 40]}
     ]
   },
+  // Test simple priority
+  {
+    input: "p1: v1 !/*comment*/important;",
+    expected: [
+      {name: "p1", value: "v1", priority: "important", offsets: [0, 29]},
+    ]
+  },
+  // Test priority without terminating ";".
+  {
+    input: "p1: v1 !important",
+    expected: [
+      {name: "p1", value: "v1", priority: "important", offsets: [0, 17]},
+    ]
+  },
+  // Test trailing "!" without terminating ";".
+  {
+    input: "p1: v1 !",
+    expected: [
+      {name: "p1", value: "v1 !", priority: "", offsets: [0, 8]},
+    ]
+  },
   // Test invalid priority
   {
     input: "p1: v1 important;",
     expected: [
       {name: "p1", value: "v1 important", priority: "", offsets: [0, 17]}
     ]
   },
+  // Test invalid priority (in the middle of the declaration).
+  // See bug 1462553.
+  {
+    input: "p1: v1 !important v2;",
+    expected: [
+      {name: "p1", value: "v1 !important v2", priority: "", offsets: [0, 21]}
+    ]
+  },
+  {
+    input: "p1: v1 !    important v2;",
+    expected: [
+      {name: "p1", value: "v1 ! important v2", priority: "", offsets: [0, 25]}
+    ]
+  },
+  {
+    input: "p1: v1 !  /*comment*/  important v2;",
+    expected: [
+      {name: "p1", value: "v1 ! important v2", priority: "", offsets: [0, 36]}
+    ]
+  },
+  {
+    input: "p1: v1 !/*hi*/important v2;",
+    expected: [
+      {name: "p1", value: "v1 ! important v2", priority: "", offsets: [0, 27]}
+    ]
+  },
   // Test various types of background-image urls
   {
     input: "background-image: url(../../relative/image.png)",
     expected: [{
       name: "background-image",
       value: "url(../../relative/image.png)",
       priority: "",
       offsets: [0, 47]
--- a/devtools/client/shared/test/unit/test_parseSingleValue.js
+++ b/devtools/client/shared/test/unit/test_parseSingleValue.js
@@ -17,17 +17,17 @@ const TEST_DATA = [
   {input: "blue", expected: {value: "blue", priority: ""}},
   {input: "blue !important", expected: {value: "blue", priority: "important"}},
   {input: "blue!important", expected: {value: "blue", priority: "important"}},
   {input: "blue ! important", expected: {value: "blue", priority: "important"}},
   {
     input: "blue !  important",
     expected: {value: "blue", priority: "important"}
   },
-  {input: "blue !", expected: {value: "blue", priority: ""}},
+  {input: "blue !", expected: {value: "blue !", priority: ""}},
   {input: "blue !mportant", expected: {value: "blue !mportant", priority: ""}},
   {
     input: "  blue   !important ",
     expected: {value: "blue", priority: "important"}
   },
   {
     input: "url(\"http://url.com/whyWouldYouDoThat!important.png\") !important",
     expected: {
--- a/devtools/shared/css/parsing-utils.js
+++ b/devtools/shared/css/parsing-utils.js
@@ -297,17 +297,26 @@ function parseDeclarationsInternal(isCss
     throw new Error("empty input string");
   }
 
   let lexer = getCSSLexer(inputString);
 
   let declarations = [getEmptyDeclaration()];
   let lastProp = declarations[0];
 
-  let current = "", hasBang = false;
+  // This tracks the "!important" parsing state.  The states are:
+  // 0 - haven't seen anything
+  // 1 - have seen "!", looking for "important" next (possibly after
+  //     whitespace).
+  // 2 - have seen "!important"
+  let importantState = 0;
+  // This is true if we saw whitespace or comments between the "!" and
+  // the "important".
+  let importantWS = false;
+  let current = "";
   while (true) {
     let token = lexer.nextToken();
     if (!token) {
       break;
     }
 
     // Ignore HTML comment tokens (but parse anything they might
     // happen to surround).
@@ -317,29 +326,33 @@ function parseDeclarationsInternal(isCss
 
     // Update the start and end offsets of the declaration, but only
     // when we see a significant token.
     if (token.tokenType !== "whitespace" && token.tokenType !== "comment") {
       if (lastProp.offsets[0] === undefined) {
         lastProp.offsets[0] = token.startOffset;
       }
       lastProp.offsets[1] = token.endOffset;
-    } else if (lastProp.name && !current && !hasBang &&
+    } else if (lastProp.name && !current && !importantState &&
                !lastProp.priority && lastProp.colonOffsets[1]) {
       // Whitespace appearing after the ":" is attributed to it.
       lastProp.colonOffsets[1] = token.endOffset;
+    } else if (importantState === 1) {
+      importantWS = true;
     }
 
     if (token.tokenType === "symbol" && token.text === ":") {
+      // Either way, a "!important" we've seen is no longer valid now.
+      importantState = 0;
+      importantWS = false;
       if (!lastProp.name) {
         // Set the current declaration name if there's no name yet
         lastProp.name = cssTrim(current);
         lastProp.colonOffsets = [token.startOffset, token.endOffset];
         current = "";
-        hasBang = false;
 
         // When parsing a comment body, if the left-hand-side is not a
         // valid property name, then drop it and stop parsing.
         if (inComment && !commentOverride &&
             !isCssPropertyKnown(lastProp.name)) {
           lastProp.name = null;
           break;
         }
@@ -352,68 +365,100 @@ function parseDeclarationsInternal(isCss
       lastProp.terminator = "";
       // When parsing a comment, if the name hasn't been set, then we
       // have probably just seen an ordinary semicolon used in text,
       // so drop this and stop parsing.
       if (inComment && !lastProp.name) {
         current = "";
         break;
       }
+      if (importantState === 2) {
+        lastProp.priority = "important";
+      } else if (importantState === 1) {
+        current += "!";
+        if (importantWS) {
+          current += " ";
+        }
+      }
       lastProp.value = cssTrim(current);
       current = "";
-      hasBang = false;
+      importantState = 0;
+      importantWS = false;
       declarations.push(getEmptyDeclaration());
       lastProp = declarations[declarations.length - 1];
     } else if (token.tokenType === "ident") {
-      if (token.text === "important" && hasBang) {
-        lastProp.priority = "important";
-        hasBang = false;
+      if (token.text === "important" && importantState === 1) {
+        importantState = 2;
       } else {
-        if (hasBang) {
+        if (importantState > 0) {
           current += "!";
+          if (importantWS) {
+            current += " ";
+          }
+          if (importantState === 2) {
+            current += "important ";
+          }
+          importantState = 0;
+          importantWS = false;
         }
         // Re-escape the token to avoid dequoting problems.
         // See bug 1287620.
         current += CSS.escape(token.text);
       }
     } else if (token.tokenType === "symbol" && token.text === "!") {
-      hasBang = true;
+      importantState = 1;
     } else if (token.tokenType === "whitespace") {
       if (current !== "") {
-        current += " ";
+        current = current.trimRight() + " ";
       }
     } else if (token.tokenType === "comment") {
       if (parseComments && !lastProp.name && !lastProp.value) {
         let commentText = inputString.substring(token.startOffset + 2,
                                                 token.endOffset - 2);
         let newDecls = parseCommentDeclarations(isCssPropertyKnown, commentText,
                                                 token.startOffset,
                                                 token.endOffset);
 
         // Insert the new declarations just before the final element.
         let lastDecl = declarations.pop();
         declarations = [...declarations, ...newDecls, lastDecl];
       } else {
-        current += " ";
+        current = current.trimRight() + " ";
       }
     } else {
+      if (importantState > 0) {
+        current += "!";
+        if (importantWS) {
+          current += " ";
+        }
+        if (importantState === 2) {
+          current += "important ";
+        }
+        importantState = 0;
+        importantWS = false;
+      }
       current += inputString.substring(token.startOffset, token.endOffset);
     }
   }
 
   // Handle whatever trailing properties or values might still be there
   if (current) {
     if (!lastProp.name) {
       // Ignore this case in comments.
       if (!inComment) {
         // Trailing property found, e.g. p1:v1;p2:v2;p3
         lastProp.name = cssTrim(current);
       }
     } else {
       // Trailing value found, i.e. value without an ending ;
+      if (importantState === 2) {
+        lastProp.priority = "important";
+      } else if (importantState === 1) {
+        current += "!";
+      }
       lastProp.value = cssTrim(current);
       let terminator = lexer.performEOFFixup("", true);
       lastProp.terminator = terminator + ";";
       // If the input was unterminated, attribute the remainder to
       // this property.  This avoids some bad behavior when rewriting
       // an unterminated comment.
       if (terminator) {
         lastProp.offsets[1] = inputString.length;