Closed Bug 1287620 Opened 4 years ago Closed 4 years ago

Devtools CSS Rules inspector incorrectly marks declarations with bogus character "\9" as valid & accepted


(DevTools :: Inspector: Rules, defect)

Not set


(firefox50 verified)

Firefox 50
Tracking Status
firefox50 --- verified


(Reporter: dholbert, Assigned: tromey)



(3 files)

Attached file testcase 1
I just ran across a CSS declaration that has "\9" at the end, which is apparently some ancient IE hack.  It seems that this makes the CSS invalid (because it's unexpected junk at the end of the declaration), but DevTools incorrectly says the declaration is just fine.

 1. Load attached testcase.
 2. Right-click the colorful div, and compare what devtools says in the "Rules" view about its color to its actual on-screen color.

* It paints as orange, but devtools Rules view shows:
       background: orange; <--- struck through, displayed as inactive.
       background: blue;   <--- displayed as being active.
(devtools "Computed" view shows that its computed background-color is orange, though.)
* Note that "\9" is not shown as part of the CSS.

* Devtools should show the "background:orange" as being the active one.
* It should also probably show the "\9" text, as a hint about why the second declaration failed to parse.
Attachment #8772190 - Attachment description: test-bg.html → testcase 1
Summary: Devtools CSS Rules inspector incorrectly marks rules with "\9" as valid & accepted → Devtools CSS Rules inspector incorrectly marks declarations with bogus character "\9" as valid & accepted
Apparently \9 is a hack that's used by web developers to target CSS at certain IE versions, since IE accepts it but other browsers reject it:     "/* IE6, IE7, IE8 */" "(15) \9 hack" [...] Internet Explorer 7 and above"  "Internet Explorer/Edge 6-8"

Anyway, it seems like this is a bogus garbage character that some IE versions incorrectly failed to check for when ending a CSS declaration. And it seems our devtools must strip it out by accident when testing whether a particular declaration can be successfully parsed, or something.
Also: the devtools Style Editor correctly shows the "\9" in the text of the CSS here. We only seem to strip it out in the Rules view.
It looks like the rule-view strips the "\9", and later, when it marks declarations as being overridden, it marks bavkground:orange as being overridden because background:blue comes after.
This probably comes from the way we parse CSS when retrieving rules for the element.
In particular parseDeclarations in devtools\shared\css-parsing-utils.js returns
{name: background, value: blue} for "background: blue \9;"
parseDeclarations uses the devtools css-lexer (devtools\shared\css-lexer.js). When I pass: "background: blue \\9;" to the lexer, it outputs the following tokens:

{ tokenType: "ident", startOffset: 0, endOffset: 10, text: "background" }
{ tokenType: "symbol", startOffset: 10, endOffset: 11, text: ":" }
{ tokenType: "whitespace", startOffset: 11, endOffset: 12 }
{ tokenType: "ident", startOffset: 12, endOffset: 16, text: "blue" }
{ tokenType: "whitespace", startOffset: 16, endOffset: 17 }
{ tokenType: "ident", startOffset: 17, endOffset: 19, text: "  " }
{ tokenType: "symbol", startOffset: 19, endOffset: 20, text: ";" }

So basically \9 becomes whitespace.

Tom: this is probably deep into css-lexer.js, which itself is a port of CSSLexer.cpp. Any idea why that is?
Flags: needinfo?(ttromey)
You can verify that css-lexer.js is doing the right thing with this patch:

diff --git a/devtools/shared/tests/unit/test_csslexer.js b/devtools/shared/tests/unit/test_csslexer.js
index cc55af7..056119e 100644
--- a/devtools/shared/tests/unit/test_csslexer.js
+++ b/devtools/shared/tests/unit/test_csslexer.js
@@ -148,7 +148,10 @@ var LEX_TESTS = [
   // earlier versions of CSS had "bad comment" tokens, but in level 3,
   // unterminated comments are just comments.
-  ["/* bad comment", ["comment"]]
+  ["/* bad comment", ["comment"]],
+  // Regression test for bug 1287620.
+  ["blue \\9", ["ident:blue", "whitespace", "ident:\u0009"]],
 function test_lexer_linecol(cssText, locations) {

Still looking for the bug.
The bug is this call to trim():

\9 refers to an identifier whose name consists of the tab character.
But, parseDeclarations turns this into an ordinary tab and then strips it -- oops.
Assignee: nobody → ttromey
Clearing needinfo.
Flags: needinfo?(ttromey)
Attachment #8772515 - Flags: review?(pbrosset) → review+
Comment on attachment 8772515 [details]
Bug 1287620 - don't de-quote identifier tokens in parseDeclarations;

Thanks for the quick investigation and the patch Tom! This looks good.
Pushed by
don't de-quote identifier tokens in parseDeclarations; r=pbro
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
I have reproduced this bug with  Nightly 50.0a1 (2016-07-18) on Windows 7 , 64 Bit!

This bug's fix is verified with latest Developer Edition (Aurora)

Build ID   20160803004014
User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0
I have reproduced this bug with Nightly 50.0a1 (2016-07-18) on Linux Mint 18, 64 bit!

The bug's fix is now verified on latest Aurora 50.0a2.

Aurora 50.0a2:
Build ID 	20160901004001
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0

Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.