<input type="text" style="color: #acacac; background-color: #f2f2f2;" disabled>
<rdar://problem/70522921>
Created attachment 416449 [details] Patch
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.
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?
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?
Created attachment 416454 [details] Before (using contrastRatio)
Created attachment 416455 [details] After (using differenceSquared)
Created attachment 416456 [details] Testcase
(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.
(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.
(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.
Created attachment 416462 [details] Before (minConstrastRatio = 1.1)
Created attachment 416464 [details] Alternate Solution (minContrastRatio = 1.195)
(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.
(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?
(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.
(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?
(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.
Created attachment 416574 [details] Patch
(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.
(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!
Committed r271021: <https://trac.webkit.org/changeset/271021> All reviewed patches have been landed. Closing bug and clearing flags on attachment 416574 [details].