885093 - SimplePush: Firefox desktop changes

Status: NEW

(Firefox :: General, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Permission prompt doorhanger

SimplePush patches that affect Firefox Desktop UI or components.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Page Info tab entry for Push permissions

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Enable/Disable Preferences UI

This adds a easy way for users to enable Push in release builds. The same can be done via about:config, so I don't know if we should be setting a precedent for toggling preferences via the Preferences dialog. This does emulate how chrome allows features to be enabled via about:flags in a nicer UI.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

How is a user supposed to know what "Allow websites to use Push Notifications" means? What do push notifications look like on desktop? Is there a support document that describes what this feature is?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

I'm not sure if you've already discussed this with Madhava, but rather than requesting ui-review on a series of patches, it's probably more effective to just sit down with someone from UX and talk it through directly.

Comment 5

[Doug Turner (:dougt)]

:gavin  good idea.  we'll work with webdev to build something similar to https://www.mozilla.org/en-US/firefox/geolocation/

:nsm, can you file a bug -- assign to me.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

:dougt, Filed the bug. IMHO the UI option in Preferences is an odd man out, and we should stick to just having an about:config pref flip option instead.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to Nikhil Marathe from comment #6)
> :dougt, Filed the bug. IMHO the UI option in Preferences is an odd man out,
> and we should stick to just having an about:config pref flip option instead.

Seems reasonable to me - I don't really see why someone would want to disable entirely given that it already has a per-site opt-in.

Comment 8

[Doug Turner (:dougt)]

>  I don't really see why someone would want to disable entirely given that it already has a per-site opt-in

it follows what we did with geo.  Also, it allows you to disable even for the sites you have already opt-ed in for.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Geolocation doesn't have preferences UI for a global toggle AFAIK.

Comment 10

[Florian Bender]

There is a defaut setting switch in about:permissions, iirc. It's not directly user-visible, though it will be at some point. 

However, this is no global switch.

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Permission prompt doorhanger

Minor formatting cleanup.

ui-review=madhava

Gavin can you review both of these please?

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 768411 [details] [diff] [review]
Permission prompt doorhanger

Looks fine, but I think this needs a "Learn more" link, and I think you should sit down with a UX person to make sure these strings make sense.

Comment 13

[Doug Turner (:dougt)]

+1 on the more info.  talking to webdev Wednesday about content.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Getting content on SUMO is probably the best bet. I would talk to mverdi and get an article with an in-product SUMO link alias set up (see app.support.baseURL).

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Page Info for Push permissions

Rebased patch onto changes from 889835 to show entry in pageinfo popup, pageinfo permissions tab and about:permissions.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 774196 [details] [diff] [review]
Page Info for Push permissions

>--- a/browser/locales/en-US/chrome/browser/sitePermissions.properties
>+++ b/browser/locales/en-US/chrome/browser/sitePermissions.properties
>@@ -11,9 +11,9 @@ permission.cookie.label = Set Cookies
> permission.desktop-notification.label = Show Notifications
> permission.image.label = Load Images
> permission.install.label = Install Add-ons
> permission.popup.label = Open Pop-up Windows
> permission.geo.label = Access Your Location
> permission.indexedDB.label = Maintain Offline Storage
> permission.fullscreen.label = Enter Fullscreen
> permission.pointerLock.label = Hide the Mouse Pointer
>-
>+permission.push.label = Receive Push Notifications

I don't think this string makes sense. If you look at the other strings, you'll notice that it's supposed to describe what a web site can do. But web sites don't receive those notifications, they send them, as I understand it.

(It's great to see how little you had to do for SitePermissions.jsm and slightly disturbing to see the about:permissions stuff next to it. about:permissions is half-abandoned and I hope we can get rid of it soon.)

Comment 17

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Dão Gottwald [:dao] from comment #16)
> Comment on attachment 774196 [details] [diff] [review]
> Page Info for Push permissions
> 
> >--- a/browser/locales/en-US/chrome/browser/sitePermissions.properties
> >+++ b/browser/locales/en-US/chrome/browser/sitePermissions.properties
> >@@ -11,9 +11,9 @@ permission.cookie.label = Set Cookies
> > permission.desktop-notification.label = Show Notifications
> > permission.image.label = Load Images
> > permission.install.label = Install Add-ons
> > permission.popup.label = Open Pop-up Windows
> > permission.geo.label = Access Your Location
> > permission.indexedDB.label = Maintain Offline Storage
> > permission.fullscreen.label = Enter Fullscreen
> > permission.pointerLock.label = Hide the Mouse Pointer
> >-
> >+permission.push.label = Receive Push Notifications
> 
> I don't think this string makes sense. If you look at the other strings,
> you'll notice that it's supposed to describe what a web site can do. But web
> sites don't receive those notifications, they send them, as I understand it.
> 

Webservers send the notification, but it is a web page (although 'invisible', see 868322 and 857464) in the browser that receives the notification.

Comment 18

[Dão Gottwald [:dao]]

(In reply to Nikhil Marathe [:nsm] from comment #17)
> > >+permission.push.label = Receive Push Notifications
> > 
> > I don't think this string makes sense. If you look at the other strings,
> > you'll notice that it's supposed to describe what a web site can do. But web
> > sites don't receive those notifications, they send them, as I understand it.
> > 
> 
> Webservers send the notification, but it is a web page (although
> 'invisible', see 868322 and 857464) in the browser that receives the
> notification.

If the web page is invisible to users, doesn't that make it an implementation detail? I'm not sure users will understand what you're trying to tell them here.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 774196 [details] [diff] [review]
Page Info for Push permissions

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

The code looks fine (though I'm not currently in a position to test it). I agree with Dao that the language might need some tweaks (see below).

::: browser/locales/en-US/chrome/browser/preferences/aboutPermissions.dtd
@@ +42,5 @@
>  <!ENTITY popup.label                     "Open Pop-up Windows">
>  
>  <!ENTITY fullscreen.label                "Fullscreen">
> +
> +<!ENTITY push.label                      "Receive Push Notifications">

I agree with Dao that from a user's perspective, this might be a bit confusing.

From a user's perspective, websites are not given permission to *receive* push notifications - they're given permission to *show* push notifications. Perhaps "Show Push Notifications" would be a little clearer for the average user.

::: browser/locales/en-US/chrome/browser/sitePermissions.properties
@@ +15,5 @@
>  permission.geo.label = Access Your Location
>  permission.indexedDB.label = Maintain Offline Storage
>  permission.fullscreen.label = Enter Fullscreen
>  permission.pointerLock.label = Hide the Mouse Pointer
> +permission.push.label = Receive Push Notifications

Same as above.

Comment 20

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

One reason I'm shying away from "Show Push Notifications" is that its similar to "Show Desktop Notifications", but both have completely different purposes. A push notification receiver is like a 'background service' in native apps. Granted the web doesn't have a good equivalent yet so users may not be aware of it. In such a scenario, what would be the best way to ask for permission from the user without confusing anyone too much? How about "Allow background activity?" or similar.

:dolske also wished to add some icon or similar when such background activity is happening (i.e. a page is loaded in the background since it got a notification).

Comment 21

[Jennifer Morrow [:Boriss] (UX)]

I'm happy to do a ui review here, but I'll need a screenshot or (even better) a build to try.  Thanks!