Closed
Bug 1206252
Opened 8 years ago
Closed 7 years ago
Display icons for blocked permission in the identity block
Categories
(Firefox :: General, defect, P1)
Firefox
General
Tracking
()
People
(Reporter: MarcoM, Assigned: johannh)
References
(Depends on 1 open bug, )
Details
(Whiteboard: [fxprivacy])
Attachments
(3 files, 3 obsolete files)
No description provided.
Flags: qe-verify?
Reporter | ||
Updated•8 years ago
|
Priority: P3 → P2
Reporter | ||
Updated•8 years ago
|
Priority: P2 → P3
Reporter | ||
Updated•8 years ago
|
Priority: P3 → P1
Updated•8 years ago
|
Flags: qe-verify? → qe-verify+
Reporter | ||
Updated•8 years ago
|
QA Contact: paul.silaghi
Comment 1•8 years ago
|
||
Will allowed permissions still show up for Firefox 49? Will we have a combination of allowed and blocked in the url bar?
Comment 2•8 years ago
|
||
(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•8 years ago
|
||
Hi Brian, do you have a link for the SVG assets for the icons of blocked permissions?
Flags: needinfo?(bbell)
Comment 5•8 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.
(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)
Updated•7 years ago
|
Priority: P1 → P2
Reporter | ||
Updated•7 years ago
|
Priority: P2 → P1
Reporter | ||
Updated•7 years ago
|
Priority: P1 → P2
Updated•7 years ago
|
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Iteration: --- → 50.2
Priority: P2 → P1
Comment 7•7 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•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/60366/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60366/
Updated•7 years ago
|
Whiteboard: [fxprivacy] → [fxprivacy][a][b][c]
Assignee | ||
Comment 9•7 years ago
|
||
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•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61232/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61232/
Assignee | ||
Comment 11•7 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•7 years ago
|
Attachment #8764564 -
Attachment is obsolete: true
Comment 12•7 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+
Assignee | ||
Comment 13•7 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•7 years ago
|
Attachment #8766250 -
Attachment is obsolete: true
Assignee | ||
Comment 14•7 years ago
|
||
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•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61556/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61556/
Assignee | ||
Comment 16•7 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•7 years ago
|
||
Sorry for the noise, hg confusion.
Assignee | ||
Updated•7 years ago
|
Attachment #8766814 -
Flags: review?(paolo.mozmail) → review+
Comment 18•7 years ago
|
||
Can be rebased over https://hg.mozilla.org/integration/fx-team/rev/050c0ad48c78 from bug 1204007.
Depends on: 1204007
Updated•7 years ago
|
Attachment #8766814 -
Attachment is obsolete: true
Comment 19•7 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•7 years ago
|
Attachment #8766814 -
Attachment is obsolete: false
Attachment #8766814 -
Flags: review+
Updated•7 years ago
|
Whiteboard: [fxprivacy][a][b][c] → [fxprivacy]
Assignee | ||
Comment 20•7 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
Updated•7 years ago
|
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
Comment 21•7 years ago
|
||
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•7 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•7 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•7 years ago
|
Attachment #8766814 -
Attachment is obsolete: true
Assignee | ||
Comment 24•7 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•7 years ago
|
Attachment #8766249 -
Flags: review?(paolo.mozmail) → review+
Comment 25•7 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•7 years ago
|
||
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•7 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•7 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/
Comment 29•7 years ago
|
||
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•7 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•7 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•7 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•7 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•7 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
Closed: 7 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Comment 35•7 years ago
|
||
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•7 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)
Comment 37•7 years ago
|
||
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
Comment 38•7 years ago
|
||
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:
--- → ?
Since we shipped this 6 months ago I don't think we need to relnote it now.
You need to log in
before you can comment on or make changes to this bug.
Description
•