Closed Bug 1201398 Opened 6 years ago Closed 6 years ago

Web Notification management interface

Categories

(Firefox :: Preferences, defect)

43 Branch
Unspecified
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: wmaggs, Assigned: jaws)

References

(Depends on 1 open bug)

Details

User Story

Acceptance Criteria: 

1. Generate set of permission for several websites to receive Push Notifications 
2. Access Notifications management interface in Content Pane via Push messages as well as via Preferences 
3. Change permissions on multiple sites using interface
4. Check permissions change per site be examining site permissions

Attachments

(5 files, 1 obsolete file)

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.
Blocks: 1201397
Attached image Control Center.png
Depends on: 1201571
Summary: Push Notifications in FF Windows needs a management interface → Web Notifications in FF Windows needs a management interface
Attached image pn-01.png
PSackl created mocks
Attached image pn-03.png
psackl created mocks
The centralized management interface design mocked above is targeted for FF 44.
Blocks: 1205399
Blocks: 1201571
No longer depends on: 1201571
The abbreviation for Firefox is Fx, not FF btw.
Keywords: meta
OS: Windows → All
Summary: Web Notifications in FF Windows needs a management interface → [meta] Web Notification management interface
: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.
Flags: needinfo?(philipp)
No longer blocks: 1205399
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.
Let's move forward with the existing controls for managing notification permissions for v1. I'll start working on this tomorrow.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(philipp)
Keywords: meta
Summary: [meta] Web Notification management interface → Web Notification management interface
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...
Attached patch Patch (obsolete) — Splinter Review
Attachment #8667483 - Flags: review?(MattN+bmo)
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.
Attached patch PatchSplinter Review
Oh sorry, I had meant to come back to that after testing and forgot. Fixed now.
Attachment #8667483 - Attachment is obsolete: true
Attachment #8667483 - Flags: review?(MattN+bmo)
Attachment #8667638 - Flags: review?(MattN+bmo)
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"?
Attachment #8667638 - Flags: review?(MattN+bmo) → review+
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.
(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.
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. :-)
https://hg.mozilla.org/mozilla-central/rev/3d5175374d15
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
User Story: (updated)
Blocks: 1216585
No longer blocks: 1216585
Depends on: 1221471
You need to log in before you can comment on or make changes to this bug.