Fractional RBG values are not supported anymore @3.0.0-pre.5

[pramilk]

Issue
Fractional RGB value is not supported starting pre.5. If its expected, then may be just need to update the change log to document it.
Previouly: "rgb(200,0.5,0)" was a valid input, not anymore.

maplibre-gl-js version: From 3.0.0-pre.5.

browser: All

Link to Demonstration

Broken with @3.0.0-pre.5 Repro link: https://jsbin.com/yejeqomoba/edit?html,console,output
Works @3.0.0-pre.4 Repro link: https://jsbin.com/milicoluxu/1/edit?html,output
PR which potentially caused it: #2376

Expected Behavior

Actual Behavior

[HarelM]

Probably related to the css color parser...
Thanks for taking the time to report this!
Cc @ChrisLoer

[HarelM]

Probably introduced here: maplibre/maplibre-style-spec#94
@kajkal, can you take a look at this please?

[kajkal]

The CSS Color 3 specification specifies that the numbers in rgb function should be integers. Library csscolorparser used for color parsing up to version 3.0.0-pre.4 parsed floating-point values in rgb(...) as integers. So the example you gave, rgb(200,0.5,0), up to version 3.0.0-pre.4 was incorrectly treated as valid and interpreted as rgb(200,0,0).
So it looks like "Fractional RBG values" were never supported by maplibre-gl-js.

According to the CSS Color 4 specification, the extra precision in the rgb(...) values should be honored but our current css color parsing library color-string does not do this.
I will make a PR to the color-string library with a proposal to fix this.

[kajkal]

I looked into this and realized that the color-string library is not properly designed to support partial rgb values, and introducing this change there would probably break many people's tests (~15mil downloads per week).
Currently color-string rounds even rgb(...) values defined as percentages to values in the range 0..255 - I didn't notice this before.

So if we want a css color parser without unnecessary additions (other color spaces, serialization, etc.), maximally compliant with the css specification, over which we have full control there is nothing left but to write our own :D
I created a POC here.
@HarelM what do you think?

[HarelM]

If we can't find a library that does the trick for us, then writing our own is three only thing left to do, but we need to make sure this is the case, as I see it as a last resort.
Did you take the existing code from the old library and modified it? I would recommend to start with something that works and modify it...
Generally speaking, this looks good and passes all the tests, which is assuring.

[kajkal]

I have not been able to find the perfect library, color-string was the best I came across, and it still has pieces of code that we are not interested in and are not tree shakeable, and as it turns out it is not satisfactorily compliant with the css color 4 specification.
As for the code, I loosely based on the approach from color-string with regexps, they are not complex and are quite straightforward - from what I can tell they work just fine.

I think we could give it a try. Let's wait until the expression tests are migrated to maplibre-style-spec #97, check if the tests pass with the new parser and then decide what to do next. And maybe someone will find a better parsing library in the meantime :)

[birkskyum]

@kajkal , this expression test migration PR you mentioned is put on hold a bit, because consultation with the Native team mean that we could take a different approach here.

[birkskyum]

I think this parce-css-color POC, at least until color-string catches up, is a better option for us if the alternative is to introduce a hard-to-find breaking change in 3.0.0. We should make sure our tests suite catches this going forward, so it's easy to re-assess color-string compatibility in half a year or so - and it would be a good idea to have an issue to remind us about that, in case it's possible to reduce the codebase again.

[birkskyum]

@kajkal , if you are somewhat certain that there are no other libraries out there that does this, then a PR for this would be greatly appreciated so it can be included in the soon-to-be-released 3.0 release of MapLibre GL JS.

[kajkal]

@birkskyum I created PR #175
I've added more tests, many of them based on the tests attached with the CSS Color 4 specification.
I also made the parser stricter, following the syntax from the specification.

[birkskyum]

Thanks!

[birkskyum]

closed by maplibre/maplibre-style-spec#175