Add support for lab(), lch() and gray() colors.
<rdar://problem/58264078>
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.
Created attachment 386699 [details] Patch
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.
Created attachment 386722 [details] Patch
Created attachment 386752 [details] Patch
Created attachment 386754 [details] Patch
Created attachment 386758 [details] Patch
Created attachment 386759 [details] Patch
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.
Noted, although WebKit already supports P3 colors so adding lab() support isn't going to newly expose out-of-sRGB colors.
Created attachment 399486 [details] Patch
Created attachment 416728 [details] Patch
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 ?
(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.
Created attachment 416793 [details] Patch
Created attachment 416915 [details] Patch
Simon, what do you think is left here to get this in?
Created attachment 416916 [details] Patch
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.
Created attachment 417011 [details] Patch
(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.
Created attachment 417022 [details] Patch
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
Created attachment 417027 [details] Patch
Created attachment 417352 [details] Patch
Created attachment 417353 [details] Patch
Created attachment 417357 [details] Patch
Committed r271362: <https://trac.webkit.org/changeset/271362> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417357 [details].
> 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?
> 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?
(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.
(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.
(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>.