Closed Bug 947733 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: ntim, Assigned: pbro)

References

Details

Attachments

(1 file, 2 obsolete files)

Title says all
Summary: CSS declarations with !important don't show color swatches → Inspector tooltips don't work when css declarations have !important
Tested with color swatches and background-image tooltips
Flags: needinfo?(pbrosset)
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.
Flags: needinfo?(pbrosset)
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.
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.
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).
Assignee: nobody → pbrosset
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8360501 - Flags: review?(mratcliffe)
(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.
(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.
(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.
(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.
Mike, here's a V2 patch with a couple more test cases.
Attachment #8360501 - Attachment is obsolete: true
Attachment #8360501 - Flags: review?(mratcliffe)
Attachment #8360585 - Flags: review?(mratcliffe)
(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.
(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.
Attachment #8360585 - Flags: review?(mratcliffe) → review+
Green try and R+ (thanks Mike!).
Going to check this in.
Simply rebased. So applying R+
Attachment #8360585 - Attachment is obsolete: true
Attachment #8367927 - Flags: review+
Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/2e08d371f66b
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2e08d371f66b
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Duplicate of this bug: 952960
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.