1462553 - Use of !important before the end of a value is wrongly considered valid in the rule-view

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P2)

Description

[Patrick Brosset <:pbro>]

STR:

- open the following test page in a tab:
data:text/html,<style>body{background: !important blue;}</style>
- notice that the background is white, because the background value is invalid
- open the inspector and look at the Rules view.

Expected:
it should display background: !important blue; exactly and mark it as invalid

Actual:
it displays background: blue !important;  and thinks it's valid.

Comment 1

[Patrick Brosset <:pbro>]

I investigated a little bit and found the root cause to be in 
/devtools/shared/css/parsing-utils.js
in the parseDeclarationsInternal function.

Somewhere in there, when we find a ! token, then we remember that we've seen it, then later, when we find an important ident, we set the priority of the declaration to important.
However we do regardless of the position of these things. So if they appear at the start of the value, the declaration is still marked as important.

I believe !important only ever works when it appears as the very last part of a value.

Comment 2

[Patrick Brosset <:pbro>]

From the cascade spec [1]: "the delimiter token "!" and keyword "important" follow the declaration".
That means they need to appear at the end of the declaration. Note that there can be spaces/tabs between ! and important.

[1] https://www.w3.org/TR/CSS2/cascade.html#important-rules

Comment 3

[Patrick Brosset <:pbro>]

Adding something like this inside this function's while loop:
const isLastToken = !inputString.substring(token.endOffset).trim();
and then using it later to condition adding the important state to the declaration would work I think.

Comment 4

[Tom Tromey :tromey]

I have a patch.

Comment 5

[Tom Tromey :tromey]

Attachment: Bug 1462553 - fix !important parsing in devtools;

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.

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

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8977075 [details]
Bug 1462553 - fix !important parsing in devtools;

https://reviewboard.mozilla.org/r/245150/#review251322

Thank you Tom!

::: devtools/shared/css/parsing-utils.js:386
(Diff revision 1)
> -        if (hasBang) {
> +        if (importantState === 1) {
>            current += "!";
> +          importantState = 0;
> +        } else if (importantState === 2) {
> +          // The "!important" is no longer valid.
> +          current += "!important ";

Do we care about preserving potential spaces between ! and important? 
The current patch doesn't.

So, something like this:
p:    !     important v;

will produce
p:    !important v;

If this is hard to fix, I don't think we should care too much. In fact I'll R+ the patch with this because this is probably never going to occur.
But just curious.

::: devtools/shared/css/parsing-utils.js:419
(Diff revision 1)
> +      if (importantState === 1) {
> +        current += "!";
> +        importantState = 0;
> +      } else if (importantState === 2) {
> +        // The !important is no longer valid.
> +        current += "!important ";

Same comment here.

Comment 7

[Tom Tromey :tromey]

(In reply to Patrick Brosset <:pbro> from comment #6)

> Do we care about preserving potential spaces between ! and important? 
> The current patch doesn't.

I'll take a look to see how hard it would be.
I suppose it's just one more state in the state machine.

It's maybe worth noting that we don't really try to faithfully preserve whitespace
here already.  Multiple spaces and comments are collapsed to a single space.

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8977075 [details]
Bug 1462553 - fix !important parsing in devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245150/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cbb0b27b6e59
fix !important parsing in devtools; r=pbro

Comment 10

[Tom Tromey :tromey]

I pushed it with (some) whitespace preservation.

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/cbb0b27b6e59

Comment 12

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 62.0a1 (2018-05-18) on Windows 10, 64 Bit!
This bug's fix is verified with latest Beta!

Build ID 	20180713213322
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0