1206252 - Display icons for blocked permission in the identity block

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Comment 1

[Tanvi Vyas[:tanvi]]

Will allowed permissions still show up for Firefox 49?  Will we have a combination of allowed and blocked in the url bar?

Comment 2

[Matthew N. [:MattN]]

(In reply to Tanvi Vyas [:tanvi] from comment #1)
> Will allowed permissions still show up for Firefox 49?  Will we have a
> combination of allowed and blocked in the url bar?

Allowed permissions don't currently show up, only webrtc ones when they're in-use. i.e. we don't show the webRTC indicator on a page just because the site has always allow, we only show it on a page that's actually using webRTC.

https://mozilla.invisionapp.com/share/Z56G11LCV#/screens/152725627 implies that the (i) icon will alternate between the (i) and the permission in-use. I'm not sure if that's the latest design though.

Comment 3

[:Paolo Amadini]

Hi Brian, do you have a link for the SVG assets for the icons of blocked permissions?

Comment 5

[:Paolo Amadini]

For blocked permissions in the identity block, we may need a whitelist of which ones to show, because we may have not have an icon at all for some rarely used permissions.

Comment 6

[bbell]

(In reply to :Paolo Amadini from comment #3)
> Hi Brian, do you have a link for the SVG assets for the icons of blocked
> permissions?

these? https://www.dropbox.com/sh/nscav5ttkzv5apy/AABKbOBzO2w1GkCxH_xycJkNa?dl=0

Comment 7

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #5)
> For blocked permissions in the identity block, we may need a whitelist of
> which ones to show, because we may have not have an icon at all for some
> rarely used permissions.

I think we'll have CSS rules to prevent an empty space appearing for permissions for which we don't have an icon.

Comment 8

[:Paolo Amadini]

Attachment: Bug 1206252 - Display icons for blocked permissions in the identity block.

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

Comment 9

[Johann Hofmann [:johannh]]

Attachment: Bug 1206252 - Part 2: Indicate blocked permissions in the identity box.

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

Comment 10

[Johann Hofmann [:johannh]]

Attachment: Bug 1206252 - Part 2: Add blocked permission icons to glyphs.svg.

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

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8766249 [details]
Bug 1206252 - Part 2: Indicate blocked permissions in the identity box.

jdm, you're the only one I know from the peer list of that module, please re-assign if you don't have time.

We're working with permissions based on particular tabs a lot right now and end up creating verbose and less performant code because we lack this kind of function.

I'll add tests in another patch if you're fine with this.

Comment 12

[:Paolo Amadini]

Comment on attachment 8766250 [details]
Bug 1206252 - Part 2: Add blocked permission icons to glyphs.svg.

https://reviewboard.mozilla.org/r/61232/#review58120

Clever!

Can you file a platform bug to make xlink:href work across chrome documents, and mark it as a dependency for reference? We may have other use cases in the future where we might want to reference glyphs from more complex graphic files.

Comment 13

[Johann Hofmann [:johannh]]

Comment on attachment 8766249 [details]
Bug 1206252 - Part 2: Indicate blocked permissions in the identity box.

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

Comment 14

[Johann Hofmann [:johannh]]

Attachment: Bug 1206252 - Part 1: Add a function to get all permissions by URI to nsIPermissionManager.

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

Comment 15

[Johann Hofmann [:johannh]]

Attachment: Bug 1206252 - Part 2: Add blocked permission icons to glyphs.svg.

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

Comment 16

[Johann Hofmann [:johannh]]

Comment on attachment 8766249 [details]
Bug 1206252 - Part 2: Indicate blocked permissions in the identity box.

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

Comment 17

[Johann Hofmann [:johannh]]

Sorry for the noise, hg confusion.

Comment 18

[:Paolo Amadini]

Can be rebased over https://hg.mozilla.org/integration/fx-team/rev/050c0ad48c78 from bug 1204007.

Comment 19

[:Paolo Amadini]

Comment on attachment 8766249 [details]
Bug 1206252 - Part 2: Indicate blocked permissions in the identity box.

https://reviewboard.mozilla.org/r/61230/#review59082

::: browser/base/content/browser.js:6908
(Diff revision 3)
> +    for (let permission of SitePermissions.getAll(this._uri)) {
> +      if (permission.state === SitePermissions.BLOCK) {
> +
> +        let icon = permissionAnchors[permission.id];
> +        if (icon) {
> +          icon.classList.add("blocked");

Could add the "blocked" class to the XUL file directly, it's going to be always present anyways.

::: browser/base/content/browser.xul:770
(Diff revision 3)
> +                <image id="geo-icon" class="notification-anchor-icon geo-icon" role="button"
> +                       aria-label="&urlbar.geolocationNotificationAnchor.label;"/>

I don't think this should be a "notification-anchor-icon". It's going to create confusion with the real prompt anchor icons. We should have a different class, simply styled to be a 16px x 16px icon.

Also the ID is not really used as an ID. We could use a "data-permissionid" attribute instead. You could also put these in an hbox so you could enumerate its children instead of using the slower querySelector.

It would be nice if we could generate the icons programmatically, but for now the fact they are associated with an aria-label prevents us from doing that. We could file a follow-up bug to move these labels to a ".properties" file, and also give a clearer indication that these are icons for blocked permissions. I believe screen readers won't notice that at present.

::: browser/modules/SitePermissions.jsm:86
(Diff revision 3)
> +  /* Returns all custom permissions for a given URI
> +   */

Document the return format, even if it's quite simple, and what it returns for invalid URIs.

Can we find a better name for getAll/getPermissionsByURI to highlight their difference?

Comment 20

[Johann Hofmann [:johannh]]

Given that we will have the actual icons in the same location once Bug 1267617 lands, I'll wait for that one until further working on this. We might be able to reuse some stuff.

Comment 21

[Josh Matthews [:jdm]]

Comment on attachment 8766813 [details]
Bug 1206252 - Part 1: Add a function to get all permissions by URI to nsIPermissionManager.

https://reviewboard.mozilla.org/r/61554/#review59218

Looks good!

::: extensions/cookie/nsPermissionManager.cpp:2256
(Diff revision 1)
> +
> +  nsCOMPtr<nsIPrincipal> principal;
> +  nsresult rv = GetPrincipal(aURI, getter_AddRefs(principal));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  PermissionHashKey* entry = nullptr;

Let's declare this as we assign it two lines down.

::: netwerk/base/nsIPermissionManager.idl:103
(Diff revision 1)
> +   * an enumerator of all permissions which are not set to default
> +   * and which belong to the matching prinicpal of the given URI.
> +   *
> +   * @param uri  the URI to get all permissions for
> +   */
> +  nsISimpleEnumerator getAll(in nsIURI uri);

For clarity, let's call this getAllForURI.

Comment 22

[Johann Hofmann [:johannh]]

Comment on attachment 8766813 [details]
Bug 1206252 - Part 1: Add a function to get all permissions by URI to nsIPermissionManager.

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

Comment 23

[Johann Hofmann [:johannh]]

Comment on attachment 8766249 [details]
Bug 1206252 - Part 2: Indicate blocked permissions in the identity box.

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

Comment 24

[Johann Hofmann [:johannh]]

This patch should address most of your comments. :)

(In reply to :Paolo Amadini from comment #19)
> Comment on attachment 8766249 [details]
> Bug 1206252 - Part 2: Indicate blocked permissions in the identity box.
> 
> https://reviewboard.mozilla.org/r/61230/#review59082
> 
> ::: browser/base/content/browser.xul:770
> (Diff revision 3)
> > +                <image id="geo-icon" class="notification-anchor-icon geo-icon" role="button"
> > +                       aria-label="&urlbar.geolocationNotificationAnchor.label;"/>
> 
> I don't think this should be a "notification-anchor-icon". It's going to
> create confusion with the real prompt anchor icons. We should have a
> different class, simply styled to be a 16px x 16px icon.
> 

Since the notification-anchor-icons are now in the same container and have the exact same style as the blocked permission icons I think it is a good idea to keep the common notification-anchor-icon class.

Comment 25

[:Paolo Amadini]

Comment on attachment 8766249 [details]
Bug 1206252 - Part 2: Indicate blocked permissions in the identity box.

https://reviewboard.mozilla.org/r/61230/#review60078

Sorry for the delay.

::: browser/modules/test/xpcshell/test_SitePermissions.js:16
(Diff revision 4)
> -add_task(function* testHasGrantedPermissions() {
> -  // check that it returns false on an invalid URI
> +add_task(function* testgetAllByURI() {
> +  // check that it returns an empty array on an invalid URI

nit: test_getAllByURI or testGetAllByURI

Comment 26

[Johann Hofmann [:johannh]]

Attachment: Bug 1206252 - Part 3: Add tests for nsIPermissionManager.getAllForURI.

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

Comment 27

[Johann Hofmann [:johannh]]

Comment on attachment 8766813 [details]
Bug 1206252 - Part 1: Add a function to get all permissions by URI to nsIPermissionManager.

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

Comment 28

[Johann Hofmann [:johannh]]

Comment on attachment 8766249 [details]
Bug 1206252 - Part 2: Indicate blocked permissions in the identity box.

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

Comment 29

[Josh Matthews [:jdm]]

Comment on attachment 8770103 [details]
Bug 1206252 - Part 3: Add tests for nsIPermissionManager.getAllForURI.

https://reviewboard.mozilla.org/r/63680/#review61090

Nice and clear. Thanks!

::: extensions/cookie/test/unit/test_permmanager_getAllForURI.js:5
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +function check_enumerator(uri, permissions) {
> +  let pm = Cc["@mozilla.org/permissionmanager;1"].

nit: . should go on the next line.

::: extensions/cookie/test/unit/test_permmanager_getAllForURI.js:21
(Diff revision 1)
> +  }
> +  do_check_false(enumerator.hasMoreElements());
> +}
> +
> +function run_test() {
> +  let pm = Cc["@mozilla.org/permissionmanager;1"].

nit: . on the next line.

Comment 30

[Johann Hofmann [:johannh]]

Comment on attachment 8766813 [details]
Bug 1206252 - Part 1: Add a function to get all permissions by URI to nsIPermissionManager.

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

Comment 31

[Johann Hofmann [:johannh]]

Comment on attachment 8766249 [details]
Bug 1206252 - Part 2: Indicate blocked permissions in the identity box.

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

Comment 32

[Johann Hofmann [:johannh]]

Comment on attachment 8770103 [details]
Bug 1206252 - Part 3: Add tests for nsIPermissionManager.getAllForURI.

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

Comment 33

[Johann Hofmann [:johannh]]

https://hg.mozilla.org/integration/fx-team/rev/324af61c39291787d7ce7a63f4370424312a81e7
Bug 1206252 - Part 1: Add a function to get all permissions by URI to nsIPermissionManager. r=jdm

https://hg.mozilla.org/integration/fx-team/rev/5a23c763032f87bc7530a017096fc389ce3ea3cd
Bug 1206252 - Part 2: Indicate blocked permissions in the identity box. r=paolo

https://hg.mozilla.org/integration/fx-team/rev/d868ba1645e6fbc6671c09f49f331ba4acae9ad6
Bug 1206252 - Part 3: Add tests for nsIPermissionManager.getAllForURI. r=jdm

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/324af61c3929
https://hg.mozilla.org/mozilla-central/rev/5a23c763032f
https://hg.mozilla.org/mozilla-central/rev/d868ba1645e6

Comment 35

[Paul Silaghi, QA [:pauly]]

If opened before blocking, the notification anchors are still displayed and some of them can still be enabled (microphone), while the corresponding permissions are blocked - http://imgur.com/a/fFEZ8
Thoughts?

Comment 36

[Johann Hofmann [:johannh]]

That's probably fallout from Bug 1287408, AFAIK you shouldn't be able to open permission doorhangers if you already made a decision on that permission (neither block nor allow will trigger the doorhanger).

Comment 37

[Paul Silaghi, QA [:pauly]]

Blocked non-default permissions displayed fine in the identity block.
Verified fixed on FX 50.0a1 (2016-07-26) Win 7, Ubuntu 14.04, OS X 10.10.5.

(In reply to Paul Silaghi, QA [:pauly] from comment #35)
> If opened before blocking, the notification anchors are still displayed and
> some of them can still be enabled (microphone), while the corresponding
> permissions are blocked - http://imgur.com/a/fFEZ8
Filed bug 1289755

Comment 38

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

Release Note Request (optional, but appreciated)
[Why is this notable]: users can easily see if the page seems broken due to a rejected permission request
[Suggested wording]: Does a site not behave as expected? Look for a denied permission icon in the AwesomeBar.
[Links (documentation, blog post, etc)]: none, unfortunately

Comment 40

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Since we shipped this 6 months ago I don't think we need to relnote it now.