1457512 - Lighten two Control Center strings that describe unexceptional states

Status: VERIFIED FIXED

(Firefox :: Site Identity, enhancement, P3)

Description

[Ryan Feeley [:rfeeley]]

Attachment: unexceptional.png

To better signal to users that at a glance that there's “nothing to see here”, we should lighten to colour of the font of these two strings to -moz-dialogtext with an opacity of 0.5.

- No tracking elements detected on this page.
- You have not granted this site any special permissions.

https://hg.mozilla.org/l10n/gecko-strings/file/default/browser/chrome/browser/browser.dtd#l905

https://hg.mozilla.org/l10n/gecko-strings/file/default/browser/chrome/browser/browser.dtd#l801

Comment 1

[Johann Hofmann [:johannh]]

That's an excellent idea, Trisha, would you like to do this?

Comment 2

[Trisha Gupta [:trisha]]

Can I please know where I'm supposed to make the change? Thanks.

Comment 3

[Johann Hofmann [:johannh]]

(In reply to Trisha Gupta [:trisha] from comment #2)
> Can I please know where I'm supposed to make the change? Thanks.

If you haven't already, this would be an excellent opportunity to get familiar with the browser toolbox (https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox). It allows you to inspect the browser UI like you would inspect a webpage (and offers debugging and storage inspection and much more, but that's probably not as relevant for this bug).

When you have found the ID or class of the element you'd like to change, you can search for it on https://searchfox.org/. This should give you an idea of which CSS file you can use to change the colors.

Please let me know if you need any help with that via IRC (though I'll be unavailable most of today).

Thanks!

Comment 4

[Trisha Gupta [:trisha]]

Attachment: bug1457512.patch

I think it works for me. Please review and let me know if anything's wrong. Thanks! :)

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8972651 [details] [diff] [review]
bug1457512.patch

Review of attachment 8972651 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like the correct file to change, and your patch definitely does what it's supposed to, just a little too much. We shouldn't change all descriptions in that dialog, just the "No tracking elements detected on this page." and the "You have not granted this site any special permissions." descriptions.

Please add a new rule for only these two elements instead.

Thank you!

Comment 6

[Ryan Feeley [:rfeeley]]

Interestingly, I spoke with BryanBell and he thought that it might in fact be better to make them all grey (except the green versions). I have NI:ed Bryan and Shorlander as I am on PTO shortly.

For what it's worth, in my mockups it does seem to work well when all the small text is grey.

Comment 7

[Johann Hofmann [:johannh]]

Attachment: gray permissions

Hm, did you try with e.g. permissions? IMO it would look a bit weird to have these grey (see screenshot, where I just put half opacity on everything).

Comment 8

[Trisha Gupta [:trisha]]

Can you please tell me how you want me to move forward with this then? Should I let them all be grey?

Comment 9

[Trisha Gupta [:trisha]]

Attachment: bug1457512.patch

Is this limitation on greying fine? Or should I make it more limited to certain content only?

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8981677 [details] [diff] [review]
bug1457512.patch

Review of attachment 8981677 [details] [diff] [review]:
-----------------------------------------------------------------

I think this selector works fine, but please use

color: var(--panel-disabled-color);

instead, which was introduced in bug 1459352 (-moz-dialogText and Graytext didn't work well on the dark theme)

Thanks!

Comment 11

[Trisha Gupta [:trisha]]

Attachment: bug1457512.patch

Comment 12

[Johann Hofmann [:johannh]]

Comment on attachment 8981828 [details] [diff] [review]
bug1457512.patch

Review of attachment 8981828 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks, but please see the comment :)

::: browser/themes/shared/controlcenter/panel.inc.css
@@ +148,5 @@
>  
> +#identity-popup-permissions-content > description,
> +#tracking-protection-content > description {
> +  color: var(--panel-disabled-color);
> +  opacity: 0.5;

I think we don't need the opacity anymore, otherwise it gets too light, do you agree?

Comment 13

[Trisha Gupta [:trisha]]

(In reply to Johann Hofmann [:johannh] from comment #12)
> Comment on attachment 8981828 [details] [diff] [review]
> bug1457512.patch
> 
> Review of attachment 8981828 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, thanks, but please see the comment :)
> 
> ::: browser/themes/shared/controlcenter/panel.inc.css
> @@ +148,5 @@
> >  
> > +#identity-popup-permissions-content > description,
> > +#tracking-protection-content > description {
> > +  color: var(--panel-disabled-color);
> > +  opacity: 0.5;
> 
> I think we don't need the opacity anymore, otherwise it gets too light, do
> you agree?

I thought the same; will go ahead and remove that then :)

Comment 14

[Trisha Gupta [:trisha]]

Attachment: bug1457512.patch

Comment 15

[Trisha Gupta [:trisha]]

Attachment: bug1457512.patch

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/237a444eda98
Lighten two Control Center strings that describe unexceptional states r=johannh

Comment 17

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/237a444eda98

Comment 18

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this enhancement was implemented, I verified using Fx 63.0a1(23-07-2018) and Fx 62.0b10, on Windows 10, Ubuntu 14.04 LTS x64 and mac OS 10.13.5.