Bug 221880 – Add experimental support for CSS Color 5 Relative Color Syntax

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sam Weinig]

Let's add experimental support for CSS Color 5 Relative Color Syntax - https://drafts.csswg.org/css-color-5/#relative-colors.

Because the spec doesn't seem quite finished, we should keep this behind an experimental feature flag for now.

[Sam Weinig]

Created attachment 420255 [details]
WIP

WIP. This adds parsing and resolving for all relative colors with a few main things missing:

- Doesn't support calc() using the component identifiers - e.g `color: hsl(from red calc(h+180) s l)`.
- Doesn't support system keywords.

It's also got a bunch of housekeeping that can go in first, so I am going to break it up to actually land what is here.

[Sam Weinig]

Created attachment 420257 [details]
WIP

[Sam Weinig]

Created attachment 420416 [details]
Patch

[Sam Weinig]

Needs tests, but implementation of parsing and basic resolving is getting close. Still no support for calc() or system keywords, but I want to get this in first.

[Sam Weinig]

Created attachment 420576 [details]
Patch

[Sam Weinig]

Created attachment 420773 [details]
Patch

[Sam Weinig]

Created attachment 420775 [details]
Patch

[Sam Weinig]

Going to have to fix the test for this after https://bugs.webkit.org/show_bug.cgi?id=222085 lands, but it will simplify the lch() tests which is great.

[Darin Adler]

Comment on attachment 420775 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420775&action=review

> Source/WebCore/ChangeLog:19
> +        Some additional caveates about this initial implementation that need further 

Typo in caveats here.

> Source/WebCore/css/parser/CSSParserContext.h:133
> +            & key.mode                                      << 20; // Keep this last.

That (non-new) comment is mysterious. Why do we need to keep it last?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:925
> +    // and clamp at the end. It won't make a difference until we support calculations on the origins

Should put a question mark here. Also "origins" should be "origin's".

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:926
> +    // components.

I see that you have orphaned a word. How do you expect me to sleep at night?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1057
> +    return convertColor<SRGBA<uint8_t>>(HSLA<float> { static_cast<float>(resolvedHue), static_cast<float>(resolvedSaturation), static_cast<float>(resolvedLightness), static_cast<float>(resolvedAlpha) });

Are these casts all narrowing from double? Or is there something else going on here? I see this pattern repeatedly and I would like us to improve the idiom.

[Sam Weinig]

(In reply to Darin Adler from comment #9)
> Comment on attachment 420775 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420775&action=review
> 
> > Source/WebCore/ChangeLog:19
> > +        Some additional caveates about this initial implementation that need further 
> 
> Typo in caveats here.

What would of change of mine be without making the stylebot red and a misspelled word :).

> 
> > Source/WebCore/css/parser/CSSParserContext.h:133
> > +            & key.mode                                      << 20; // Keep this last.
> 
> That (non-new) comment is mysterious. Why do we need to keep it last?

I think its due to mode being more than one bit, but can't say for sure. I will resolve with https://bugs.webkit.org/show_bug.cgi?id=222043.

> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:925
> > +    // and clamp at the end. It won't make a difference until we support calculations on the origins
> 
> Should put a question mark here. Also "origins" should be "origin's".

Fixed.

> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:926
> > +    // components.
> 
> I see that you have orphaned a word. How do you expect me to sleep at night?

Fixed. Deepest apologies.

> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1057
> > +    return convertColor<SRGBA<uint8_t>>(HSLA<float> { static_cast<float>(resolvedHue), static_cast<float>(resolvedSaturation), static_cast<float>(resolvedLightness), static_cast<float>(resolvedAlpha) });
> 
> Are these casts all narrowing from double? Or is there something else going
> on here? I see this pattern repeatedly and I would like us to improve the
> idiom.

It's just narrowing from double. All of the CSS parser code is in terms of double, so I have been trying to just keep it double until the very end to avoid casts in more places. This is definitely something I can improve.

[Sam Weinig]

Created attachment 420914 [details]
Patch

[EWS]

Committed r273127: <https://commits.webkit.org/r273127>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420914 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74509452>

[Michael Catanzaro]

Reopening to attach new patch.

[Michael Catanzaro]

Created attachment 421436 [details]
Patch for landing

[EWS]

Committed r273428: <https://commits.webkit.org/r273428>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421436 [details].