1201398 - Web Notification management interface

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Bill Maggs (bmaggs)]

Attachment: Screenshot 2015-09-02 22.28.00.png

There is currently no way for the user to quickly and easily change the permissions related to Push Notifications in FF to permanently or temporarily turn them off. Each message that appears from each different web page must be clicked on, then the user must know enough about FF to navigate to the lock on the url bar to access Control Center. There is no way for the user to see in one place all the sites that are allowed or the ability to change the permission of multiple sites in one interface. The Control Center is first and foremost a per-site interface. 

A simple centralized management interface for Push in the browser can be made as part of the Content Preferences Pane, alongside Pop-up blocking (see attachment).

How would this interface be discovered and accessed in FF 43? A settings cog in each message on FF Windows  (see Bug 1201397) would take the user to the detail list view of a new Push Notifications setting in the Content prefs pane (see attachment for example of view). 

For FF 42 this centralized interface would not be available, but a settings cog could take the user to a the Control Center view of the Push permissions (see attachment), thus allowing at least a one-click way to change the permission for the one site at a time.

Comment 1

[Bill Maggs (bmaggs)]

Attachment: Control Center.png

Comment 2

[Edwin Wong [:edwong]]

Attachment: pn-01.png

PSackl created mocks

Comment 3

[Edwin Wong [:edwong]]

Attachment: pn-03.png

psackl created mocks

Comment 4

[Bill Maggs (bmaggs)]

The centralized management interface design mocked above is targeted for FF 44.

Comment 5

[Matthew N. [:MattN]]

The abbreviation for Firefox is Fx, not FF btw.

Comment 6

[Edwin Wong [:edwong]]

:phlsa - Mattn proposed using the existing controls for managing notification permissions in https://bugzilla.mozilla.org/attachment.cgi?id=8656402 .  There's still a lot of detail work here - but please confirm this is the plan for Fx44 which will help us estimate and set scope.

Comment 7

[Bill Maggs (bmaggs)]

The experience as described in the bug and attachment above is the plan for 44. There will be a Notifications heading of the Content Pane, with subheading that includes a checkbox for a Do Not Disturb  function and a detail view  similar to that for Popups that allows turning off Notifications centrally (across sites). 

In 45, we plan to incorporate a new design for Notifications Management into a Permissions Preferences area that includes all your permissions.

Comment 8

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

Let's move forward with the existing controls for managing notification permissions for v1. I'll start working on this tomorrow.

Comment 9

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

Dialog Title: Exceptions - Notifications
Text: You can specify which websites are allowed to show notifications. Type the exact address of the site you want to allow and then click Allow.
Description text from content page: Choose which sites are allowed to show notifications.
Button text from content page: Choose...

Comment 10

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

Attachment: Patch

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8667483 [details] [diff] [review]
Patch

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

