Bug 9506 – Implement HSL and HSLA colors in CSS

• Component: CSS
• Status: RESOLVED FIXED

[Joost de Valk (AlthA)]

The CSS3 color module introduces, next to RGBA colors, HSL and HSLA colors. The Hue-Saturation-Lightness colors are very easy to use and easier to get used to. Mozilla has them implemented, the testcase in the URL above even works in Firefox 1.5.

Because the CSS3 color module is a CR, i am of the opinion that WebKit should implement both HSL and HSLA colors. The url for the CR: http://www.w3.org/TR/css3-color . A testcase for the HSLA colours will be added to the above page in the next few weeks, but could be added faster if needed.

[Dave Hyatt]

This is very easy to implement.

[Joost de Valk (AlthA)]

Cool, it would really be nice to have.

[Joost de Valk (AlthA)]

Spoke with Hyatt and Olliej about this on IRC, apparently the HSL to RGB functions are already in Color, so you could convert HSL to RGB on load. I am not a coder though :)

[David Carson]

Could not find build in support for HSL.  Suggestion was to code it from algorithm defined here 
http://en.wikipedia.org/wiki/HSL_color_space and here http://www.w3.org/TR/css3-color/#hsl-color

[David Carson]

It seems that the current CSS3 spec examples for HSL are wrong:
* { color: hsl(120, 100%, 25%) } /* light green */ 
* { color: hsl(120, 100%, 75%) } /* dark green */ 

should actually be:
* { color: hsl(120, 100%, 25%) } /* dark green */ 
* { color: hsl(120, 100%, 75%) } /* light green */ 

The last parameter is lightness. The higher the value the lighter the colour.

[David Carson]

Created attachment 9155 [details]
patch, test cases and change logs

Added support for HSL and HSLA colors.

[Darin Adler]

Comment on attachment 9155 [details]
patch, test cases and change logs

Looks pretty good. I have some relatively unimportant comments, but I think I'm going to review- so you can address at least some of them.

Some minor formatting mistakes:

+    ValueList *args = value->function->args;
+    Value *v = args->current();

Please put the * next to tyhe type.

+    colorValues[0] = ((((int)(v->fValue) % 360) + 360) % 360)/360.;

Need spaces around all the operators, including the "/" and we prefer 360.0 to 360. for double constants.

