885366 - Show granted and refused site permissions in site identity panel

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jennifer Morrow [:Boriss] (UX)]

To improve the site identity button in Firefox, we should expose relevant information to users about their relationship with the site.  Perhaps the most relevant information are what permissions the site has asked for and been granted or refused.  Particularly important is the ability for users to revisit permission notifications they have seen before, which is not current possible without visiting about:permissions, which no one does (or should, until it's been developed further).

Comment 1

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: States of the site identity button with permissions widgets

Attached are four states of the site identity button.  1 Shows what would be viewed in most cases - a non-encrypted site that has asked for no permissions.  Once a single permission has been asked, a “Permissions” section is added to the site identity window, which is shown in 2-4.  After 7 permissions have been asked for, the rest are viewable via a disclosure button.

Comment 3

[Dão Gottwald [:dao]]

Attachment: wip

Comment 4

[Dão Gottwald [:dao]]

Attachment: screenshot

Boriss, is this ok as a first step? This screenshot shows how the popup would look like for an encrypted connection with three permissions set.

I don't know if you expected me to overhaul the popup's identity section in this bug. Your mockup gives me that impression, but I think we should handle that separately.

I added the permissions stuff *under* the "more information" button, as that button opens the Page Info window with the Security tab selected, which is unrelated to permissions.

Your mockup has strings like "Location is not shared with this site" and a button to change that setting ("Share Location"). When clicking that button, would the label and the button change to "Location is shared with this site" and "Stop Sharing Location", respectively? This seems a bit weird. In my patch, I've opted for labels that don't spell out the current state and a menulist instead of a button. The menulist exposes the current state and allows changing it. This also allowed me to use much shorter labels, which is probably better especially for languages that tend to require more space than English does.

Comment 5

[Dão Gottwald [:dao]]

Also, should we provide a way for users to clear a setting such that it wouldn't appear any longer in the popup? When a site asks for that permission again and the user grants or denies that requests, the permission would reappear in the identity popup.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 768917 [details] [diff] [review]
wip

Tentatively requesting review in the hope that Boriss is mostly fine with this. Not sure who has review cycles...

Plugin permissions are missing from this patch. Bug 880735 added a separate UI to handle those. We'll want to unify this stuff at some point.

I don't know if we'll want Page Info's Permissions tab or about:permissions to exist much longer, but if so, I tried to design SitePermissions.jsm such that it could be used for them as well, with some extra work.

Comment 7

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Dão Gottwald [:dao] from comment #4)
> Created attachment 768926 [details]
> screenshot
> 
> Boriss, is this ok as a first step? This screenshot shows how the popup
> would look like for an encrypted connection with three permissions set.

Pretty solid first step, yes.  Some visual tweaks are needed.  The "More Information" button is a bit out of place with more info below it.  Let's move that button to the bottom of the window to visually link it to permissions.  

> I don't know if you expected me to overhaul the popup's identity section in
> this bug. Your mockup gives me that impression, but I think we should handle
> that separately.

That's the intention, but I agree, let's handle that separately in another bug.

> I added the permissions stuff *under* the "more information" button, as that
> button opens the Page Info window with the Security tab selected, which is
> unrelated to permissions.

I wouldn't say it's unrelated.  The Privacy and History section includes two related categories to permissions (cookies & password saving).  Frankly, permissions themselves and security info are intrinsically linked.  As I said above, as a temporary step until we have better Page Info and window view, let's move the More Info to the bottom.

> Your mockup has strings like "Location is not shared with this site" and a
> button to change that setting ("Share Location"). When clicking that button,
> would the label and the button change to "Location is shared with this site"
> and "Stop Sharing Location", respectively? This seems a bit weird. In my
> patch, I've opted for labels that don't spell out the current state and a
> menulist instead of a button. The menulist exposes the current state and
> allows changing it. This also allowed me to use much shorter labels, which
> is probably better especially for languages that tend to require more space
> than English does.

For the sake of simplicity (and shipping), let's keep keep this as it is in your patch for now.  Redoing a lot of strings to reference current state is another project.  Ultimately, giving the string in this menu to tell users their state while showing the toggleable options identical to the permissions panel they originally saw is probably the way to go.

Comment 8

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Dão Gottwald [:dao] from comment #5)
> Also, should we provide a way for users to clear a setting such that it
> wouldn't appear any longer in the popup? When a site asks for that
> permission again and the user grants or denies that requests, the permission
> would reappear in the identity popup.

We don't need a way to clear, but the second part is buggy behavior.  If the site asks for the permission again, that permission should go to the top of the list like a new permission would - but the old entry should disappear.  Does your patch prevent duplicate items?

Comment 9

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Tweaks to current patch, more info on bottom, spacings consistent

The attached shows what Comment 7 refers to and subsequently my r=me:
- More Information is at the bottom, aligned with other Permissions buttons
- Grey background highlights More Information as separate from Permissions buttons
- Spacings consistent.  Line items and new sections all currently slightly differently spaced

Comment 10

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Tweaks to current patch, more info on bottom, spacings consistent (

(Same mockup as png)

Comment 11

[Dão Gottwald [:dao]]

Attachment: patch

ui-review comments addressed

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch v2

fixed the blur event handling to deal with multiple focusable elements in the panel

Comment 13

[Dão Gottwald [:dao]]

Attachment: patch v3

Updated the CSS selector to hide the "more information" button for chrome pages.

Also removed SitePermission's "remove" method since it would currently be unused.

Added some documentation.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 769637 [details] [diff] [review]
patch v3

>-          <!-- Footer button to open security page info -->
>-          <hbox id="identity-popup-button-container" pack="end">
>-            <button id="identity-popup-more-info-button"
>-                    label="&identity.moreInfoLinkText;"
>-                    onblur="gIdentityHandler.hideIdentityPopup();"
>-                    oncommand="gIdentityHandler.handleMoreInfoClick(event);"/>
>-          </hbox>
...
>+      <!-- Footer button to open security page info -->
>+      <hbox id="identity-popup-button-container" pack="end">
>+        <button id="identity-popup-more-info-button"
>+                label="&identity.moreInfoLinkText;"
>+                oncommand="gIdentityHandler.handleMoreInfoClick(event);"/>
>+      </hbox>

Clicking on the More Information button shows the Page Info dialog, but since the onblur event handler has been removed the popup stays visible.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

I see where the blur event handler was moved to, but the identity popup is remaining as the active element, and the Page Info dialog is appearing between the browser and the identity popup. Might be easiest to just call hidePopup when the command for the More Information button is run.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 769637 [details] [diff] [review]
patch v3

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

I like that the SitePermissions.jsm module is not loaded until the popup is going to be opened.

All other changes look good, so f+ until the popup issue is fixed.

Comment 17

[Dão Gottwald [:dao]]

Attachment: patch v4

Comment 18

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/add063e3ff2b

Comment 19

[Phil Ringnalda (:philor)]

Sorry, backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e82910515c85 because that had two things with id="identity-popup-permissions" and I wasn't sure what the id for the other one was supposed to be.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Repushed with the id-on-the-label removed,
https://hg.mozilla.org/integration/mozilla-inbound/rev/83a2966d94fb

Comment 21

[Dão Gottwald [:dao]]

(In reply to Phil Ringnalda (:philor) from comment #19)
> Sorry, backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e82910515c85 because
> that had two things with id="identity-popup-permissions" and I wasn't sure
> what the id for the other one was supposed to be.

I suppose you didn't just file a bug on it because you expected this typo to break something in a bad way? It didn't, nothing actually enforces that ids are unique in a document.

(In reply to Jared Wein [:jaws] from comment #20)
> Repushed with the id-on-the-label removed,
> https://hg.mozilla.org/integration/mozilla-inbound/rev/83a2966d94fb

Thanks, but something went wrong with the encoding. You're probably not using UTF-8 in your console.

Comment 22

[Phil Ringnalda (:philor)]

No, I didn't just file a bug on it because I noticed it due to browser_duplicateIDs.js, the browser-chrome test which fails if we have duplicate IDs and thus does enforce unique IDs.

Comment 23

[Dão Gottwald [:dao]]

(In reply to Phil Ringnalda (:philor) from comment #22)
> No, I didn't just file a bug on it because I noticed it due to
> browser_duplicateIDs.js, the browser-chrome test which fails if we have
> duplicate IDs and thus does enforce unique IDs.

Oh, alright, I was looking at the wrong changeset on tbpl...

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/83a2966d94fb

Comment 25

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Dão Gottwald [:dao] from comment #11)
> Created attachment 769406 [details] [diff] [review]
> patch
> 
> ui-review comments addressed

Could you possibly post a screenshot and/or build?

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #25)
> (In reply to Dão Gottwald [:dao] from comment #11)
> > Created attachment 769406 [details] [diff] [review]
> > patch
> > 
> > ui-review comments addressed
> 
> Could you possibly post a screenshot and/or build?

You can test it in tomorrow's Nightly build. Please file new bugs for any issues that you find while testing.

Comment 27

[Florian Bender]

(In reply to Jared Wein [:jaws] from comment #26)
> You can test it in tomorrow's Nightly build. Please file new bugs for any
> issues that you find while testing.

Do I need to enable this feature somewhere? My Nightly 25.0a1 (2013-07-10) does not show me any permissions. I tried by visiting maps.google.com and sharing my location: Identitiy doorhanger did not change at all. I'm on Mac OS X 10.8.4.

Comment 28

[Dão Gottwald [:dao]]

(In reply to Florian Bender from comment #27)
Only permanent permissions are handled here, e.g. if you select "Always Share Location" or "Never Share Location". The decision to grant or refuse a permission only once can't be revoked.

Comment 29

[Florian Bender]

Yep. 

There's an awful lot of space above the "More information" button (compared to below the button). See screenshot. Looks unbalanced. Also, the space below the permission appears bigger than the the spaces used in the rest of the notification, as suggested in attachment #769382 [details], though I did not measure it (don't have a tool at hand to do that). 

(BTW: Looking forward to the redesign of the doorhanger as indicated in attachment #765397 [details] – is there a bug for that?)

Comment 30

[Florian Bender]

Attachment: current nightly (Mac OS X)

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 773294 [details]
current nightly (Mac OS X)

(In reply to Florian Bender from comment #29)
> Yep. 
> 
> There's an awful lot of space above the "More information" button (compared
> to below the button). See screenshot. Looks unbalanced.

Please file a new bug.

Comment 32

[Bogdan Maris, Desktop QA [Holiday+PTO 1-6 May]]

I saw that "Maintain Offline Storage" does not have the "Allow" option in the identity panel, I noticed that is the default options but other permissions have the default option in the identity panel. Should I file a bug for this?

On Mac there is a lot of white space in the drop down buttons in the right side of "Allow", "Block".

Verified on Mac OS X 10.8, Ubuntu 12.10 x32 and Windows 7 x64.

Comment 33

[Dão Gottwald [:dao]]

(In reply to Bogdan Maris [QA] [:bogdan_maris] from comment #32)
> I saw that "Maintain Offline Storage" does not have the "Allow" option in
> the identity panel, I noticed that is the default options but other
> permissions have the default option in the identity panel. Should I file a
> bug for this?

Bug 890566 covers this.