Bug 219991 – REGRESSION (r262729): Poor contrast for specific color/background combinations on disabled input fields

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Aditya Keerthi]

<input type="text" style="color: #acacac; background-color: #f2f2f2;" disabled>

[Aditya Keerthi]

<rdar://problem/70522921>

[Aditya Keerthi]

Created attachment 416449 [details]
Patch

[Tim Horton]

Comment on attachment 416449 [details]
Patch

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

> Source/WebCore/rendering/RenderTheme.cpp:1425
> -    if (contrastRatio(disabledColor.toSRGBALossy<float>(), backgroundColor.toSRGBALossy<float>()) < minColorContrastValue)
> +    if (differenceSquared(disabledColor.toSRGBALossy<uint8_t>(), backgroundColor.toSRGBALossy<uint8_t>()) < minColorContrastValue)

I wonder if it's preferable to use textColorIsLegibleAgainstBackgroundColor, to share the definition of legibility with the rest of WebKit? Or maybe that is too strict.

[Sam Weinig]

Hi Aditya,

I'm a bit confused by this bug report since the description doesn't actually say what is wrong. Can you provide before and after screenshots so we can see how it changed?

[Sam Weinig]

I'd like to avoid re-introducing difference squared here, as its pretty much non-sense, color-metrically, and won't scale to different color spaces, so if we could clarify what the issue is, that would be great.

You say in the title, "specific color/background combinations" but I am not clear what you mean by that. Are these specific color/background combinations that exist in the wild?

[Aditya Keerthi]

Created attachment 416454 [details]
Before (using contrastRatio)

[Aditya Keerthi]

Created attachment 416455 [details]
After (using differenceSquared)

[Aditya Keerthi]

Created attachment 416456 [details]
Testcase

[Aditya Keerthi]

