Bug 9506 - Implement HSL and HSLA colors in CSS
: Implement HSL and HSLA colors in CSS
Status: RESOLVED FIXED
: WebKit
CSS
: 420+
: Macintosh Mac OS X 10.4
: P2 Enhancement
Assigned To:
: http://www.css3.info/preview/hsl.html
: EasyFix
:
:
  Show dependency treegraph
 
Reported: 2006-06-19 06:58 PST by
Modified: 2006-07-04 20:05 PST (History)


Attachments
patch, test cases and change logs (79.10 KB, patch)
2006-07-02 13:55 PST, David Carson
darin: review-
Review Patch | Details | Formatted Diff | Diff
Patch updated with some of the changes. (79.16 KB, patch)
2006-07-03 19:13 PST, David Carson
mjs: review+
Review Patch | Details | Formatted Diff | Diff
updated patch (79.54 KB, patch)
2006-07-04 06:16 PST, Sam Weinig
no flags Review Patch | Details | Formatted Diff | Diff
even more updated (79.49 KB, patch)
2006-07-04 06:46 PST, Sam Weinig
no flags Review Patch | Details | Formatted Diff | Diff
extended test case to more extensively test error cases and code paths (166.30 KB, patch)
2006-07-04 07:38 PST, David Carson
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2006-06-19 06:58:55 PST
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.
------- Comment #1 From 2006-06-19 16:03:20 PST -------
This is very easy to implement.
------- Comment #2 From 2006-06-19 22:21:45 PST -------
Cool, it would really be nice to have.
------- Comment #3 From 2006-06-20 00:28:01 PST -------
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 :)
------- Comment #4 From 2006-07-02 12:37:05 PST -------
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
------- Comment #5 From 2006-07-02 13:20:44 PST -------
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.
------- Comment #6 From 2006-07-02 13:55:40 PST -------
Created an attachment (id=9155) [details]
patch, test cases and change logs

Added support for HSL and HSLA colors.
------- Comment #7 From 2006-07-03 18:20:35 PST -------
(From update of attachment 9155 [details])
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?
------- Comment #8 From 2006-07-03 18:47:45 PST -------
(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.
------- Comment #9 From 2006-07-03 19:13:15 PST -------
Created an attachment (id=9181) [details]
Patch updated with some of the changes.
------- Comment #10 From 2006-07-04 00:07:05 PST -------
"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.
------- Comment #11 From 2006-07-04 06:16:11 PST -------
Created an attachment (id=9188) [details]
updated patch

updated patch
------- Comment #12 From 2006-07-04 06:46:31 PST -------
Created an attachment (id=9190) [details]
even more updated
------- Comment #13 From 2006-07-04 07:38:45 PST -------
Created an attachment (id=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.
------- Comment #14 From 2006-07-04 10:33:00 PST -------
(From update of attachment 9191 [details])
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.
------- Comment #15 From 2006-07-04 10:39:26 PST -------
(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.
------- Comment #16 From 2006-07-04 20:05:00 PST -------
landed in r 15151