Use context-fill for identity, connection and tracking protection icons in the site identity block

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
No description provided.

Comment 2

2 years ago
mozreview-review
Comment on attachment 8868894 [details]
Bug 1365846 - Use context-fill for identity, connection and tracking protection icons in the site identity block.

https://reviewboard.mozilla.org/r/140518/#review144054

Looks good, thanks for solving that!

r=me if the extension icon is fixed and we can clarify the right color.

::: browser/themes/shared/controlcenter/extension.svg:7
(Diff revision 1)
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
>       width="64" height="64" viewBox="0 0 64 64">
> -#include ../icon-colors.inc.svg
> +  <path d="M42,62c2.2,0,4-1.8,4-4l0-14.2c0,0,0.4-3.7,2.8-3.7c2.4,0,2.2,3.9,6.7,3.9c2.3,0,6.2-1.2,6.2-8.2 c0-7-3.9-7.9-6.2-7.9c-4.5,0-4.3,3.7-6.7,3.7c-2.4,0-2.8-3.8-2.8-3.8V22c0-2.2-1.8-4-4-4H31.5c0,0-3.4-0.6-3.4-3 c0-2.4,3.8-2.6,3.8-7.1c0-2.3-1.3-5.9-8.3-5.9s-8,3.6-8,5.9c0,4.5,3.4,4.7,3.4,7.1c0,2.4-3.4,3-3.4,3H6c-2.2,0-4,1.8-4,4l0,7.8 c0,0-0.4,6,4.4,6c3.1,0,3.2-4.1,7.3-4.1c2,0,4,1.9,4,6c0,4.2-2,6.3-4,6.3c-4,0-4.2-4.1-7.3-4.1c-4.8,0-4.4,5.8-4.4,5.8L2,58 c0,2.2,1.8,4,4,4H19c0,0,6.3,0.4,6.3-4.4c0-3.1-4-3.6-4-7.7c0-2,2.2-4.5,6.4-4.5c4.2,0,6.6,2.5,6.6,4.5c0,4-3.9,4.6-3.9,7.7 c0,4.9,6.3,4.4,6.3,4.4H42z"/>

You're missing a fill=context-fill here and you might need to use a <use>, too?

::: browser/themes/shared/controlcenter/panel.inc.css:259
(Diff revision 1)
>  
>  #identity-popup[connection=extension] #identity-popup-securityView,
>  #identity-popup[connection=extension] #identity-popup-security-content {
>    background-image: url(chrome://browser/skin/controlcenter/extension.svg);
> +  -moz-context-properties: fill;
> +  fill: #60bf4c;

I agree this icon looks better in green, but bug 1266012 made it grey for some reason. It might have been because I said in that bug that green shouldn't be used, but I was only referring to the text color. I'd personally be okay with the icon being green.

We can either needinfo Markus to find that out or you just change this to GrayText and we can land and follow up with them later. Up to you.

::: browser/themes/shared/identity-block/identity-block.inc.css:180
(Diff revision 1)
>  }
>  
>  /* CONNECTION ICON, EXTENSION ICON */
>  
> -#connection-icon, #extension-icon {
> -  width: 16px;
> +#connection-icon,
> +#extension-icon {

Any specific reason for linebreaking here? You can keep the blame if you revert that.
Attachment #8868894 - Flags: review?(jhofmann) → review+

Comment 3

2 years ago
mozreview-review-reply
Comment on attachment 8868894 [details]
Bug 1365846 - Use context-fill for identity, connection and tracking protection icons in the site identity block.

https://reviewboard.mozilla.org/r/140518/#review144054

> I agree this icon looks better in green, but bug 1266012 made it grey for some reason. It might have been because I said in that bug that green shouldn't be used, but I was only referring to the text color. I'd personally be okay with the icon being green.
> 
> We can either needinfo Markus to find that out or you just change this to GrayText and we can land and follow up with them later. Up to you.

Ugh, nevermind, this is the control center, not the identity block. Sorry, I was confused by the icon being gray.
Assignee

Comment 4

2 years ago
(In reply to Johann Hofmann [:johannh] from comment #2)
> ::: browser/themes/shared/controlcenter/panel.inc.css:259
> (Diff revision 1)
> >  
> >  #identity-popup[connection=extension] #identity-popup-securityView,
> >  #identity-popup[connection=extension] #identity-popup-security-content {
> >    background-image: url(chrome://browser/skin/controlcenter/extension.svg);
> > +  -moz-context-properties: fill;
> > +  fill: #60bf4c;
> 
> I agree this icon looks better in green, but bug 1266012 made it grey for
> some reason. It might have been because I said in that bug that green
> shouldn't be used, but I was only referring to the text color. I'd
> personally be okay with the icon being green.
> 
> We can either needinfo Markus to find that out or you just change this to
> GrayText and we can land and follow up with them later. Up to you.

It's green in the mockup, attachment 8867145 [details].

> ::: browser/themes/shared/identity-block/identity-block.inc.css:180
> (Diff revision 1)
> >  }
> >  
> >  /* CONNECTION ICON, EXTENSION ICON */
> >  
> > -#connection-icon, #extension-icon {
> > -  width: 16px;
> > +#connection-icon,
> > +#extension-icon {
> 
> Any specific reason for linebreaking here? You can keep the blame if you
> revert that.

We always break the line after the comma. It keeps rules readable.
Comment hidden (mozreview-request)

Comment 6

2 years ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a254d1c9a487
Use context-fill for identity, connection and tracking protection icons in the site identity block. r=johannh

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a254d1c9a487
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Not sure how, but mozscreenshots says that this caused the permission icons in the identity block to go full black in LWT:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=cc65f9233e5b16265ae44b643c7d255556da14fb&newProject=mozilla-central&newRev=3801e9bd76ad91b5754faacd3fec9ad4bc3fc782

I'll make a bug.
Depends on: 1367565
You need to log in before you can comment on or make changes to this bug.