924136 - css-color.js interprets "inherit" as black

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
let {colorUtils} = devtools.require("devtools/css-color");
let color = new colorUtils.CssColor("inherit");

color will have been interpreted as rgb(0, 0, 0). We need to treat it specially and always return "inherit" when it is used (similar to transparent).

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: css-color-js-interprets-inherit-as-black-924136.patch

Another simple one:
- Moved check for invalid color, "transparent" and "inherit" into a function.
- Return these values if necessary from each color type.
- Added "inherit" to browser/devtools/shared/test/browser_css_color.js

Comment 2

[Robert Longson [:longsonr]]

Do other keywords like "initial", "unset" (bug 921731) need special treatment too?

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Robert Longson from comment #2)
> Do other keywords like "initial", "unset" (bug 921731) need special
> treatment too?

I had checked the specs but only found "transparent" and "initial" but I agree that initial should receive this treatment.

I don't believe a color can be unset.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

 stand corrected, unset === initial when it comes to colors. I will add both.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: css-color-js-interprets-inherit-as-black-924136.patch

Added initial and unset.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Simplified variable name

Simplified variable name as an afterthought.

Comment 7

[Robert Longson [:longsonr]]

There is one other keyword: currentColor http://www.w3.org/TR/css3-color/#currentcolor I think you should test that too.

Comment 8

[Tim Nguyen :ntim]

I'm not sure if the bug is still there, but I've one seen transition:all 0.3s; replaced by transition:#000 0.3s;

Comment 9

[Tim Nguyen :ntim]

I think black listing those keywords is not a powerful way to solve this bug. It'll be not convenient for future implementations. I suggest you whitelist the color names instead.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 814428 [details] [diff] [review]
Simplified variable name

I agree, a color name list is a better fix.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Made special color values list dynamic

We now build the special color value list by doing the following:
1. Get all autocompletion values for color.
2. Remove all HTML, rgb, rgba, hsl, hsla and system colors from the list.

This way we are future proof.

Comment 12

[Joe Walker [:jwalker]]

Comment on attachment 815357 [details] [diff] [review]
Made special color values list dynamic

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

I find the definition of _getRGBATuple to be bizarre. It returns either a string or dictionary depending on something. Isn't there a way to specify 'transparent' in the tuple?
i.e. sometimes _getRGBATuple doesn't return a tuple. So it should be called _getRGBATupleOrStringDependingOnStuff() which should set alarm bells off.
What do you think?

::: toolkit/devtools/css-color.js
@@ +116,5 @@
> +      return "transparent";
> +    }
> +    if (SPECIALVALUES.has(this.authored)) {
> +      return this.authored;
> +    }

Isn't "transparent" a special value?

@@ +428,5 @@
>  loader.lazyGetter(this, "DOMUtils", function () {
>    return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
>  });
> +
> +loader.lazyGetter(this, "SPECIALVALUES", function () {

This needs some explanation as to what's going on.

I'm also concerned that new system colors will magically be marked 'special', so how is this code better than:

    const SPECIALVALUES = new Set([ 'inherit', 'initial', 'transparent', 'unset' ]);

@@ +486,5 @@
> +  let allColorValues = DOMUtils.getCSSValuesForProperty("color");
> +
> +  // Remove all color names in the HTML color table, rgb, rgba, hsl and hsla.
> +  let specialValues = allColorValues.filter(function(color) {
> +    return !(HTMLCOLORS.has(color) || color.contains("rgb") || color.contains("hsl"));

What if there is a color called roughslate. Shouldn't we test against the strings exactly?

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Fixed special values

(In reply to Joe Walker [:jwalker] from comment #12)
> Comment on attachment 815357 [details] [diff] [review]
> Made special color values list dynamic
> 
> Review of attachment 815357 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I find the definition of _getRGBATuple to be bizarre. It returns either a
> string or dictionary depending on something. Isn't there a way to specify
> 'transparent' in the tuple?
> i.e. sometimes _getRGBATuple doesn't return a tuple. So it should be called
> _getRGBATupleOrStringDependingOnStuff() which should set alarm bells off.
> What do you think?
> 

Ew, ugly hack, changed to return {r:0, g:0, b:0, a:0}

> ::: toolkit/devtools/css-color.js
> @@ +116,5 @@
> > +      return "transparent";
> > +    }
> > +    if (SPECIALVALUES.has(this.authored)) {
> > +      return this.authored;
> > +    }
> 
> Isn't "transparent" a special value?
> 

Yes, we were also processing e.g. rgba(0, 0, 0, 0) as "transparent." We now only treat "transparent" as "transparent."

> @@ +428,5 @@
> >  loader.lazyGetter(this, "DOMUtils", function () {
> >    return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
> >  });
> > +
> > +loader.lazyGetter(this, "SPECIALVALUES", function () {
> 
> This needs some explanation as to what's going on.
> 

Not any more, I have removed it.

> I'm also concerned that new system colors will magically be marked
> 'special', so how is this code better than:
> 
>     const SPECIALVALUES = new Set([ 'inherit', 'initial', 'transparent',
> 'unset' ]);
> 

I weighed up whether a new special value or -moz prefixed special color is most likely to be added and decided to calculate from there. e.g. 'all' was a special value for a few days. At the same time currentcolor is missing from the autocomplete list (bug 927367 logged).

A short list is more easily maintained and that has to win out.

Changed to:
const SPECIALVALUES = new Set([
  "currentcolor",
  "initial",
  "inherit",
  "transparent",
  "unset"
]);

> @@ +486,5 @@
> > +  let allColorValues = DOMUtils.getCSSValuesForProperty("color");
> > +
> > +  // Remove all color names in the HTML color table, rgb, rgba, hsl and hsla.
> > +  let specialValues = allColorValues.filter(function(color) {
> > +    return !(HTMLCOLORS.has(color) || color.contains("rgb") || color.contains("hsl"));
> 
> What if there is a color called roughslate. Shouldn't we test against the
> strings exactly?

That was what HTMLCOLORS.has(color) did but as we no longer generate the list this is not important.

Comment 14

[Joe Walker [:jwalker]]

Comment on attachment 817828 [details] [diff] [review]
Fixed special values

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

Thanks.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/600d74aea793

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/600d74aea793

Comment 17

[Bogdan Maris, Desktop Test Engineering]

Is there any help needed for manual testing here? If yes can you please provide some steps in order to reproduce and verify that this is fixed?

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #17)
> Is there any help needed for manual testing here? If yes can you please
> provide some steps in order to reproduce and verify that this is fixed?

Sure:
1. Open http://fiddle.jshell.net/5rNrx/show/
2. Right-click the page and choose inspect element
3. In the markup panel select the body tag
4. Ensure that in the rule view it displays background-color as inherit (not black).

Comment 19

[Bogdan Maris, Desktop Test Engineering]

Thanks Michael,
Backgrould-color is displayed as inherit so this is verified as fixed using the latest Aurora 27.0a2 (buildID: 20131203004003) on Windows 7 x64, Ubuntu 12.04 x64 and Mac OS X 10.8.5.