809085 - permissions UI for social content

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Florian Quèze [:florian]]

When working on the WebRTC in Social API demo (https://github.com/anantn/socialapi-demo) we had to completely turn off the permission UI with the media.navigator.permission.disabled preference because the UI didn't appear when requesting access to the camera/microphone from a SocialAPI iframe. 

For reference, the permission UI for regular web pages has been added in bug 729522.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: geolocatoin permission

I just wanted to see what it would take to get a permission dialog working against a social panel, this patch illustrates getting geolocation working.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Sorry for the bug-spam.  This is a first cut at bugs that need (or potentially need) a change to the social api to be implemented appropriately.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

It's not at all clear this does require an API change, so clearing that flag until we have more info

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #3)
> It's not at all clear this does require an API change, so clearing that flag
> until we have more info

this is not an api change, but rather fixing the permissions dialogs (which there are multiple it appears) so they can work with social content.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: WIP permissions patch

This patch adds support for the webrtc permissions and offline apps.  

we probably need to figure out a better mechanism for selecting the notifications anchor, having each permission make that selection is error prone.  

offline apps are using the notificationbox and this patch adds the use of the popupnotifications for social content, that area could also use refactoring.

Comment 6

[Shane Caraveo (:mixedpuppy)]

We also need to think about how to re-view cancelled permission panels.  Maybe have a 'permissions' menuitem under the toolbar button.  could use ux feedback here.  image coming for how this currently looks.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: perms panel.png

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 696361 [details] [diff] [review]
WIP permissions patch

>--- a/browser/modules/webrtcUI.jsm
>+++ b/browser/modules/webrtcUI.jsm
>@@ -125,12 +125,16 @@ function prompt(aBrowser, aCallID, aAudi
>     callback: function () {
>       Services.obs.notifyObservers(null, "getUserMedia:response:deny", aCallID);
>     }
>   }];
> 
>   let options = {
>   };
> 
>+  var anchor = "webRTC-notification-icon";
>+  if (aBrowser.getAttribute("origin"))
>+      anchor = "social-provider-button";
>+
>   chromeWin.PopupNotifications.show(aBrowser, "webRTC-shareDevices", message,
>-                                    "webRTC-notification-icon", mainAction,
>+                                    anchor, mainAction,
>                                     secondaryActions, options);
> }

I'm not sure this is due to the WIP nature of the patch, but this reads very cryptic. It's not clear to me what the origin attribute means and what its presence has to do with that social-provider-button anchor.

I'd also if this and similar code didn't have to know about social content specifically at all. Can we get a generic solution? Something like a popupnotificationanchor attribute on the browser?

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to Dão Gottwald [:dao] from comment #8)
> Comment on attachment 696361 [details] [diff] [review]
> WIP permissions patch

> I'm not sure this is due to the WIP nature of the patch, but this reads very
> cryptic. It's not clear to me what the origin attribute means and what its
> presence has to do with that social-provider-button anchor.

an origin attribute is attached to all social content iframes.  I do have it in mind to do something better later, perhaps a social-frame class.

> I'd also if this and similar code didn't have to know about social content
> specifically at all. Can we get a generic solution? Something like a
> popupnotificationanchor attribute on the browser?

excellent idea, that could be done easily enough.  I'll take a look at that.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: WIP permissions patch 1.1

implementation with suggestion from :dao

Comment 11

[Mark Banner (:standard8)]

I've recently been testing this. What I did notice was that if the permissions UI was accidentally dismissed (click somewhere else on the browser window), then there was no apparent way to get back to the dialog for the social content.

Comment 12

[Florian Quèze [:florian]]

