Closed Bug 971234 Opened 10 years ago Closed 9 years ago

Styles inspector interprets words that happen to be named colors as colors in non-color declarations

Categories

(DevTools :: Inspector, defect, P2)

29 Branch
defect

Tracking

(firefox40 fixed, firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: me, Assigned: tromey)

References

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(1 file, 10 obsolete files)

14.26 KB, patch
tromey
: review+
Details | Diff | Splinter Review
Minimal test case:

    data:text/html,<style>*{font-family:Crimson Text}</style>

Open the Inspect panel of the Developer Tools and observe the value of the `font-family` rule for the selector `*` in the Rules panel.

Expected result: text, `Crimson Text`.

Actual result: `● #DC143C Text` (with the ● being crimson in color, a proper color selector).

It has incorrectly interpreted the word "Crimson" as a color.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
I've been taking a look at this to see if I can fix it myself.

It comes down to the color-detecting part of OutputParser (toolkit/devtools/output-parser.js) not paying any attention to the CSS property name. parseCssProperty takes the name, but then it's discarded when it passes it on to _parse, which actually produces the swatches.

The simple, imperfect solution that I see is this:

- Make OutputParser._parse take the property name as well as the value. (Note that parseHTMLAttribute also calls _parse and it *doesn't* have the property name. Have property name default to undefined.)

- Inside _parse, instead of blindly calling this._appendColorOnMatch, first checks if the property is in the list of ones that *cannot* contain a color. If it is, then it skips it.

This of course doesn't cope with any properties which can have both a color and arbitrary unquoted text in their values, but I can't think of any immediately.

The question also remains of where such a list would be maintained, and I don't know the correct answer to that. Should it be in DOMUtils like getCSSPropertyNames()?
(In reply to Chris Morgan from comment #1)
> I've been taking a look at this to see if I can fix it myself.
> 

Great!

> It comes down to the color-detecting part of OutputParser
> (toolkit/devtools/output-parser.js) not paying any attention to the CSS
> property name. parseCssProperty takes the name, but then it's discarded when
> it passes it on to _parse, which actually produces the swatches.
> 
> The simple, imperfect solution that I see is this:
> 
> - Make OutputParser._parse take the property name as well as the value.
> (Note that parseHTMLAttribute also calls _parse and it *doesn't* have the
> property name. Have property name default to undefined.)
> 

parseHTMLAttribute() is no longer used so we should remove it.

> - Inside _parse, instead of blindly calling this._appendColorOnMatch, first
> checks if the property is in the list of ones that *cannot* contain a color.
> If it is, then it skips it.
> 

Even better, we can use this method:
function isColorProperty(prop) {
  return DOMUtils.getCSSValuesForProperty(prop).indexOf("rgb") !== -1;    
}

Then isColorProperty("color") will return true and isColorProperty("font") will return false.


> This of course doesn't cope with any properties which can have both a color
> and arbitrary unquoted text in their values, but I can't think of any
> immediately.
> 
> The question also remains of where such a list would be maintained, and I
> don't know the correct answer to that. Should it be in DOMUtils like
> getCSSPropertyNames()?

If you use the method above you can easily check whether or not a CSS property can contain a color so you don't need to maintain a list.
I have a more-or-less completed patch (including handling CSS variables properly), but getCSSValuesForProperty just doesn't cut it for detecting whether a property can contain a color or not.

background-image, for example, can contain a linear-gradient, which can contain a color, but what does DOMUtils.getCSSValuesForProperty("background-image") get us?

    ["-moz-element", "-moz-image-rect", "inherit", "initial", "none", "unset", "url"]

☹

Any other suggestions?
Flags: needinfo?
Our css parsing process is a bit of a mess today.

When asking for the styles applied to a particular element:

1) we send a request to our styles actor, which sends back a string containing all declarations. So something like that: "color: blue, font-family: arial; background-image: linear-gradient(to left, blue, red);".

2) on the client-side (in the rule-view), we parse this string using css-parsing-utils.js.
This is a simple tokenizer that doesn't give type-related information or internal value structure. It only will give you something like this: [{name: "color", value: "blue"}, {name: "font-family", value: "arial"}, {name: background-image", value: "linear-gradient(to left, blue, red)"}]

3) when creating each property in the rule-view (TextPropertyEditor), we parse the value again, with another simple parser: output-parser.js. This one is specialized in parsing css values to recognize urls, colors, etc. and as you saw, it fails because it doesn't know where to expect colors and where to expect just strings.

