Bug 233507 - [CSS Color 4] Add support for oklab() and oklch() colors
Summary: [CSS Color 4] Add support for oklab() and oklch() colors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-25 16:36 PST by Sam Weinig
Modified: 2021-11-27 12:56 PST (History)
13 users (show)

See Also:


Attachments
Patch (182.98 KB, patch)
2021-11-25 17:07 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (191.45 KB, patch)
2021-11-27 11:42 PST, Sam Weinig
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-11-25 16:36:39 PST
[CSS Color 4] Add support for oklab() and oklch() colors
Comment 1 Sam Weinig 2021-11-25 17:07:54 PST
Created attachment 445162 [details]
Patch
Comment 2 EWS Watchlist 2021-11-25 17:08:50 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 3 Cameron McCormack (:heycam) 2021-11-26 22:35:05 PST
Comment on attachment 445162 [details]
Patch

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

I didn't review the tests closely.

> Source/WebCore/ChangeLog:25
> +        Adds support for oklab() and oklch() css colors and as interpolation

"CSS colors"

> Source/WebCore/ChangeLog:56
> +        (WebCore::N>::subset const):

(prepare-ChangeLog got confused here.)

> Source/WebCore/ChangeLog:97
> +        Add OKLab and OKLCH to the list fo enumerated colorspaces and add mappings to their

"list of"

> Source/WebCore/ChangeLog:109
> +        Generalize isBlack and isWhite to have variant that works with Lab, LCH, OKLab and OKLCH (as they

"to have a variant that works" or "to have variants that work"

> Source/WebCore/platform/graphics/ColorConversion.cpp:294
> +    // matrix with any subsequent matricies in the conversion. This would mean teaching the main conversion about this matrix

"matrices"

> Source/WebCore/platform/graphics/ColorConversion.cpp:303
> +    static constexpr ColorMatrix<3, 3> LMSToXYZD65 {
> +        1.2268798733741557f,  -0.5578149965554813f,  0.28139105017721583f,
> +       -0.04057576262431372f,  1.1122868293970594f, -0.07171106666151701f,
> +       -0.07637294974672142f, -0.4214933239627914f,  1.5869240244272418f
> +    };

Looks funny to write out these numbers with enough precision to be doubles but actually be floats. But I guess it's better to have these transcribed from the spec than rounded by hand.

Also, maybe the matrix names could be clearer about whether they are linear or non-linear LMS?

> Source/WebCore/platform/graphics/ColorConversion.cpp:330
> +    // matrix with any previous matricies in the conversion. This would mean teaching the main conversion about this matrix

"matricies"

> Source/WebCore/platform/graphics/ColorConversion.cpp:344
> +        0.2104542553f,  0.7936177850f, -0.0040720468f,
> +        1.9779984951f, -2.4285922050f,  0.4505937099f,
> +        0.0259040371f,  0.7827717662f, -0.8086757660f

Why does this matrix have less precision (here and in the spec)? Doesn't matter for us since we're using floats, but it looks odd.

> Source/WebCore/platform/graphics/ColorTypes.h:482
> +    static constexpr WhitePoint whitePoint = WhitePoint::D65;

This could be `static constexpr auto`, like how ColorSpaceMapping<T>::colorSpace is defined.

> Source/WebCore/platform/graphics/ColorTypes.h:483
> +    using Reference =  XYZA<T, whitePoint>;

Nit: one too many spaces after "=".

> Source/WebCore/platform/graphics/ColorUtilities.h:57
> +template<typename ColorType, typename std::enable_if_t<std::is_same_v<typename ColorType::Model, LabModel<typename ColorType::ComponentType>> || std::is_same_v<typename ColorType::Model, LCHModel<typename ColorType::ComponentType>>>* = nullptr> constexpr bool isBlack(const ColorType&);

Maybe define `IsLabModel` and `IsLCHModel` template variables to make this expression (which is repeated a few times) more readable?

> LayoutTests/fast/css/parsing-color-mix.html:75
> +        // What should happen if you provide a negative percent? https://github.com/w3c/csswg-drafts/issues/6047

That issue is resolved now. I think the test for negative percentages is correct?
Comment 4 Sam Weinig 2021-11-27 11:10:32 PST
(In reply to Cameron McCormack (:heycam) from comment #3)
> Comment on attachment 445162 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445162&action=review
> 
> > Source/WebCore/platform/graphics/ColorConversion.cpp:344
> > +        0.2104542553f,  0.7936177850f, -0.0040720468f,
> > +        1.9779984951f, -2.4285922050f,  0.4505937099f,
> > +        0.0259040371f,  0.7827717662f, -0.8086757660f
> 
> Why does this matrix have less precision (here and in the spec)? Doesn't
> matter for us since we're using floats, but it looks odd.

I don't know, but I would like to find out.

> > LayoutTests/fast/css/parsing-color-mix.html:75
> > +        // What should happen if you provide a negative percent? https://github.com/w3c/csswg-drafts/issues/6047
> 
> That issue is resolved now. I think the test for negative percentages is
> correct?

It is resolved, but nah, it's incorrect. Negative percentages are now a parse error. I have a fix for this coming up in https://bugs.webkit.org/show_bug.cgi?id=233527.


Thanks for the review!
Comment 5 Sam Weinig 2021-11-27 11:42:30 PST
Created attachment 445219 [details]
Patch
Comment 6 EWS 2021-11-27 12:55:05 PST
Committed r286191 (?): <https://commits.webkit.org/r286191>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445219 [details].
Comment 7 Radar WebKit Bug Importer 2021-11-27 12:56:27 PST
<rdar://problem/85781356>