Display icons for blocked permission in the identity block

VERIFIED FIXED in Firefox 50

Status

()

P1
normal
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: MarcoM, Assigned: johannh)

Tracking

(Depends on: 1 bug)

unspecified
Firefox 50
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(relnote-firefox -, firefox50 verified)

Details

(Whiteboard: [fxprivacy], URL)

Attachments

(3 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

4 years ago
Priority: P3 → P2
(Reporter)

Updated

3 years ago
Priority: P2 → P3
(Reporter)

Updated

3 years ago
Priority: P3 → P1

Updated

3 years ago
Flags: qe-verify? → qe-verify+
(Reporter)

Updated

3 years ago
QA Contact: paul.silaghi
Will allowed permissions still show up for Firefox 49?  Will we have a combination of allowed and blocked in the url bar?
(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

3 years ago
Hi Brian, do you have a link for the SVG assets for the icons of blocked permissions?
Flags: needinfo?(bbell)

Updated

3 years ago
Duplicate of this bug: 1206250

Comment 5

3 years ago
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

3 years ago
(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
Flags: needinfo?(bbell)
Priority: P1 → P2
(Reporter)

Updated

3 years ago
Priority: P2 → P1
(Reporter)

Updated

3 years ago
Priority: P1 → P2

Updated

3 years ago
Depends on: 1280919
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Iteration: --- → 50.2
Priority: P2 → P1

Comment 7

3 years ago
(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

3 years ago
Created attachment 8764564 [details]
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/

Updated

3 years ago
Whiteboard: [fxprivacy] → [fxprivacy][a][b][c]

Updated

3 years ago
Blocks: 1282768
(Assignee)

Comment 9

3 years ago
Created attachment 8766249 [details]
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/
Attachment #8766249 - Flags: review?(josh)
Attachment #8766250 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 10

3 years ago
Created attachment 8766250 [details]
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/
(Assignee)

Comment 11

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8764564 - Attachment is obsolete: true

Comment 12

3 years ago
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.
Attachment #8766250 - Flags: review?(paolo.mozmail) → review+

Updated

3 years ago
Blocks: 1203292
(Assignee)

Comment 13

3 years ago
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/
Attachment #8766249 - Attachment description: Bug 1206252 - Part 1: Add a function to get all permissions by URI to nsIPermissionManager. → Bug 1206252 - Part 3: Indicate blocked permissions in the identity box.
Attachment #8766249 - Flags: review?(josh) → review?(paolo.mozmail)
(Assignee)

Updated

3 years ago
Attachment #8766250 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
Created attachment 8766813 [details]
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/
Attachment #8766813 - Flags: review?(josh)
Attachment #8766814 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 15

3 years ago
Created attachment 8766814 [details]
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/
(Assignee)

Comment 16

3 years ago
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/
(Assignee)

Comment 17

3 years ago
Sorry for the noise, hg confusion.
(Assignee)

Updated

3 years ago
Attachment #8766814 - Flags: review?(paolo.mozmail) → review+
(Assignee)

Updated

3 years ago
Depends on: 1283857

Updated

3 years ago
Attachment #8766814 - Attachment is obsolete: true

Comment 19

3 years ago
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?
Attachment #8766249 - Flags: review?(paolo.mozmail)

Updated

3 years ago
Attachment #8766814 - Attachment is obsolete: false
Attachment #8766814 - Flags: review+

Updated

3 years ago
Whiteboard: [fxprivacy][a][b][c] → [fxprivacy]
(Assignee)

Comment 20

3 years ago
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.
Depends on: 1267617
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
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.
Attachment #8766813 - Flags: review?(josh) → review+
(Assignee)

Comment 22

3 years ago
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/
Attachment #8766249 - Attachment description: Bug 1206252 - Part 3: Indicate blocked permissions in the identity box. → Bug 1206252 - Part 2: Indicate blocked permissions in the identity box.
Attachment #8766249 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 23

3 years ago
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/
(Assignee)

Updated

3 years ago
Attachment #8766814 - Attachment is obsolete: true
(Assignee)

Comment 24

3 years ago
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.

Updated

3 years ago
Attachment #8766249 - Flags: review?(paolo.mozmail) → review+

Comment 25

3 years ago
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
(Assignee)

Comment 26

3 years ago
Created attachment 8770103 [details]
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/
Attachment #8770103 - Flags: review?(josh)
(Assignee)

Comment 27

3 years ago
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/
(Assignee)

Comment 28

3 years ago
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/
(Assignee)

Updated

3 years ago
Blocks: 1285893
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.
Attachment #8770103 - Flags: review?(josh) → review+
(Assignee)

Comment 30

3 years ago
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/
(Assignee)

Comment 31

3 years ago
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/
(Assignee)

Comment 32

3 years ago
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/
(Assignee)

Comment 33

3 years ago
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/324af61c3929
https://hg.mozilla.org/mozilla-central/rev/5a23c763032f
https://hg.mozilla.org/mozilla-central/rev/d868ba1645e6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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?
Flags: needinfo?(jhofmann)
(Assignee)

Comment 36

3 years ago
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).
Flags: needinfo?(jhofmann)
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
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
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
relnote-firefox: --- → ?
Duplicate of this bug: 1127353
Since we shipped this 6 months ago I don't think we need to relnote it now.
relnote-firefox: ? → -
You need to log in before you can comment on or make changes to this bug.