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

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screenshot of bug

Comment 2

[Daniel Holbert [:dholbert]]

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.

Comment 3

[Daniel Holbert [:dholbert]]

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.

Comment 4

[Patrick Brosset <:pbro>]

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;"

Comment 5

[Patrick Brosset <:pbro>]

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?

Comment 6

[Tom Tromey :tromey]

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.

Comment 7

[Tom Tromey :tromey]

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.

Comment 8

[Tom Tromey :tromey]

Attachment: Bug 1287620 - don't de-quote identifier tokens in parseDeclarations;

Review commit: https://reviewboard.mozilla.org/r/65308/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65308/

Comment 9

[Tom Tromey :tromey]

Clearing needinfo.

Comment 10

[Patrick Brosset <:pbro>]

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.

Comment 11

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b540f7a0df85a0186c9e92700636e08e995726e

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cf94b234e6a
don't de-quote identifier tokens in parseDeclarations; r=pbro

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7cf94b234e6a

Comment 14

[Mohammad Maruf Rahman]

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

Comment 15

[Farhadur Reja Fahim]

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]