1303291 - Clean up identity block and control center icons

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P2)

Description

[Dão Gottwald [:dao]]

Attachment: patch

I started fixing bug 1301945 but noticed that that many icons still use some shade of gray where they should use -moz-fieldtext. While taking care of that I also combined some similar icons into a single SVG to avoid code duplication, and renamed some files when I found the name confusing.

Sorry for the large patch, I hope it's all understandable. Of course, if you have questions, don't hesitate to ask and I'll explain the respective change in more detail.

Comment 1

[Johann Hofmann [:johannh]]

Comment on attachment 8791904 [details] [diff] [review]
patch

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

The changes seem good to me from looking at the code and running it, but as you mentioned it's a pretty big patch with a lot of fine-grained styling changes so I wouldn't be surprised if I missed a regression.

I think combining the SVGs is a good idea but this patch is only doing part of what could be possible. I'd really like us to start a follow-up bug that tries to reduce the number of files even more by combining the controlcenter and identity icons or at least merging e.g. conn-not-secure.svg into lock.svg. There are many cases like tracking-protection and tracking-protection-16 where I'm not sure if there's any difference between the two files that justifies keeping them separate.

Note that this patch causes/reveals the permission anchors and blocked permission icons to have a different tone of gray than the other identity block icons, most noticeably in dark devedition theme. I'd be ok with solving that in a different patch or another bug though, I think there's always been a slight difference it's just a bit more noticeable now.

::: browser/themes/shared/controlcenter/lock.svg
@@ +1,1 @@
>  <?xml version="1.0" encoding="utf-8"?>

Calling this file lock.svg kind of isn't adhering to the convention set by most other files in that it describes how it looks rather than what it's expressing. Maybe connection.svg or conn.svg (ugh) would be more appropriate?

::: browser/themes/shared/controlcenter/mcb-disabled.svg
@@ +1,1 @@
>  <?xml version="1.0" encoding="utf-8"?>

We should rename this file too, as it no longer only applies to mixed content blocking and mcb is a confusing abbreviation anyway. If we look into merging controlcenter and identity block icons we can also leave the name for now, since this file would hopefully be merged into another.

::: browser/themes/shared/jar.inc.mn
@@ +26,5 @@
>    skin/classic/browser/addons/addon-install-anchor.svg         (../shared/addons/addon-install-anchor.svg)
>    skin/classic/browser/controlcenter/arrow-subview.svg         (../shared/controlcenter/arrow-subview.svg)
>    skin/classic/browser/controlcenter/arrow-subview-back.svg    (../shared/controlcenter/arrow-subview-back.svg)
>    skin/classic/browser/controlcenter/conn-not-secure.svg       (../shared/controlcenter/conn-not-secure.svg)
> +  skin/classic/browser/controlcenter/lock.svg                  (../shared/controlcenter/lock.svg)

I'm not a big fan of having a lock icon called conn-not-secure.svg and another called lock.svg, but I'm not sure there's a good workaround for this except merging these icons with the identity icons.

Comment 2

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #1)
> Comment on attachment 8791904 [details] [diff] [review]
> patch
> 
> Review of attachment 8791904 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The changes seem good to me from looking at the code and running it, but as
> you mentioned it's a pretty big patch with a lot of fine-grained styling
> changes so I wouldn't be surprised if I missed a regression.
> 
> I think combining the SVGs is a good idea but this patch is only doing part
> of what could be possible. I'd really like us to start a follow-up bug that
> tries to reduce the number of files even more by combining the controlcenter
> and identity icons or at least merging e.g. conn-not-secure.svg into
> lock.svg. There are many cases like tracking-protection and
> tracking-protection-16 where I'm not sure if there's any difference between
> the two files that justifies keeping them separate.

I'll file a bug.

> Note that this patch causes/reveals the permission anchors and blocked
> permission icons to have a different tone of gray than the other identity
> block icons, most noticeably in dark devedition theme. I'd be ok with
> solving that in a different patch or another bug though, I think there's
> always been a slight difference it's just a bit more noticeable now.

ditto

> ::: browser/themes/shared/controlcenter/lock.svg
> @@ +1,1 @@
> >  <?xml version="1.0" encoding="utf-8"?>
> 
> Calling this file lock.svg kind of isn't adhering to the convention set by
> most other files in that it describes how it looks rather than what it's
> expressing. Maybe connection.svg or conn.svg (ugh) would be more appropriate?

I'll rename it to connection.svg.

> ::: browser/themes/shared/controlcenter/mcb-disabled.svg
> @@ +1,1 @@
> >  <?xml version="1.0" encoding="utf-8"?>
> 
> We should rename this file too, as it no longer only applies to mixed
> content blocking and mcb is a confusing abbreviation anyway. If we look into
> merging controlcenter and identity block icons we can also leave the name
> for now, since this file would hopefully be merged into another.

Yeah, bad name. I don't have a good idea for a better one right now so I'll leave this to a followup.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42a77283ee64
Clean up identity block and control center icons. r=jhofmann

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The landed patch caused perma test failures for Firefox-ui remote tests. Can we get those fixed or the patch backed out for a complete fix? Thanks.

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/42a77283ee64

Comment 6

[Dão Gottwald [:dao]]

Uplifted to fix bug 1301945:
https://hg.mozilla.org/releases/mozilla-aurora/rev/805d5fc3d66c