1274480 - Use SVG for permission icons and notifications

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[[e-mail]]

There's lot of discussion of replacing existing png to svg in Bug 1193006

And the change should be done after bug 1147981

Bryan provided svg in Bug 1193006 comment 55, let's discuss the icon related issue and land the sprite here

Comment 1

[[e-mail]]

Thanks for providing icons. 

According to interactive spec https://mozilla.invisionapp.com/share/DM3XGA6YE#/screens/96586951
Some icons has not only allowed/blocked state, but also have active state.

I think we might need some more active state icons, and need UX feedback to make sure if we what show the same icon in both searchbar and control center?


Icon might missed

1. miss 'dot on(i) with red dot' to indicate state change

2. miss red dot on camera/microphone to indicate its in recording

I guess we need them to replace existing orange icons
https://github.com/mozilla/gecko-dev/blob/master/browser/themes/shared/webrtc/webRTC-sharingDevice-16.png


3. the blue popupBlocked icon will be missed when we replace png with svg
https://github.com/mozilla/gecko-dev/blob/master/browser/themes/osx/urlbar-popup-blocked.png

it might be used at searchbar now, present as active state. but svg file does not contain icon in that state.

Comment 2

[[e-mail]]

According to https://mozilla.invisionapp.com/share/DM3XGA6YE#/screens/100296204

We also missed icons with restore flag

Therefore, most icon should have normal/block/restore state, and some icon should have active state

Please help decide proper combinations and update it on spec.

Comment 3

[Ash Grigas]