I don't see any immediate trick we could use to quickly fix this.
The only longer term plan I can see is actually using a real css parser that we would couple to our already existing css tokenizer.

The gecko css parser doesn't expose today the internal structure and types of each property. I don't think it makes sense to ask it to because it would have to keep in memory these extra information, and also parse all invalid css properties (like vendor-prefixed properties) which it doesn't do today.

It might not be so hard to write a parser for our use cases, knowing that we don't care about all the different types of values, we really only want to extract urls and colors so far.
Flags: needinfo?
The test case here is flawed as Crimson Text contains a space so it should be surrounded in quotes, in which case it works correctly:
data:text/html,<style>*{font-family:"Crimson Text"}</style>

There is still the case where a custom font could be badly named:
data:text/html,<style>*{font-family:red}</style>

Regardless of this we should be checking the property names to check if a color should be valid.
Assignee: nobody → mratcliffe
data:text/html,<style>*{font-family:arial black}</style><div>Black Text

The "Black Text" shows up in Arial Black, and the inspector says: "arial () #000".
So the browser is parsing <arial black> the same as it would parse <"arial black"> (for the 'font-family' case. 'font' is different for obvious reasons)
We used to use the output parser in the markup view but removed it some time back so we now only use it for parsing CSS attributes. This meant that I was able to remove a bunch of cruft that we no longer need, namely parseHTMLAttribute and all related items.

We now only parse colors in CSS properties that really do support them.
Attachment #8414545 - Flags: review?(jwalker)
What about: "font: bold 18px arial black;"?
Will your fix make it so that it doesn't appear as "font: bold 18px arial () #000;"?

What if we had a blacklist of property names we know their values can never be colors?
For all other properties, it might be safe to parse their values to find colors.
Comment on attachment 8414545 [details] [diff] [review]
not-checking-for-color-attribute-971234.patch

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #9)
> What about: "font: bold 18px arial black;"?
> Will your fix make it so that it doesn't appear as "font: bold 18px arial ()
> #000;"?
> 
> What if we had a blacklist of property names we know their values can never
> be colors?
> For all other properties, it might be safe to parse their values to find
> colors.

Hmm... you are right, we need a whitelist or at least a reliable way to check if a property accepts a color.
Attachment #8414545 - Flags: review?(jwalker)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #10)
> Comment on attachment 8414545 [details] [diff] [review]
> not-checking-for-color-attribute-971234.patch
> 
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #9)
> > What about: "font: bold 18px arial black;"?
> > Will your fix make it so that it doesn't appear as "font: bold 18px arial ()
> > #000;"?
> > 
> > What if we had a blacklist of property names we know their values can never
> > be colors?
> > For all other properties, it might be safe to parse their values to find
> > colors.
> 
> Hmm... you are right, we need a whitelist or at least a reliable way to
> check if a property accepts a color.

Blacklists are difficult to maintain so I decided to check if the actual property can accept colors or not. We do this by checking if "rgba" is in the autocomplete options for a property or if any of it's autocomplete values contain the word gradient. We specifically block "font-family" as it will never contain valid color values.

This should work in all situations I can think of.
Attachment #8414545 - Attachment is obsolete: true
Attachment #8415859 - Flags: review?(pbrosset)
Comment on attachment 8415859 [details] [diff] [review]
Properly check that color names match values

Review of attachment 8415859 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this is the right approach yet (see my comments in the code below).

Not sure what's the best solution to deal with this problem, but the black list idea I formulated earlier seems feasible to me.
I just went over a list of ~180 css properties (I think that's all of them, but can't be sure):

- the vast majority of them (~150) are properties that accepts specific keywords, neither colors, nor free-text (i.e. transition-delay, margin-left, ...),
- less than 10 accept only colors (i.e. color, ouline-color, ...),
- about 10 accept complex values that may contain colors, these are usually shorthand properties (i.e. border, box-shadow, ...),
- the rest, less than 10, accept some sort of free text. In this group are all the URL types of properties (list-style-image, icon, background-image, ...), the quoted strings (content) and the unquoted strings (font, font-family, animation, animation-name).

I don't remember exactly where this is done but our code already avoids parsing quoted strings and urls, so the number of cases where an author may have used a value named after a color is pretty low. This makes me think that simply maintaining a list of properties we shouldn't try to parse at all is a good idea. I would start with this:

animation
animation-name
background-image
content
font
font-family
icon
list-style-image

