Bug 205675

Summary: Support lab(), lch() and color(lab ...) colors
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: CSSAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, benjamin, bkardell, cdumez, chris, clopez, cmarcelo, dino, emilio, esprehn+autocc, ews-watchlist, felipeerias, fred.wang, glenn, gyuyoung.kim, kondapallykalyan, kyle.bavender, lea, macpherson, menard, michael, mjs, nvasilyev, redbalex, ryuan.choi, sam, sergio, simon.fraser, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar, WebExposed
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=220684
Bug Depends on:    
Bug Blocks: 205725    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch none

Description Simon Fraser (smfr) 2020-01-01 11:58:49 PST
Add support for lab(), lch() and gray() colors.
Comment 1 Radar WebKit Bug Importer 2020-01-01 15:27:23 PST
<rdar://problem/58264078>
Comment 2 Simon Fraser (smfr) 2020-01-02 12:11:28 PST
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.
Comment 3 Simon Fraser (smfr) 2020-01-03 11:01:55 PST
Created attachment 386699 [details]
Patch
Comment 4 Dean Jackson 2020-01-03 11:11:55 PST
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.
Comment 5 Simon Fraser (smfr) 2020-01-03 15:40:45 PST
Created attachment 386722 [details]
Patch
Comment 6 Simon Fraser (smfr) 2020-01-03 21:07:32 PST
Created attachment 386752 [details]
Patch
Comment 7 Simon Fraser (smfr) 2020-01-03 21:27:11 PST
Created attachment 386754 [details]
Patch
Comment 8 Simon Fraser (smfr) 2020-01-03 22:53:37 PST
Created attachment 386758 [details]
Patch
Comment 9 Simon Fraser (smfr) 2020-01-03 23:04:36 PST
Created attachment 386759 [details]
Patch
Comment 10 Felipe Erias 2020-04-30 00:20:21 PDT
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.
Comment 11 Simon Fraser (smfr) 2020-04-30 09:37:12 PDT
Noted, although WebKit already supports P3 colors so adding lab() support isn't going to newly expose out-of-sRGB colors.
Comment 12 Simon Fraser (smfr) 2020-05-15 09:35:45 PDT
Created attachment 399486 [details]
Patch
Comment 13 Sam Weinig 2020-12-23 15:30:55 PST
Created attachment 416728 [details]
Patch
Comment 14 Simon Fraser (smfr) 2020-12-23 15:42:35 PST
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 ?
Comment 15 Sam Weinig 2020-12-23 15:59:17 PST
(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.
Comment 16 Sam Weinig 2020-12-26 15:07:47 PST Comment hidden (obsolete)
Comment 17 Sam Weinig 2021-01-03 11:58:00 PST Comment hidden (obsolete)
Comment 18 Sam Weinig 2021-01-03 12:03:42 PST Comment hidden (obsolete)
Comment 19 Sam Weinig 2021-01-03 12:09:53 PST Comment hidden (obsolete)
Comment 20 Simon Fraser (smfr) 2021-01-05 08:41:04 PST
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.
Comment 21 Sam Weinig 2021-01-05 08:57:49 PST Comment hidden (obsolete)
Comment 22 Sam Weinig 2021-01-05 08:59:25 PST
(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.
Comment 23 Sam Weinig 2021-01-05 11:39:05 PST Comment hidden (obsolete)
Comment 24 EWS Watchlist 2021-01-05 11:39:53 PST Comment hidden (obsolete)
Comment 25 Sam Weinig 2021-01-05 12:39:14 PST Comment hidden (obsolete)
Comment 26 Sam Weinig 2021-01-10 11:40:34 PST Comment hidden (obsolete)
Comment 27 Sam Weinig 2021-01-10 11:51:40 PST Comment hidden (obsolete)
Comment 28 Sam Weinig 2021-01-10 15:24:02 PST
Created attachment 417357 [details]
Patch
Comment 29 EWS 2021-01-11 08:57:03 PST
Committed r271362: <https://trac.webkit.org/changeset/271362>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417357 [details].
Comment 30 Lea Verou 2021-01-16 04:34:58 PST
> 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?
Comment 31 Chris Lilley 2021-01-16 04:36:46 PST
> 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?
Comment 32 Chris Lilley 2021-01-16 04:46:41 PST
(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.
Comment 33 Sam Weinig 2021-01-16 12:51:42 PST
(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.
Comment 34 Sam Weinig 2021-01-21 14:07:13 PST
(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>.