Always show permissions section in the Control Center

VERIFIED FIXED in Firefox 43

Status

()

P1
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: bgrins, Assigned: ttaubert)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 43
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox43 verified)

Details

(Whiteboard: [fxprivacy])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Right now, the Permissions section is only visible when there are some non-default permissions for the site.  For Bug 1188355, we should make it visible all the time, and then have the following text visible if there are no non-default preferences:

"You have not granted this page any special permissions."
Flags: qe-verify+
Flags: firefox-backlog?

Updated

3 years ago
Flags: firefox-backlog? → firefox-backlog+
(Reporter)

Updated

3 years ago
Blocks: 1193006
(Reporter)

Updated

3 years ago
No longer blocks: 1193006

Updated

3 years ago
Priority: -- → P1
(Assignee)

Updated

3 years ago
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 43.2 - Sep 7
(Assignee)

Comment 1

3 years ago
Created attachment 8657064 [details] [diff] [review]
0001-Bug-1193004-Always-show-permissions-section-in-the-C.patch
Attachment #8657064 - Flags: review?(bgrinstead)
(Reporter)

Comment 2

3 years ago
Comment on attachment 8657064 [details] [diff] [review]
0001-Bug-1193004-Always-show-permissions-section-in-the-C.patch

Review of attachment 8657064 [details] [diff] [review]:
-----------------------------------------------------------------

The browser_permissions.js is going to need to be updated since it will fail with this patch applied.  Can you modify that test to check is_hidden on the description element instead of permissionsContainer?

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +729,5 @@
>  
>  <!ENTITY identity.moreInfoLinkText2 "More Information">
>  
>  <!ENTITY identity.permissions "Permissions">
> +<!ENTITY identity.permissionsEmpty "You have not granted this page any special permissions.">

Ash, is this string still correct with the new design work?
Attachment #8657064 - Flags: review?(bgrinstead)
(Reporter)

Comment 3

3 years ago
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Ash, is this string still correct with the new design work?

Also, do we still want the 'always shown' permissions section in the main view for 43 or should that wait for 44 along with the subview changes?
Flags: needinfo?(agrigas)

Comment 4

3 years ago
(In reply to Brian Grinstead [:bgrins] from comment #3)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > Ash, is this string still correct with the new design work?
> 
> Also, do we still want the 'always shown' permissions section in the main
> view for 43 or should that wait for 44 along with the subview changes?

The main view can show as long as it matches existing behavior of the current release version users have (https://www.dropbox.com/s/c2telu40akl3p10/Screenshot%202015-09-03%2017.56.43.png?dl=0) in terms of functionality until we update it with the subpanel in 44.

For the string - we have currently: 'Requested by this site.' - it doesnt change whether you have something showing or not below it. For now, we can use 'No permissions have been requested.'
Flags: needinfo?(agrigas)
(In reply to agrigas from comment #4)
> For now, we can use 'No permissions have been requested.'

This isn't accurate either. A site could have requested a permission with the user ignoring the request or granting / denying it without persisting that choice.

Updated

3 years ago
Iteration: 43.2 - Sep 7 → 43.3 - Sep 21
(Assignee)

Comment 6

3 years ago
Created attachment 8658715 [details] [diff] [review]
0001-Bug-1193004-Always-show-permissions-section-in-the-C.patch, v2

Didn't change the string, the current version seems to make the most sense.
Attachment #8657064 - Attachment is obsolete: true
Attachment #8658715 - Flags: review?(bgrinstead)
Permissions are generally site-specific, not page-specific, and the string should probably reflect that.

Comment 8

3 years ago
(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to agrigas from comment #4)
> > For now, we can use 'No permissions have been requested.'
> 
> This isn't accurate either. A site could have requested a permission with
> the user ignoring the request or granting / denying it without persisting
> that choice.

Dao - we're changing the behavior or allowing and blocking notifications. The new design will not have a 'dismiss' action so a user has to decide to allow or block temporarily or permanently for the requested permission.

I agree we should use 'site' - that is what we have in our latest design. We will have a second copy pass at all this work so what we decide now is placeholder.

https://www.dropbox.com/s/fevthj7yelzre27/Screenshot%202015-09-09%2010.20.19.png?dl=0
(Reporter)

Comment 9

3 years ago
Comment on attachment 8658715 [details] [diff] [review]
0001-Bug-1193004-Always-show-permissions-section-in-the-C.patch, v2

Review of attachment 8658715 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but please wait to land this until after Bug 1203280, since otherwise there will be some complicated rebasing needed.  I will attach a version of this patch rebased on top of 1203280 for convenience

::: browser/base/content/test/general/browser_permissions.js
@@ +40,5 @@
>  
>    gIdentityHandler.setPermission("install", SitePermissions.getDefault("install"));
>  
>    gIdentityHandler._identityBox.click();
> +  ok(!is_hidden(emptyLabel), "The container is hidden");

"List of permissions is empty"
Attachment #8658715 - Flags: review?(bgrinstead) → review+
(Reporter)

Comment 10

3 years ago
Created attachment 8658906 [details] [diff] [review]
119300-site-perm-mainview-rebase.patch

Rebased version of this patch on top of patches in bug 1203280.  Also updated the assertion message as I noted in the review
(Reporter)

Comment 11

3 years ago
Comment on attachment 8658715 [details] [diff] [review]
0001-Bug-1193004-Always-show-permissions-section-in-the-C.patch, v2

Review of attachment 8658715 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +729,5 @@
>  
>  <!ENTITY identity.moreInfoLinkText2 "More Information">
>  
>  <!ENTITY identity.permissions "Permissions">
> +<!ENTITY identity.permissionsEmpty "You have not granted this page any special permissions.">

I guess change this to 'site' as per Comment 8
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4664200&repo=fx-team
Flags: needinfo?(ttaubert)
(Reporter)

Comment 15

3 years ago
Not positive, but you may need to use `gBrowser.ownerDocument` instead of `document` here: https://hg.mozilla.org/integration/fx-team/rev/98f6e793e383#l2.14
(Assignee)

Comment 16

3 years ago
I can reproduce this locally when I run the test with --run-until-failure. It opens the control center and I can see the <description>, yet document.querySelector() isn't able to find it. Even assigning an ID and using getElementById() doesn't help.
Flags: needinfo?(ttaubert)
(Assignee)

Comment 17

3 years ago
Found something that seems to work.
https://hg.mozilla.org/mozilla-central/rev/04cc4ac17e65
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
QA Contact: paul.silaghi
I reproduced this bug on Firefox Nightly Version 42.0a1

It's fixed and verified on Latest Nightly
Build ID 	20150917030229
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:43.0) Gecko/20100101 Firefox/43.0

Tested OS -- Windows8.1 32bit
QA Whiteboard: [bugday-20150916]
Successfully reproduce this bug in Nightly 42.0a1 (2015-08-10) on Linux x64

This Bug is now verified as fixed on Latest Nightly 43.0a1 (2015-09-17) 

Build ID: 20150917030229
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0

As it is also verified on Windows (Comment 20), Marking it as verified!
Status: RESOLVED → VERIFIED
status-firefox43: fixed → verified
You need to log in before you can comment on or make changes to this bug.