(In reply to Mark Banner (:standard8) from comment #11)
> I've recently been testing this. What I did notice was that if the
> permissions UI was accidentally dismissed (click somewhere else on the
> browser window), then there was no apparent way to get back to the dialog
> for the social content.

Is this specific to the social content, or does it also happen for the media permissions popup of a regular web page?

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> (In reply to Mark Banner (:standard8) from comment #11)
> > I've recently been testing this. What I did notice was that if the
> > permissions UI was accidentally dismissed (click somewhere else on the
> > browser window), then there was no apparent way to get back to the dialog
> > for the social content.
> 
> Is this specific to the social content, or does it also happen for the media
> permissions popup of a regular web page?

The patch is generally generic, allowing a non-browser tab iframe to define an element to use for the anchor of the permissions panel.  It does not however define how to get back the prompt if you dismiss it, still need to figure that out.

Comment 14

[Shane Caraveo (:mixedpuppy)]

related bug report Bug 828269

Comment 15

[Jason Smith [:jsmith]]

Not blocking for v1 FF 20 release of gUM.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Per the attached image, it is easy enough to link the panel to the social toolbarbutton.  We need to figure out UX around these permissions panels for social content.  The primary issue is, if the panel is dismissed, how does a user get the panel back?  This has been on my mind and I'm not coming up with any stellar ideas.  I can think of two possibilities when there is an unanswered permission request:

- add some kind of "permissions dropdown button" to the toolbarbutton 
- add a menuitem under the toolbarbutton

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 696425 [details] [diff] [review]
WIP permissions patch 1.1

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

::: browser/base/content/browser.js
@@ +1066,5 @@
>                                OfflineApps, false);
> +    // listen for offline apps on social
> +    let SocialBrowser = document.getElementById("social-sidebar-browser");
> +    SocialBrowser.addEventListener("MozApplicationManifest",
> +                              OfflineApps, false);

unconventional capitalization of local variable here.

I'm assuming the patch is missing adding this listener for the chat frames, right?

@@ +5784,5 @@
>        }
>      }
>  
> +    // is this from a non-tab browser/iframe?
> +    browsers = document.querySelectorAll("iframe[popupnotificationanchor]");

why not do these two querySelectorAll as only one
"iframe[popupnotificationanchor]|browser[popupnotificationanchor]" ?