(In reply to Sam Weinig from comment #5)
> I'd like to avoid re-introducing difference squared here, as its pretty much
> non-sense, color-metrically, and won't scale to different color spaces, so
> if we could clarify what the issue is, that would be great.
> 
> You say in the title, "specific color/background combinations" but I am not
> clear what you mean by that. Are these specific color/background
> combinations that exist in the wild?

Attached before/after screenshots and the test case.

The specific combination was reported by a developer through Feedback Assistant.

[Sam Weinig]

(In reply to Aditya Keerthi from comment #9)
> (In reply to Sam Weinig from comment #5)
> > I'd like to avoid re-introducing difference squared here, as its pretty much
> > non-sense, color-metrically, and won't scale to different color spaces, so
> > if we could clarify what the issue is, that would be great.
> > 
> > You say in the title, "specific color/background combinations" but I am not
> > clear what you mean by that. Are these specific color/background
> > combinations that exist in the wild?
> 
> Attached before/after screenshots and the test case.
> 
> The specific combination was reported by a developer through Feedback
> Assistant.

Thanks.

Can you also clarify which tests failed when you upped the minimum color contrast?

Another idea here is to improve the lighten and darken functions to be meaningful. I think they currently operate in same non-linear space. Would probably make more sense for them to also be a luminance transform, though I am not sure exactly on the math to do that.

If it's really important to revert this change now, that's ok, but if there is a little time to find a solution that maintains some of the nice aspects of the change I think that would be good.

[Sam Weinig]

(In reply to Sam Weinig from comment #10)
> (In reply to Aditya Keerthi from comment #9)
> > (In reply to Sam Weinig from comment #5)
> > > I'd like to avoid re-introducing difference squared here, as its pretty much
> > > non-sense, color-metrically, and won't scale to different color spaces, so
> > > if we could clarify what the issue is, that would be great.
> > > 
> > > You say in the title, "specific color/background combinations" but I am not
> > > clear what you mean by that. Are these specific color/background
> > > combinations that exist in the wild?
> > 
> > Attached before/after screenshots and the test case.
> > 
> > The specific combination was reported by a developer through Feedback
> > Assistant.
> 
> Thanks.
> 
> Can you also clarify which tests failed when you upped the minimum color
> contrast?
> 
> Another idea here is to improve the lighten and darken functions to be
> meaningful. I think they currently operate in same non-linear space. Would
> probably make more sense for them to also be a luminance transform, though I
> am not sure exactly on the math to do that.
> 
> If it's really important to revert this change now, that's ok, but if there
> is a little time to find a solution that maintains some of the nice aspects
> of the change I think that would be good.

Thinking about making a better lightened/darkened, I think it would probably be something like:

auto xyz = toXYZ(toLinearSRGB(color));
xyz.y *= someMultiplied;
return toSRGBA(toLinearSRGBA(xyz));

Where someMultiplied would have to be tweaked like the current lightened/darkened "v - 0.33f" is.

[Aditya Keerthi]

Created attachment 416462 [details]
Before (minConstrastRatio = 1.1)

[Aditya Keerthi]

Created attachment 416464 [details]
Alternate Solution (minContrastRatio = 1.195)

[Aditya Keerthi]

(In reply to Sam Weinig from comment #10)
> (In reply to Aditya Keerthi from comment #9)
> > (In reply to Sam Weinig from comment #5)
> > > I'd like to avoid re-introducing difference squared here, as its pretty much
> > > non-sense, color-metrically, and won't scale to different color spaces, so
> > > if we could clarify what the issue is, that would be great.
> > > 
> > > You say in the title, "specific color/background combinations" but I am not
> > > clear what you mean by that. Are these specific color/background
> > > combinations that exist in the wild?
> > 
> > Attached before/after screenshots and the test case.
> > 
> > The specific combination was reported by a developer through Feedback
> > Assistant.
> 
> Thanks.
> 
> Can you also clarify which tests failed when you upped the minimum color
> contrast?

Attached screenshots of fast/form/input-disabled-color.html with the existing contrast ratio threshold (1.1) and the minimum which would fix the broken testcase (1.195).

Note the changes in the left column on rows 3 and 4.

[Aditya Keerthi]

(In reply to Sam Weinig from comment #11)
> (In reply to Sam Weinig from comment #10)
> > (In reply to Aditya Keerthi from comment #9)
> > > (In reply to Sam Weinig from comment #5)
> > > > I'd like to avoid re-introducing difference squared here, as its pretty much
> > > > non-sense, color-metrically, and won't scale to different color spaces, so
> > > > if we could clarify what the issue is, that would be great.
> > > > 
> > > > You say in the title, "specific color/background combinations" but I am not
> > > > clear what you mean by that. Are these specific color/background
> > > > combinations that exist in the wild?
> > > 
> > > Attached before/after screenshots and the test case.
> > > 
> > > The specific combination was reported by a developer through Feedback
> > > Assistant.
> > 
> > Thanks.
> > 
> > Can you also clarify which tests failed when you upped the minimum color
> > contrast?
> > 
> > Another idea here is to improve the lighten and darken functions to be
> > meaningful. I think they currently operate in same non-linear space. Would
> > probably make more sense for them to also be a luminance transform, though I
> > am not sure exactly on the math to do that.
> > 
> > If it's really important to revert this change now, that's ok, but if there
> > is a little time to find a solution that maintains some of the nice aspects
> > of the change I think that would be good.
> 
> Thinking about making a better lightened/darkened, I think it would probably
> be something like:
> 
> auto xyz = toXYZ(toLinearSRGB(color));
> xyz.y *= someMultiplied;
> return toSRGBA(toLinearSRGBA(xyz));
> 
> Where someMultiplied would have to be tweaked like the current
> lightened/darkened "v - 0.33f" is.

Can you clarify how making a better lightened/darkened would solve the issue?

Wouldn't that just change the color combination for which the minConstrastRatio check fails?

[Aditya Keerthi]

(In reply to Tim Horton from comment #3)
> Comment on attachment 416449 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416449&action=review
> 
> > Source/WebCore/rendering/RenderTheme.cpp:1425
> > -    if (contrastRatio(disabledColor.toSRGBALossy<float>(), backgroundColor.toSRGBALossy<float>()) < minColorContrastValue)
> > +    if (differenceSquared(disabledColor.toSRGBALossy<uint8_t>(), backgroundColor.toSRGBALossy<uint8_t>()) < minColorContrastValue)
> 
> I wonder if it's preferable to use textColorIsLegibleAgainstBackgroundColor,
> to share the definition of legibility with the rest of WebKit? Or maybe that
> is too strict.

The minimum contrast ratio for textColorIsLegibleAgainstBackgroundColor is 4.5, which is much more strict than the existing 1.1.

[Sam Weinig]

(In reply to Aditya Keerthi from comment #15)
> (In reply to Sam Weinig from comment #11)
> > (In reply to Sam Weinig from comment #10)
> > > (In reply to Aditya Keerthi from comment #9)
> > > > (In reply to Sam Weinig from comment #5)
> > > > > I'd like to avoid re-introducing difference squared here, as its pretty much
> > > > > non-sense, color-metrically, and won't scale to different color spaces, so
> > > > > if we could clarify what the issue is, that would be great.
> > > > > 
> > > > > You say in the title, "specific color/background combinations" but I am not
> > > > > clear what you mean by that. Are these specific color/background
> > > > > combinations that exist in the wild?
> > > > 
> > > > Attached before/after screenshots and the test case.
> > > > 
> > > > The specific combination was reported by a developer through Feedback
> > > > Assistant.
> > > 
> > > Thanks.
> > > 
> > > Can you also clarify which tests failed when you upped the minimum color
> > > contrast?
> > > 
> > > Another idea here is to improve the lighten and darken functions to be
> > > meaningful. I think they currently operate in same non-linear space. Would
> > > probably make more sense for them to also be a luminance transform, though I
> > > am not sure exactly on the math to do that.
> > > 
> > > If it's really important to revert this change now, that's ok, but if there
> > > is a little time to find a solution that maintains some of the nice aspects
> > > of the change I think that would be good.
> > 
> > Thinking about making a better lightened/darkened, I think it would probably
> > be something like:
> > 
> > auto xyz = toXYZ(toLinearSRGB(color));
> > xyz.y *= someMultiplied;
> > return toSRGBA(toLinearSRGBA(xyz));
> > 
> > Where someMultiplied would have to be tweaked like the current
> > lightened/darkened "v - 0.33f" is.
> 
> Can you clarify how making a better lightened/darkened would solve the issue?
> 
> Wouldn't that just change the color combination for which the
> minConstrastRatio check fails?

The goal was to change the look so that we were actually lightening / darkening things correctly such that the contrast ratio would more likely to be useful, but actually looking at the alternative solution output of fast/forms/input-disabled-color.html, I think the new results subjectively (that's really all there is here) better, as I can't really read the disabled rows 3 and 4 in the current solution. What do you think?

[Aditya Keerthi]

(In reply to Sam Weinig from comment #17)
> (In reply to Aditya Keerthi from comment #15)
> > (In reply to Sam Weinig from comment #11)
> > > (In reply to Sam Weinig from comment #10)
> > > > (In reply to Aditya Keerthi from comment #9)
> > > > > (In reply to Sam Weinig from comment #5)
> > > > > > I'd like to avoid re-introducing difference squared here, as its pretty much
> > > > > > non-sense, color-metrically, and won't scale to different color spaces, so
> > > > > > if we could clarify what the issue is, that would be great.
> > > > > > 
> > > > > > You say in the title, "specific color/background combinations" but I am not
> > > > > > clear what you mean by that. Are these specific color/background
> > > > > > combinations that exist in the wild?
> > > > > 
> > > > > Attached before/after screenshots and the test case.
> > > > > 
> > > > > The specific combination was reported by a developer through Feedback
> > > > > Assistant.
> > > > 
> > > > Thanks.
> > > > 
> > > > Can you also clarify which tests failed when you upped the minimum color
> > > > contrast?
> > > > 
> > > > Another idea here is to improve the lighten and darken functions to be
> > > > meaningful. I think they currently operate in same non-linear space. Would
> > > > probably make more sense for them to also be a luminance transform, though I
> > > > am not sure exactly on the math to do that.
> > > > 
> > > > If it's really important to revert this change now, that's ok, but if there
> > > > is a little time to find a solution that maintains some of the nice aspects
> > > > of the change I think that would be good.
> > > 
> > > Thinking about making a better lightened/darkened, I think it would probably
> > > be something like:
> > > 
> > > auto xyz = toXYZ(toLinearSRGB(color));
> > > xyz.y *= someMultiplied;
> > > return toSRGBA(toLinearSRGBA(xyz));
> > > 
> > > Where someMultiplied would have to be tweaked like the current
> > > lightened/darkened "v - 0.33f" is.
> > 
> > Can you clarify how making a better lightened/darkened would solve the issue?
> > 
> > Wouldn't that just change the color combination for which the
> > minConstrastRatio check fails?
> 
> The goal was to change the look so that we were actually lightening /
> darkening things correctly such that the contrast ratio would more likely to
> be useful, but actually looking at the alternative solution output of
> fast/forms/input-disabled-color.html, I think the new results subjectively
> (that's really all there is here) better, as I can't really read the
> disabled rows 3 and 4 in the current solution. What do you think?

I agree that the new results are subjectively better. I'll upload a new patch with the alternate solution once I rebuild + rebaseline the tests.

[Aditya Keerthi]

Created attachment 416574 [details]
Patch

[Sam Weinig]

(In reply to Aditya Keerthi from comment #18)
> (In reply to Sam Weinig from comment #17)
> > (In reply to Aditya Keerthi from comment #15)
> > > (In reply to Sam Weinig from comment #11)
> > > > (In reply to Sam Weinig from comment #10)
> > > > > (In reply to Aditya Keerthi from comment #9)
> > > > > > (In reply to Sam Weinig from comment #5)
> > > > > > > I'd like to avoid re-introducing difference squared here, as its pretty much
> > > > > > > non-sense, color-metrically, and won't scale to different color spaces, so
> > > > > > > if we could clarify what the issue is, that would be great.
> > > > > > > 
> > > > > > > You say in the title, "specific color/background combinations" but I am not
> > > > > > > clear what you mean by that. Are these specific color/background
> > > > > > > combinations that exist in the wild?
> > > > > > 
> > > > > > Attached before/after screenshots and the test case.
> > > > > > 
> > > > > > The specific combination was reported by a developer through Feedback
> > > > > > Assistant.
> > > > > 
> > > > > Thanks.
> > > > > 
> > > > > Can you also clarify which tests failed when you upped the minimum color
> > > > > contrast?
> > > > > 
> > > > > Another idea here is to improve the lighten and darken functions to be
> > > > > meaningful. I think they currently operate in same non-linear space. Would
> > > > > probably make more sense for them to also be a luminance transform, though I
> > > > > am not sure exactly on the math to do that.
> > > > > 
> > > > > If it's really important to revert this change now, that's ok, but if there
> > > > > is a little time to find a solution that maintains some of the nice aspects
> > > > > of the change I think that would be good.
> > > > 
> > > > Thinking about making a better lightened/darkened, I think it would probably
> > > > be something like:
> > > > 
> > > > auto xyz = toXYZ(toLinearSRGB(color));
> > > > xyz.y *= someMultiplied;
> > > > return toSRGBA(toLinearSRGBA(xyz));
> > > > 
> > > > Where someMultiplied would have to be tweaked like the current
> > > > lightened/darkened "v - 0.33f" is.
> > > 
> > > Can you clarify how making a better lightened/darkened would solve the issue?
> > > 
> > > Wouldn't that just change the color combination for which the
> > > minConstrastRatio check fails?
> > 
> > The goal was to change the look so that we were actually lightening /
> > darkening things correctly such that the contrast ratio would more likely to
> > be useful, but actually looking at the alternative solution output of
> > fast/forms/input-disabled-color.html, I think the new results subjectively
> > (that's really all there is here) better, as I can't really read the
> > disabled rows 3 and 4 in the current solution. What do you think?
> 
> I agree that the new results are subjectively better. I'll upload a new
> patch with the alternate solution once I rebuild + rebaseline the tests.

Awesome. Thanks! Patch looks good. r=me.

[Aditya Keerthi]

(In reply to Sam Weinig from comment #20)
> (In reply to Aditya Keerthi from comment #18)
> > (In reply to Sam Weinig from comment #17)
> > > (In reply to Aditya Keerthi from comment #15)
> > > > (In reply to Sam Weinig from comment #11)
> > > > > (In reply to Sam Weinig from comment #10)
> > > > > > (In reply to Aditya Keerthi from comment #9)
> > > > > > > (In reply to Sam Weinig from comment #5)
> > > > > > > > I'd like to avoid re-introducing difference squared here, as its pretty much
> > > > > > > > non-sense, color-metrically, and won't scale to different color spaces, so
> > > > > > > > if we could clarify what the issue is, that would be great.
> > > > > > > > 
> > > > > > > > You say in the title, "specific color/background combinations" but I am not
> > > > > > > > clear what you mean by that. Are these specific color/background
> > > > > > > > combinations that exist in the wild?
> > > > > > > 
> > > > > > > Attached before/after screenshots and the test case.
> > > > > > > 
> > > > > > > The specific combination was reported by a developer through Feedback
> > > > > > > Assistant.
> > > > > > 
> > > > > > Thanks.
> > > > > > 
> > > > > > Can you also clarify which tests failed when you upped the minimum color
> > > > > > contrast?
> > > > > > 
> > > > > > Another idea here is to improve the lighten and darken functions to be
> > > > > > meaningful. I think they currently operate in same non-linear space. Would
> > > > > > probably make more sense for them to also be a luminance transform, though I
> > > > > > am not sure exactly on the math to do that.
> > > > > > 
> > > > > > If it's really important to revert this change now, that's ok, but if there
> > > > > > is a little time to find a solution that maintains some of the nice aspects
> > > > > > of the change I think that would be good.
> > > > > 
> > > > > Thinking about making a better lightened/darkened, I think it would probably
> > > > > be something like:
> > > > > 
> > > > > auto xyz = toXYZ(toLinearSRGB(color));
> > > > > xyz.y *= someMultiplied;
> > > > > return toSRGBA(toLinearSRGBA(xyz));
> > > > > 
> > > > > Where someMultiplied would have to be tweaked like the current
> > > > > lightened/darkened "v - 0.33f" is.
> > > > 
> > > > Can you clarify how making a better lightened/darkened would solve the issue?
> > > > 
> > > > Wouldn't that just change the color combination for which the
> > > > minConstrastRatio check fails?
> > > 
> > > The goal was to change the look so that we were actually lightening /
> > > darkening things correctly such that the contrast ratio would more likely to
> > > be useful, but actually looking at the alternative solution output of
> > > fast/forms/input-disabled-color.html, I think the new results subjectively
> > > (that's really all there is here) better, as I can't really read the
> > > disabled rows 3 and 4 in the current solution. What do you think?
> > 
> > I agree that the new results are subjectively better. I'll upload a new
> > patch with the alternate solution once I rebuild + rebaseline the tests.
> 
> Awesome. Thanks! Patch looks good. r=me.

Thank you for the review!

[EWS]

Committed r271021: <https://trac.webkit.org/changeset/271021>

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