1371230 - Stop using the :not(:target) selector in notification-icons.svg

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement)

Description

[Johann Hofmann [:johannh]]

See bug 1358998. This is a pretty big file and it's likely not loaded at startup, but I feel like we should still do it. We should definitely also put the updater icon somewhere else as it's really not related to the identity block notifications.

https://searchfox.org/mozilla-central/source/browser/themes/shared/notification-icons.svg

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Hey Dão, various of these icons have pairs of normal / blocked, and we are unsure of how to best approach that duplication. What would be best:

A) Entirely duplicate the paths and clip-paths for them

B) Define these paths and clip-paths in some separate .inc files and #include them on each file

C) Keep defining each pair of icon using one file, and use e.g. `<use id="microphone-blocked" class="blocked>"`.  This means we won't be using the :not(:target) rule (as requested by bug 1358998), but still each file will be defining two icons.

D) Something else?

Comment 2

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1371230 - Move notification-icons.svg icons into separate files

I chose to move elements used multiple times into their own .inc.svg files. Let me know if something else is preferred.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8890981 [details] [diff] [review]
Bug 1371230 - Move notification-icons.svg icons into separate files

>-  list-style-image: url(chrome://browser/skin/notification-icons.svg#login);
>+  list-style-image: url(chrome://browser/skin/login.svg);

Please package these files in chrome://browser/skin/notification-icons/ rather than chrome://browser/skin/.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8890981 [details] [diff] [review]
Bug 1371230 - Move notification-icons.svg icons into separate files

>+++ b/browser/themes/shared/notifications/desktop-notification-blocked.svg
>@@ -0,0 +1,10 @@
>+<!-- 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 fill="context-fill" fill-opacity="context-fill-opacity" xmlns="http://www.w3.org/2000/svg"
>+     width="32" height="32" viewBox="0 0 32 32">
>+#include clip-path.inc.svg
>+#include desktop-notification-icon.inc.svg
>+  <use id="desktop-notification-blocked" style="clip-path: url(#blocked-clipPath)" href="#desktop-notification-icon" />
>+#include strikeout.inc.svg
>+</svg>

Don't need id="desktop-notification-blocked", it's unused (same in your other svgs). And while you're at it, you could remove the superfluous space before />...

Comment 5

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1371230 - Move notification-icons.svg icons into separate files

Moved icons from chrome://browser/skin/ to chrome://browser/skin/notification-icons, removed unused IDs, and removed spaces before />.

Comment 6

[Perry Jiang [:perry] [no longer employee, use ni?]]

Comment on attachment 8892228 [details] [diff] [review]
Bug 1371230 - Move notification-icons.svg icons into separate files

Moved icons from chrome://browser/skin/ to chrome://browser/skin/notification-icons, removed unused IDs, and removed spaces before />.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8892228 [details] [diff] [review]
Bug 1371230 - Move notification-icons.svg icons into separate files

Sorry for the delay... This looks better, just a few minor issues.

>+* skin/classic/browser/notification-icons/camera-blocked.svg                (../shared/notifications/camera-blocked.svg)

Can you call both the packaged and the source folder notification-icons for consistency?

>+  skin/classic/browser/notification-icons/update.svg                        (../shared/update.svg)

Why is this in a different source folder?

>+++ b/browser/themes/shared/notifications/camera-icon.inc.svg
>@@ -0,0 +1,3 @@
>+<defs>
>+  <path id="camera-icon" d="m 2,23 a 3,3 0 0 0 3,3 l 14,0 a 3,3 0 0 0 3,-3 l 0,-4 6,5.5 c 0.5,0.5 1,0.7 2,0.5 l 0,-18 c -1,-0.2 -1.5,0 -2,0.5 l -6,5.5 0,-4 a 3,3 0 0 0 -3,-3 l -14,0 a 3,3 0 0 0 -3,3 z"/>
>+</defs>

>+++ b/browser/themes/shared/notifications/camera-indicator.svg
>@@ -0,0 +1,8 @@
>+<!-- 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 fill="context-fill" fill-opacity="context-fill-opacity" xmlns="http://www.w3.org/2000/svg"
>+     width="32" height="32" viewBox="0 0 32 32">
>+#include camera-icon.inc.svg
>+  <use href="#camera-icon"/>
>+</svg>

It doesn't make sense anymore to use <defs> and <use> for this. You can just import the <path> directly.

Comment 8

[Perry Jiang [:perry] [no longer employee, use ni?]]

(In reply to Dão Gottwald [::dao] from comment #7)
> Comment on attachment 8892228 [details] [diff] [review]
> >+  skin/classic/browser/notification-icons/update.svg                        (../shared/update.svg)
> 
> Why is this in a different source folder?

I was following the bug's description; do we want it also in a notification-icon source folder?

Comment 9

[Dão Gottwald [:dao]]

(In reply to :Perry Jiang from comment #8)
> (In reply to Dão Gottwald [::dao] from comment #7)
> > Comment on attachment 8892228 [details] [diff] [review]
> > >+  skin/classic/browser/notification-icons/update.svg                        (../shared/update.svg)
> > 
> > Why is this in a different source folder?
> 
> I was following the bug's description; do we want it also in a
> notification-icon source folder?

It's only used as a notification icon, so yes.

Comment 10

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1371230 - Move notification-icons.svg icons into separate files

Comment 11

[Perry Jiang [:perry] [no longer employee, use ni?]]

(In reply to Dão Gottwald [::dao] from comment #7)
> >+++ b/browser/themes/shared/notifications/camera-icon.inc.svg
> >@@ -0,0 +1,3 @@
> >+<defs>
> >+  <path id="camera-icon" d="m 2,23 a 3,3 0 0 0 3,3 l 14,0 a 3,3 0 0 0 3,-3 l 0,-4 6,5.5 c 0.5,0.5 1,0.7 2,0.5 l 0,-18 c -1,-0.2 -1.5,0 -2,0.5 l -6,5.5 0,-4 a 3,3 0 0 0 -3,-3 l -14,0 a 3,3 0 0 0 -3,3 z"/>
> >+</defs>
> 
> >+++ b/browser/themes/shared/notifications/camera-indicator.svg
> >@@ -0,0 +1,8 @@
> >+<!-- 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 fill="context-fill" fill-opacity="context-fill-opacity" xmlns="http://www.w3.org/2000/svg"
> >+     width="32" height="32" viewBox="0 0 32 32">
> >+#include camera-icon.inc.svg
> >+  <use href="#camera-icon"/>
> >+</svg>
> 
> It doesn't make sense anymore to use <defs> and <use> for this. You can just
> import the <path> directly.

Do you prefer the blocked icons also avoiding <defs> and <use>? I don't have those removed since they use the inline clip-path style, but the style could be pulled out into a different file to import and applied to the included icons using a class.

Comment 12

[Dão Gottwald [:dao]]

(In reply to :Perry Jiang from comment #11)
> Do you prefer the blocked icons also avoiding <defs> and <use>? I don't have
> those removed since they use the inline clip-path style, but the style could
> be pulled out into a different file to import and applied to the included
> icons using a class.

What do you think makes more sense? Could also file a followup on cleaning this up further once this has landed.

Comment 13

[Perry Jiang [:perry] [no longer employee, use ni?]]

(In reply to Dão Gottwald [::dao] from comment #12)
> (In reply to :Perry Jiang from comment #11)
> > Do you prefer the blocked icons also avoiding <defs> and <use>? I don't have
> > those removed since they use the inline clip-path style, but the style could
> > be pulled out into a different file to import and applied to the included
> > icons using a class.
> 
> What do you think makes more sense? Could also file a followup on cleaning
> this up further once this has landed.

I think keeping the <defs> and <use> in blocked icons makes more sense and is clearer, so I'll keep it like that for now.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8894670 [details] [diff] [review]
Bug 1371230 - Move notification-icons.svg icons into separate files

>+++ b/browser/themes/shared/notification-icons/default-info.svg
>@@ -0,0 +1,16 @@
>+<!-- 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 fill="context-fill" fill-opacity="context-fill-opacity" xmlns="http://www.w3.org/2000/svg"
>+     width="32" height="32" viewBox="0 0 32 32">
>+  <defs>
>+    <mask id="i-mask" style="fill-opacity: 1;">

nit: fill-opacity="1"

>+++ b/browser/themes/shared/notification-icons/geo-linux-detailed.svg
>@@ -0,0 +1,7 @@
>+<!-- 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 fill="context-fill" fill-opacity="context-fill-opacity" xmlns="http://www.w3.org/2000/svg"
>+     width="32" height="32" viewBox="0 0 32 32">
>+  <path d="m 2,15.9 a 14,14 0 1 1 0,0.2 z m 3,2.1 a 11,11 0 0 0 9,9 l 1,-5 2,0 1,5 a 11,11 0 0 0 9,-9 l -5,-1 0,-2 5,-1 a 11,11 0 0 0 -9,-9 l -1,5 -2,0 -1,-5 a 11,11 0 0 0 -9,9 l 5,1 0,2 z" />
>+</svg>

nit: remove space before /> throughout these SVG files

>+++ b/browser/themes/shared/notification-icons/geo-osx-blocked.svg
>@@ -0,0 +1,12 @@
>+<!-- 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 fill="context-fill" fill-opacity="context-fill-opacity" xmlns="http://www.w3.org/2000/svg"
>+     width="32" height="32" viewBox="0 0 32 32">
>+  <defs>
>+#include clip-path.inc.svg
>+#include geo-osx-icon.inc.svg
>+  </defs>
>+  <use style="clip-path: url(#blocked-clipPath)" href="#geo-osx-icon"/>

nit: use clip-path="url(...)" instead of style="clip-path: url(...)" throughout these SVG files

>+++ b/browser/themes/shared/notification-icons/strikeout.inc.svg
>@@ -0,0 +1,1 @@
>+<path id="strikeout" style="display: block" d="m 2,28 2,2 26,-26 -2,-2 z"/>

nit: remove style="display: block"

>+++ b/browser/themes/shared/notification-icons/update.svg
>@@ -0,0 +1,14 @@
>+<!-- 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 fill="context-fill" fill-opacity="context-fill-opacity" xmlns="http://www.w3.org/2000/svg"
>+     width="32" height="32" viewBox="0 0 32 32">
>+  <style>
>+    #update-icon {
>+      stroke: #fff;
>+      stroke-width: 3px;
>+      stroke-linecap: round;
>+    }
>+  </style>
>+  <path id="update-icon" d="M 16,9 L 16,24 M 16,9 L 11,14 M 16,9 L 21,14" />

nit: move stroke, stroke-width and stroke-linecap as attributes to the path element, and remove the path element's id.

Comment 15

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1371230 notificaiton-icons.svg into separate files

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d0005fc3da
Split notification-icons.svg into separate files. r=dao

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9e26256d212
Backed out changeset e0d0005fc3da for breaking installer packaging on a CLOSED TREE.

Comment 18

[:Felipe Gomes (needinfo for replies!)]

So the packaging step failed because it detected duplicated files (they were are already duplicated icons before but since they were part of the same file this didn't trigger any detection).

The duplicates are:

camera-indicator.svg
camera-sharing.svg
camera.svg

screen-indicator.svg
screen-sharing.svg
screen.svg

microphone-indicator.svg
microphone-sharing.svg
microphone.svg

(from 
https://treeherder.mozilla.org/logviewer.html#?job_id=122353953&repo=mozilla-inbound&lineNumber=30450)

I suggest we just drop the -indicator and -sharing variants and just use camera.svg, screen.svg and microphone.svg

Comment 19

[Dão Gottwald [:dao]]

(In reply to :Felipe Gomes (needinfo me!) from comment #18)
> I suggest we just drop the -indicator and -sharing variants and just use
> camera.svg, screen.svg and microphone.svg

Sounds good.

Comment 20

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1371230 move notificaiton-icons.svg into separate files and remove duplicate icons

Removed duplicate icons (*-indicator.svg and *-sharing.svg).

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0336be288ee9
Split notification-icons.svg into separate files. r=dao

Comment 22

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/32d826b95b7c for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=123101202&repo=mozilla-inbound

Comment 23

[Johann Hofmann [:johannh]]

Attachment: Replace permission icons in preferences and make location icon platform-specific.

Note that one of these failures was caused by bug 1381401 adding some icons under your feet. Sorry!

I've attached a patch for fixing those icons, because we wanted to fix the location icon not being correctly platform-specific anyway.

For the other failures, you should probably ifdef out the platform-specific files in the manifest file.

Thanks!

Comment 24

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1371230 move notificaiton-icons.svg into separate files and remove duplicate icons

Made platform-specific icons platform-specific in jar.inc.mn.

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Let's send the two patches here together to tryserver to make sure there's nothing else missing that would back out this again.

Comment 26

[Perry Jiang [:perry] [no longer employee, use ni?]]

Here's a try submission for both patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c096f51c85124eb904dcd9b2c1ff571e8b659124

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b45e1e1ae216
Split notification-icons.svg into separate files. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/96b18e67de26
Replace permission icons in preferences and make location icon platform-specific. r=Perry

Comment 28

[:Felipe Gomes (needinfo for replies!)]

3rd time is the charm

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b45e1e1ae216
https://hg.mozilla.org/mozilla-central/rev/96b18e67de26