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

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: johannh, Assigned: perry)

Tracking

(Blocks 1 bug)

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

2 years ago
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
Assignee: nobody → jiangperry
Status: NEW → ASSIGNED
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?
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 2

2 years ago
I chose to move elements used multiple times into their own .inc.svg files. Let me know if something else is preferred.
Attachment #8890981 - Flags: review?(dao+bmo)
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/.
Flags: needinfo?(dao+bmo)
Attachment #8890981 - Flags: review?(dao+bmo)
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 />...
(Assignee)

Comment 5

2 years ago
Moved icons from chrome://browser/skin/ to chrome://browser/skin/notification-icons, removed unused IDs, and removed spaces before />.
Attachment #8890981 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
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 />.
Attachment #8892228 - Flags: review?(dao+bmo)
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.
Attachment #8892228 - Flags: review?(dao+bmo)
(Assignee)

Comment 8

2 years ago
(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?
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 10

2 years ago
Attachment #8892228 - Attachment is obsolete: true
Attachment #8894670 - Flags: review?(dao+bmo)
(Assignee)

Comment 11

2 years ago
(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.
(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.
(Assignee)

Comment 13

2 years ago
(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 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.
Attachment #8894670 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 15

2 years ago
Attachment #8894670 - Attachment is obsolete: true
Attachment #8895927 - Flags: review+

Comment 16

2 years ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d0005fc3da
Split notification-icons.svg into separate files. r=dao

Comment 17

2 years ago
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9e26256d212
Backed out changeset e0d0005fc3da for breaking installer packaging on a CLOSED TREE.
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
(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.
(Assignee)

Comment 20

2 years ago
Removed duplicate icons (*-indicator.svg and *-sharing.svg).
Attachment #8895927 - Attachment is obsolete: true
Attachment #8896405 - Flags: review+

Comment 21

2 years ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0336be288ee9
Split notification-icons.svg into separate files. r=dao
(Reporter)

Comment 23

2 years ago
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!
Attachment #8897167 - Flags: review?(jiangperry)
(Assignee)

Comment 24

2 years ago
Made platform-specific icons platform-specific in jar.inc.mn.
Attachment #8896405 - Attachment is obsolete: true
Flags: needinfo?(jiangperry)
Attachment #8897474 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8897167 - Flags: review?(jiangperry) → review+
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 27

2 years ago
Pushed by felipc@gmail.com:
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
3rd time is the charm

Comment 29

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b45e1e1ae216
https://hg.mozilla.org/mozilla-central/rev/96b18e67de26
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Reporter)

Updated

2 years ago
Depends on: 1391550
You need to log in before you can comment on or make changes to this bug.