Bug 205675 – Support lab(), lch() and color(lab ...) colors

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

[Simon Fraser (smfr)]

Add support for lab(), lch() and gray() colors.

[Radar WebKit Bug Importer]

<rdar://problem/58264078>

[Simon Fraser (smfr)]

I have parsing and conversion done, but for computed style we need to store lab/lch colors without converting them to sRGB.

Also gradients/animations will need work.

[Simon Fraser (smfr)]

Created attachment 386699 [details]
Patch

[Dean Jackson]

Comment on attachment 386699 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:722
> +    float alpha = 1.0f;
> +    if (consumeSlashIncludingWhitespace(args)) {
> +        auto alphaParameter = consumePercent(args, ValueRangeAll);
> +        if (alphaParameter)
> +            alpha = clampTo<float>(alphaParameter->doubleValue() / 100.0, 0.0, 1.0);
> +        else {
> +            alphaParameter = consumeNumber(args, ValueRangeAll);
> +            if (!alphaParameter)
> +                return Color();
> +            alpha = clampTo<float>(alphaParameter->doubleValue(), 0.0, 1.0);
> +        }
> +    }

Seems like we do this often enough to separate into a function.

> Source/WebCore/platform/graphics/ExtendedColor.h:61
> +    const FloatComponents& value() const { return m_value; }

Maybe channels() or something?

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:121
> +            // FIXME: This will give incorrect results on platforms that don't support CGColorSpaceCreateLab(). We should
> +            // convert to sRGB.

What platforms are they? This is already inside ColorCG.

[Simon Fraser (smfr)]

Created attachment 386722 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 386752 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 386754 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 386758 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 386759 [details]
Patch

[Felipe Erias]

I would like to call your attention to the TAG discussion about this feature, specially when it comes to identifying which other pieces of the web platform would need to change in order to correctly support colors outside the sRGB gamut:

https://github.com/w3ctag/design-reviews/issues/488

I started looking into implementing lab() and lch() in Chromium, but it seems that WebKit is in a better position to lead the way here because it already has support for higher-precision colors.

[Simon Fraser (smfr)]

Noted, although WebKit already supports P3 colors so adding lab() support isn't going to newly expose out-of-sRGB colors.

[Simon Fraser (smfr)]

Created attachment 399486 [details]
Patch

[Sam Weinig]

Created attachment 416728 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 416728 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:150
> +        // FIXME: CGColorSpace indicates that the D50 White Point is { 0.9642, 1.0, 0.8249 }
> +        // and can be accessed via CGColorSpaceCreateWithName(kCGColorSpaceGenericLab).
> +
> +        constexpr CGFloat d50WhitePoint[] = { 0.96422, 1.00000, 0.82521 };

I'm not sure what value this comment is adding. Does it mean we should just use kCGColorSpaceGenericLab ?

[Sam Weinig]

