Identity box security text color has low contrast on dark themes

RESOLVED FIXED in Firefox 61

Status

()

defect
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: ntim, Assigned: ntim)

Tracking

(Blocks 1 bug)

unspecified
Firefox 61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

Last year
Posted image Screenshot
No description provided.
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
Flags: needinfo?(ntim.bugs)
Sorry, you assigned this bug to yourself.
Flags: needinfo?(ntim.bugs)
Priority: -- → P3
Comment hidden (mozreview-request)
Assignee

Updated

Last year
Attachment #8969978 - Flags: review?(jaws) → review?(jhofmann)
Assignee

Updated

Last year
Attachment #8969978 - Flags: review?(jaws)
Attachment #8969978 - Flags: review?(dao+bmo)
Assignee

Comment 4

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

Comment 5

Last year
mozreview-review
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.
Attachment #8969978 - Flags: review?(jhofmann) → review+
Comment hidden (mozreview-request)

Comment 7

Last year
mozreview-review
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.
Assignee

Updated

Last year
Depends on: 1457099
Assignee

Comment 8

Last year
(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

Last year
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7e1f48fed0da
Fix identity box security color on dark themes. r=johannh
https://hg.mozilla.org/mozilla-central/rev/7e1f48fed0da
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.