Closed
Bug 1205399
Opened 5 years ago
Closed 5 years ago
OS X: Implement disabling Notifications for the site
Categories
(Toolkit :: Notifications and Alerts, defect)
Tracking
()
People
(Reporter: wmaggs, Assigned: MattN, NeedInfo)
References
Details
User Story
Acceptance Critera: 1. In OSX, generate Push-based notifications for all message types (title only, body text, etc) 2. Verfiy that Disable Notifications for Site is choosable for each 3. Verify disabelment by checking per-site permission (lock icon)
Attachments
(4 files)
+++ This bug was initially created as a clone of Bug #1201397 +++ The FF Windows user needs an actionable anchor to access an interface to turn off or turn down Push notifications for the sites the user has granted permission to send them. To do this currently the user must click on the message, then know where to go on the page to access Control Center, which exposes the permission to receive push notifications, as well as show notifications. It only works for one site, and if the user finds herself making a company presentation after allowing several website pages in FF to receive push notifications, it becomes a huge problem to manage them site by site. She needs an immediate way to get to the notification permissions in one place. A simple anchor would be something like a settings cog in the corner of every message on FF Windows (see attachment for concept, NOT finished mock). In FF 43 this cog would link to a centralized interface to manage Push notifications, described in Bug For FF 42, this cog graphic involves no strings and could possibly be uplifted A more simple management interface than the one described in this bug would probably be necessary when the cog is clicked on. This bug requires using the documented (and perhaps undocumented) features of Notifications in OSX to provide as similar an experience as possible to messages shown and managed on Ff Windows. This includes a button or control to allow the user to manage the notifications per site or centrally, as well as a Do Not Disturb button.
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 44.1 - Oct 5
Summary: Make the Push Notification Message Design on FF OSX match features of message design for Windows → OS X: Implement disabling Notifications for the site
Target Milestone: mozilla44 → ---
Version: 43 Branch → Trunk
Assignee | ||
Comment 1•5 years ago
|
||
Bug 1205399 - Remove some leftover Growl strings. r=mstange
Attachment #8665034 -
Flags: review?(mstange)
Assignee | ||
Comment 2•5 years ago
|
||
Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications.
Assignee | ||
Comment 3•5 years ago
|
||
Comment on attachment 8665035 [details] MozReview Request: Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm William, Nikhil: What is a good architecture to support revoking the permission from the 3 contexts I see: regular content, web worker and service worker? Do I have to duplicate logic in all of them to remove the permission or is there a better place I should put an observer so that we don't need to duplicate this for each context (worker, etc.) or UI backend (XUL/OS X/libnotify)? Any observer in the correct process can easily remove the permission since the nsIPrincipal is passed as the subject. Feel free to ping me to discuss on IRC if you prefer.
Attachment #8665035 -
Flags: feedback?(wchen)
Attachment #8665035 -
Flags: feedback?(nsm.nikhil)
Assignee | ||
Comment 4•5 years ago
|
||
Also, I'm proposing that the alert service will take care of adding additional actions for any nsIPrincipal representing a domain rather than add more arguments to the alert service method and passing all of the additional option strings in. Any objections or thoughts on that?
<nsm|away> so one way to refactor the observer code in Notification.cpp is to have a common observer that does all the main thread only actions like revoking permissions, and then delegate to the individuals for the click/close. also 'onshow' has been removed, so we can get rid of it <-- Dexter_ (Alessio@moz-kf56p4.retail.telecomitalia.it) has quit (Quit: Leaving) <nsm|away> since ::Observe always runs on the main thread, this should be fine <nsm|away> the worker observer is only complicated due to ownership stuff
Assignee | ||
Comment 6•5 years ago
|
||
Comment on attachment 8665035 [details] MozReview Request: Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm Bug 1205399 - Backend for supporting disabling of notifications for a site from the UI. r=nsm
Attachment #8665035 -
Attachment description: MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. → MozReview Request: Bug 1205399 - Backend for supporting disabling of notifications for a site from the UI. r=nsm
Attachment #8665035 -
Flags: review?(nsm.nikhil)
Attachment #8665035 -
Flags: feedback?(wchen)
Attachment #8665035 -
Flags: feedback?(nsm.nikhil)
Assignee | ||
Comment 7•5 years ago
|
||
Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications.
Comment on attachment 8665035 [details] MozReview Request: Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm https://reviewboard.mozilla.org/r/20129/#review18095 Almost there. ::: dom/notification/Notification.cpp:658 (Diff revision 2) > class NotificationObserver : public nsIObserver `class NotificationObserver final` ::: dom/notification/Notification.cpp:666 (Diff revision 2) > - explicit NotificationObserver(UniquePtr<NotificationRef> aRef) > + explicit NotificationObserver(nsIObserver* aObserver, nsIPrincipal* aPrincipal) Nit: Don't need explicit anymore since it takes 2 arguments. ::: dom/notification/Notification.cpp:668 (Diff revision 2) > + mObserver = aObserver; > + mPrincipal = aPrincipal; Please use member initialization list syntax for setting members in the constructor - http://stackoverflow.com/questions/1711990/what-is-this-weird-colon-member-syntax-in-the-constructor ::: dom/notification/Notification.cpp:1154 (Diff revision 2) > + MOZ_ASSERT(mObserver); This assert can be moved to the constructor. ::: dom/notification/Notification.cpp:1157 (Diff revision 2) > + nsCOMPtr<nsIPermissionManager> permissionManager = services::GetPermissionManager(); > + MOZ_ASSERT(mPrincipal); I'd prefer using the fully qualified `mozilla::services::GetPermissionManager()` Please add a if (!permissionManager) { return NS_ERROR_FAILURE; } since it is quite possible we are shutting down or something and may not get a permission manager. ::: dom/notification/Notification.cpp:1158 (Diff revision 2) > + MOZ_ASSERT(mPrincipal); No need for this, the fact that you are using nsCOMPtr<> guarantees this will be valid. I'd rather you move this assertion to the constructor so nobody passes a null principal to the constructor. ::: dom/notification/Notification.cpp:1160 (Diff revision 2) > + printf("removing permission\n"); You need to return here so we don't call mObserver->Observe() when we already dealt with the event. Nit: get rid of the prinft. ::: dom/notification/Notification.cpp:1436 (Diff revision 2) > - // The observer is wholly owned by the alerts service. > + // The observer is wholly owned by the alerts service. // TODO I think the only TODO here is that it is now owned by the top level observer, so only the comment needs fixing. ::: dom/notification/Notification.cpp:1477 (Diff revision 2) > appNotifier->ShowAppNotification(iconUrl, mTitle, mBody, > observer, val); > return; > } > } We have this other callsite for b2g where you'll have to do the same thing :( ::: dom/notification/Notification.cpp:1508 (Diff revision 2) > + > + nsCOMPtr<nsIObserver> alertObserver = new NotificationObserver(observer, > + GetPrincipal()); > + I'd highly recommend you move this initialization to the top around line 196 right after the `MOZ_ASSERT(observer)`.
Attachment #8665035 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 9•5 years ago
|
||
Comment on attachment 8665035 [details] MozReview Request: Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm
Attachment #8665035 -
Attachment description: MozReview Request: Bug 1205399 - Backend for supporting disabling of notifications for a site from the UI. r=nsm → MozReview Request: Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm
Attachment #8665035 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 10•5 years ago
|
||
Comment on attachment 8665148 [details] MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications.
Assignee | ||
Comment 11•5 years ago
|
||
Comment on attachment 8665034 [details] MozReview Request: Bug 1205399 - Remove some leftover Growl strings. r=mstange Bug 1205399 - Remove some leftover Growl strings. r=mstange
Assignee | ||
Comment 12•5 years ago
|
||
Comment on attachment 8665035 [details] MozReview Request: Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm
Assignee | ||
Comment 13•5 years ago
|
||
Comment on attachment 8665148 [details] MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r=mstange
Attachment #8665148 -
Attachment description: MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. → MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r=mstange
Attachment #8665148 -
Flags: review?(mstange)
Comment 14•5 years ago
|
||
Comment on attachment 8665034 [details] MozReview Request: Bug 1205399 - Remove some leftover Growl strings. r=mstange https://reviewboard.mozilla.org/r/20127/#review18137
Attachment #8665034 -
Flags: review?(mstange) → review+
Comment 15•5 years ago
|
||
Comment on attachment 8665148 [details] MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange https://reviewboard.mozilla.org/r/20149/#review18135 ::: widget/cocoa/OSXNotificationCenter.mm:32 (Diff revision 3) > + NSUserNotificationActivationTypeReplied = 3, > + NSUserNotificationActivationTypeAdditionalActionClicked = 4 These two need to be moved out of the MAC_OS_X_VERSION_10_8 #if because they were introduced in 10.9 and 10.10, respectively. You need to wrap each of them in their own enum and #if block. With the current code, people who compile with the 10.8 SDK or the 10.9 SDK will probably get compilation errors, because this #if block will not be compiled and the 10.8/10.9 SDK header won't contain the definitions of these enums either. ::: widget/cocoa/OSXNotificationCenter.mm:96 (Diff revision 3) > - mOSXNC->OnClick([[notification userInfo] valueForKey:@"name"]); > + NSNumber *alternateActionIndex = [(NSObject*)notification valueForKey:@"_alternateActionIndex"]; Is this the right way to do this even on 10.10, where additionalActions was introduced as a public API? ::: widget/cocoa/OSXNotificationCenter.mm:231 (Diff revision 3) > + // Don't show additional actions dealing with permissions for application/extension alerts. I think it would be slightly clearer if this comment were reversed: "If this is not an application/extension alert, show additional actions dealing with permissions."
Attachment #8665148 -
Flags: review?(mstange) → review+
Comment on attachment 8665035 [details] MozReview Request: Bug 1205399 - Backend for disabling of notifications for a site from the UI. r=nsm https://reviewboard.mozilla.org/r/20129/#review18217
Attachment #8665035 -
Flags: review?(nsm.nikhil) → review+
Assignee | ||
Comment 17•5 years ago
|
||
https://reviewboard.mozilla.org/r/20149/#review18135 > Is this the right way to do this even on 10.10, where additionalActions was introduced as a public API? notification.additionalActivationAction and notification.additionalActions seems to always be `nil` on 10.10 for me (even with the 10.10 SDK), even if I make the notification an "alert" type. I couldn't find any other code on Github or Google succesfully using these new properties either. It's also weird that the NSUserNotificationAction class isn't documented.
Assignee | ||
Comment 18•5 years ago
|
||
Comment on attachment 8665148 [details] MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange
Attachment #8665148 -
Attachment description: MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r=mstange → MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange
Assignee | ||
Comment 19•5 years ago
|
||
Comment on attachment 8665148 [details] MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange I ended up having to implement didDismissAlert otherwise the webpage wouldn't get the close event if the user clicked the Close button.
Attachment #8665148 -
Flags: review+ → review?(mstange)
Assignee | ||
Comment 20•5 years ago
|
||
Philipp, your spec[1] says to set the permission to "block" but my understanding is that the url bar icon with a strikethrough for blocked permissions isn't being worked on for 44 so it may not be discoverable how to change ones mind. With "block" specified, the site won't be able to ask for the permission again until the user reverts their decision in some secondary UI. Do you think we should set to "block" for 44 or the default which means they won't work but a site can still make the doorhanger appear again upon future visits? [1] https://mozilla.invisionapp.com/share/3A4A895XZ#/screens/103249711
Flags: needinfo?(psackl)
Updated•5 years ago
|
Attachment #8665148 -
Flags: review?(mstange) → review+
Comment 21•5 years ago
|
||
Comment on attachment 8665148 [details] MozReview Request: Bug 1205399 - OS X: Implement disabling Notifications for the site from the native notifications. r?mstange https://reviewboard.mozilla.org/r/20149/#review18307
Reporter | ||
Comment 22•5 years ago
|
||
(In reply to Matthew N. [:MattN] (behind on mail) from comment #20) > Philipp, your spec[1] says to set the permission to "block" but my > understanding is that the url bar icon with a strikethrough for blocked > permissions isn't being worked on for 44 so it may not be discoverable how > to change ones mind. With "block" specified, the site won't be able to ask > for the permission again until the user reverts their decision in some > secondary UI. > > Do you think we should set to "block" for 44 or the default which means they > won't work but a site can still make the doorhanger appear again upon future > visits? > > [1] https://mozilla.invisionapp.com/share/3A4A895XZ#/screens/103249711 I think we set to the default because is what the user will expect. When the user gets bugged agina by the site they respond in the prompt to block permanently.
Comment 23•5 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/a79534562dd8 https://hg.mozilla.org/integration/fx-team/rev/1c884fcfdf38 https://hg.mozilla.org/integration/fx-team/rev/9898e4c13876
Comment 25•5 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a79534562dd8 https://hg.mozilla.org/mozilla-central/rev/1c884fcfdf38 https://hg.mozilla.org/mozilla-central/rev/9898e4c13876 https://hg.mozilla.org/mozilla-central/rev/e597556a8433
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment 26•5 years ago
|
||
This patch appears to have broken my build on OS X 10.9.5 because it can't find NSUserNotificationActivationTypeAdditionalActionClicked. Do I need to upgrade XCode or something? Relevant portion of the build log at https://pastebin.mozilla.org/8847697 - I verified that backing out these patches gets my build past that error.
Comment 27•5 years ago
|
||
Casting the switch statement as per http://stackoverflow.com/a/11076977 also worked for me.
Attachment #8666743 -
Flags: review?(MattN+bmo)
Comment 28•5 years ago
|
||
Re-opening until build-bustage fix in comment 27 has landed, or separate bug has been filed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 29•5 years ago
|
||
Comment on attachment 8666743 [details] [diff] [review] Follow-up to fix build bustage stealing review
Attachment #8666743 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 30•5 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #29) > stealing review Sorry, I was afk the last two days.
Comment 32•5 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5cb68f2221bc
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•5 years ago
|
User Story: (updated)
You need to log in
before you can comment on or make changes to this bug.
Description
•