@@ +5810,5 @@
>  
> +    var popupAnchor = aBrowser.getAttribute("popupnotificationanchor");
> +    var notificationID = "offline-app-usage";
> +    var notificationBox, notification;
> +    if (popupAnchor) {

the duality of the notificationbox/anchor in this function (and the next) will make this code write-only IMO (it will be very hard to tell why these if conditions exist just by looking at them).

I think a cleaner way to go here would be to augment gBrowser.getNotificationBox or create a new function (either in this file or in tabbrowser, wherever it feels a better fit) to encapsulate both "getNotificationBox || getElementById(#anchorName)", and then check in this function the node type of the returned element and proceed properly.

Something like

"""
let notif = getNotificationTarget(aBrowser);

if (notif.nodeName == "notificationbox") {
 // do notificationbox stuff
} else {
 // do PopupNotification stuff
}
"""

Comment 18

[Shane Caraveo (:mixedpuppy)]

Attachment: WIP permissions patch 1.2

patch refresh, still considering how best to handle showing notifications that were dismissed.

Comment 19

[Mark Hammond [:markh] [:mhammond]]

Given this isn't a very high priority, we should make this depend on bug 588305 and bug 857868 to reduce the complexity of the patch.  If those bugs stagnate we can revisit this.

Comment 20

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Retrieving a dismissed permissions notification via the menu

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> We also need to think about how to re-view cancelled permission panels. 
> Maybe have a 'permissions' menuitem under the toolbar button.  could use ux
> feedback here.  image coming for how this currently looks.

Using the menu makes a lot of sense, since that's where the dismissed permissions panel will originate from.  We originated dismissed panels from the same place it originated from in site-level dialogs also.  However, one toggleable menu item won't be quite enough, since there's more than one option in that permissions dialog. 

In the attached mockup, a new menu item appears under the user's identity: "Share camera with [SITE]."  That's phrased consistently with the other items in that menu.  However, clicking that item does not just toggle the camera on, but bring up the original dialog.  The con is that it's an extra step the user may not expect, but the pro is they are given the same dialog they dismissed, complete with the ability to switch devices & times the permission is granted.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Attachment: sidebar permissions.png

current state of permission panel (including bug 857868 patch) when sidebar requests more than one permission.

Comment 22

[Shane Caraveo (:mixedpuppy)]

Boris, fixing the issues displayed in Comment 21, as well as the general issue if hooking into a bunch of places to catch permissions and create menuitems, leads me to feel that adding icons to the toolbarbutton would be simpler and work better with the current permissions system.  Thoughts?

Comment 23

[Shane Caraveo (:mixedpuppy)]

Attachment: sidebar request for permission

Comment 24

[Shane Caraveo (:mixedpuppy)]

Attachment: chat window request for permission

Comment 25

[Shane Caraveo (:mixedpuppy)]

Attachment: permissions with icons patch

This patch moves to using an icon/button for the permissions anchor.  You can see the effect in the previous image attachments.  The button only appears when permissions have been requested, and I am limiting to one icon, thus the info icon (happy to use something better).  

Realistically, the permissions panel needs an overhaul to make this easier, but this is the minimal patch I could do to get permissions working correctly with social frames.  There is a chunk of background behind this that would be easier to communicate in person (or vidyo).

Comment 26

[Shane Caraveo (:mixedpuppy)]

Attachment: permissions with icons patch

patch sets the appropriate icon for the permission icon in the common case, when there is only one permission request.  When there are multiple permission requests at the same time, the default icon is used.

This works really nice for webrtc permissions.  The first notification panel is the request for permissions, then a second notification is done that shows the green camera.

Comment 27

[Shane Caraveo (:mixedpuppy)]

Attachment: sidebar geolocation.png

Comment 28

[Shane Caraveo (:mixedpuppy)]

Attachment: chat window request for permission

Comment 29

[Shane Caraveo (:mixedpuppy)]

Attachment: chat window camera accepted.png

Comment 30

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Bumping video permission icon in chat window to the left two pixels

Summing up a discussion between Shane and me - it's pretty obvious that the permission notification displaying from the toolbar (attachment 760987 [details]) and chat window (attachment 760989 [details]) is not ideal behavior.  That permission panel was designed for the Firefox site identity button, and thus was designed to be large because the space available was large.  In SocialAPI - and in future projects - we need to be able to ask users for permissions in ways better integrated with the entity asking for the permission.  However, changing the permission panels is going to mean substantial refactoring of Firefox's code: very worth doing, but not worth blocking SocialAPI for a long time waiting for.  So, I'm going to r+ this behavior, but with the caveat that it's not idea (and that's putting it lightly), and file another bug to fix the overall problem of permissions.

For now: attachment 760992 [details] needs that video icon bumped to the left two pixels (see attachment).  The other two are <s>good</s> OK to go.

Comment 31

[Jennifer Morrow [:Boriss] (UX)]

Comment on attachment 760992 [details]
chat window camera accepted.png

Needs 2 pixel nudge left, then fine, see comment 30 and attachment 762107 [details]

Comment 32

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 760663 [details] [diff] [review]
permissions with icons patch

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

On the right track, but I don't like how complicated it's making PopupNotifications.jsm, specially with the two added cases:
>if (anchor) {
>  if (anchor instanceof Ci.nsIDOMXULElement) {
>    anchorElement = anchor;
>  } else {
>    anchorElement = iconBox.ownerDocument.getElementById(anchor);
>  }
>}

when does this difference happen (toolbar vs. chat frame?), and can't this be merged somehow in something that can handle both?

The PopupNotifications._update is the most concerning.. If there's no easy way to simplify that we'll need to add some comments on not only what that code does, but why is that behavior necessary

::: browser/base/content/browser.js
@@ +5673,5 @@
>        label: gNavigatorBundle.getString("offlineApps.manageUsage"),
>        accessKey: gNavigatorBundle.getString("offlineApps.manageUsageAccessKey"),
>        callback: OfflineApps.manage
>      };
> +    var popupAnchor = aBrowser.getAttribute("popupnotificationanchor") || "indexedDB-notification-icon";

move this change to below and remove the let anchorId = ".." line.

::: browser/base/content/socialchat.xml
@@ +25,5 @@
>  
>      <implementation implements="nsIDOMEventListener">
>        <constructor><![CDATA[
>          let Social = Components.utils.import("resource:///modules/Social.jsm", {}).Social;
> +        this.iframe.popupnotificationanchor = document.getAnonymousElementByAttribute(this, "id", "notification-icon");

does this need to be attached to the iframe, and not simply this.popupnotificationanchor? If not, it'd be nice to make this simply a getter function in the binding instead of keeping the reference

::: toolkit/modules/PopupNotifications.jsm
@@ +395,5 @@
>      if (index == -1)
>        return;
>  
> +    if (notification.anchorElement.parentNode != this.iconBox ||
> +        notification.browser == this.tabbrowser.selectedBrowser)

should also do the same replacement here to use .isActive?

@@ +714,2 @@
>  
>    _reshowNotificationForAnchor: function PopupNotifications_reshowNotificationForAnchor(anchor) {

can you rewrite this one to reuse the new _reshowNotificationForBrowser?

Comment 33

[Shane Caraveo (:mixedpuppy)]

(In reply to :Felipe Gomes from comment #32)
> Comment on attachment 760663 [details] [diff] [review]
> permissions with icons patch
> 
> Review of attachment 760663 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> On the right track, but I don't like how complicated it's making
> PopupNotifications.jsm, specially with the two added cases:
> >if (anchor) {
> >  if (anchor instanceof Ci.nsIDOMXULElement) {
> >    anchorElement = anchor;
> >  } else {
> >    anchorElement = iconBox.ownerDocument.getElementById(anchor);
> >  }
> >}
> 
> when does this difference happen (toolbar vs. chat frame?), and can't this
> be merged somehow in something that can handle both?

I'm thinking about this more.  the problem is that one of the notification icons is an anonymous node in xbl, thus allowing nsIDOMXULElement here.  In other area's, it's easier if this is an attribute.

> The PopupNotifications._update is the most concerning.. If there's no easy
> way to simplify that we'll need to add some comments on not only what that
> code does, but why is that behavior necessary

Yeah, it may be more than what is desired, but it really was the shortest path I could find to making notifications work.  Ultimately the notifications code needs a good refactor and should be detached from gBrowser assumptions.

> ::: browser/base/content/browser.js
> @@ +5673,5 @@
> >        label: gNavigatorBundle.getString("offlineApps.manageUsage"),
> >        accessKey: gNavigatorBundle.getString("offlineApps.manageUsageAccessKey"),
> >        callback: OfflineApps.manage
> >      };
> > +    var popupAnchor = aBrowser.getAttribute("popupnotificationanchor") || "indexedDB-notification-icon";
> 
> move this change to below and remove the let anchorId = ".." line.

Simply removed, the Notifications class handles this now.

> ::: browser/base/content/socialchat.xml
> @@ +25,5 @@
> >  
> >      <implementation implements="nsIDOMEventListener">
> >        <constructor><![CDATA[
> >          let Social = Components.utils.import("resource:///modules/Social.jsm", {}).Social;
> > +        this.iframe.popupnotificationanchor = document.getAnonymousElementByAttribute(this, "id", "notification-icon");
> 
> does this need to be attached to the iframe, and not simply
> this.popupnotificationanchor? If not, it'd be nice to make this simply a
> getter function in the binding instead of keeping the reference

Yeah, it needs to be on the iframe.  I made it a getter.

I'll post a new patch that should take care of a bunch of the feedback, others I'm uncertain of better ways to handle this.

Comment 34

[Shane Caraveo (:mixedpuppy)]

Attachment: permissions with icons patch

Comment 35

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=350d1cbd194d

Comment 36

[Shane Caraveo (:mixedpuppy)]

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

Comment 37

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b96decc34910