1193006 - Show icons next to non-default permissions in the Permissions section of the Control Center

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Brian Grinstead [:bgrins]]

Attachment: proposed-ui-icon.png

Right now the Permissions section in the Control Center just shows the name of the permission.  It should also include the icon - see the attached design.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: current-ui-icon.png

Current design - notice that there isn't an icon

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1193006 - Show icons next to permissions in main view;r=paolo

Bug 1193006 - Show icons next to permissions in main view;r=paolo

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1204016 - Move permissions list directly underneath the section icon;r=ttaubert

Bug 1204016 - Move permissions list directly underneath the section icon;r=ttaubert

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: with-all-permissions-set.png

Screenshot comparing current UI with what it looks like with these patches applied (and all permissions set).

Ash, what do you think about landing this as an intermediate state before we land the entire new permissions UI in the control center?

Comment 5

[Ash Grigas]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Created attachment 8659991 [details]
> with-all-permissions-set.png
> 
> Screenshot comparing current UI with what it looks like with these patches
> applied (and all permissions set).
> 
> Ash, what do you think about landing this as an intermediate state before we
> land the entire new permissions UI in the control center?

If we hid 'set cookies', 'install add-ons' and 'load images' permissions, I would be ok releasing this just in Nightly. We will never show 'install add-ons' as a permission in the control panel. And cookies we said is to destructive to allow the 'block' setting here. Load images shouldn't be there since it has little utility.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to agrigas from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > Created attachment 8659991 [details]
> > with-all-permissions-set.png
> > 
> > Screenshot comparing current UI with what it looks like with these patches
> > applied (and all permissions set).
> > 
> > Ash, what do you think about landing this as an intermediate state before we
> > land the entire new permissions UI in the control center?
> 
> If we hid 'set cookies', 'install add-ons' and 'load images' permissions, I
> would be ok releasing this just in Nightly. We will never show 'install
> add-ons' as a permission in the control panel. And cookies we said is to
> destructive to allow the 'block' setting here. Load images shouldn't be
> there since it has little utility.

Yeah those wouldn't show up in any situation other than going into Page Info dialog and flipping them all (which is what I did to take the screenshot).

Comment 7

[Ash Grigas]

Ok - eventually we'll want to remove them from the page info dialog as well so user's don't get into weird states and so we don't have have placeholder icons for load images and add-ons.

Comment 8

[:Paolo Amadini]

Comment on attachment 8659984 [details]
MozReview Request: Bug 1193006 - Show icons next to permissions in main view;r=paolo

I think we should try and use the new SVG icons in this bug, and also try to decouple this logic from the "notification-anchor" concept, as we discussed in the meeting at a high level yesterday.

I think there's no problem if we wait to land this, as we're scheduling this redesign for Firefox 43 anyways when we'll also have the front-end code module.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8659985 [details]
MozReview Request: Bug 1204016 - Move permissions list directly underneath the section icon;r=ttaubert

https://reviewboard.mozilla.org/r/19043/#review17393

