Closed Bug 1365846 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Theme, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file)

No description provided.
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 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.
(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.
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
https://hg.mozilla.org/mozilla-central/rev/a254d1c9a487
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.