947733 - Inspector tooltips don't work when css declarations have !important

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Tim Nguyen :ntim]

Title says all

Comment 1

[Tim Nguyen :ntim]

Tested with color swatches and background-image tooltips

Comment 2

[Patrick Brosset <:pbro>]

Indeed, it seems that as soon as !important is in a CSS value, it doesn't get interpreted the same way as the others.

STR:
- Open the inspector and select a node that has a CSS color applied somewhere.
- Verify that a color swatch exists for the color value and that clicking it opens the color picker.
- Add !important at the end of this value and commit the change.
- Verify that no color swatch exists anymore.

The same happens for background-image URLs.

Comment 3

[Patrick Brosset <:pbro>]

It works however with css-transform preview tooltips, narrowing down the cause of the bug to the output-parser.
Indeed, the output-parser is the piece of code we use to parse the CSS value into something understandable and, among other things, it's the one that recognizes image uri and color patterns.

Comment 4

[Patrick Brosset <:pbro>]

I found a fix and, while creating a test for it, I stumbled upon 2 other bugs:

In this declaration:
content: 'red'
red gets interpreted as a color

In this declaration:
color: transparent
transparent doesn't get interpreted as a color.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: bug947733-ruleview-tooltips-on-important-properties.patch

Hi Mike, this patch fixes the 3 following things in output-parser:

- values with !important in them are parsed correctly now (just striping !important from the value part in _cssPropertySupportsValue).
- single-quoted strings in values don't get parsed, just like strings with double-quotes.
- transparent is treated as a valid color.

Also, there's a new test in there that can be use to quickly test all sorts of outputs (the test doesn't require any UI, so it runs quickly, and it's easy to add more and more edge cases as we encounter them).

Comment 6

[Tim Nguyen :ntim]

(In reply to Patrick Brosset [:pbrosset] from comment #4)
> I found a fix and, while creating a test for it, I stumbled upon 2 other
> bugs:
> 
> In this declaration:
> content: 'red'
> red gets interpreted as a color

It doesn't seem for me.

> In this declaration:
> color: transparent
> transparent doesn't get interpreted as a color.

There was a bug that removed color swatches for special values (including transparent), since it was causing weird things with currentColor and inherit. But I guess transparent could be back.

Comment 7

[Tim Nguyen :ntim]

(In reply to Patrick Brosset [:pbrosset] from comment #5)
> Created attachment 8360501 [details] [diff] [review]
> bug947733-ruleview-tooltips-on-important-properties.patch
> 
> Hi Mike, this patch fixes the 3 following things in output-parser:
> 
> - values with !important in them are parsed correctly now (just striping
> !important from the value part in _cssPropertySupportsValue).
> - single-quoted strings in values don't get parsed, just like strings with
> double-quotes.
> - transparent is treated as a valid color.
> 
> Also, there's a new test in there that can be use to quickly test all sorts
> of outputs (the test doesn't require any UI, so it runs quickly, and it's
> easy to add more and more edge cases as we encounter them).

I'd recommend adding linear/radial gradient tests too.

Comment 8

[Patrick Brosset <:pbro>]

(In reply to Tim Nguyen [:ntim] from comment #6)
> (In reply to Patrick Brosset [:pbrosset] from comment #4)
> > I found a fix and, while creating a test for it, I stumbled upon 2 other
> > bugs:
> > 
> > In this declaration:
> > content: 'red'
> > red gets interpreted as a color
> 
> It doesn't seem for me.
Try these simple steps:
- Inspect any element
- In the css rule-view, click to add a new property in any of the rules
- Enter content as the name and 'red' as the value (with the single-quotes)
=> For me, 'red' turns into '()red' with () being the color swatch.

> > In this declaration:
> > color: transparent
> > transparent doesn't get interpreted as a color.
> 
> There was a bug that removed color swatches for special values (including
> transparent), since it was causing weird things with currentColor and
> inherit. But I guess transparent could be back.
Yeah, I think it's both safe and useful to have transparent back in there.

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Tim Nguyen [:ntim] from comment #7)
> I'd recommend adding linear/radial gradient tests too.
You're right, these can be pretty complex in syntax and therefore are good test cases to be added.
I'll add this now.

Comment 10

[Patrick Brosset <:pbro>]

Attachment: bug947733-ruleview-tooltips-on-important-properties V2.patch

Mike, here's a V2 patch with a couple more test cases.

Comment 11

[Patrick Brosset <:pbro>]

And, ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=c4c0bce09b5f

Comment 12

[Tim Nguyen :ntim]

(In reply to Patrick Brosset [:pbrosset] from comment #10)
> Created attachment 8360585 [details] [diff] [review]
> bug947733-ruleview-tooltips-on-important-properties V2.patch
> 
> Mike, here's a V2 patch with a couple more test cases.

I think radial-gradient can be unprefixed. :)
Also, there are also repeating-linear-gradient and repeating-radial-gradient you also might want to test.

Comment 13

[Tim Nguyen :ntim]

(In reply to Patrick Brosset [:pbrosset] from comment #8)
> (In reply to Tim Nguyen [:ntim] from comment #6)
> > (In reply to Patrick Brosset [:pbrosset] from comment #4)
> > > I found a fix and, while creating a test for it, I stumbled upon 2 other
> > > bugs:
> > > 
> > > In this declaration:
> > > content: 'red'
> > > red gets interpreted as a color
> > 
> > It doesn't seem for me.
> Try these simple steps:
> - Inspect any element
> - In the css rule-view, click to add a new property in any of the rules
> - Enter content as the name and 'red' as the value (with the single-quotes)
> => For me, 'red' turns into '()red' with () being the color swatch.

Yep, I can reproduce that way. (I couldn't by editing the property value or by loading a testcase with content:'red').

Also, I noticed a bug that makes the color picker not update the value when using the picker itself. Unfortunately, I haven't figured out some specific STR. It seems really random.

Comment 14

[Patrick Brosset <:pbro>]

Green try and R+ (thanks Mike!).
Going to check this in.

Comment 15

[Patrick Brosset <:pbro>]

Attachment: bug947733-ruleview-tooltips-on-important-properties v3.patch

Simply rebased. So applying R+

Comment 16

[Patrick Brosset <:pbro>]

Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/2e08d371f66b

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2e08d371f66b