Fix Color.rgbToHsl function to not return NaN for certain cases.

[gdotdesign]

The current implementation of Color.rgbToHsl (https://github.com/elm-lang/core/blob/master/src/Color.elm#L176-L208) is not correct and gives invalid results for some colors:

• white -> (NaN, NaN, 1)
• black -> (NaN, 0, 0)

Following the formulas in this website http://www.rapidtables.com/convert/color/rgb-to-hsl.htm this PR fixes this behavior.

Also added some tests for these and some other colors.

[process-bot]

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

[process-bot]

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

[evancz]

Is there some way we can test that it round trips for all the RGB colors? Is that even a reasonable thing to expect from these conversions? Or maybe just check that all RGB colors map to valid HSL colors?

[evancz]

Is there some way we can test that it round trips for all the RGB colors? Is that even a reasonable thing to expect from these conversions? Or maybe just check that all RGB colors map to valid HSL colors?

[gdotdesign]

As I have seen in other color libraries, they are testing for palette not for all possible values:
https://github.com/bgrins/TinyColor/blob/master/test/test.js#L45-L92
https://github.com/One-com/one-color/blob/master/test/conversion.js#L1-L22
https://github.com/brehaut/color-js/blob/master/test/color-test.js#L128-L166

Maybe there is a way to test it with generators, I can look into it if you feel it is necessary.

[gdotdesign]

As I have seen in other color libraries, they are testing for palette not for all possible values:
https://github.com/bgrins/TinyColor/blob/master/test/test.js#L45-L92
https://github.com/One-com/one-color/blob/master/test/conversion.js#L1-L22
https://github.com/brehaut/color-js/blob/master/test/color-test.js#L128-L166

Maybe there is a way to test it with generators, I can look into it if you feel it is necessary.

[evancz]

I just remembered there were some issues where HSL colors were showing up shifted or something. I forget the particulars now. But I was trying to find out how to be more confident that this is "correct".

Like, are these formulas the same as the ones on this page? I believe that's what I used, but I could have gotten things wrong. Or they could have been wrong at the time!

I am skeptical because the saturation calculation seems worse. What happens when lightness is zero? Isn't that a divide-by-zero with the new version? And when c is zero, isn't the result zero on both branches? Why have branches then?

[evancz]

I just remembered there were some issues where HSL colors were showing up shifted or something. I forget the particulars now. But I was trying to find out how to be more confident that this is "correct".

Like, are these formulas the same as the ones on this page? I believe that's what I used, but I could have gotten things wrong. Or they could have been wrong at the time!

I am skeptical because the saturation calculation seems worse. What happens when lightness is zero? Isn't that a divide-by-zero with the new version? And when c is zero, isn't the result zero on both branches? Why have branches then?

[evancz]

Also, these formulas are saying to fall back on zero if none of the cases apply, but is zero the same as "these color spaces do not have a one-to-one mapping so we did our best"?

[evancz]

Also, these formulas are saying to fall back on zero if none of the cases apply, but is zero the same as "these color spaces do not have a one-to-one mapping so we did our best"?

[gdotdesign]

I'm not an expert in converting colors so I used that page as a guideline, the main problem here is that for some cases it is returning NaN which in my opinion equals to in seriousness as a runtime exception.

I'm not trying to find the perfect solution here I'm just trying to fix a bug, however there are similar implementations in a given library that I posted source code from, that maybe we can use:
https://github.com/bgrins/TinyColor/blob/master/tinycolor.js#L377-L406

The other two libraries are going by converting RGB to HSV then to HSL, maybe that's a better path but It would mean that this is not a bugfix but a feature.

[gdotdesign]

I'm not an expert in converting colors so I used that page as a guideline, the main problem here is that for some cases it is returning NaN which in my opinion equals to in seriousness as a runtime exception.

I'm not trying to find the perfect solution here I'm just trying to fix a bug, however there are similar implementations in a given library that I posted source code from, that maybe we can use:
https://github.com/bgrins/TinyColor/blob/master/tinycolor.js#L377-L406

The other two libraries are going by converting RGB to HSV then to HSL, maybe that's a better path but It would mean that this is not a bugfix but a feature.

[evancz]

I get the goal. Let's just resolve this:

I am skeptical because the saturation calculation seems worse. What happens when lightness is zero? Isn't that a divide-by-zero with the new version? And when c is zero, isn't the result zero on both branches? Why have branches then?

[evancz]

I get the goal. Let's just resolve this:

I am skeptical because the saturation calculation seems worse. What happens when lightness is zero? Isn't that a divide-by-zero with the new version? And when c is zero, isn't the result zero on both branches? Why have branches then?

[gdotdesign]

I am skeptical because the saturation calculation seems worse. What happens when lightness is zero? Isn't that a divide-by-zero with the new version?

I think you are referring to this line: c / (1 - abs (2 * lightness - 1))

• If c is not zero then lightness will always be > 0 because either cMax or cMin are not zero

There is only one color which lightness is zero and that is the pure black (#000000) any other color will have some lightness, and for it the c will be zero.

And when c is zero, isn't the result zero on both branches? Why have branches then?

I don't understand what you are getting at here.

[gdotdesign]

I am skeptical because the saturation calculation seems worse. What happens when lightness is zero? Isn't that a divide-by-zero with the new version?

I think you are referring to this line: c / (1 - abs (2 * lightness - 1))

• If c is not zero then lightness will always be > 0 because either cMax or cMin are not zero

There is only one color which lightness is zero and that is the pure black (#000000) any other color will have some lightness, and for it the c will be zero.

And when c is zero, isn't the result zero on both branches? Why have branches then?

I don't understand what you are getting at here.

[evancz]

if c == 0 then
  0
else
  c / (1 - abs (2 * lightness - 1))

If c is zero, the answer is just zero, whichever branch you take. It seems like the only risky thing that can happen here is that lightness is 0, which is what the old code was checking for. I guess you are saying that lightness == 0 always implies that c == 0?

[evancz]

if c == 0 then
  0
else
  c / (1 - abs (2 * lightness - 1))

If c is zero, the answer is just zero, whichever branch you take. It seems like the only risky thing that can happen here is that lightness is 0, which is what the old code was checking for. I guess you are saying that lightness == 0 always implies that c == 0?

[gdotdesign]

Yes.

[gdotdesign]

Yes.