::: browser/components/preferences/in-content/content.js
@@ +93,5 @@
> +   */
> +  showNotificationExceptions()
> +  {
> +    var bundlePreferences = document.getElementById("bundlePreferences");
> +    var params = { blockVisible: false, sessionVisible: false, allowVisible: true,

Why not blockVisible?

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties
@@ +25,5 @@
>  addons_permissions_title=Allowed Sites - Add-ons Installation
>  popuppermissionstext=You can specify which websites are allowed to open pop-up windows. Type the exact address of the site you want to allow and then click Allow.
>  popuppermissionstitle=Allowed Sites - Pop-ups
> +notificationspermissionstext=You can specify which websites are allowed to show notifications. Type the exact address of the site you want to allow and then click Allow.
> +notificationspermissionstitle=Exceptions - Notifications

These should probably change if we show allow and block.

Comment 12

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

Attachment: Patch

Oh sorry, I had meant to come back to that after testing and forgot. Fixed now.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8667638 [details] [diff] [review]
Patch

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

::: browser/components/preferences/in-content/content.js
@@ +93,5 @@
> +   */
> +  showNotificationExceptions()
> +  {
> +    var bundlePreferences = document.getElementById("bundlePreferences");
> +    var params = { blockVisible: true, sessionVisible: false, allowVisible: true,

Nit: Why `var` for new code?

@@ +95,5 @@
> +  {
> +    var bundlePreferences = document.getElementById("bundlePreferences");
> +    var params = { blockVisible: true, sessionVisible: false, allowVisible: true,
> +                   prefilledHost: "", permissionType: "desktop-notification" }
> +    params.windowTitle = bundlePreferences.getString("notificationspermissionstitle");

Nit: When there are many properties on an object literal I prefer one property per line for readability and looking at blame. Then you may not need to do the special handling for the string properties.

::: browser/locales/en-US/chrome/browser/preferences/content.dtd
@@ +7,5 @@
>  <!ENTITY  blockPopups.label           "Block pop-up windows">
>  <!ENTITY  blockPopups.accesskey       "B">
> +
> +<!ENTITY  notificationsPolicy.label            "Notifications">
> +<!ENTITY  notificationsPolicyDesc.label        "Choose which sites are allowed to show notifications.">

Should the period be there? It's not for popups. These should get run by UX/matej at some point if they haven't already.

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties
@@ +25,5 @@
>  addons_permissions_title=Allowed Sites - Add-ons Installation
>  popuppermissionstext=You can specify which websites are allowed to open pop-up windows. Type the exact address of the site you want to allow and then click Allow.
>  popuppermissionstitle=Allowed Sites - Pop-ups
> +notificationspermissionstext=You can specify which websites are always or never allowed to show notifications. Type the exact address of the site you want to manage and then click Block or Allow.
> +notificationspermissionstitle=Exceptions - Notifications

Ditto on UX/matej review on these at some point. I think the "Exceptions" title doesn't apply in this case and perhaps should be replaced with "Permissions"?

Comment 14

[Bill Maggs (bmaggs)]

I think replacing Exceptions with Permissions or even better Change would be much much preferred. We will eventually make the Management interface for fine-grained, and include options not just in the Control Center, so I'm not sure Permissions is future-proof.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3d5175374d15

Comment 16

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

(In reply to Matthew N. [:MattN] (behind on mail) from comment #13)
> Nit: Why `var` for new code?

Because these were top-level variables and didn't get any benefit from using 'let'. I switched them to 'let' since you called it out.

> Nit: When there are many properties on an object literal I prefer one
> property per line for readability and looking at blame. Then you may not
> need to do the special handling for the string properties.

Changed.
 
> Should the period be there? It's not for popups. These should get run by
> UX/matej at some point if they haven't already.

Ran it by Dolske in Matej and Philipp's absence.
 
> Ditto on UX/matej review on these at some point. I think the "Exceptions"
> title doesn't apply in this case and perhaps should be replaced with
> "Permissions"?

Went with "Notification Permissions" as it is more human-sounding (as opposed to robot sounding). IRC conversation with Dolske below:

> <jaws> for the push notification management ui, we will open a dialog similar to the one for Options > Security > Add-ons > Exceptions...
> <•dolske> 1201398, right? :)
> <jaws> The patch for this has "Exceptions - Notifications", but I'd like to land it with "Notification Permissions"
> <jaws> that's right
> <jaws> I think "Notification Permissions" sounds more human (less technical/roboty)
> <jaws> i could probably land it with "Permissions - Notifications" with no major complaints, but i think it can be better
> <•dolske> can you screenshot the Content tab with and without this UI opened?
> <jaws> ok, one sec
> <jaws> without it opened, http://screencast.com/t/Y3g1dgOpu
> <jaws> with the dialog opened, http://screencast.com/t/nKnPsy8Nq
> <jaws> dolske: ^
> <•dolske> jaws: It would be nice if we were consistent with the Popup stuff that's already there, but it's not a big deal
> <•dolske> worst case we land this now and change in before Aurora uplift
> <•dolske> so r+=me.
> <jaws> dolske: yeah, that's the other option. land it with "Permissions - Notifications" and then file a different bug to update all of them
> <•dolske> I think "Exceptions…" is a slightly better button label, because it tells a bit more about its purpose without having to read the text description.
> <•dolske> (yeah, I know, the Languages button already has Choose. :)
> <jaws> dolske: "Exceptions..." is a bit confusing though. compared to pop-ups which are assumed blocked everywhere, and the popups dialog only allows adding exceptions to always show them, the notifications one allows the user to allow or block them
> <jaws> it's all fudgy language tbh
> <•dolske> yeah
> <jaws> fine with keeping "Choose..." and "Notification Permissions" ?
> <•dolske> we could probably just remove the "Allow popups" box entirely (really, does anyone want to globally enable that?!), and make both "Permissions". But still an issue for any other spots where we'd want to keep a checkbox + exceptions
> <•dolske> jaws: yeah, fine as it. just mulling possible options.
> <•dolske> * fine as is.
> <jaws> ok thanks

I will file a new bug to make the same word changes to the other permission dialogs for consistency.

Comment 17

[Justin Dolske [:Dolske]]

To be precise - I don't have a firm opinion on the final wording here or in existing usage, and am happy to let Matej et al chew on it. I do have a firm opinion that the current patch was good enough to land on Nightly, and can easily be changed before uplift if folks want to tweak it. :-)

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3d5175374d15