1365869 - Better contrast comparison for Selection#setColors()

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1365869 - Bug 1365869 - Better contrast comparison for Selection#setColors().

Instead of hardcoding a color-to-compare, compare the contrast between
the text and the background color v.s. text and the alternative
background color, and use the color that has better contrast.

Review commit: https://reviewboard.mozilla.org/r/140570/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140570/

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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?

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8868918 [details]
Bug 1365869 - Bug 1365869 - Better contrast comparison for Selection#setColors().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140570/diff/1-2/

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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?

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8868918 [details]
Bug 1365869 - Bug 1365869 - Better contrast comparison for Selection#setColors().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140570/diff/2-3/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7b47bab0515
Bug 1365869 - Better contrast comparison for Selection#setColors(). r=masayuki

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d7b47bab0515