Issue 5512 - Manage multiple notifications displayed at the same time

lib/notificationHelper.js

Index: lib/notificationHelper.js
===================================================================
--- a/lib/notificationHelper.js
+++ b/lib/notificationHelper.js
@@ -21,37 +21,64 @@
 
 const {startIconAnimation, stopIconAnimation} = require("icon");
 const {Utils} = require("utils");
 const {Notification: NotificationStorage} = require("notification");
 const {stringifyURL} = require("url");
 const {initAntiAdblockNotification} = require("antiadblockInit");
 const {Prefs} = require("prefs");
 
+// "Open" notifications are any notifications currently being displayed in the

[Manish Jethani, 2017/08/12 16:57:48]

Comments explaining the conceptual difference between an "open" notification and
an "active" notification.

[kzar, 2017/08/21 13:59:25]

Thanks for adding those, I think it helps quite a bit.

+// platform's notification center. There can be multiple notifications open at
+// the same time.
+let openNotificationsInfo = new Map();
+let openNotificationIds = new Set();
+
+// The "active" notification is the one that is displayed in the extension's
+// popup. It may be the same as one of the open notifications, or it may be a
+// different one.
 let activeNotification = null;
-let activeButtons = null;
+
 let defaultDisplayMethods = ["popup"];
 let displayMethods = Object.create(null);
+
 displayMethods.critical = ["icon", "notification", "popup"];
 displayMethods.question = ["notification"];
 displayMethods.normal = ["notification"];
 displayMethods.relentless = ["notification"];
 displayMethods.information = ["icon", "popup"];
 
-function prepareNotificationIconAndPopup()
+function prepareNotificationIconAndPopup(notification)

[kzar, 2017/08/21 13:59:24]

Since we're no longer always making the given notification active this
function's name seems a bit misleading.

[Manish Jethani, 2017/08/24 09:54:12]

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?

 {
-  let animateIcon = shouldDisplay("icon", activeNotification.type);
-  activeNotification.onClicked = () =>
+  let animateIcon = shouldDisplay("icon", notification.type);
+  let showInPopup = shouldDisplay("popup", notification.type);
+
+  if (animateIcon || showInPopup)
   {
+    // If another notification is active, close it first. This will stop any
+    // existing icon animation.
+    if (activeNotification)
+      activeNotification.onClosed();
+
+    activeNotification = notification;
+  }
+
+  notification.onClosed = () =>
+  {
+    if (notification != activeNotification)
+      return;
+
     if (animateIcon)
       stopIconAnimation();
-    notificationClosed();
+
+    activeNotification = null;
   };
+
   if (animateIcon)
-    startIconAnimation(activeNotification.type);
+    startIconAnimation(notification.type);
 }
 
 function getNotificationButtons(notificationType, message)
 {
   let buttons = [];
   if (notificationType == "question")
   {
     buttons.push({
@@ -94,165 +121,214 @@
         title: ext.i18n.getMessage("notification_configure")
       });
     }
   }
 
   return buttons;
 }
 
-function openNotificationLinks()
+function openNotificationLinks(notification)
 {
-  if (activeNotification.links)
+  if (notification.links)
   {
-    for (let link of activeNotification.links)
+    for (let link of notification.links)
       ext.pages.open(Utils.getDocLink(link));
   }
 }
 
