Closed Bug 1365869 Opened 3 years ago Closed 3 years ago

Better contrast comparison for Selection#setColors()

Categories

(Firefox :: Preferences, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(1 file)

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 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 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+
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
https://hg.mozilla.org/mozilla-central/rev/d7b47bab0515
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.