Closed Bug 1457512 Opened 2 years ago Closed 2 years ago

Lighten two Control Center strings that describe unexceptional states

Categories

(Firefox :: Site Identity, enhancement, P3)

55 Branch
enhancement

Tracking

()

VERIFIED FIXED
Firefox 62
Tracking Status
firefox62 --- verified
firefox63 --- verified

People

(Reporter: rfeeley, Assigned: trisha, Mentored, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

Attached image 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
That's an excellent idea, Trisha, would you like to do this?
Mentor: jhofmann
Flags: needinfo?(guptatrisha97)
Priority: -- → P3
Assignee: nobody → guptatrisha97
Flags: needinfo?(guptatrisha97)
Status: NEW → ASSIGNED
Can I please know where I'm supposed to make the change? Thanks.
(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!
Attached patch bug1457512.patch (obsolete) — Splinter Review
I think it works for me. Please review and let me know if anything's wrong. Thanks! :)
Attachment #8972651 - Flags: review?(jhofmann)
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!
Attachment #8972651 - Flags: review?(jhofmann) → review-
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.
Flags: needinfo?(shorlander)
Flags: needinfo?(bbell)
Attached image 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).
Can you please tell me how you want me to move forward with this then? Should I let them all be grey?
Attached patch bug1457512.patch (obsolete) — Splinter Review
Is this limitation on greying fine? Or should I make it more limited to certain content only?
Attachment #8972651 - Attachment is obsolete: true
Attachment #8981677 - Flags: review?(jhofmann)
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!
Attachment #8981677 - Flags: review?(jhofmann) → review-
Attached patch bug1457512.patch (obsolete) — Splinter Review
Attachment #8981677 - Attachment is obsolete: true
Attachment #8981828 - Flags: review?(jhofmann)
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?
Attachment #8981828 - Flags: review?(jhofmann) → review+
(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 :)
Attached patch bug1457512.patch (obsolete) — Splinter Review
Attachment #8981828 - Attachment is obsolete: true
Keywords: checkin-needed
Keywords: checkin-needed
Attachment #8981838 - Attachment is obsolete: true
Attached patch bug1457512.patchSplinter Review
Keywords: checkin-needed
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/237a444eda98
Lighten two Control Center strings that describe unexceptional states r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/237a444eda98
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.