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

Categories

(DevTools :: Inspector: Rules, defect)

defect
Not set

Tracking

(firefox50 verified)

VERIFIED FIXED
Firefox 50
Tracking Status
firefox50 --- verified

People

(Reporter: dholbert, Assigned: tromey)

Details

Attachments

(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.

STR:
 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.

ACTUAL RESULTS:
* 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.

EXPECTED RESULTS:
* 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:
https://css-tricks.com/snippets/css/browser-specific-hacks/     "/* IE6, IE7, IE8 */"
http://mynthon.net/howto/-/webdev/CSS-big-list-of-css-hacks.txt "(15) \9 hack" [...] Internet Explorer 7 and above"
http://browserhacks.com/#hack-69e3a6b66e01ffc5d9ef77c0ff65aa1d  "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():

https://dxr.mozilla.org/mozilla-central/source/devtools/shared/css-parsing-utils.js#386

\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
Status: NEW → ASSIGNED
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;

https://reviewboard.mozilla.org/r/65308/#review62456

Thanks for the quick investigation and the patch Tom! This looks good.
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7cf94b234e6a
don't de-quote identifier tokens in parseDeclarations; r=pbro
https://hg.mozilla.org/mozilla-central/rev/7cf94b234e6a
Status: ASSIGNED → RESOLVED
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

[bugday-20160831]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.