(In reply to Simon Fraser (smfr) from comment #14)
> Comment on attachment 416728 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416728&action=review
> 
> > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:150
> > +        // FIXME: CGColorSpace indicates that the D50 White Point is { 0.9642, 1.0, 0.8249 }
> > +        // and can be accessed via CGColorSpaceCreateWithName(kCGColorSpaceGenericLab).
> > +
> > +        constexpr CGFloat d50WhitePoint[] = { 0.96422, 1.00000, 0.82521 };
> 
> I'm not sure what value this comment is adding. Does it mean we should just
> use kCGColorSpaceGenericLab ?

It was meant to remind me to ask you why you picked the values you did.

[Sam Weinig]

Created attachment 416793 [details]
Patch

[Sam Weinig]

Created attachment 416915 [details]
Patch

[Sam Weinig]

Simon, what do you think is left here to get this in?

[Sam Weinig]

Created attachment 416916 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 416916 [details]
Patch

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

Would be nice to have some ref tests that show that the color conversions work correctly.

> Source/WebCore/ChangeLog:70
> +        Add Lab to callWithColorType sinc it is now a valid ColorSpace.

sinc

> Source/WebCore/platform/graphics/Color.h:61
>  //    - 4x float (0-1) Linear sRGBA, stored in a reference counted sub-object
>  //    - 4x float (0-1) DisplayP3, stored in a reference counted sub-object
> +//    - 4x float (0-1) Lab, stored in a reference counted sub-object

It's a bit weird that we spell all these out here.

[Sam Weinig]

Created attachment 417011 [details]
Patch

[Sam Weinig]

(In reply to Simon Fraser (smfr) from comment #20)
> Comment on attachment 416916 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416916&action=review
> 
> Would be nice to have some ref tests that show that the color conversions
> work correctly.

There are some WPT tests that are skipped, turning those on to see if we pass :).

> 
> > Source/WebCore/ChangeLog:70
> > +        Add Lab to callWithColorType sinc it is now a valid ColorSpace.
> 
> sinc

Fixed.

> 
> > Source/WebCore/platform/graphics/Color.h:61
> >  //    - 4x float (0-1) Linear sRGBA, stored in a reference counted sub-object
> >  //    - 4x float (0-1) DisplayP3, stored in a reference counted sub-object
> > +//    - 4x float (0-1) Lab, stored in a reference counted sub-object
> 
> It's a bit weird that we spell all these out here.

I liked it. Seems like good documentation for Color. If we end up adding a lot of colors it won't be useful to have each spelled out like this, but for now it seems ok.

[Sam Weinig]

Created attachment 417022 [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

[Sam Weinig]

Created attachment 417027 [details]
Patch

[Sam Weinig]

Created attachment 417352 [details]
Patch

[Sam Weinig]

Created attachment 417353 [details]
Patch

[Sam Weinig]

Created attachment 417357 [details]
Patch

[EWS]

Committed r271362: <https://trac.webkit.org/changeset/271362>

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

[Lea Verou]

> This change does not add support for the host platform layer (e.g. CoreGraphics, etc)
> understanding of the Lab colorspace, and as such, when creating host color objects
> (for instance via the cachedCGColor() helper) Lab<float> colors are converted to sRGB
> first and then the host color is made. The ramification of this is for platforms that
> use backingstores with greater than sRGB gamuts, Lab colors will be clampted to sRGB.
> This should be rectified in a follow up change that adds proper creation of these host
> objects but is being left off the initial change to limit changes.

If I'm reading this right, it appears that lab()/lch() colors are currently clamped to sRGB? If so, please hold off shipping them until this can be resolved. It will be detrimental to their adoption if they cannot be trusted to go beyond sRGB, as authors cannot detect this programmatically. We do not want to see people resorting to using lch() with a color(display-p3) fallback, do we?

[Chris Lilley]

> Some of the imported tests have been updated to use rectangles rather than text to avoid anti-aliasing issues and will be upstreamed to WPT following landing.

Thanks, I agree that rectangles are better (for example the patch can be measured with a spectrophotometer). Ping me for review once the revised tests land on WPT?

[Chris Lilley]

(In reply to Lea Verou from comment #30)
> > This change does not add support for the host platform layer (e.g. CoreGraphics, etc)
> > understanding of the Lab colorspace, and as such, when creating host color objects
> > (for instance via the cachedCGColor() helper) Lab<float> colors are converted to sRGB
> > first and then the host color is made. The ramification of this is for platforms that
> > use backingstores with greater than sRGB gamuts, Lab colors will be clampted to sRGB.
> > This should be rectified in a follow up change that adds proper creation of these host
> > objects but is being left off the initial change to limit changes.
> 
> If I'm reading this right, it appears that lab()/lch() colors are currently
> clamped to sRGB? If so, please hold off shipping them until this can be
> resolved. It will be detrimental to their adoption if they cannot be trusted
> to go beyond sRGB, as authors cannot detect this programmatically. We do not
> want to see people resorting to using lch() with a color(display-p3)
> fallback, do we?

I agree that content creators will be super confused if they specify, say, color(display-p3 0 1 0) and it gets displayed as P3 primary green, BUT if they specify the same color as lab(86.61% -106.56 102.87) it gets stored as the sRGB-clamped value lab(86.67% -78.41 80.1) which is rgb(0% 98.54% 0%) or color(display-p3 0.4513 0.9709 0.2934).

https://colorjs.io/apps/convert/?color=color(display-p3%200%201%200)&precision=4

So I'm happy to see this initial patch land, but suggest either not closing this bug, or opening another one to resolve the undesirable early clamping.

[Sam Weinig]

(In reply to Lea Verou from comment #30)
> > This change does not add support for the host platform layer (e.g. CoreGraphics, etc)
> > understanding of the Lab colorspace, and as such, when creating host color objects
> > (for instance via the cachedCGColor() helper) Lab<float> colors are converted to sRGB
> > first and then the host color is made. The ramification of this is for platforms that
> > use backingstores with greater than sRGB gamuts, Lab colors will be clampted to sRGB.
> > This should be rectified in a follow up change that adds proper creation of these host
> > objects but is being left off the initial change to limit changes.
> 
> If I'm reading this right, it appears that lab()/lch() colors are currently
> clamped to sRGB? If so, please hold off shipping them until this can be
> resolved. It will be detrimental to their adoption if they cannot be trusted
> to go beyond sRGB, as authors cannot detect this programmatically. We do not
> want to see people resorting to using lch() with a color(display-p3)
> fallback, do we?

Yep, no intention of shipping like this, just breaking up the patch to make it easier to focus on the few lines needed to finish it off without the distraction of all the rest.

If you are interested, I am finishing things off in https://bugs.webkit.org/show_bug.cgi?id=220684.

[Sam Weinig]

(In reply to Sam Weinig from comment #33)
> (In reply to Lea Verou from comment #30)
> > > This change does not add support for the host platform layer (e.g. CoreGraphics, etc)
> > > understanding of the Lab colorspace, and as such, when creating host color objects
> > > (for instance via the cachedCGColor() helper) Lab<float> colors are converted to sRGB
> > > first and then the host color is made. The ramification of this is for platforms that
> > > use backingstores with greater than sRGB gamuts, Lab colors will be clampted to sRGB.
> > > This should be rectified in a follow up change that adds proper creation of these host
> > > objects but is being left off the initial change to limit changes.
> > 
> > If I'm reading this right, it appears that lab()/lch() colors are currently
> > clamped to sRGB? If so, please hold off shipping them until this can be
> > resolved. It will be detrimental to their adoption if they cannot be trusted
> > to go beyond sRGB, as authors cannot detect this programmatically. We do not
> > want to see people resorting to using lch() with a color(display-p3)
> > fallback, do we?
> 
> Yep, no intention of shipping like this, just breaking up the patch to make
> it easier to focus on the few lines needed to finish it off without the
> distraction of all the rest.
> 
> If you are interested, I am finishing things off in
> https://bugs.webkit.org/show_bug.cgi?id=220684.

Following up, the clamping has been removed as of <https://trac.webkit.org/changeset/271712>.