Issue 29513555: Issue 5512 - Manage multiple notifications displayed at the same time
|
Created:
Aug. 12, 2017, 4:49 p.m. by Manish Jethani Modified:
May 9, 2018, 10:31 a.m. Reviewers:
kzar CC:
Sebastian Noack, Thomas Greiner Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 5512 - Manage multiple notifications displayed at the same time
Patch Set 1 #
Total comments: 14
MessagesTotal messages: 8
Patch Set 1 Comments inline. https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... File lib/notificationHelper.js (left): https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:139: activeNotification.onClicked(); Calling onClicked (now onClosed) is no longer needed, because that is handled at the time of closing, whereas this is the time of button clicking. https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:29: // "Open" notifications are any notifications currently being displayed in the Comments explaining the conceptual difference between an "open" notification and an "active" notification. https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:206: chrome.notifications.clear(key, wasCleared => Renamed "notificationId" to "key" here, because the former has multiple meanings. It could mean the id property of the notification object we get from core, or it could mean the notification ID of the actual notification UI widget. Since we're using a map, let's call this "key". We also generate a random key ourselves in case chrome.notifications is not supported and we end up having to use the Notification API. https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:223: // Known issue on macOS: When the user closes a notification by clicking on Maybe it's just me, but macOS doesn't seem to trigger the onClosed handler if you click on the little close button on the top right of the notification widget. https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:260: // Critical notifications should not go away in a few seconds, as I took the liberty to add this here. If it's a critical notification, maybe it shouldn't just go away in five seconds the way it does on macOS? macOS's security update notifications, for example, persist until the user closes them explicitly.
(Adding Thomas to Cc.) https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:29: // "Open" notifications are any notifications currently being displayed in the On 2017/08/12 16:57:48, Manish Jethani wrote: > Comments explaining the conceptual difference between an "open" notification and > an "active" notification. Thanks for adding those, I think it helps quite a bit. https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:49: function prepareNotificationIconAndPopup(notification) Since we're no longer always making the given notification active this function's name seems a bit misleading. https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:260: // Critical notifications should not go away in a few seconds, as On 2017/08/12 16:57:48, Manish Jethani wrote: > I took the liberty to add this here. If it's a critical notification, maybe it > shouldn't just go away in five seconds the way it does on macOS? macOS's > security update notifications, for example, persist until the user closes them > explicitly. Sounds sensible to me but you've not mentioned this in the issue or (I assume) discussed this with Thomas. We need to check with him first before making this change. https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:276: // (because memory leaks), we generate a random key here. Couldn't we rather use a WeakMap?
https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:260: // Critical notifications should not go away in a few seconds, as On 2017/08/21 13:59:24, kzar wrote: > On 2017/08/12 16:57:48, Manish Jethani wrote: > > I took the liberty to add this here. If it's a critical notification, maybe it > > shouldn't just go away in five seconds the way it does on macOS? macOS's > > security update notifications, for example, persist until the user closes them > > explicitly. > > Sounds sensible to me but you've not mentioned this in the issue or (I assume) > discussed this with Thomas. We need to check with him first before making this > change. I can confirm that critical notifications are not meant to disappear as you can also read from the comment above: "We use the highest priority to prevent the notification from closing automatically." Note that at the time when we first implemented them, the "requireInteraction" option didn't exist yet (see https://developer.chrome.com/apps/notifications#type-NotificationOptions) which is why we had to resort to using "priority" instead to keep it open as long as possible. It's news to me that the notification would only go away on some platforms though because I was under the impression that this was the case across all platforms.
https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:49: function prepareNotificationIconAndPopup(notification) On 2017/08/21 13:59:24, kzar wrote: > Since we're no longer always making the given notification active this > function's name seems a bit misleading. It seems appropriate to me, since the function will animate the icon and set up the notification to be displayed in the popup. Suggestions for a better name? setActiveNotification? https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:260: // Critical notifications should not go away in a few seconds, as On 2017/08/21 13:59:24, kzar wrote: > On 2017/08/12 16:57:48, Manish Jethani wrote: > > I took the liberty to add this here. If it's a critical notification, maybe it > > shouldn't just go away in five seconds the way it does on macOS? macOS's > > security update notifications, for example, persist until the user closes them > > explicitly. > > Sounds sensible to me but you've not mentioned this in the issue or (I assume) > discussed this with Thomas. We need to check with him first before making this > change. I've updated the issue now: https://issues.adblockplus.org/ticket/5512?action=diff&version=5 https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:260: // Critical notifications should not go away in a few seconds, as On 2017/08/22 18:16:59, Thomas Greiner wrote: > On 2017/08/21 13:59:24, kzar wrote: > > On 2017/08/12 16:57:48, Manish Jethani wrote: > > > I took the liberty to add this here. If it's a critical notification, maybe > it > > > shouldn't just go away in five seconds the way it does on macOS? macOS's > > > security update notifications, for example, persist until the user closes > them > > > explicitly. > > > > Sounds sensible to me but you've not mentioned this in the issue or (I assume) > > discussed this with Thomas. We need to check with him first before making this > > change. > > I can confirm that critical notifications are not meant to disappear as you can > also read from the comment above: "We use the highest priority to prevent the > notification from closing automatically." It's true that the notification doesn't close automatically, but it does get hidden on macOS after a few seconds (it just slides away into the notification center). This has the effect of the user not seeing it at all if they're away from their computer when it is displayed. I don't think most users explicitly open the notification center to see what notifications they've missed. In other words, it has the same effect as the notification closing automatically. Chrome has recently moved to native notifications on macOS, and this might explain the new behavior: https://developers.google.com/web/updates/2017/04/native-mac-os-notifications
https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:49: function prepareNotificationIconAndPopup(notification) On 2017/08/21 13:59:24, kzar wrote: > Since we're no longer always making the given notification active this > function's name seems a bit misleading. It seems appropriate to me, since the function will animate the icon and set up the notification to be displayed in the popup. Suggestions for a better name? setActiveNotification? https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:260: // Critical notifications should not go away in a few seconds, as On 2017/08/21 13:59:24, kzar wrote: > On 2017/08/12 16:57:48, Manish Jethani wrote: > > I took the liberty to add this here. If it's a critical notification, maybe it > > shouldn't just go away in five seconds the way it does on macOS? macOS's > > security update notifications, for example, persist until the user closes them > > explicitly. > > Sounds sensible to me but you've not mentioned this in the issue or (I assume) > discussed this with Thomas. We need to check with him first before making this > change. I've updated the issue now: https://issues.adblockplus.org/ticket/5512?action=diff&version=5 https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:260: // Critical notifications should not go away in a few seconds, as On 2017/08/22 18:16:59, Thomas Greiner wrote: > On 2017/08/21 13:59:24, kzar wrote: > > On 2017/08/12 16:57:48, Manish Jethani wrote: > > > I took the liberty to add this here. If it's a critical notification, maybe > it > > > shouldn't just go away in five seconds the way it does on macOS? macOS's > > > security update notifications, for example, persist until the user closes > them > > > explicitly. > > > > Sounds sensible to me but you've not mentioned this in the issue or (I assume) > > discussed this with Thomas. We need to check with him first before making this > > change. > > I can confirm that critical notifications are not meant to disappear as you can > also read from the comment above: "We use the highest priority to prevent the > notification from closing automatically." It's true that the notification doesn't close automatically, but it does get hidden on macOS after a few seconds (it just slides away into the notification center). This has the effect of the user not seeing it at all if they're away from their computer when it is displayed. I don't think most users explicitly open the notification center to see what notifications they've missed. In other words, it has the same effect as the notification closing automatically. Chrome has recently moved to native notifications on macOS, and this might explain the new behavior: https://developers.google.com/web/updates/2017/04/native-mac-os-notifications
https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29513555/diff/29513556/lib/notificationHel... lib/notificationHelper.js:260: // Critical notifications should not go away in a few seconds, as On 2017/08/24 09:54:12, Manish Jethani wrote: > On 2017/08/21 13:59:24, kzar wrote: > > On 2017/08/12 16:57:48, Manish Jethani wrote: > > > I took the liberty to add this here. If it's a critical notification, maybe > it > > > shouldn't just go away in five seconds the way it does on macOS? macOS's > > > security update notifications, for example, persist until the user closes > them > > > explicitly. > > > > Sounds sensible to me but you've not mentioned this in the issue or (I assume) > > discussed this with Thomas. We need to check with him first before making this > > change. > > I've updated the issue now: > > https://issues.adblockplus.org/ticket/5512?action=diff&version=5 Thanks
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
