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

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement)

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1365846 - Use context-fill for identity, connection and tracking protection icons in the site identity block.

Review commit: https://reviewboard.mozilla.org/r/140518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140518/

Comment 2

[Johann Hofmann [:johannh]]

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.

Comment 3

[Johann Hofmann [:johannh]]

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.

Comment 4

[Dão Gottwald [:dao]]

(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 5

[Dão Gottwald [:dao]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140518/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a254d1c9a487

Comment 8

[Johann Hofmann [:johannh]]

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.