+    for (int i = 1; i < 3; i++)
+    {

Please put the { on the same line as the for statement.

+    colorValues[0] = ((((int)(v->fValue) % 360) + 360) % 360)/360.;

What does the line above to if fValue does not fit in an int? Need to check if it's in range?

+        colorValues[i] = v->fValue/100.; // needs to be value between 0 and 1.0

This code says it "needs to be" -- what checks that the number is between 0 and 100?

-        if (!validUnit(v, FInteger|FPercent, true))

Spaces around operators like "|" too.

In makeRGBAfromHSLA, we'd normally capitalize the f.

+        bool parseColorParameters(Value* value, int* colorValues, bool parseAlpha);
+        bool parseHSLParameters(Value* value, float* colorValues, bool parseAlpha);

We'd normally leave out the word "value" in these declarations.

+float calcHue(float temp1, float temp2, float hueVal)

Is there a better name than temp1/temp2 for these? If not, can you refer to place you found the algorithm?

+RGBA32 makeRGBAfromHSLA(float h, float s, float l, float a)

Why the use of float here, given the use of double in the code that's passing things in and the use of double-precision constants like "1.0"? This code is going to do some conversion from float to double and back that we might be able to avoid.

The HSV code in this file references the URL of the place the color algorithm came from. Can you do the same with this new code?

[David Carson]

(In reply to comment #7)
> (From update of attachment 9155 [details] [edit])
> Looks pretty good. I have some relatively unimportant comments, but I think I'm
> going to review- so you can address at least some of them.
> 
> Some minor formatting mistakes:
> 
> +    ValueList *args = value->function->args;
> +    Value *v = args->current();
> 
> Please put the * next to tyhe type.

Will correct. But this was the format of the original code.
 
> +    colorValues[0] = ((((int)(v->fValue) % 360) + 360) % 360)/360.;
> 
> Need spaces around all the operators, including the "/" and we prefer 360.0 to
> 360. for double constants.

yep. missed this one.

> +    for (int i = 1; i < 3; i++)
> +    {
> 
> Please put the { on the same line as the for statement.

k.

> +    colorValues[0] = ((((int)(v->fValue) % 360) + 360) % 360)/360.;
> 
> What does the line above to if fValue does not fit in an int? Need to check if
> it's in range?

If it does not fit in an int, it will be zero, and it can be ignored.

> +        colorValues[i] = v->fValue/100.; // needs to be value between 0 and
> 1.0
> 
> This code says it "needs to be" -- what checks that the number is between 0 and
> 100?
k. Will be corrected.

> -        if (!validUnit(v, FInteger|FPercent, true))
> 
> Spaces around operators like "|" too.

It is a '-' so this line is being removed. This line is existing code.

> In makeRGBAfromHSLA, we'd normally capitalize the f.

yep.
 
> +        bool parseColorParameters(Value* value, int* colorValues, bool
> parseAlpha);
> +        bool parseHSLParameters(Value* value, float* colorValues, bool
> parseAlpha);
> 
> We'd normally leave out the word "value" in these declarations.

If I leave out the word value, then there would be no name for the first variable :-)
Changed ColorValues to ColorArray.

> +float calcHue(float temp1, float temp2, float hueVal)
> 
> Is there a better name than temp1/temp2 for these? If not, can you refer to
> place you found the algorithm?

Last time I refered to a URL in the code, I was asked to remove it. This time I 
refered to the algorithm in the error report. See comment #4.

> +RGBA32 makeRGBAfromHSLA(float h, float s, float l, float a)
> 
> Why the use of float here, given the use of double in the code that's passing
> things in and the use of double-precision constants like "1.0"? This code is
> going to do some conversion from float to double and back that we might be able
> to avoid.

Changed to double.

> The HSV code in this file references the URL of the place the color algorithm
> came from. Can you do the same with this new code?

Is that the right approach? I am having difficulty in understanding when to refer to a URL and when not to.

[David Carson]

Created attachment 9181 [details]
Patch updated with some of the changes.

[Maciej Stachowiak]

"Is that the right approach? I am having difficulty in understanding when to
refer to a URL and when not to."

IMO referring to the applicable spec is usually redundant, unless the algorithm is highly non-obvious. However, referring to an algorithm that might be hard to understand without detailed explanation, I thinka  source URL is useful. We do this for hash functions for instance.

Looks like you addressed Darin's comments and I have no new concerns so r=me.

[Sam Weinig]

Created attachment 9188 [details]
updated patch

updated patch

[Sam Weinig]

Created attachment 9190 [details]
even more updated

[David Carson]

Created attachment 9191 [details]
extended test case to more extensively test error cases and code paths

No code changes from Weinig updated patch, just improved the test hsl-color.html test cases. Regenerated expected results.

[Darin Adler]

Comment on attachment 9191 [details]
extended test case to more extensively test error cases and code paths

Based on Maciej's earlier review, I'll say r=me.

You didn't really address all my comments. For example, there's still no URL of a source for the color algorithm. But lets just get this into the tree as-is.

[Darin Adler]

(In reply to comment #8)
> > +    colorValues[0] = ((((int)(v->fValue) % 360) + 360) % 360)/360.;
> > 
> > What does the line above to if fValue does not fit in an int? Need to check if
> > it's in range?
> 
> If it does not fit in an int, it will be zero, and it can be ignored.

What makes it zero? Casting an out of range value to an int doesn't guarantee a zero as far as I know, but maybe you're referring to some code elsewhere that does.

> > +        bool parseColorParameters(Value* value, int* colorValues, bool parseAlpha);
> > +        bool parseHSLParameters(Value* value, float* colorValues, bool parseAlpha);
> > 
> > We'd normally leave out the word "value" in these declarations.
> 
> If I leave out the word value, then there would be no name for the first
> variable :-)

Right. We leave out names for parameters in declarations when the type alone makes it clear what the parameter is. See, for example, HTMLAnchorElement.h, and note that when needed for clarity we use parameter names and when not needed we omit them.

> > +float calcHue(float temp1, float temp2, float hueVal)
> > 
> > Is there a better name than temp1/temp2 for these? If not, can you refer to
> > place you found the algorithm?
> 
> Last time I refered to a URL in the code, I was asked to remove it. This time I 
> refered to the algorithm in the error report. See comment #4.
>
> > The HSV code in this file references the URL of the place the color algorithm
> > came from. Can you do the same with this new code?
> 
> Is that the right approach? I am having difficulty in understanding when to
> refer to a URL and when not to.

I think that a URL in the code for which standard we are implementing is not usually good. But URL for a place to find an otherwise hard-to-understand algorithm is great. We have that for our HSV algorithm and our hash functions, for example. Sorry for seeing to be inconsistent. Maybe more of this should be written down.

Given Maciej's earlier comments it seems that at least he and I see eye to eye on this. Should probably work toward having a broader consensus.

I'm still happy with the patch as-is, so review+, and also, thanks very much for adding this feature.

[Sam Weinig]

landed in r 15151