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.

Current design - notice that there isn't an icon

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

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

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?

(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.

(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).

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 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 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?

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

(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

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.

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

Hi Fred, please feel free to take this over

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

(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

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

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

(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.

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

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

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

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)

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?

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

(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.

Checking with Bryan Bell

(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.

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?

(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.

(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 ?

(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/urlbar-popup-blocked@2x.png 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.

(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/urlbar-popup-blocked@2x.png 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

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.

(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.

(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

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?

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

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

(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

(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?

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

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

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 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

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

is anything else needed from UX?

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?

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

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.

(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.

(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.

Ash, what about comment 48?

(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...

(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.

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 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 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 on attachment 8754257 [details] permissions with icons These icons have been updated in the dropbox folder so this screenshot is not correct.

> > + 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?

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

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/

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 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.

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

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.

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.

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"/>

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"/>

(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.

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!

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 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 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/

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?

(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.

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 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 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 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/

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?)

(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

Pushed by paolo.mozmail@amadzone.org: 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

'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

(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.

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?

(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?

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

(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).

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?

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

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.

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.

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!

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

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

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