Bug 233507 – [CSS Color 4] Add support for oklab() and oklch() colors

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[CSS Color 4] Add support for oklab() and oklch() colors

[Sam Weinig]

Created attachment 445162 [details]
Patch

[EWS Watchlist]

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

[Cameron McCormack (:heycam)]

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?

[Sam Weinig]

(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!

[Sam Weinig]

Created attachment 445219 [details]
Patch

[EWS]

Committed r286191 (?): <https://commits.webkit.org/r286191>

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

[Radar WebKit Bug Importer]

<rdar://problem/85781356>