::: browser/themes/shared/controlcenter/panel.inc.css:295
(Diff revision 1)
> +.identity-popup-permission-headline {

Hmmm, that should be an #ID. Is that rule not needed or did you just miss the visual effect?

Comment 10

[:Gijs (out for now; he/him)]

Did this get lost? Seems to have unfinished patches...

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #10)
> Did this get lost? Seems to have unfinished patches...

IIRC the design for this UI changed while I was working on it so we dropped it.  Ash, with the current designs for the permissions UI in the main view are we still going to need this bug, or should it be closed out as wontfix?  If this is the latest design then it looks like maybe we will still need it: https://mozilla.invisionapp.com/share/DM3XGA6YE#/screens/100404627

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

The design changes that we were discussing back then were about the drop-down box appearance and behavior IIRC. There has been some additional discussion recently about alternative approaches to the entire permission section, but these are not fully baked yet nor have we reached consensus. I'd say the main issue here is addressing review feedback first.

Comment 13

[[e-mail]]

Brain, are you still interested in finish this issue? 
Or may I help to make some progress here?

Comment 14

[Brian Grinstead [:bgrins]]

Hi Fred, please feel free to take this over

Comment 15

[[e-mail]]

Brain, could you point me where to find the sample page or the instructions to show the similar screen at comment 4?

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Fred Lin [:gasolin] from comment #15)
> Brain, could you point me where to find the sample page or the instructions
> to show the similar screen at comment 4?

If you open any URL then open the Page Info box (cmd/ctrl+i) and go to the permissions tab you can give all of these permissions non-default values which will cause them to show up in the popup.

Also, here's a test page for getUserMedia: https://mozilla.github.io/webrtc-landing/gum_test.html

Comment 17

[[e-mail]]

Thanks Brain, Here are the steps to show these permissions:

1. Go to https://mozilla.github.io/webrtc-landing/gum_test.html
2. tap i icon at left of the search bar
3. tap `>` icon at right of `mozilla.github.io`, then tap `more information`
4. switch to Permissions tab
5. tweak all permissions to non-default values
6. then tap i icon at left of the search bar again, you can see the screen in comment 4


Aislinn, if 'set cookies', 'install add-ons' and 'load images' permissions should not be shown in permissions doorhanger, should we hiding those options in the Permissions tab of Page Info panel? Or provide new images for them to show on doorhanger

Comment 18

[[e-mail]]

BTW, According to reviewer's request in comment 8 , please help provide svg icons to replace the existing icons.

Comment 19

[Ash Grigas]

(In reply to Fred Lin [:gasolin] from comment #17)
> Thanks Brain, Here are the steps to show these permissions:
> 
> 1. Go to https://mozilla.github.io/webrtc-landing/gum_test.html
> 2. tap i icon at left of the search bar
> 3. tap `>` icon at right of `mozilla.github.io`, then tap `more information`
> 4. switch to Permissions tab
> 5. tweak all permissions to non-default values
> 6. then tap i icon at left of the search bar again, you can see the screen
> in comment 4
> 
> 
> Aislinn, if 'set cookies', 'install add-ons' and 'load images' permissions
> should not be shown in permissions doorhanger, should we hiding those
> options in the Permissions tab of Page Info panel? Or provide new images for
> them to show on doorhanger

They should remain only in the Page info panel and not on the permissions dropdown of the control center.

Comment 20

[Ash Grigas]

can you provide a link the the svg icons for the permissions in the control panel

Comment 21

[[e-mail]]

Thanks for clarification. Created related bug 1271868 to hide 'set cookies', 'install add-ons' and 'load images' permissions in the Permissions section of the Control Center

Comment 22

[[e-mail]]

Checking specs, I guess svg icons are already available at https://www.dropbox.com/sh/uqzkxu8ku3hk3lj/AAClWxkyw4i73bzTRHkPbo2na/SVG/Output?dl=0 ?

Comment 23

[[e-mail]]

Attachment: lack icon

Checked icons provided in dropbox, some images is still lacked:

1. Offline storage icon
2. open popup windows icon
3. receive notifications icon
4. webRTC shareDevice icon (It's not shown in permission tab yet)

Comment 24

[[e-mail]]

Paolo, I've made a WIP and would like to solve this issue. Could you help elaborate what does "try to decouple this logic from the notification-anchor concept" mean in comment 8?

Comment 25

[[e-mail]]

Default Icons such as geolocation use OS specific icon (the screenshot in Comment 23 shows blue geolocation icon in linux) with more richer details.

https://github.com/mozilla/gecko-dev/blob/master/browser/themes/linux/Geolocation-16.png
https://github.com/mozilla/gecko-dev/blob/master/browser/themes/osx/Geolocation-16.png
https://github.com/mozilla/gecko-dev/blob/master/browser/themes/windows/Geolocation-16.png

Does UX prefer to replace geolocation icons to those icons in dropbox?

https://www.dropbox.com/sh/uqzkxu8ku3hk3lj/AADOue8MP365nNf2y1K4PSHGa/SVG/Output/glyph-location-16-mac.svg?dl=0
https://www.dropbox.com/sh/uqzkxu8ku3hk3lj/AADPDoBPS8YQLtcgfByoW_fia/SVG/Output/glyph-location-16-win.svg?dl=0

Comment 26

[:Paolo Amadini]

(In reply to Fred Lin [:gasolin] from comment #24)
> Paolo, I've made a WIP and would like to solve this issue. Could you help
> elaborate what does "try to decouple this logic from the notification-anchor
> concept" mean in comment 8?

If I remember correctly, it means to avoid the "notification-anchor-icon" class as these icons are not notification anchors, but still use an additional class that describes the icon type that would be shared with the anchor icons, for example "geo-icon". So, the elements added to the control center would have a classes like "identity-popup-permission-icon geo-icon", "identity-popup-permission-icon camera-icon", and so on.

The default class could be derived from the permission name in SitePermissions.jsm plus "-icon" at the end.

Comment 27

[Ash Grigas]

Checking with Bryan Bell

Comment 28

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #26)
> The default class could be derived from the permission name in
> SitePermissions.jsm plus "-icon" at the end.

See bgrins' patch which adds a helper to get the class. Please use the existing identity block classes which just provide the list-style-image as I assume we're going to use the new icon in both places.

Comment 29

[:Paolo Amadini]

I suggested using a default class derived from the permission name so it doesn't have to be declared for all permissions, only for those that have a different class.

Did you see bug 1147981 comment 23 for how I think the shorter class names like "geo-icon" would work?

Comment 30

[bbell]

(In reply to Fred Lin [:gasolin] from comment #25)
> Default Icons such as geolocation use OS specific icon (the screenshot in
> Comment 23 shows blue geolocation icon in linux) with more richer details.
> 
> https://github.com/mozilla/gecko-dev/blob/master/browser/themes/linux/
> Geolocation-16.png
> https://github.com/mozilla/gecko-dev/blob/master/browser/themes/osx/
> Geolocation-16.png
> https://github.com/mozilla/gecko-dev/blob/master/browser/themes/windows/
> Geolocation-16.png
> 
> Does UX prefer to replace geolocation icons to those icons in dropbox?
> 
> https://www.dropbox.com/sh/uqzkxu8ku3hk3lj/AADOue8MP365nNf2y1K4PSHGa/SVG/
> Output/glyph-location-16-mac.svg?dl=0
> https://www.dropbox.com/sh/uqzkxu8ku3hk3lj/AADPDoBPS8YQLtcgfByoW_fia/SVG/
> Output/glyph-location-16-win.svg?dl=0

yes, please use the platform specific icons found in the dropbox. Location has a unique icon specific for its host platform. We should conform to whatever the local standard is.

Comment 31

[[e-mail]]

(In reply to Fred Lin [:gasolin] from comment #23)
> Created attachment 8752719 [details]
> lack icon
> 
> Checked icons provided in dropbox, some images is still lacked:
> 
> 1. Offline storage icon
> 2. open popup windows icon
> 3. receive notifications icon
> 4. webRTC shareDevice icon (It's not shown in permission tab yet)

Bryan, could you help provide above icons plus a 'x' icon ?

Comment 32

[Matthew N. [:MattN]]

(In reply to Fred Lin [:gasolin] from comment #31)
> > Checked icons provided in dropbox, some images is still lacked:
> > 
> > 1. Offline storage icon
> > 2. open popup windows icon

The current pngs are chrome://browser/skin/urlbar-popup-blocked.png and chrome://browser/skin/[e-mail] which we many want as SVG

> > 3. receive notifications icon

We have that already at https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/web-notifications-tray.svg?force=1

> > 4. webRTC shareDevice icon (It's not shown in permission tab yet)
> 
> Bryan, could you help provide above icons plus a 'x' icon ?

Is the intention to use the same X that we use everywhere else (e.g. notification bars and tabs?). If so, you can use the .close-icon class. Note that the X is not part of this bug IIUC but I think you already know that.

Comment 33

[bbell]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #32)
> (In reply to Fred Lin [:gasolin] from comment #31)
> > > Checked icons provided in dropbox, some images is still lacked:
> > > 
> > > 1. Offline storage icon
> > > 2. open popup windows icon
> 
> The current pngs are chrome://browser/skin/urlbar-popup-blocked.png and
> chrome://browser/skin/[e-mail] which we many want as SVG
> 
> > > 3. receive notifications icon
> 
> We have that already at
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/web-
> notifications-tray.svg?force=1
> 
> > > 4. webRTC shareDevice icon (It's not shown in permission tab yet)
> > 
> > Bryan, could you help provide above icons plus a 'x' icon ?
> 
> Is the intention to use the same X that we use everywhere else (e.g.
> notification bars and tabs?). If so, you can use the .close-icon class. Note
> that the X is not part of this bug IIUC but I think you already know that.

I'll get you these new icons ASAP. However, I was wondering if anyone has talked you abut styling these icons for each platform externally? 

they're black silhouettes because they were intended to be displayed using this type of technique:  http://people.mozilla.org/~shorlander/icons-svg/SVG-Icon-Template-Test/svg-icon-template-test.html

Comment 34

[Matthew N. [:MattN]]

Shorlander/Bryan, do you know if we're using this filter approach anywhere in Fx yet? Since this is secondary UI it's probably less performance sensitive (compared to things like primary toolbar icons) though the blocked permission icons in the address bar could be shown during window open and would want a similar approach I assume.

Comment 35

[Stephen Horlander [:shorlander]]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #34)
> Shorlander/Bryan, do you know if we're using this filter approach anywhere
> in Fx yet? Since this is secondary UI it's probably less performance
> sensitive (compared to things like primary toolbar icons) though the blocked
> permission icons in the address bar could be shown during window open and
> would want a similar approach I assume.

We are using it in DevTools I think. But we haven't worked out what it will look like elsewhere yet. Working through some possibilities in bug 1054016.

Comment 36

[Stephen Horlander [:shorlander]]

(In reply to Stephen Horlander [:shorlander] from comment #35)
> (In reply to Matthew N. [:MattN] (behind on reviews) from comment #34)
> > Shorlander/Bryan, do you know if we're using this filter approach anywhere
> > in Fx yet? Since this is secondary UI it's probably less performance
> > sensitive (compared to things like primary toolbar icons) though the blocked
> > permission icons in the address bar could be shown during window open and
> > would want a similar approach I assume.
> 
> We are using it in DevTools I think. But we haven't worked out what it will
> look like elsewhere yet. Working through some possibilities in bug 1054016.

Filed bug 1274089

Comment 37

[[e-mail]]

Thanks Matthrew, the 'X' icon is used for bug 1203292. 


And I found `urlbar-popup-blocked` is only available at windows/osx. 
https://dxr.mozilla.org/mozilla-central/search?q=urlbar-popup-blocked.png&redirect=false&case=true

To reuse that icon will cause `icon not found` on linux. Do you know is it intended or its a bug?

Comment 38

[[e-mail]]

Bryan, I also found all the svg icon size provided in dropbox are doubled, 

ex: `glyph-addOn-16.svg` is actually 32x32

please update a new set as well, thanks

Comment 39

[[e-mail]]

BTW, it might helpful to pre-fill icon color as #808080 to show as dark gray icon

Comment 40

[[e-mail]]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #34)
> Shorlander/Bryan, do you know if we're using this filter approach anywhere
> in Fx yet? Since this is secondary UI it's probably less performance
> sensitive (compared to things like primary toolbar icons) though the blocked
> permission icons in the address bar could be shown during window open and
> would want a similar approach I assume.

Seems the web-notifications-notification-icon already use the svg sprite
https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/dist/bin/browser/chrome/browser/skin/classic/browser/browser.css#2057

Comment 41

[Matthew N. [:MattN]]

(In reply to Fred Lin [:gasolin] from comment #37)
> Thanks Matthrew, the 'X' icon is used for bug 1203292. 

Right, I just wanted to make sure you were going to keep the patches separate so they're easier to review.

> And I found `urlbar-popup-blocked` is only available at windows/osx. 
> https://dxr.mozilla.org/mozilla-central/search?q=urlbar-popup-blocked.
> png&redirect=false&case=true
> 
> To reuse that icon will cause `icon not found` on linux. Do you know is it
> intended or its a bug?

I'm not sure. It seems like Linux uses chrome://browser/skin/Info.png which is basically the equivalent icon but just with a poor name. I also see that cross-platform code uses chrome://browser/skin/Info.png for the popup-blocking notification bar so I think you could use chrome://browser/skin/Info.png (@1x) if we don't want a new icon. A separate task could be to unify the filenames for the icons if one wants.

(In reply to Fred Lin [:gasolin] from comment #38)
> Bryan, I also found all the svg icon size provided in dropbox are doubled, 
> 
> ex: `glyph-addOn-16.svg` is actually 32x32
> 
> please update a new set as well, thanks

Since they're SVG perhaps that's fine?

Comment 42

[[e-mail]]

I can edit .svg and change the `width="16" height="16" fill: #808080;` myself though.

And I found those icons in dropbox (with different name)

Offline storage icon: glyph-storeData-16.svg
https://www.dropbox.com/sh/uqzkxu8ku3hk3lj/AADrc6QMotXOksU7b8X5Q6OVa/SVG/Output/glyph-storeData-16.svg?dl=0

open popup windows icon: glyph-popupBlock-16.svg
https://www.dropbox.com/sh/uqzkxu8ku3hk3lj/AACc0S_WAQF6IxPrlMzasQf3a/SVG/Output/glyph-popupBlock-16.svg?dl=0

receive notifications icon: 
https://www.dropbox.com/sh/uqzkxu8ku3hk3lj/AABgoX7B0wONmymNVMF1-0MPa/SVG/Output/glyph-notifications-16.svg?dl=0

to replace urlbar-popup-blocked.png to svg, some color code are required, such as
the enable state of pop-up-blocked icon https://github.com/mozilla/gecko-dev/blob/master/browser/themes/osx/urlbar-popup-blocked.png
https://github.com/mozilla/gecko-dev/blob/master/browser/themes/shared/webrtc/webRTC-sharingMicrophone-16.png

Comment 43

[[e-mail]]

Attachment: Bug 1193006 - Show icons next to non-default permissions in the Permissions section of the Control Center;

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

Comment 44

[[e-mail]]

Comment on attachment 8754250 [details]
Bug 1193006 - Show icons next to non-default permissions in the Permissions section of the Control Center;

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

Comment 45

[[e-mail]]

Comment on attachment 8754250 [details]
Bug 1193006 - Show icons next to non-default permissions in the Permissions section of the Control Center;

WIP, add some icons.svg and address above issues

Comment 46

[[e-mail]]

Attachment: permissions with icons

WIP screenshot,

Need TODO:
1. replace location icons for each platform
2. replace mouse pointer icon to svg
3. replace webRTC icons

Comment 47

[Ash Grigas]

is anything else needed from UX?

Comment 48

[:Paolo Amadini]

For the geolocation prompt for example, we also have a PNG versions rendered at 64px. This is used for the icon inside the popup notification itself.

We're changing the geolocation iconography on Mac to use the symbol that is common there, so I guess we should probably change that image as well.

Are we going to use the 16px glyph scaled up to 64px?

Should we do the same for all the other popup notifications?

Comment 49

[:Paolo Amadini]

At some point you may find it useful to rebase this bug on top of the patch in bug 1147981.

Comment 50

[:Paolo Amadini]

Another question is whether we still want to use an hover and active state for these new glyph icons when used on the URL bar. We're using these effects inconsistently now.

Comment 51

[Ash Grigas]

(In reply to :Paolo Amadini from comment #50)
> Another question is whether we still want to use an hover and active state
> for these new glyph icons when used on the URL bar. We're using these
> effects inconsistently now.

there is a hover state which is in the design doc here:
https://mozilla.invisionapp.com/share/DM3XGA6YE#/screens/96586951

It is a joined hover state with the control center area.

Comment 52

[[e-mail]]

(In reply to :Paolo Amadini from comment #49)
> At some point you may find it useful to rebase this bug on top of the patch
> in bug 1147981.

I'll put css/svg change in a separate patch then the code part will be fine.

Comment 53

[:Paolo Amadini]

Ash, what about comment 48?

Comment 54

[Ash Grigas]

(In reply to :Paolo Amadini from comment #48)
> For the geolocation prompt for example, we also have a PNG versions rendered
> at 64px. This is used for the icon inside the popup notification itself.
> 
> We're changing the geolocation iconography on Mac to use the symbol that is
> common there, so I guess we should probably change that image as well.
> 
> Are we going to use the 16px glyph scaled up to 64px?
> 
> Should we do the same for all the other popup notifications?

bryan will provide svgs that can be scaled to each size needed. he is working on it today. as a placeholder you could scle the png to have something there...

Comment 55

[bbell]

(In reply to bbell from comment #30)
> (In reply to Fred Lin [:gasolin] from comment #25)
> > Default Icons such as geolocation use OS specific icon (the screenshot in
> > Comment 23 shows blue geolocation icon in linux) with more richer details.
> > 
> > https://github.com/mozilla/gecko-dev/blob/master/browser/themes/linux/
> > Geolocation-16.png
> > https://github.com/mozilla/gecko-dev/blob/master/browser/themes/osx/
> > Geolocation-16.png
> > https://github.com/mozilla/gecko-dev/blob/master/browser/themes/windows/
> > Geolocation-16.png
> > 
> > Does UX prefer to replace geolocation icons to those icons in dropbox?
> > 
> > https://www.dropbox.com/sh/uqzkxu8ku3hk3lj/AADOue8MP365nNf2y1K4PSHGa/SVG/
> > Output/glyph-location-16-mac.svg?dl=0
> > https://www.dropbox.com/sh/uqzkxu8ku3hk3lj/AADPDoBPS8YQLtcgfByoW_fia/SVG/
> > Output/glyph-location-16-win.svg?dl=0
> 
> yes, please use the platform specific icons found in the dropbox. Location
> has a unique icon specific for its host platform. We should conform to
> whatever the local standard is.

I redid all the icons so they're ready for production 

https://www.dropbox.com/sh/rffviyl7m4x7p7c/AABoqFM6EqO3opLSds01TnYla?dl=0

There are only 6 SVG files. one for each platform, and for each size. 16, and 32. Example HTML files are included.

Comment 56

[[e-mail]]

Since there's lot of discussion related to replace existing png icon to svg, let's discuss icon switch in a separate bug 1274480, and focus this bug on code part.

Comment 57

[[e-mail]]

Comment on attachment 8754250 [details]
Bug 1193006 - Show icons next to non-default permissions in the Permissions section of the Control Center;

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

Comment 58

[:Paolo Amadini]

Comment on attachment 8754250 [details]
Bug 1193006 - Show icons next to non-default permissions in the Permissions section of the Control Center;

https://reviewboard.mozilla.org/r/53864/#review50930

::: browser/base/content/browser.js:7165
(Diff revision 3)
>      label.setAttribute("class", "identity-popup-permission-label");
>      label.setAttribute("control", menulist.getAttribute("id"));
>      label.textContent = SitePermissions.getPermissionLabel(aPermission);
>  
> +    let image = document.createElement("image");
> +    image.setAttribute("showing", "true");

Sorry, I haven't tested this yet. What is this "showing" attribute used for?

::: browser/base/content/browser.js:7166
(Diff revision 3)
>      label.setAttribute("control", menulist.getAttribute("id"));
>      label.textContent = SitePermissions.getPermissionLabel(aPermission);
>  
> +    let image = document.createElement("image");
> +    image.setAttribute("showing", "true");
> +    image.setAttribute("class", aPermission + "-icon");

I think we still want a helper in SitePermissions that will return 'aPermission + "-icon"' unless there is a different class specified in the permissions declarations object.

Comment 59

[Ash Grigas]

Comment on attachment 8754257 [details]
permissions with icons

These icons have been updated in the dropbox folder so this screenshot is not correct.

Comment 60

[[e-mail]]

> > +    image.setAttribute("showing", "true");
> > +    image.setAttribute("class", aPermission + "-icon");
> 
> I think we still want a helper in SitePermissions that will return
> 'aPermission + "-icon"' unless there is a different class specified in the
> permissions declarations object.

I'll add `getIconClass` back.

Instead of return `aPermissionID + "-icon"`, Does `getIconClass` function need to return `labelID + "-icon"` if exist?

Comment 61

[[e-mail]]

Seems labelID is only used for get string, use aPermissionID might sufficient.

Comment 62

[[e-mail]]

Comment on attachment 8754250 [details]
Bug 1193006 - Show icons next to non-default permissions in the Permissions section of the Control Center;

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

Comment 63

[[e-mail]]

The patch should land after bug 1147981 and bug 1274480, issue addressed and append the block class to icon list to show related icon.

Comment 64

[:Paolo Amadini]

Comment on attachment 8754250 [details]
Bug 1193006 - Show icons next to non-default permissions in the Permissions section of the Control Center;

https://reviewboard.mozilla.org/r/53864/#review51756

::: browser/modules/SitePermissions.jsm:99
(Diff revision 4)
> +  /* Returns the class name used to represent the given permission.
> +   * If the state is BLOCK, append the block class to show related icon
> +   */
> +  getIconClass: function (aPermissionID, aState) {

I don't think we need to handle aState here. The advantage of having the separate class is that we don't need to create a combination that is specific to a given permission ID.

If we had an object that represented the current set of permissions associated to a given site including their state, then it might contain a predefined combination of classes for the full state, but it's not the case here, this object is just a singleton service. In other words there is no need for an extra parameter to the function, when the caller can apply the same logic.

::: browser/modules/SitePermissions.jsm:107
(Diff revision 4)
> +    let state = aState == this.BLOCK ? " block" : "";
> +    return aPermissionID + "-icon" + state;

You should check gPermissionObject for overrides. I've updated bug 1147981, you can check the patch there to see which permissions have a class whose base name is different from the permission ID.

Comment 65

[[e-mail]]

Thanks for check. Will wait bug 1274480 to reflect the correspondent class

Comment 67

[:Paolo Amadini]

I've moved the SitePermissions.jsm changes to their own bug 1280919, so that this bug can focus on adding the icons to the Control Center with the correct alignment described in bug 1204016.

Comment 68

[[e-mail]]

Hi Paolo, I resume work on this bug and found several issues that need your suggestion:


1. the mouse pointer icon name is not the same as the css name.

option 1. add `iconID: "point"` in gPermissionObject.pointerLock and return point-icon while calling getIconClass (currently take this way)


```
"pointerLock": {
  iconID: "point",
  exactHostMatch: true
}
```

Or 

option 2. change `point` style to `pointLock`

https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/notification-icons.inc.css#178

This way may left some composite style name when replace existing icon styles. 


2, I need add `popup` svg icon into `glyph.svg`. Can you point me what svg conversion tool you use for glyphs.svg? (because the syntax looks different from the download version in dropbox) 


3. The icon size should be 16x16 when icons show on door hanger. 

Option 1. add a style to turn door hanger img to the proper size. (currently take this way)


```
hbox[align="center"] > img {
  width: 16px;
  height: 16px;
}
```

Though it doesn't work (I guess XUL querySelector can't locate door hanger's hbox when it is created in code, query #identity-popup-permission-list returns an element with no child)


Option 2. add a class in img element to specify the size.

Comment 69

[[e-mail]]

FYR, The popup icon svg path from dropbox is

<path id="popup" d="M625.042,152h-20v14.969a1,1,0,0,0,1,1h6.375A9.977,9.977,0,0,0,613.982,171h-9.94a2,2,0,0,1-2-2V147a2,2,0,0,1,2-2h22a2,2,0,0,1,2,2v10.068a9.969,9.969,0,0,0-3-1.582V152Zm-3.073,5a8,8,0,1,1-8,8A8,8,0,0,1,621.969,157Zm-5,8a4.971,4.971,0,0,0,.831,2.754l6.923-6.922A4.994,4.994,0,0,0,616.969,165Zm10,0a4.972,4.972,0,0,0-.832-2.754l-6.922,6.923A5,5,0,0,0,626.969,165Z"/>

Comment 70

[[e-mail]]

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

Use http://petercollingridge.appspot.com/svg-editor and set optimize parameter as:
* conservative
* remove white space
* turn all Decimal places attribute to 1

the result:

<path id="popup" d="M625 152h-20v15a1 1 0 0 0 1 1h6.4A10 10 0 0 0 614 171h-9.9a2 2 0 0 1-2-2V147a2 2 0 0 1 2-2h22a2 2 0 0 1 2 2v10.1a10 10 0 0 0-3-1.6V152Zm-3.1 5a8 8 0 1 1-8 8A8 8 0 0 1 622 157Zm-5 8a5 5 0 0 0 0.8 2.8l6.9-6.9A5 5 0 0 0 617 165Zm10 0a5 5 0 0 0-0.8-2.8l-6.9 6.9A5 5 0 0 0 627 165Z"/>

Comment 71

[:Paolo Amadini]

(In reply to Fred Lin [:gasolin] from comment #68)
> Hi Paolo, I resume work on this bug and found several issues that need your
> suggestion:

Hi Fred, thanks for the update!

> option 1. add `iconID: "point"` in gPermissionObject.pointerLock and return
> point-icon while calling getIconClass (currently take this way)

I've moved this part of the patch to bug 1280919 so that I could unblock another bug I'm working on, and this is exactly the way I followed in the updated patch.

> 2, I need add `popup` svg icon into `glyph.svg`. Can you point me what svg
> conversion tool you use for glyphs.svg? (because the syntax looks different
> from the download version in dropbox)

I used Inkscape (<https://inkscape.org/>), but before I could open the file with the version I'm using, I had to convert the paths to add a comma before negative numbers:

  sed -E "s/([0-9])\-/\1,-/g" source.svg >dest.svg

This program allowed me to remove the "transform" on the paths, reduce the precision, and scale everything to fit a 32px grid. After that I've further aligned the paths to the grid manually.

Here is the correct path to add to the file. It's the 16px version of the icon scaled up to fit a 32px grid.

<path id="popup" d="m 2,24 a 4,4 0 0 0 4,4 l 8,0 a 10,10 0 0 1 -2,-4 l -4,0 a 2,2 0 0 1 -2,-2 l 0,-12 18,0 0,2 a 10,10 0 0 1 4,2 l 0,-8 a 4,4 0 0 0 -4,-4 l -18,0 a 4,4 0 0 0 -4,4 z m 12,-2.1 a 8,8 0 1 1 0,0.2 m 10.7,-4.3 a 5,5 0 0 0 -6.9,6.9 z m -5.4,8.4 a 5,5 0 0 0 6.9,-6.9 z" />

> 3. The icon size should be 16x16 when icons show on door hanger. 
> ```
> hbox[align="center"] > img {
>   width: 16px;
>   height: 16px;
> }
> ```
> Option 2. add a class in img element to specify the size.

The first selector is too generic, we definitely need the second option.

Comment 72

[:Paolo Amadini]

Fred, over in bug 1280919 we decided to change the approach and make all the front-end classes match the permission IDs, so we don't need the helper function anymore. You can now just use the permission ID + "-icon" in the front-end code without having to call the helper function. Hope this helps!

Comment 73

[[e-mail]]

Attachment: snap.png

Thanks for suggestion. Here's how current door hanger looks like.

Based on comment 72 I'll left web-notification and pointerLock unchanged, therefore the desktop-notification and pointLock img will not be found.

Comment 74

[[e-mail]]

Comment on attachment 8754250 [details]
Bug 1193006 - Show icons next to non-default permissions in the Permissions section of the Control Center;

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

Comment 75

[[e-mail]]

Comment on attachment 8754250 [details]
Bug 1193006 - Show icons next to non-default permissions in the Permissions section of the Control Center;

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

Comment 76

[[e-mail]]

I installed inkscape then do

1. sed -E "s/([0-9])\-/\1,-/g" source.svg >dest.svg to convert the paths to add a comma before negative numbers

2. open inkscape , do file > save as (optimized svg), set precision as 1

The result looks like comment 70 (d="M625 152h-20v15a1 1 0 0 0 1...) instead of comment 71 (d="m 2,24 a 4,4 0 0 0...)


By manipulating inkscape, I'm not sure what is the correct way to remove the "transform" on the paths (might be change the X, Y position?)

And why we need scale everything to fit a 32px grid from 16px? Shouldn't we ask for 32x32 svg icon from UX?

Comment 77

[:Paolo Amadini]

(In reply to Fred Lin [:gasolin] from comment #76)
> The result looks like comment 70 (d="M625 152h-20v15a1 1 0 0 0 1...) instead
> of comment 71 (d="m 2,24 a 4,4 0 0 0...)
> 
> By manipulating inkscape, I'm not sure what is the correct way to remove the
> "transform" on the paths (might be change the X, Y position?)

Yes, you can use the arrow keys to move the object slightly and then back in place, or you can apply a "relative move" of zero from the Transform panel. You also have to ensure to select the "Relative" option in the SVG output settings and numeric precision "3". This should give a path in the same format, although for this specific path I've then aligned the result manually.

> And why we need scale everything to fit a 32px grid from 16px? Shouldn't we
> ask for 32x32 svg icon from UX?

You're right, and future glyphs will be provided directly using the 32px grid, even when they are designed to be displayed at 16px. This is probably the last one we need to convert manually. More information is available in bug 1274480 comment 7 onwards.

Comment 78

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/53864/#review57254

We're almost there! I'll review a new patch once the comments below are addressed.

::: browser/base/content/browser.js:7246
(Diff revision 6)
> +    let img = document.createElement("image");
> +    img.setAttribute("class", "identity-popup-permission-icon " +
> +      SitePermissions.getIconClass(aPermission));

Just use aPermission + "-icon" directly.

::: browser/modules/SitePermissions.jsm:156
(Diff revision 6)
> +  /* Returns the class name used to represent the given permission. */
> +  getIconClass: function (aPermission) {
> +    if (!(aPermission.id in gPermissionObject)) {
> +      throw "Invalid permission ID: " + aPermission.id;
> +    }
> +
> +    return aPermission.id + "-icon";
> +  },
> +

Remove.

::: browser/modules/test/xpcshell/test_SitePermissions.js:119
(Diff revision 6)
> +
> +add_task(function* testPermissionsIconClass() {
> +  Assert.deepEqual(SitePermissions.listPermissions().sort().map(p => {
> +      return SitePermissions.getIconClass({id: p});
> +    }),
> +    ["camera-icon","cookie-icon","desktop-notification-icon","geo-icon",
> +     "image-icon", "indexedDB-icon","install-icon","microphone-icon",
> +     "pointerLock-icon","popup-icon"],
> +    "Correct list of all permissions icon class");
> +});

Remove.

::: browser/themes/shared/controlcenter/panel.inc.css:352
(Diff revision 6)
> +#identity-popup-permission-list {
> +  margin-inline-start: -30px;
> +}
> +

It still makes sense to caclulate this margin as 1em + 16px so that the individual icons are aligned with the header icon on their right margin. See Brian's patch here for inspiration:

https://reviewboard.mozilla.org/r/19043/diff/1#0

Also, the min-height on #identity-popup-permission-headline that I see in that patch makes sense to me.

::: browser/themes/shared/notification-icons.inc.css:66
(Diff revision 6)
> +.identity-popup-permission-icon {
> +  width: 16px;
> +  height: 16px;
> +}

Move this to panel.inc.css as it's mostly related to the styles there.

::: browser/themes/shared/notification-icons.inc.css:71
(Diff revision 6)
> +@media (min-resolution: 1.1dppx) {
> +  .identity-popup-permission-icon {
> +    width: 32px;
> +    height: 32px;
> +  }
> +}

This doesn't look right - the CSS icon size should still be 16px even on HiDPI. Did you test this patch on HiDPI? Do you need help with testing there?

::: browser/themes/shared/notification-icons.inc.css:108
(Diff revision 6)
>  .popup-notification-icon[popupid="web-notifications"] {
>    filter: url(chrome://browser/skin/filters.svg#fill);
>    fill: #999;
>  }

You need to add .popup-icon to this rule so that it's styled with the filter.

::: browser/themes/shared/notification-icons.inc.css:121
(Diff revision 6)
> +.image-icon,
> +.cookie-icon {
> +  list-style-image: url("chrome://browser/skin/info.svg");
> +}
> +

I think we can just leave an empty space if we don't have a specific icon for the permission.

::: browser/themes/shared/notification-icons.inc.css:136
(Diff revision 6)
> +.popup-icon {
> +  list-style-image: url("chrome://browser/skin/glyphs.svg#popup");
> +}
> +

Move this after the .pointer-icon, so that it's not in-between the two geolocation rules.

Comment 79

[:Paolo Amadini]

Comment on attachment 8764175 [details]
snap.png

And thanks a lot for the screenshot! Bug 1280919 landed on fx-team and once you rebase on top of it you should still see the correct icons for Pointer Lock and Notifications. I'm not sure why the indexedDB permission looks like a circle here, this should have its own icon now.

Comment 80

[[e-mail]]

Comment on attachment 8754250 [details]
Bug 1193006 - Show icons next to non-default permissions in the Permissions section of the Control Center;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53864/diff/6-7/

Comment 81

[[e-mail]]

Comment on attachment 8754250 [details]
Bug 1193006 - Show icons next to non-default permissions in the Permissions section of the Control Center;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53864/diff/7-8/

Comment 82

[[e-mail]]

Thanks for feedback! I've address all issues and set for review. 


Regarding HiDPI, I removed the styles but still like some help with testing there. (According the style declaration, does that mean test on macbook's retina screen?)

Comment 83

[:Paolo Amadini]

(In reply to Fred Lin [:gasolin] from comment #82)
> Regarding HiDPI, I removed the styles but still like some help with testing
> there. (According the style declaration, does that mean test on macbook's
> retina screen?)

Yes, I tested on a Retina screen and this version works just fine. Thanks!

I've edited one last detail in the patch and I'm running a tryserver build here:

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

Comment 84

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/bfc23df4f226
Show icons next to non-default permissions in the Permissions section of the Control Center. r=paolo

Comment 85

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bfc23df4f226

Comment 86

[Paul Silaghi, QA [:pauly]]

'Load images' and 'Set cookies' still have no icons in the Permissions dropdown list.
Is it expected? http://i.imgur.com/My80Dqk.png
50.0a1 (2016-06-29) Win 7

Comment 87

[:Paolo Amadini]

(In reply to Paul Silaghi, QA [:pauly] from comment #86)
> 'Load images' and 'Set cookies' still have no icons in the Permissions
> dropdown list.
> Is it expected?

Yes, they have no icon, because they are very rarely set.

Comment 88

[Paul Silaghi, QA [:pauly]]

One more observation: 'Maintain Offline Storage' has a strange icon on Linux (Ubuntu 14.04).
Works fine on OS X 10.11, Win 7.
What do you think?

Comment 89

[:Paolo Amadini]

(In reply to Paul Silaghi, QA [:pauly] from comment #88)
> One more observation: 'Maintain Offline Storage' has a strange icon on Linux
> (Ubuntu 14.04).
> Works fine on OS X 10.11, Win 7.
> What do you think?

Do you have a screenshot?

Comment 90

[Paul Silaghi, QA [:pauly]]

(In reply to :Paolo Amadini from comment #89)
> Do you have a screenshot?
http://i.imgur.com/LNXiVni.png

Comment 91

[:Paolo Amadini]

(In reply to Paul Silaghi, QA [:pauly] from comment #90)
> (In reply to :Paolo Amadini from comment #89)
> > Do you have a screenshot?
> http://i.imgur.com/LNXiVni.png

Ah, I've seen this before in attachment 8764175 [details]. Fred, can you investigate? Is it something in the path definition?

We may need to file a platform bug and maybe implement a workaround until fixed (that can be as simple as showing no icon on Linux).

Comment 92

[[e-mail]]

On Linux the indexedDB icon shows the grey circle.

.indexedDB-icon {
  list-style-image: url(chrome://browser/skin/glyphs.svg#indexedDB);
}


Though in bug 1204007 the blocked version with `.indexedDB-icon.blocked` style works fine.


.indexedDB-icon.blocked {
  list-style-image: url(chrome://browser/skin/glyphs.svg#indexedDB-blocked);
}

Therefore I try to change the indexedDB style to


.identity-popup-permission-icon.indexedDB-icon {
  ...
}


The workaround works(!), but it definitely looks like a platform bug on linux.

David, could you find someone help from the platform side?

Comment 93

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't understand what you think the bug is.  Could you explain?

Comment 94

[[e-mail]]

We use `xxx-icon` class with `.identity-popup-permission-icon` in browser.js
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7251

As paul addressed in comment 88, the `indexedDB` icon style shows a strange icon on Linux (Ubuntu 14.04).
But works fine on OS X 10.11, Win 7. (screenshot in Comment 91)

https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/notification-icons.inc.css#136

At the end I tried comment 92 and find the compound style (.identity-popup-permission-icon.indexedDB-icon)  show the icon correctly but the standalone style (.indexedDB-icon) doesn't show the icon.

Comment 95

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you look at the style rules in devtools?  Were there other relevant style rules matching on Linux? that overrode the rule you wanted to win?  The workaround (adding another class to the selector) would increase the CSS specificity, so it would change which other CSS rules that rule would override.

Comment 96

[[e-mail]]

Oops I found Linux specified browser.css does overwrite the `.indexedDB-icon` style

https://dxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#1023

I'll file another bug to fix it. Thanks for suggestion and sorry for the false alert!

Comment 97

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 50.0a1 (2016-07-07) considering bug 1284172.

Comment 98

[Panos Astithas (he/him) [:past] (please ni?)]

Release Note Request (optional, but appreciated)
[Why is this notable]: combined with bug 1203292, which also landed in 50, this allows a more intuitive interaction with the site identity panel. 
[Suggested wording]: a more intuitive site identity panel with icons for non-default permissions and single click interaction
[Links (documentation, blog post, etc)]: none

[Alternative wording]: the site identity panel is now 2x more fun thanks to spiffy new permission icons and single click interactions

Comment 99

[Liz Henry (:lizzard) (relman/hg->git project)]

At this point I don't think we need a release note - we may have missed noting this for 50.