Better contrast comparison for Selection#setColors()

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

I've figure out a way to get rid of an XXX from bug 1360500 while making sure the text remain readable on both normal and high contrast mode. Patch to come.
Comment hidden (mozreview-request)
Comment on attachment 8868918 [details]
Bug 1365869 - Bug 1365869 - Better contrast comparison for Selection#setColors().

https://reviewboard.mozilla.org/r/140570/#review143968

::: layout/generic/nsTextFrame.cpp:3873
(Diff revision 1)
> -      int32_t foreLuminosityDifference =
> +      int32_t luminosityDifference =
>                  NS_LUMINOSITY_DIFFERENCE(foreColor, backColor);
> +      int32_t altLuminosityDifference =
> +                NS_LUMINOSITY_DIFFERENCE(foreColor, *customColors->mAltBackgroundColor);
>  
> -      // The sufficient luminosity difference is based on the link color of
> +      if (luminosityDifference < altLuminosityDifference) {

I don't think that this is good behavior. If alternative background color has better contrast, this makes always use it even when primary background color has sufficient contrast.

How about to use alternative color only when primary color doesn't have sufficient contrast?

::: layout/generic/nsTextFrame.cpp:3893
(Diff revision 1)
> -    if (customColors->mAltForegroundColor &&
> -        EnsureSufficientContrast(&foreColor, &backColor)) {
> +    if (customColors->mAltForegroundColor) {
> +      int32_t luminosityDifference =
> +                NS_LUMINOSITY_DIFFERENCE(foreColor, backColor);
> +      int32_t altLuminosityDifference =
> +                NS_LUMINOSITY_DIFFERENCE(*customColors->mForegroundColor, backColor);
> +
> +      if (luminosityDifference < altLuminosityDifference) {

Same here.

::: layout/generic/nsTextFrame.cpp:3905
(Diff revision 1)
> -      backColor = mFrameBackgroundColor;
> -    }
> +      }
> +    }
>  
>      *aForeColor = foreColor;
>      *aBackColor = backColor;

And I realized that in this case, shouldn't we return transparent color instead of solid color?
Attachment #8868918 - Flags: review?(masayuki) → review-
Comment hidden (mozreview-request)
Comment on attachment 8868918 [details]
Bug 1365869 - Bug 1365869 - Better contrast comparison for Selection#setColors().

https://reviewboard.mozilla.org/r/140570/#review144006

::: layout/generic/nsTextFrame.cpp:3895
(Diff revision 2)
>    }
>  
>    if (customColors->mForegroundColor) {
>      nscolor foreColor = *customColors->mForegroundColor;
>      // !mBackgroundColor means "transparent"; the current color of the background.
>      nscolor backColor = mFrameBackgroundColor;

Could you remove backColor variable and replace it with mFrameBackgroundColor in this block?
Attachment #8868918 - Flags: review?(masayuki) → review+
Comment hidden (mozreview-request)

Comment 6

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d7b47bab0515
Bug 1365869 - Better contrast comparison for Selection#setColors(). r=masayuki
Keywords: checkin-needed

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d7b47bab0515
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.