permissions UI for social content

RESOLVED FIXED in Firefox 24

Status

()

Firefox
SocialAPI
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: florian, Assigned: mixedpuppy)

Tracking

({uiwanted})

Trunk
Firefox 24
uiwanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [getUserMedia][blocking-gum-])

Attachments

(8 attachments, 8 obsolete attachments)

51.22 KB, image/png
Details
546.33 KB, image/png
Details
68.25 KB, image/png
Details
39.91 KB, image/png
Boriss
: ui-review+
Details
71.89 KB, image/png
Boriss
: ui-review+
Details
24.79 KB, image/png
Boriss
: ui-review-
Details
44.01 KB, image/png
Details
29.82 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 681249 [details] [diff] [review]
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.
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.
Whiteboard: [needs-api-change]
It's not at all clear this does require an API change, so clearing that flag until we have more info
Whiteboard: [needs-api-change]
(Assignee)

Comment 4

5 years ago
(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.
(Assignee)

Updated

5 years ago
Summary: Permission UI for WebRTC navigator.getUserMedia() in SocialAPI floating chat windows → permissions UI for social content
Whiteboard: [webrtc]
(Assignee)

Comment 5

5 years ago
Created attachment 696361 [details] [diff] [review]
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.
Attachment #681249 - Attachment is obsolete: true
Attachment #696361 - Flags: feedback?(gavin.sharp)
(Assignee)

Updated

5 years ago
Severity: enhancement → normal
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Created attachment 696362 [details]
perms panel.png
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?
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
Created attachment 696425 [details] [diff] [review]
WIP permissions patch 1.1

implementation with suggestion from :dao
Attachment #696361 - Attachment is obsolete: true
Attachment #696361 - Flags: feedback?(gavin.sharp)
Attachment #696425 - Flags: feedback?(gavin.sharp)
Attachment #696425 - Flags: feedback?(dao)
(Assignee)

Updated

5 years ago
Assignee: nobody → mixedpuppy
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.
(Reporter)

Comment 12

5 years ago
(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?
(Assignee)

Comment 13

5 years ago
(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.
Attachment #696425 - Flags: feedback?(gavin.sharp) → feedback?(felipc)
(Assignee)

Comment 14

5 years ago
related bug report Bug 828269
Whiteboard: [webrtc] → [getUserMedia][webrtc]
Not blocking for v1 FF 20 release of gUM.
Whiteboard: [getUserMedia][webrtc] → [getUserMedia][blocking-gum-]
(Assignee)

Comment 16

5 years ago
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
Whiteboard: [getUserMedia][blocking-gum-] → [getUserMedia][blocking-gum-][needs-ux]
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
}
"""
Attachment #696425 - Flags: feedback?(felipc) → feedback+
(Assignee)

Comment 18

5 years ago
Created attachment 727860 [details] [diff] [review]
WIP permissions patch 1.2

patch refresh, still considering how best to handle showing notifications that were dismissed.
Attachment #696425 - Attachment is obsolete: true
Attachment #696425 - Flags: feedback?(dao)
(Assignee)

Updated

5 years ago
Keywords: uiwanted
Whiteboard: [getUserMedia][blocking-gum-][needs-ux] → [getUserMedia][blocking-gum-]
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.
Depends on: 588305, 857868
(Reporter)

Updated

5 years ago
Blocks: 859886
Created attachment 753584 [details]
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.
(Assignee)

Comment 21

5 years ago
Created attachment 754595 [details]
sidebar permissions.png

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

Comment 22

5 years ago
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?
Flags: needinfo?(jboriss)
(Assignee)

Comment 23

5 years ago
Created attachment 759544 [details]
sidebar request for permission
Attachment #759544 - Flags: ui-review?(jboriss)
(Assignee)

Comment 24

5 years ago
Created attachment 759545 [details]
chat window request for permission
Attachment #759545 - Flags: ui-review?(jboriss)
(Assignee)

Comment 25

5 years ago
Created attachment 759548 [details] [diff] [review]
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).
Attachment #759548 - Flags: feedback?(gavin.sharp)
Attachment #759548 - Flags: feedback?(felipc)
(Assignee)

Comment 26

5 years ago
Created attachment 760663 [details] [diff] [review]
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.
Attachment #759548 - Attachment is obsolete: true
Attachment #759548 - Flags: feedback?(gavin.sharp)
Attachment #759548 - Flags: feedback?(felipc)
Attachment #760663 - Flags: review?(felipc)
(Assignee)

Comment 27

5 years ago
Created attachment 760987 [details]
sidebar geolocation.png
Attachment #759544 - Attachment is obsolete: true
Attachment #759544 - Flags: ui-review?(jboriss)
Attachment #760987 - Flags: ui-review?(jboriss)
(Assignee)

Comment 28

5 years ago
Created attachment 760989 [details]
chat window request for permission
Attachment #759545 - Attachment is obsolete: true
Attachment #759545 - Flags: ui-review?(jboriss)
Attachment #760989 - Flags: ui-review?(jboriss)
(Assignee)

Comment 29

5 years ago
Created attachment 760992 [details]
chat window camera accepted.png
Attachment #760992 - Flags: ui-review?(jboriss)
Created attachment 762107 [details]
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.
Flags: needinfo?(jboriss)
Attachment #760987 - Flags: ui-review?(jboriss) → ui-review+
Attachment #760989 - Flags: ui-review?(jboriss) → ui-review+
Comment on attachment 760992 [details]
chat window camera accepted.png

Needs 2 pixel nudge left, then fine, see comment 30 and attachment 762107 [details]
Attachment #760992 - Flags: ui-review?(jboriss) → ui-review-
(Assignee)

Updated

5 years ago
Blocks: 883346
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?
Attachment #760663 - Flags: review?(felipc) → feedback+
(Assignee)

Updated

5 years ago
Depends on: 880911
(Assignee)

Comment 33

5 years ago
(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.
(Assignee)

Comment 34

5 years ago
Created attachment 765704 [details] [diff] [review]
permissions with icons patch
Attachment #727860 - Attachment is obsolete: true
Attachment #760663 - Attachment is obsolete: true
Attachment #765704 - Flags: review?(felipc)
Attachment #765704 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/b96decc34910
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24

Updated

5 years ago
Depends on: 896314
(Reporter)

Updated

5 years ago
Depends on: 943292
You need to log in before you can comment on or make changes to this bug.