-function notificationButtonClick(buttonIndex)
+function notificationButtonClick(notificationInfo, buttonIndex)
 {
-  if (!(activeButtons && buttonIndex in activeButtons))
+  let {notification, buttons} = notificationInfo || {};
+
+  if (!(buttons && buttonIndex in buttons))
     return;
 
-  switch (activeButtons[buttonIndex].type)
+  switch (buttons[buttonIndex].type)
   {
     case "link":
-      ext.pages.open(Utils.getDocLink(activeNotification.links[buttonIndex]));
+      ext.pages.open(Utils.getDocLink(notification.links[buttonIndex]));
       break;
     case "open-all":
-      openNotificationLinks();
+      openNotificationLinks(notification);
       break;
     case "configure":
       Prefs.notifications_showui = true;
       ext.showOptions(page =>
       {
         page.sendMessage({
           type: "app.respond",
           action: "focusSection",
           args: ["notifications"]
         });
       });
       break;
     case "question":
-      NotificationStorage.triggerQuestionListeners(activeNotification.id,
+      NotificationStorage.triggerQuestionListeners(notification.id,
                                                    buttonIndex == 0);
-      NotificationStorage.markAsShown(activeNotification.id);
-      activeNotification.onClicked();

[Manish Jethani, 2017/08/12 16:57:48]

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.

+      NotificationStorage.markAsShown(notification.id);
       break;
   }
 }
 
-function notificationClosed()
+function notificationOpened(key, notificationInfo)
+{
+  let {notification} = notificationInfo;
+
+  openNotificationsInfo.set(key, notificationInfo);
+  openNotificationIds.add(notification.id);
+}
+
+function notificationClosed(key)
 {
-  activeNotification = null;
+  let {notification} = openNotificationsInfo.get(key) || {};
+  if (notification)
+  {
+    if (notification.onClosed)
+      notification.onClosed();
+
+    openNotificationIds.delete(notification.id);
+  }
+
+  openNotificationsInfo.delete(key);
 }
 
 function initChromeNotifications()
 {
   // Chrome hides notifications in notification center when clicked so
   // we need to clear them.
-  function clearActiveNotification(notificationId)
+  function clearNotification(key)
   {
-    if (activeNotification &&
-        activeNotification.type != "question" &&
-        !("links" in activeNotification))
+    let {notification} = openNotificationsInfo.get(key) || {};
+    if (notification &&
+        notification.type != "question" &&
+        !("links" in notification))
       return;
 
-    chrome.notifications.clear(notificationId, wasCleared =>
+    chrome.notifications.clear(key, wasCleared =>

[Manish Jethani, 2017/08/12 16:57:48]

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.

     {
       if (wasCleared)
-        notificationClosed();
+        notificationClosed(key);
     });
   }
 
   chrome.notifications.onButtonClicked.addListener(
-    (notificationId, buttonIndex) =>
+    (key, buttonIndex) =>
     {
-      notificationButtonClick(buttonIndex);
-      clearActiveNotification(notificationId);
+      notificationButtonClick(openNotificationsInfo.get(key), buttonIndex);
+      clearNotification(key);
     }
   );
-  chrome.notifications.onClicked.addListener(clearActiveNotification);
+
+  chrome.notifications.onClicked.addListener(clearNotification);
+
+  // Known issue on macOS: When the user closes a notification by clicking on

[Manish Jethani, 2017/08/12 16:57:48]

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.

+  // the close button in the notification widget's title bar, it does not
+  // trigger the onClosed handler.
   chrome.notifications.onClosed.addListener(notificationClosed);
 }
 
 function showNotification(notification)
 {
-  if (activeNotification && activeNotification.id == notification.id)
+  if ((activeNotification && activeNotification.id == notification.id) ||
+      openNotificationIds.has(notification.id))
     return;
 
-  activeNotification = notification;
-  if (shouldDisplay("notification", activeNotification.type))
+  if (shouldDisplay("notification", notification.type))
   {
+    let notificationInfo = {notification};
+
     let texts = NotificationStorage.getLocalizedTexts(notification);
     let title = texts.title || "";
     let message = (texts.message || "").replace(/<\/?(a|strong)>/g, "");
     let iconUrl = ext.getURL("icons/detailed/abp-128.png");
-    let linkCount = (activeNotification.links || []).length;
+    let linkCount = (notification.links || []).length;
+
+    notificationInfo.buttons = getNotificationButtons(notification.type,
+                                                      texts.message);
 
     if ("notifications" in chrome)
     {
-      activeButtons = getNotificationButtons(activeNotification.type,
-                                             texts.message);
       chrome.notifications.create("", {
         type: "basic",
         title,
         iconUrl,
         message,
-        buttons: activeButtons.map(button => ({title: button.title})),
+        buttons: notificationInfo.buttons.map(button =>
+                                              ({title: button.title})),
         // We use the highest priority to prevent the notification
         // from closing automatically.
-        priority: 2
+        priority: 2,
+        // Critical notifications should not go away in a few seconds, as

[Manish Jethani, 2017/08/12 16:57:48]

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.

[kzar, 2017/08/21 13:59:24]

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.

[Thomas Greiner, 2017/08/22 18:16:59]

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.

[Manish Jethani, 2017/08/24 09:54:12]

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

[Manish Jethani, 2017/08/24 09:54:12]

I've updated the issue now:

https://issues.adblockplus.org/ticket/5512?action=diff&version=5

[kzar, 2017/08/29 12:16:30]

Thanks

+        // happens on some platforms (e.g. macOS).
+        requireInteraction: notification.type == "critical"
+      },
+      key =>
+      {
+        notificationOpened(key, notificationInfo);
       });
     }
-    else if ("Notification" in window && activeNotification.type != "question")
+    else if ("Notification" in window && notification.type != "question")
     {
       if (linkCount > 0)
         message += " " + ext.i18n.getMessage("notification_without_buttons");
 
+      // Since the Notification API doesn't given us a "notification ID", and
+      // we don't want to use the Notification object itself as the key
+      // (because memory leaks), we generate a random key here.

[kzar, 2017/08/21 13:59:24]

Couldn't we rather use a WeakMap?

+      let key = String(Date.now() + Math.random());
+
       let widget = new Notification(
         title,
         {
           lang: Utils.appLocale,
           dir: ext.i18n.getMessage("@@bidi_dir"),
           body: message,
           icon: iconUrl
         }
       );
 
-      widget.addEventListener("click", openNotificationLinks);
-      widget.addEventListener("close", notificationClosed);
+      notificationOpened(key, notificationInfo);
+
+      widget.addEventListener("click", () =>
+      {
+        openNotificationLinks(notification);
+      });
+
+      widget.addEventListener("close", () => notificationClosed(key));
+      widget.addEventListener("error", () => notificationClosed(key));
     }
     else
     {
       message = title + "\n" + message;
       if (linkCount > 0)
         message += "\n\n" + ext.i18n.getMessage("notification_with_buttons");
 
       let approved = confirm(message);
-      if (activeNotification.type == "question")
-        notificationButtonClick(approved ? 0 : 1);
+      if (notification.type == "question")
+        notificationButtonClick(notificationInfo, approved ? 0 : 1);
       else if (approved)
-        openNotificationLinks();
+        openNotificationLinks(notification);
     }
   }
-  prepareNotificationIconAndPopup();
 
-  if (notification.type !== "question")
+  prepareNotificationIconAndPopup(notification);
+
+  if (notification.type != "question")
     NotificationStorage.markAsShown(notification.id);
 }
 
 /**
  * Initializes the notification system.
  */
 exports.initNotifications = () =>
 {
   if ("notifications" in chrome)
     initChromeNotifications();
+
   initAntiAdblockNotification();
 };
 
 /**
  * Gets the active notification to be shown if any.
  *
  * @return {?object}
  */