Bug 221880 - Add experimental support for CSS Color 5 Relative Color Syntax
Summary: Add experimental support for CSS Color 5 Relative Color Syntax
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on: 221881
Blocks: 221873
  Show dependency treegraph
 
Reported: 2021-02-14 14:47 PST by Sam Weinig
Modified: 2022-07-15 05:48 PDT (History)
11 users (show)

See Also:


Attachments
WIP (78.00 KB, patch)
2021-02-14 14:51 PST, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP (78.03 KB, patch)
2021-02-14 16:00 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (27.83 KB, patch)
2021-02-15 19:16 PST, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (27.88 KB, patch)
2021-02-16 18:23 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (81.45 KB, patch)
2021-02-17 17:58 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (81.13 KB, patch)
2021-02-17 18:01 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (80.54 KB, patch)
2021-02-18 20:57 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch for landing (1.70 KB, patch)
2021-02-24 12:05 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2021-02-14 14:47:05 PST
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.
Comment 1 Sam Weinig 2021-02-14 14:51:03 PST
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.
Comment 2 Sam Weinig 2021-02-14 16:00:03 PST Comment hidden (obsolete)
Comment 3 Sam Weinig 2021-02-15 19:16:08 PST
Created attachment 420416 [details]
Patch
Comment 4 Sam Weinig 2021-02-15 19:17:54 PST
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.
Comment 5 Sam Weinig 2021-02-16 18:23:02 PST
Created attachment 420576 [details]
Patch
Comment 6 Sam Weinig 2021-02-17 17:58:59 PST
Created attachment 420773 [details]
Patch
Comment 7 Sam Weinig 2021-02-17 18:01:15 PST
Created attachment 420775 [details]
Patch
Comment 8 Sam Weinig 2021-02-18 08:54:21 PST
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.
Comment 9 Darin Adler 2021-02-18 10:02:24 PST
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.
Comment 10 Sam Weinig 2021-02-18 20:19:12 PST
(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.
Comment 11 Sam Weinig 2021-02-18 20:57:21 PST
Created attachment 420914 [details]
Patch
Comment 12 EWS 2021-02-18 21:51:13 PST
Committed r273127: <https://commits.webkit.org/r273127>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420914 [details].
Comment 13 Radar WebKit Bug Importer 2021-02-18 21:52:16 PST
<rdar://problem/74509452>
Comment 14 Michael Catanzaro 2021-02-24 12:05:09 PST
Reopening to attach new patch.
Comment 15 Michael Catanzaro 2021-02-24 12:05:12 PST
Created attachment 421436 [details]
Patch for landing
Comment 16 EWS 2021-02-24 12:46:01 PST
Committed r273428: <https://commits.webkit.org/r273428>

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