Closed Bug 1371230 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Theme, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: johannh, Assigned: perry)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

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)
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 />...
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
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)
(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)
(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.
(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+
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d0005fc3da
Split notification-icons.svg into separate files. r=dao
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.
Removed duplicate icons (*-indicator.svg and *-sharing.svg).
Attachment #8895927 - Attachment is obsolete: true
Attachment #8896405 - Flags: review+
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0336be288ee9
Split notification-icons.svg into separate files. r=dao
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)
Made platform-specific icons platform-specific in jar.inc.mn.
Attachment #8896405 - Attachment is obsolete: true
Flags: needinfo?(jiangperry)
Attachment #8897474 - Flags: review+
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.
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
https://hg.mozilla.org/mozilla-central/rev/b45e1e1ae216
https://hg.mozilla.org/mozilla-central/rev/96b18e67de26
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.