(In reply to Fred Lin [:gasolin] from comment #1)
> Thanks for providing icons. 
> 
> According to interactive spec
> https://mozilla.invisionapp.com/share/DM3XGA6YE#/screens/96586951
> Some icons has not only allowed/blocked state, but also have active state.
> 
> I think we might need some more active state icons, and need UX feedback to
> make sure if we what show the same icon in both searchbar and control center?
> 
> 
> Icon might missed
> 
> 1. miss 'dot on(i) with red dot' to indicate state change
> 
> 2. miss red dot on camera/microphone to indicate its in recording
> 
> I guess we need them to replace existing orange icons
> https://github.com/mozilla/gecko-dev/blob/master/browser/themes/shared/
> webrtc/webRTC-sharingDevice-16.png
> 
> 
> 3. the blue popupBlocked icon will be missed when we replace png with svg
> https://github.com/mozilla/gecko-dev/blob/master/browser/themes/osx/urlbar-
> popup-blocked.png
> 
> it might be used at searchbar now, present as active state. but svg file
> does not contain icon in that state.

There is no red dot on the i anymore - the sgoogle doc spec was updated to reflect the newest design which is to just show a red version of the permission icon that is active. The svgs provided should work since its just a color change of the icons given.

Comment 4

[:Paolo Amadini]

(In reply to Fred Lin [:gasolin] from comment #2)
> We also missed icons with restore flag

Even though the mockup is not yet updated to reflect that, I think we will not have a specific version of the icon with the "reload" indication superimposed on it, we will just replace the "X" button after the label with the reload icon. This will happen when we implement that part of the design.

I'm looking into what it takes to implement the "in use" state of the icons with the SVG filter solution that was proposed some time ago, with a filter that changes the color. We probably can do that here because these icons are not displayed by default and we probably won't incur in startup performance issues.

Given the above I'll probably take this entire bug, because when the SVG icons and effects are ready and polished there will be little left to do in the CSS files.

Comment 5

[Ash Grigas]

(In reply to :Paolo Amadini from comment #4)
> (In reply to Fred Lin [:gasolin] from comment #2)
> > We also missed icons with restore flag
> 
> Even though the mockup is not yet updated to reflect that, I think we will
> not have a specific version of the icon with the "reload" indication
> superimposed on it, we will just replace the "X" button after the label with
> the reload icon. This will happen when we implement that part of the design.
> 
> I'm looking into what it takes to implement the "in use" state of the icons
> with the SVG filter solution that was proposed some time ago, with a filter
> that changes the color. We probably can do that here because these icons are
> not displayed by default and we probably won't incur in startup performance
> issues.
> 
> Given the above I'll probably take this entire bug, because when the SVG
> icons and effects are ready and polished there will be little left to do in
> the CSS files.

yes that is correct - the mockup is out of date - the only reload icon should be on the right side of the panel. The left icons should stay the same.

Comment 6

[:Paolo Amadini]

I did quite a lot of tests these days starting from the files and demos that Brian and Stephen shared, and by now I think I've made up my mind on a solution to use here. This bug, differently from bug 1054016, is specifically for icons that are not used in any area that is displayed by default on startup.

Anything that lands here can always be changed later if we find a better solution to align with what we do for the toolbar in bug 1054016.

Comment 7

[:Paolo Amadini]

I'm trying to find a solution that allows us to import a new glyph in the tree and use it with as little boilerplate code as possible. Something for which we can take an SVG <path>, put it somewhere in the tree, and we're mostly done.

I'm thinking of icons with a square aspect ratio, since this is what we'll use here.

Since <path> elements specify drawing instructions using coordinates that must be interpreted depending on the SVG canvas size they were designed for, the first choice to make is the natural canvas size for the <path> elements we want to import. This way we can copy the "d" attribute without converting the coordinate system. Everything can be changed using the "transform" attribute, but if we can avoid it, it creates more compact and unambiguous code, especially if the path is referenced elsewhere with a <use> attribute.

For these permission icons, I went for 32px as the native size.

This is convenient because in my experience, everything can be easily scaled with CSS when you use the "background-image" and "list-style-image", and is rendered at the target size. There is one excpetion I've found though, and they are menu icons, as I noticed in bug 1191442 comment 15. I've tested recently both with "list-style-image" and "image" attribute on the <menuitem> using the patch in bug 1275432. Using 32px there allows the menu image to display well on HiDPI, and most glyphs will also render well when downscaled to 16px after being rendered at 32px.

This is also the natural rendering size for 16px icons on a HiDPI screen, and I guess it will be the main rendering size for most icons in that case. So, this probably also makes sense in general when designing and testing the toolbar icons for HiDPI in a vector graphics program like Inkscape or Illustrator. The designers can surely provide feedback on this.

Comment 8

[:Paolo Amadini]

For the container where the <path> elements are located, for now I went for a single SVG file that can be addressed using a "glyphs.svg#name" syntax.

There are no styles specified in it except for the one used to show only the addressed <path> element.

This allows other SVG files to reference the paths with an external <use> element, and apply their own styles very easily. There are no translations applied to the original paths so there is no doubt on how they apply across <use> elements.

The result is that, when this file is referenced directly using its URL and hash part, it is rendered as a black shape on a transparent black background. When displayed in the browser, this results in a black image on a white background, at the size of 32px, so we can easily see what the image is if we wonder.

To render the icons in XUL using a different color than black, we can keep the alpha channel and reset the RGB channels to a well-defined different value using a color transform filter.

The filter can be applied with the "filter" property in CSS, that can be specified in a different rule from the one that selects the glyph using its URL. This allows us to apply some of the hover effects we need easily across different icons. There might be the option of using "clip-path" and "mask" but I've not looked into them.

More complex arrangements without filters can be made by referencing the paths from other SVG files and applying color using styles, so this gives us flexibility.

Comment 9

[[e-mail]]

I think it may make things inevitably more complex when we try to do svg color filters in CSS, while designer can provide all icon with states in svg sprite and all we need is refer it by `file.svg#id`.


Cons:

* More icons means need load a bigger sprite at first time

Pros:

* UX and Designer can help themselves when they start design some new look and feel. 
They just need to drop a new sprite image when they prefer a red line across the Block icon, or show `i` icon with red dot instead of a mono color block icon 

* Developer can save lots of potential error in CSS
It will save lots of time back and forth to confirm with designer if all colors and positions are correct when there's any icon change in the future (There will be).

* Avoid potential performance issue
Since svg is considered as a not evolved feature, I bet CSS filter with svg is not well tested in product yet


My point is it will save both designer and developer's time in short and long run if we just define all icon with states in sprite and refer them with id.

Comment 10

[:Paolo Amadini]

Thanks for the feedback!

(In reply to Fred Lin [:gasolin] from comment #9)
> designer can provide all icon with states in svg
> sprite and all we need is refer it by `file.svg#id`.

Although this looks ideal at first glance, in my experience the process of importing SVG into the Firefox tree is never that easy. We often need to format and review the SVG code manually to ensure it matches our coding and quality standards, even when it otherwise renders well on the provided file.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines

> * UX and Designer can help themselves when they start design some new look
> and feel. 
> They just need to drop a new sprite image when they prefer a red line across
> the Block icon, or show `i` icon with red dot instead of a mono color block
> icon

More complex icons with color in them, stored in their specific files, are always possible and will not go away. In fact, you have a good point in that the solution I'm using here is only specific to the icons we need to render in one color, for the permission notifications in particular.

The example you made about making the line red for blocked permissions, in other words replacing a monochrome icon with one with multiple colors, should be treated as any other redesign. We always have to think about the specific places where the icons are used. It's rarely just a matter of changing the SVG file, because for example other places where the same icon appears might want to keep the monochrome look, or other themes may need a different color.

Sometimes we might be able to compose monochrome glyphs with other colored items with a <use> element in a different SVG file, reducing code duplication.

> * Developer can save lots of potential error in CSS
> It will save lots of time back and forth to confirm with designer if all
> colors and positions are correct when there's any icon change in the future
> (There will be).

Can you elaborate on which errors you are thinking about? Most CSS and design changes have to be reviewed with screenshots, and this applies equally regardless of the solution used to build the SVG file.

> * Avoid potential performance issue
> Since svg is considered as a not evolved feature, I bet CSS filter with svg
> is not well tested in product yet

This is being investigated in bug 1054016, and as I mentioned we might find a more efficient solution there. The reason to use "filter" here for the color change is that it is a separate CSS property from the one defining the shape of the icon, so they can be combined without having to write a different URL and rule for each combination. We're also not limited to the colors originally specified in the SVG file.

One good example is attachment 8743790 [details] [diff] [review], solved with one line of CSS instead of having to ask designers or create ourselves a new grayscale version of all the different icons that we could have in the identity block, each with its own separate file to update. 

We may be able to do the same with "clip-path", "mask", or even better the new techniques in bug 1058040. It will be easy to change one solution for the other once we know which one is better.

Comment 11

[[e-mail]]

I'll support your decision since the alternative way has been considered.

If we stick with mono color icons, it's also no obvious risk with basic css filter.

Comment 12

[:Paolo Amadini]

Attachment: Bug 1274480 - Part 1 - Use SVG for permission icons and notifications.

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

Comment 13

[:Paolo Amadini]

Comment on attachment 8758294 [details]
Bug 1274480 - Part 1 - Use SVG for permission icons and notifications.

This is a proof of concept replacing two permission icons, obviously we may want to unify the hover behavior among the icons but this case illustrates the versatility of the filter that can take the fill color from the context.

The glyphs are slightly different from the previous ones in the tree, since part of this bug is replacing the icon appearance. The paths in the patch are already cleaned up. Adding new shapes requires just a single line in "glyphs.svg".

As mentioned, this glyph technique is used for icons that are not performance sensitive, until we can develop an equivalent and efficient method.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8758294 [details]
Bug 1274480 - Part 1 - Use SVG for permission icons and notifications.

If we're happy this code is not perf-sensitive then this approach looks OK to me.

Comment 15

[:Paolo Amadini]

The next glyph I'm adding is the one for geolocation.

This is the only permission glyph that is different across platforms, and I see three main options to deal with this:
1. Include a different "platform-glyphs.svg" file in each of the theme folders.
2. Preprocess the "glyphs.svg" file to include different shapes.
3. Include all the glyphs in the SVG file and preprocess the CSS.

The first two options create a more compact package by excluding shapes we don't need, but in the case of the first option this is at the expense of more boilerplate in the source tree and one more file to load. Given that we're moving to a model where differences in style and icons between platforms are kept to a minimum, I think the saving in package size is not worth the maintenance burden. And we save a lot more space by using SVG instead of PNG in the first place.

The second option is the most compact but it prevents the SVG file itself from being opened in an editor.

The third option is already in use inside "notification-icons.inc.css" for system icons, and it makes sense to me that we can look for the preprocessing in a single place. This is the approach I'm following.

Comment 16

[:Paolo Amadini]

Comment on attachment 8758294 [details]
Bug 1274480 - Part 1 - Use SVG for permission icons and notifications.

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

Comment 17

[:Paolo Amadini]

Comment on attachment 8758294 [details]
Bug 1274480 - Part 1 - Use SVG for permission icons and notifications.

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

Comment 18

[:Paolo Amadini]

./mach try -b o -p linux,linux64,macosx64,win32,win64 -u mochitest-browser-screenshots["Ubuntu","10.10","Windows XP","Windows 7","Windows 8"] -t none --setenv MOZSCREENSHOTS_SETS=PermissionPrompts

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41a19cb838b2
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e94e85e966b3

./mach try -b d -u all -p all

https://treeherder.mozilla.org/#/jobs?repo=try&revision=287676de4629

Comment 19

[:Paolo Amadini]

I'm removing the PNG icons for the key from the Toolkit desktop themes because there doesn't seem to be another Tier 1 consumer besides Firefox, and we're replacing the icon with the SVG version in the front-end theme.

The only reference I've found in Toolkit code appears to be for handling the logins notification bar:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#733

I've updated it so we show the default icon instead.

Matt, do you confirm that we don't ever show the notification bar in Firefox?

Comment 20

[:Paolo Amadini]

Comment on attachment 8758294 [details]
Bug 1274480 - Part 1 - Use SVG for permission icons and notifications.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56584/diff/3-4/

Comment 21

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #19)
> Matt, do you confirm that we don't ever show the notification bar in Firefox?

Not at the moment but bug 1189618 may add a consumer of the key icon and that bug is on the radar of ones to finish.

I'm not sure why we wouldn't put the SVG image in toolkit? It seems like a small optimization at first glance.

Comment 22

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #21)
> Not at the moment but bug 1189618 may add a consumer of the key icon and
> that bug is on the radar of ones to finish.
> 
> I'm not sure why we wouldn't put the SVG image in toolkit? It seems like a
> small optimization at first glance.

It can be a good idea to try and reuse the SVG icon as part bug 1189618 if we can, but we might have other limitations of size or format. I would leave the work to figure out if we can use the same SVG based glyph system in the autocomplete popup to that bug. We might even end up with an icon provided by the consumer rather than hard-coded in Toolkit.

Comment 23

[:Gijs (he/him)]

Comment on attachment 8758294 [details]
Bug 1274480 - Part 1 - Use SVG for permission icons and notifications.

https://reviewboard.mozilla.org/r/56584/#review54784

I haven't reviewed the images themselves. This generally looks OK but I would really prefer a diff that doesn't move quite so much of the code around. Also unsure about some of the offline-app stuff. Did you test that? I think it might be a little tricky to get those particular notifications to appear without messing around with prefs.

::: browser/themes/shared/notification-icons.inc.css
(Diff revision 4)
> -.popup-notification-icon {
> -  width: 64px;
> -  height: 64px;
> -  margin-inline-end: 10px;
> -}

Just a note that the rearranging you did in this file made the diff significantly harder to review. It's kind of odd because the raw diff shows bits as kept the same that are moved in mozreview, and vice versa. Either way, some bits are having blame adjusted unnecessarily. Is there a particular reason to do it? It seems the result tries to keep icons per-subject rather than keeping the notification icons in the URL bar separate from the icons in the panel, or something, but even that isn't followed consistently. So I'm not sure what we're moving stuff around for, in the end.

::: browser/themes/shared/notification-icons.inc.css
(Diff revision 4)
> -.popup-notification-icon[popupid="indexedDB-permissions-prompt"],
> -.popup-notification-icon[popupid*="offline-app-requested"],
> -.popup-notification-icon[popupid="offline-app-usage"] {
> -  list-style-image: url(chrome://global/skin/icons/question-64.png);

The bottom two now still have question-64.png listed, but the top one has its own image in the SVG. Why the difference?

::: browser/themes/shared/notification-icons.inc.css:46
(Diff revision 4)
> +  width: 16px;
> +  height: 16px;
> +  margin: 0 2px;

Why rearrange this within this selector?

::: browser/themes/shared/notification-icons.inc.css:76
(Diff revision 4)
> +.webRTC-sharingDevices-notification-icon,
> +.webRTC-sharingMicrophone-notification-icon,
> +.camera-icon,
> +.geo-icon,
> +.indexedDB-icon,
> +.login-icon,
> +.microphone-icon,
> +.pointer-icon,
> +.screen-icon,
> +.web-notifications-icon,
> +.popup-notification-icon[popupid="geolocation"],
> +.popup-notification-icon[popupid="indexedDB-permissions-prompt"],
> +.popup-notification-icon[popupid="password"],
> +.popup-notification-icon[popupid="pointerLock"],
> +.popup-notification-icon[popupid="webRTC-shareDevices"],
> +.popup-notification-icon[popupid="webRTC-shareMicrophone"],
> +.popup-notification-icon[popupid="webRTC-shareScreen"],
> +.popup-notification-icon[popupid="webRTC-sharingDevices"],
> +.popup-notification-icon[popupid="webRTC-sharingMicrophone"],
> +.popup-notification-icon[popupid="webRTC-sharingScreen"],
> +.popup-notification-icon[popupid="web-notifications"] {

Can you file a followup to use a class for this? This selector is a bit ridiculous.

Comment 24

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #23)
> would really prefer a diff that doesn't move quite so much of the code

My fault - as I worked on the patch I moved code to cluster rules by icon type. Later I realized that it made sense to have structural rules at the beginning and group rules for specific notifications together, with "icon using glyphs" as their own category. Ideally most of the individual sections would then go away as they are moved to the glyphs section.

I've reverse-engineered the moves and I'll post a new version that sorts the rules in a separate changeset.

> Also unsure about some of the offline-app stuff. Did you test that?

This rule didn't change, will be clearer in the new diff. Therefore, I didn't test it specifically.

> ::: browser/themes/shared/notification-icons.inc.css
> (Diff revision 4)
> > -.popup-notification-icon[popupid="indexedDB-permissions-prompt"],
> > -.popup-notification-icon[popupid*="offline-app-requested"],
> > -.popup-notification-icon[popupid="offline-app-usage"] {
> > -  list-style-image: url(chrome://global/skin/icons/question-64.png);
> 
> The bottom two now still have question-64.png listed, but the top one has
> its own image in the SVG. Why the difference?

We now have an icon for offline storage, we didn't have a specific one before.

> ::: browser/themes/shared/notification-icons.inc.css:46
> (Diff revision 4)
> > +  width: 16px;
> > +  height: 16px;
> > +  margin: 0 2px;
> 
> Why rearrange this within this selector?

To move the image rule closer to the HiDPI version. (Not that I mind reverting the change if you prefer. This is a case where I could easily accept "preserve blame" as a reason not to move code around.)

> ::: browser/themes/shared/notification-icons.inc.css:76
> (Diff revision 4)
> > +.webRTC-sharingDevices-notification-icon,
> > +.webRTC-sharingMicrophone-notification-icon,
> > +.camera-icon,
> > +.geo-icon,
> > +.indexedDB-icon,
> > +.login-icon,
> > +.microphone-icon,
> > +.pointer-icon,
> > +.screen-icon,
> > +.web-notifications-icon,
> > +.popup-notification-icon[popupid="geolocation"],
> > +.popup-notification-icon[popupid="indexedDB-permissions-prompt"],
> > +.popup-notification-icon[popupid="password"],
> > +.popup-notification-icon[popupid="pointerLock"],
> > +.popup-notification-icon[popupid="webRTC-shareDevices"],
> > +.popup-notification-icon[popupid="webRTC-shareMicrophone"],
> > +.popup-notification-icon[popupid="webRTC-shareScreen"],
> > +.popup-notification-icon[popupid="webRTC-sharingDevices"],
> > +.popup-notification-icon[popupid="webRTC-sharingMicrophone"],
> > +.popup-notification-icon[popupid="webRTC-sharingScreen"],
> > +.popup-notification-icon[popupid="web-notifications"] {
> 
> Can you file a followup to use a class for this? This selector is a bit
> ridiculous.

Bug 1278561, it will get rid of all ".popup-notification-icon" rules. And this selector will no longer be necessary once we have enough usage to make the filter (or whatever technique replaces it) the filter opt-out.

Comment 25

[:Paolo Amadini]

Comment on attachment 8758294 [details]
Bug 1274480 - Part 1 - Use SVG for permission icons and notifications.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56584/diff/4-5/

Comment 26

[:Paolo Amadini]

Attachment: Bug 1274480 - Part 2 - Sort notification-icons.inc.css by notification type.

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

Comment 27

[:Paolo Amadini]

Attachment: Bug 1274480 - Part 1 - Use SVG for permission icons and notifications.

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

Comment 28

[:Paolo Amadini]

https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=41a19cb838b2b46d316cb8bac630309449321282&newProject=try&newRev=e94e85e966b3fec43b4ed93c4aa3656251bf4223

Comment 29

[:Gijs (he/him)]

Comment on attachment 8760757 [details]
Bug 1274480 - Part 1 - Use SVG for permission icons and notifications.

https://reviewboard.mozilla.org/r/58220/#review55312

With the fixes below, r=me.

As a courtesy, could you either file a followup bug to move this icon into toolkit or a bug against seamonkey/thunderbird to add the `key*.png` icons to their own bits of comm-central, as they use the ones you're removing.

::: browser/themes/shared/notification-icons.inc.css
(Diff revision 1)
> -  .translation-icon {
> -    list-style-image: url(chrome://browser/skin/[e-mail]);
> -    -moz-image-region: rect(0px, 32px, 32px, 0px);
> -  }
> -
> -  .translation-icon.in-use {
> -    -moz-image-region: rect(0px, 64px, 32px, 32px);
> -  }

These should stay, I think? You're not replacing this with SVG, nor are you removing the relevant png files.

Comment 30

[:Gijs (he/him)]

Comment on attachment 8760756 [details]
Bug 1274480 - Part 2 - Sort notification-icons.inc.css by notification type.

https://reviewboard.mozilla.org/r/58218/#review55316

::: browser/themes/shared/notification-icons.inc.css:109
(Diff revision 1)
> +/* The first selector is used by socialchat.xml (bug 1275558). */
> +.webRTC-sharingDevices-notification-icon,
> +.camera-icon,
> +.popup-notification-icon[popupid="webRTC-shareDevices"],
> +.popup-notification-icon[popupid="webRTC-sharingDevices"] {
> +  list-style-image: url(chrome://browser/skin/glyphs.svg#camera);
> +}

Maybe put this together with the other webrtc items?

::: browser/themes/shared/notification-icons.inc.css:169
(Diff revision 1)
> -.plugin-icon.plugin-blocked {
> -  list-style-image: url(chrome://browser/skin/notification-pluginBlocked.png);
> +.popup-notification-icon[popupid="pointerLock"],
> +.pointer-icon {
> +  list-style-image: url(chrome://browser/skin/glyphs.svg#pointer);
>  }
>  

Nit: move the pointerlock one out from between the webrtc ones?

::: browser/themes/shared/notification-icons.inc.css:190
(Diff revision 1)
> -     when showing the notification repeatedly. */
> -  display: -moz-box;
> -  visibility: collapse;
> +.popup-notification-icon[popupid="drmContentPlaying"],
> +.drm-icon {
> +  list-style-image: url("chrome://browser/skin/drm-icon.svg#chains");
>  }
>  
> -#plugins-notification-icon.plugin-blocked[showing] {
> -  animation: pluginBlockedNotification 500ms ease 0s 5 alternate both;
> +.drm-icon:hover:active {
> +  list-style-image: url("chrome://browser/skin/drm-icon.svg#chains-pressed");

Can you file a followup to incorporate this into the SVG you're creating here (make sure to also update the reference in browser-media.js when we do that).

Comment 31

[:Paolo Amadini]

Comment on attachment 8760757 [details]
Bug 1274480 - Part 1 - Use SVG for permission icons and notifications.

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

Comment 32

[:Paolo Amadini]

Comment on attachment 8760756 [details]
Bug 1274480 - Part 2 - Sort notification-icons.inc.css by notification type.

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

Comment 33

[:Paolo Amadini]

./mach try -b o -p linux,linux64,macosx64,win32,win64 -u mochitest-browser-screenshots["Ubuntu","10.10","Windows XP","Windows 7","Windows 8"] -t none --setenv MOZSCREENSHOTS_SETS=PermissionPrompts

https://treeherder.mozilla.org/#/jobs?repo=try&revision=935ab5230c28
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f47dbc7b868e

Comment 34

[:Paolo Amadini]

Following up to the discussions with Bryan and the rest of the team, I removed the detailed version of the screen icon because we can use the other one equally.

I also went ahead and removed the detailed version of the indexedDB icon because we know that this permission is used by a very small percentage of users, and the non-detailed designed renders quite well at a higher resolution, only the thickness of the line is slightly different.

Comment 35

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #30)
> > -#plugins-notification-icon.plugin-blocked[showing] {
> > -  animation: pluginBlockedNotification 500ms ease 0s 5 alternate both;
> > +.drm-icon:hover:active {
> > +  list-style-image: url("chrome://browser/skin/drm-icon.svg#chains-pressed");
> 
> Can you file a followup to incorporate this into the SVG you're creating
> here (make sure to also update the reference in browser-media.js when we do
> that).

I added a comment to bug 1267630, that is already on file to track all the work still missing for the DRM notification. I've also added similar comments to bugs for other notifications that haven't been fully converted to glyph format here.

Comment 36

[:Paolo Amadini]

https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=935ab5230c28835779aa7adbce42b987077224fb&newProject=try&newRev=f47dbc7b868e48f4b2632b9f435c59a9584454a3

This link doesn't show any results at present. Since the comparison worked before with pushes using the same try syntax, it might easily be a bug triggered by the particular order of the pushes in the repository. I'd expect the tool to always work the same when I provide the revision numbers I used for the reference and the final try builds. Matt, can you take a look?

Comment 37

[:Paolo Amadini]

I have tested most of the notifications again locally. The geolocation notification is the only one that differs between platforms, and the associated code and graphics hasn't changed since the comparison in comment 28.

Since the old comparison and tests look good, and we need to land this bug soon to unblock other work, I'll land it now on the integration branch even though we don't have results from the latest screenshot comparison yet. If we have any regressions from the latest changes, we can easily fix in follow-ups.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/a1558d048e76
Part 1 - Use SVG for permission icons and notifications. r=Gijs
https://hg.mozilla.org/integration/fx-team/rev/d463c1a271c4
Part 2 - Sort notification-icons.inc.css by notification type. r=Gijs

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/fae4bb6ae895
followup: use image that exists on android, rs=bustage

Comment 40

[:Paolo Amadini]

(In reply to Pulsebot from comment #39)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/fx-team/rev/fae4bb6ae895
> followup: use image that exists on android, rs=bustage

Thanks for that - I totally missed the failure in the noise of intermittents, although it was already present in the build I made for the purpose:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=287676de4629&selectedJob=21943861

I look forward to auto-classification working better for tryserver builds...

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a1558d048e76
https://hg.mozilla.org/mozilla-central/rev/d463c1a271c4
https://hg.mozilla.org/mozilla-central/rev/fae4bb6ae895

Comment 42

[Paul Silaghi, QA [:pauly]]

Should there be seen any visual changes, now that we use SVG for notifications?

Comment 43

[:Paolo Amadini]

(In reply to Paul Silaghi, QA [:pauly] from comment #42)
> Should there be seen any visual changes, now that we use SVG for
> notifications?

Yes, we changed the appearance of most popup notifications that didn't have an SVG icon before. This link shows some of the expected changes, although some notifications may not be covered:

https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=935ab5230c28835779aa7adbce42b987077224fb&newProject=try&newRev=f47dbc7b868e48f4b2632b9f435c59a9584454a3

Comment 44

[Paul Silaghi, QA [:pauly]]

The new SVG icons look ok, as in the screenshots.
Verified fixed on FX 50.0a1 (2016-06-20) Win 7, Ubuntu 14.04, OS X 10.11.