160550 - [FIXr][RFE] Implement CSS3 Color module's hsl() and hsla() color types

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Jason Davis]

The CSS3 color module (currently in last call working draft) implements an hsl()
color value:
http://www.w3.org/TR/2002/WD-css3-color-20020418/#hsl-color
which is trivial to convert to rgb().

It also specifies an hsla() color value:
http://www.w3.org/TR/2002/WD-css3-color-20020418/#hsla-color
which is simply hsl() with an alpha channel.

Since CSS color values are converted to rgb() format, with the addition of the
alpha channel it seems that they should be converted to rgba() instead, in which
case this RFE would be dependent on
http://bugzilla.mozilla.org/show_bug.cgi?id=147017 (Support rgba colors in CSS).

Since the specs are still in working draft, the values would have to be
implemented as -moz-hsl() and -moz-hsla(), but nonetheless this is still an
incredibly powerful design ability. hsl() is much better for guessing and
checking values (makes it easier for the web page designers, or to quickly find
matching colors for XUL skins for example), and an alpha channel makes it much
more convenient than having to create a 1*1 PNG image for each different background.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase for -moz-hsl support

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: rough patch

This is unfortunately entangled with the patch to bug 154755.  This just
implements -moz-hsl(), since we don't yet support alpha for CSS colors....

Comment 3

[Brant Gurganus]

[RFE] is deprecated in favor of severity: enhancement.  They have the same meaning.

Comment 4

[Boris Zbarsky [:bzbarsky]]

glazou, do you have any interest in this?  Or should we wait till this draft is
in something resembling CR?

Comment 5

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

boris: yes, we should implement this. And forget about -moz- whatever is the
status of the spec, please.

Comment 6

[Boris Zbarsky [:bzbarsky]]

taking.

Daniel, why forget about -moz-?  Is this stable enough that it will not change?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: proposed patch

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 114441 [details] [diff] [review]
proposed patch

I had to put the HSL code in nsColor because the parser does not get to see
nsCSSColorUtils.  Perhaps the code could just live entirely within the parser,
but this seems like it could be generally useful...

Comment 10

[Hixie (not reading bugmail)]

This must be -moz-hsl until css3-color is in CR. Only by strictly adhering to
this convention can we ensure we never cause interoperability problems.

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK.  That's a 2-line change to that patch; if people want I can post a new
patch, of course....

Comment 12

[Boris Zbarsky [:bzbarsky]]

sometime.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: updated testcase (using -moz-hsl)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: -moz-hsla testcase

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 114441 [details] [diff] [review]
proposed patch

I'll be updating this to do -moz-hsl and also -moz-hsla and -moz-rgba.	roc is
working on the GFX impl of the latter and it'd make life easier for him if the
style system were to actually support them....

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: updated patch -- does -moz-hsl, -moz-rgba, -moz-hsla

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 117117 [details] [diff] [review]
updated patch -- does -moz-hsl, -moz-rgba, -moz-hsla

I'm not too happy with adding this much code, but I couldn't see a smaller way
to do it...

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  while (h < 0.0f) {
+    h += 360.0f;
+  }
+  while (h >= 360.0f) {
+    h -= 360.0f;
+  }

Hello denial of service! How about
  h = h - 360.0*floor(h/360.0);

+  float value = mToken.mNumber;
+  if (value < 0.0f) {
+    value = 0.0f;
+  }
+  if (value > 1.0f) {
+    value = 1.0f;
+  }
...
+  aOpacity = (PRUint8)value;

You have to multiply by 255, and I worry a little about rounding. So
I think this should be
  PRUint32 value = (PRUint32)rint(mToken.mNumber*255);
  if (value < 0) value = 0;
  if (value > 255) value = 255;
  aOpacity = (PRUint8)value;

Comment 19

[Boris Zbarsky [:bzbarsky]]

Doh.  Good catch on both of those.  I've made those changes.  (I should have
thought a bit more about doing the wraparound well... ;))

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you attach the latest version of the patch?  Everything seems pretty good,
except I didn't look closely at the part roc commented on, and I didn't check
your nsColor math.

Also, note that roc's floor expression could be simplified further if you divide
by 360 first (since you do it right below).

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Sure thing; patch against tonight's trunk.

This includes the "divide by 360 first" thing; the code in nsColor is just
copied and pasted from the pseudocode at
http://www.w3.org/TR/2003/WD-css3-color-20030214/#hsl-color

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 117440 [details] [diff] [review]
Sure thing; patch against tonight's trunk.

>+  h = h - floor(h);

floorf seems like it would be better, but is it as portable?

>+  PRUint32 value = (PRUint32)rint(mToken.mNumber*255);

What does rint do, exactly?  (Why don't you want floorf, ceilf, or round, or
straight conversion?  What's the difference?)


>+// helper
>+static float
>+HSL_HueToRGB(float m1, float m2, float h)
>+{
>+  if (h < 1.0f)

h < 0.0f
??

>+    h += 1.0f;
>+  if (h > 1.0f)
>+    h -= 1.0f;
>+  if (h * 6.0f < 1.0f)

how about
if (h < 1.0f/6.0f)
to ensure constant folding?
(and similar for the next two -- or might "(float)(1.0/6.0)" be more accurate?)

>+    return m1 + (m2 - m1)*h*6.0f;
>+  if (h*2.0f < 1.0f)
>+    return m2;
>+  if (h*3.0f < 2.0f)
>+    return m1 + (m2 - m1)*(2.0f/3.0f - h)*6.0f;
>+  return m1;      
>+}

Comment 23

[Boris Zbarsky [:bzbarsky]]

To be truthful, I don't know how portable floorf is.  I could check it in with
floorf and see if anything breaks.  ;)

rint() seems to round based on what the "current rounding mode" is, so it's not
quite equivalent to "floor" or "ceil".  I don't know how it differs from round()
or just straight conversion, if at all.  roc?

> h < 0.0f
> ??

Eeep, yes.

Made the constant-folding changes; (float)(1.0/2.0) seems best to me.

roc?  Comments on the rounding stuff?

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Just call round(), I guess. It doesn't really matter what we do with the corner
case opacity values. I wouldn't use ceil() or floor() or trunc() because we do
want values "near" 0 or 1 to become 0 or 255 respectively.

Comment 25

[Boris Zbarsky [:bzbarsky]]

checked in, with round() and floorf().

Comment 26

[Boris Zbarsky [:bzbarsky]]

Change round() to NSToIntRound(), due to bustage...

Comment 27

[Boris Zbarsky [:bzbarsky]]

AIX and Solaris failed to deal with floorf(), so I went back to floor()

Comment 28

[Jason Davis]

The CSS3 Color module has been moved to Candidate Recommendation. Is it now safe
to rename -moz-hsl, -moz-hsla, and -moz-rgba to the corresponding properties in
the CSS3 specs?

Comment 29

[Hixie (not reading bugmail)]

-moz-hsl() certainly; we shouldn't rename -moz-rgba() and -moz-hsla() unless we
support painting with RGBA colours. (It would appear that we do not. Is there a
bug number for that?)

Comment 30

[Hixie (not reading bugmail)]

In answer to my last question, that's bug 147017, which I should have known, I'm
the QA contact and have commented on it. Ahem. Moving on...

Comment 31

[Steffen Wilberg]

Attachment: hsl testcase (-moz-hsl has been renamed into hsl)

As -moz-hsl() has been renamed into hsl() and doesn't work anymore, I've
attached a hsl testcase. Someone should mark the -moz-hsl testcase obsolete.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 116941 [details]
updated testcase (using -moz-hsl)

Actaully, the second testcase attached to this bug was already for hsl()...