Also, I just realized that in rule-view.js, in `TextPropertyEditor.prototype.update()`, we call `this.isValid()`, which checks if the property name accepts a given property value, which is exactly what `OutputParser.prototype._cssPropertySupportsValue()` does too. So, we essentially do the work twice.
One fix we should therefore do is, in `TextPropertyEditor.prototype.update()`, if `this.isValid()` returns false, then don't even bother using the outputParser. Only do this if it's true.

::: toolkit/devtools/output-parser.js
@@ +238,5 @@
>     *         CSS Property name to check
>     * @param  {String} value
>     *         CSS Property value to check
>     */
>    _cssPropertySupportsValue: function(name, value) {

This function has now 2 very distinct parts: the part that actually checks if a given property supports a given value, and the part that checks if a color can be expected inside a given property's value expression.
I think these 2 parts should be split in 2 functions.

@@ +253,5 @@
> +      if (autoCompleteValues.indexOf("rgba") !== -1) {
> +        return true;
> +      }
> +
> +      let containsGradient = !!autoCompleteValues.find(element => {

I thought this wouldn't work because DOMUtils.getCSSValuesForProperty only returned [-moz-element,-moz-image-rect,inherit,initial,none,unset,url] for background-image on Aurora. But it seems to return all the gradient expressions too on Nightly now.

However, box-shadow won't work, because DOMUtils.getCSSValuesForProperty doesn't return any color-related values for it.

I'm pretty sure there might be other cases too (what about vendor-prefixed properties? Oh and another example I found: `animation: black;`).

Also, this line of code is inside the `if(this._isValidColor(value))` block, so I don't see how we can find gradient values here.
Attachment #8415859 - Flags: review?(pbrosset)
Gradients are defined as CSS <image> data types. Therefore they can be used anywhere in CSS where an image data type is required. But, Gecko currently only supports CSS gradients as values of the background-image property, as well as within the shorthand background.

The reason that DOMUtils.getCSSValuesForProperty("background-image") returns more than [-moz-element,-moz-image-rect,inherit,initial,none,unset,url] in nightly is because CSS gradients were added to it this release.

It is true that a blacklist is probably better than the method I used but as you can see our CSS properties are in continuous development so I hesitate to use a blacklist approach... otherwise we would not only have to update the blacklist every time a new CSS property is added but we would have to update it whenever a property is updated to support CSS gradients.

I will see if there is a platform way to check this, otherwise we should ask for an API, something like:
DOMUtils.GetCSSPropertySupportsColor("background-image")
DOMUtils.GetCSSPropertySupportsGradient("background-image")

This would be way more future proof.
Whiteboard: [devedition-40]
Jeff, I thinks this makes sense in the list for devedition-40, do you agree? Please assign a priority.
Flags: needinfo?(jgriffiths)
As discussed on IRC... assigning this to ttromey so that he can use our new CSS API to fix up the output parser.
Assignee: mratcliffe → ttromey
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #14)
> Jeff, I thinks this makes sense in the list for devedition-40, do you agree?
> Please assign a priority.

+1, done.
Flags: needinfo?(jgriffiths)
Priority: -- → P2
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
This patch uses cssPropertySupportsType to determine whether the color
swatch is needed.  I added a box-shadow test case.  I chose not to use
the CSS lexer in this patch, instead deferring a rewrite of
output-parser.js to bug 1154809, as this reduces this bug's
dependencies.
Attachment #8415859 - Attachment is obsolete: true
Attachment #8594926 - Flags: review?(pbrosset)
Comment on attachment 8594926 [details] [diff] [review]
only apply color swatch depending on cssPropertySupportsType

Review of attachment 8594926 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. I like it when code gets removed!
I made a comment about adding more test cases in browser_outputparser.js
And while testing, I realized that we still have some color detection related problems. Try the following declaration:
filter: blur(1px) drop-shadow(0 0 0 blue) url("red.svg#blue");
the words red and blue inside the url function are not colors, that works, but the word blue inside the drop-shadow function is a color, and is not highlighted as such in the rule-view with your patch.
That's because cssPropertySupportsType can't tell us if part of the property supports colors, and other parts not.
I don't think we can fix this without a proper parsing of the value, to know when we're in a context that expects colors and when not.

Overall, I'm ok to land this as such, because it simplifies the code, and because it gets rid of the main problem (font-family: arial black), and because it's less of a problem if a color swatch is missing than when one is added extra. So let's go with this, if TRY is green, and let's wait for bug 1154809 to fully rewrite output-parser.

::: browser/devtools/shared/test/browser_outputparser.js
@@ +59,5 @@
> +      value: "0 0 1em red",
> +      expected: '0 0 1em <span data-color="#F00">' +
> +        '<span style="background-color:red" class="test-box-shadow">' +
> +        '</span><span>#F00</span></span>'
> +    }

Thanks for having made this test loop through test cases, that makes it easier to add more in the future.
Maybe you could add a few test cases in this patch:
- multiple box-shadows:
  box-shadow: 0 0 1em red, 2px 2px 0 0 rgba(0, 0, 0, .5);
- content string:
  content: "red";
- filters:
  filter: blur(1px) drop-shadow(0 0 0 blue) url(red.svg#blue);
Attachment #8594926 - Flags: review?(pbrosset) → review+
Here's an updated version with the new tests.  I commented out the
"filter" case as the parser isn't prepared for this yet.  I refactored
a bit to make it even simpler to write a new test.
Attachment #8594926 - Attachment is obsolete: true
Comment on attachment 8595438 [details] [diff] [review]
only apply color swatch depending on cssPropertySupportsType

Re-requesting review as I refactored the test case a bit.
Attachment #8595438 - Flags: review?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #18)

> Overall, I'm ok to land this as such, because it simplifies the code, and
> because it gets rid of the main problem (font-family: arial black), and
> because it's less of a problem if a color swatch is missing than when one is
> added extra. So let's go with this, if TRY is green, and let's wait for bug
> 1154809 to fully rewrite output-parser.

I will definitely push to try.
I have been waiting because the dependency of this bug is still
waiting for review.

> Thanks for having made this test loop through test cases, that makes it
> easier to add more in the future.

Thanks!  But the credit really goes to Mike, who did most of this patch.

> - filters:
>   filter: blur(1px) drop-shadow(0 0 0 blue) url(red.svg#blue);

As noted in the patch this doesn't work yet.
However I will make a note in the other bug.
Comment on attachment 8595438 [details] [diff] [review]
only apply color swatch depending on cssPropertySupportsType

Review of attachment 8595438 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with some comments about making makeColorTest more self-explanatory, but no need to ask again for review.

::: browser/devtools/shared/test/browser_outputparser.js
@@ +29,5 @@
> +// expected strings.  Otherwise, it must be an object with a |value|
> +// property and a |name| property.  These describe the color.  This
> +// approach is taken to reduce boilerplate and to make it simpler to
> +// modify the test when the parseCssProperty output changes.
> +function makeColorTest(className, name, segments) {

I like the fact that this new function simplifies writing more tests, but I have a couple of comments about it:
- className doesn't seem all that useful anymore when adding new tests. It's good that the test logic in the loop further down defines one for colorSwatchClass, and later checks that it's found in the fragment HTML, but I don't see a reason to ask for each test case to define this class. Maybe it should always just be hard-coded, so you could remove this argument.
- In the segments array, it's not very clear what's expected vs. what's the input. I had to read the code in this function a couple of times to understand. It probably is just a naming thing. Like, when I call makeColorTest, I can pass a property name, but no property value. And then I need to pass a list of strings/objects which are both used as the input value and the expected value. So I think, even if that means sometimes duplicating data, it would make more sense if makeColorTest also accepted a value string as argument. This way you could remove the result.value string concatenation.
- Finally, again about naming, the {name, value} objects in the segment list is a bit misleading because it feels like a css declaration name and value, but it's not, it's really more input vs. expectedOutput.
Anyway, if you add a value argument to makeColorTest, the confusion would go away.

@@ +32,5 @@
> +// modify the test when the parseCssProperty output changes.
> +function makeColorTest(className, name, segments) {
> +  let result = {
> +    className: className,
> +    name: name,

can be simplified to:
let result = {
  className,
  name,
  value: "",
  expected: ""
};

@@ +52,5 @@
> +  }
> +
> +  result.desc = "Testing " + name + ": " + result.value;
> +
> +  info("COMPUTED " + JSON.stringify(result));

is this a debug info that you forgot to remove? If not, fine, but it probably needs something better than just COMPUTED. Maybe "Building a color test case with "
Attachment #8595438 - Flags: review?(pbrosset) → review+
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #22)

> - In the segments array, it's not very clear what's expected vs. what's the
> input. I had to read the code in this function a couple of times to
> understand.

I did it this way to reduce the amount of duplication needed.
But, it isn't a lot of text, so I went ahead and made the change.

> is this a debug info that you forgot to remove? 

Yeah, sorry about that.  It's gone now.
Here's the new version.  I made the requested changes and also updated
the makeColorTest comment to explain the color object a bit better.

Carrying over the r+; but note that this still needs to wait for the
dependency to be reviewed.
Attachment #8595438 - Attachment is obsolete: true
Attachment #8596588 - Flags: review+
One more revision -- today I realized that
OutputParser.parseCssProperty could also check TYPE_TIMING_FUNCTION.
Attachment #8596588 - Attachment is obsolete: true
Blocks: 1158288
Mention the correct (new) bug number in the commented-out test.
Attachment #8597349 - Attachment is obsolete: true
Comment on attachment 8597390 [details] [diff] [review]
only apply color swatch depending on cssPropertySupportsType

Review of attachment 8597390 [details] [diff] [review]:
-----------------------------------------------------------------

Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8596588&action=interdiff&newid=8597390&headers=1
Looks good to me.
Attachment #8597390 - Flags: review+
Attachment #8602835 - Flags: review+
Attachment #8597390 - Attachment is obsolete: true
Testing showed that we hit the case where cssPropertySupportsType
throws an exception.  This happens in
browser_ruleview_multiple-properties-unfinished_02.js.

This version introduces a wrapper function in output-parser.js to deal
with this situation.

Arguably perhaps we should change inDOMUtils::CssPropertySupportsType
instead.

Re-requesting review.
Attachment #8602835 - Attachment is obsolete: true
Attachment #8603004 - Flags: review?(pbrosset)
Comment on attachment 8603004 [details] [diff] [review]
only apply color swatch depending on cssPropertySupportsType

Review of attachment 8603004 [details] [diff] [review]:
-----------------------------------------------------------------

As far as I can see, the interdiff with the last patch is simply the try/catch wrapper function around DOMUtils, which looks fine to me. If a property is invalid, we don't want to try and parse the value to highlight colors and stuff. So that works for me.

::: browser/devtools/shared/test/browser_outputparser.js
@@ +25,5 @@
> +// Class name used in color swatch.
> +let COLOR_TEST_CLASS = 'test-class';
> +
> +// Create a new CSS color-parsing test.  |name| is the name of the CSS
> +// property.  |input| is the CSS text to use.  |segments| is an array

s/input/value

@@ +90,5 @@
> +    // See bug 1158288.
> +    // makeColorTest("test-filters", "filter",
> +    //               ["blur(1px) drop-shadow(0 0 0 ", {name: "blue", value: "#00F"},
> +    //                ") url(red.svg#blue)"])
> +

Since your last change was about handling invalid properties, that makes me think we should have a test case here with an invalid property too.

::: toolkit/devtools/output-parser.js
@@ +565,5 @@
>    }
>  };
> +
> +/* A wrapper for DOMUtils.cssPropertySupportsType that ignores invalid
> +   properties.  */

Can you use the jsdoc style comment here?

/**
 * A wrapper for DOMUtils.cssPropertySupportsType that ignores invalid
 * properties.
 * @param {String} name The property name.
 * @param {String} type The type tested for support.
 * @return {Boolean}
 */
Attachment #8603004 - Flags: review?(pbrosset) → review+
This addresses the review comments.
I'm going to carry over the r+.
Attachment #8603004 - Attachment is obsolete: true
Attachment #8604080 - Flags: review+
I had a silly doc bug.
Attachment #8604080 - Attachment is obsolete: true
Attachment #8604088 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/1e2f8f31ac9c
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1e2f8f31ac9c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=easy][fixed-in-fx-team] → [devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 41
Comment on attachment 8604088 [details] [diff] [review]
only apply color swatch depending on cssPropertySupportsType

Approval Request Comment
[Feature/regressing bug #]: bug 918716
[User impact if declined]:
In the inspector, color swatches may appear in unusual and incorrect places.
It's confusing to users when it happens, and is also a common "demo bug".
[Describe test coverage new/current, TreeHerder]:
New tests are added in the patch, including tests for the cases currently
known to fail.
[Risks and why]: 
We of course believe this change to be correct.  However, the main risk is
that, if it is incorrect, then the style inspector might not show a color
swatch where one ought to appear.  If this were to happen, it would not be
very serious, as the user can still edit the color by hand.
[String/UUID change made/needed]: None.
Attachment #8604088 - Flags: approval-mozilla-aurora?
Attachment #8604088 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.