1450554 - Identity box security text color has low contrast on dark themes

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P3)

Description

[Tim Nguyen :ntim]

Attachment: Screenshot

Comment 1

[Manish [:manishkk]]

Hello :ntim 
I want to work on this bug because now I want to improve level.

Can you guide me, how I can edit code for this bug?

Thanks

Comment 2

[Manish [:manishkk]]

Sorry, you assigned this bug to yourself.

Comment 3

[Tim Nguyen :ntim]

Attachment: Bug 1450554 - Fix identity box security color on dark themes.

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

Comment 4

[Tim Nguyen :ntim]

Set multiple reviewers since both jaws and johannh are "slow to respond". :)

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8969978 [details]
Bug 1450554 - Fix identity box security color on dark themes.

https://reviewboard.mozilla.org/r/238760/#review245612

This makes sense to me, noting that I don't know much about the theme API parts :)

Thanks!

::: browser/themes/shared/identity-block/identity-block.inc.css:34
(Diff revision 1)
>  
>  #urlbar[pageproxystate="valid"] > #identity-box.verifiedIdentity > #identity-icon-labels {
>    color: #058B00;
>  }
> +
> +:root[lwt-toolbar-field-brighttext] #urlbar[pageproxystate="valid"] > #identity-box.verifiedIdentity > #identity-icon-labels {

Do we still need the #urlbar[pageproxystate="valid"] parts in this rule and above? Aren't the labels always hidden in invalid pageproxystate anyway?

Not a problem with your patch, though :)

::: toolkit/modules/LightweightThemeConsumer.jsm:58
(Diff revision 1)
> +      if (!rgbaChannels) {
> +        element.removeAttribute("lwt-toolbar-field-brighttext");
> +        return null;
> +      }
> +      const {r, g, b, a} = rgbaChannels;
> +      const luminance = 0.2125 * r + 0.7154 * g + 0.0721 * b;

You should have a shared function for computing the luminance.

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8969978 [details]
Bug 1450554 - Fix identity box security color on dark themes.

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

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8969978 [details]
Bug 1450554 - Fix identity box security color on dark themes.

https://reviewboard.mozilla.org/r/238760/#review245678

::: toolkit/modules/LightweightThemeConsumer.jsm:33
(Diff revisions 1 - 2)
>          element.removeAttribute("lwtheme");
>          return null;
>        }
>        const {r, g, b, a} = rgbaChannels;
> -      const luminance = 0.2125 * r + 0.7154 * g + 0.0721 * b;
> +      const luminance = _getLuminance(r, g, b);
>        element.setAttribute("lwthemetextcolor", luminance <= 110 ? "dark" : "bright");

You can share more here, namely the luminance <= 110 part, and rename the function to isTextColorDark or something.

Comment 8

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #7)
> Comment on attachment 8969978 [details]
> Bug 1450554 - Fix identity box security color on dark themes.
> 
> https://reviewboard.mozilla.org/r/238760/#review245678
> 
> ::: toolkit/modules/LightweightThemeConsumer.jsm:33
> (Diff revisions 1 - 2)
> >          element.removeAttribute("lwtheme");
> >          return null;
> >        }
> >        const {r, g, b, a} = rgbaChannels;
> > -      const luminance = 0.2125 * r + 0.7154 * g + 0.0721 * b;
> > +      const luminance = _getLuminance(r, g, b);
> >        element.setAttribute("lwthemetextcolor", luminance <= 110 ? "dark" : "bright");
> 
> You can share more here, namely the luminance <= 110 part, and rename the
> function to isTextColorDark or something.

Filed bug 1457099 since I already triggered landing on autoland.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e1f48fed0da
Fix identity box security color on dark themes. r=johannh

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7e1f48fed0da