1193004 - Always show permissions section in the Control Center

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Brian Grinstead [:bgrins]]

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

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1193004-Always-show-permissions-section-in-the-C.patch

Comment 2

[Brian Grinstead [:bgrins]]

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?

Comment 3

[Brian Grinstead [:bgrins]]

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

Comment 4

[Ash Grigas]

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

Comment 5

[Dão Gottwald [:dao]]

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

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 7

[Dão Gottwald [:dao]]

Permissions are generally site-specific, not page-specific, and the string should probably reflect that.

Comment 8

[Ash Grigas]

(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

Comment 9

[Brian Grinstead [:bgrins]]

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"

Comment 10

[Brian Grinstead [:bgrins]]

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

Comment 11

[Brian Grinstead [:bgrins]]

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

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/98f6e793e383

Comment 13

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4664200&repo=fx-team

Comment 14

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/9e23f3276571

Comment 15

[Brian Grinstead [:bgrins]]

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

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Found something that seems to work.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/04cc4ac17e65

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/04cc4ac17e65

Comment 20

[Saheda Reza [:Antora]]

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

Comment 21

[Nazir Ahmed Sabbir [:NaSb]]

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!