Opened 10 months ago

Last modified 10 months ago

#5512 new change

Add support for multiple notifications in lib/notificationHelper.js

Reported by: mjethani Assignee: mjethani
Priority: P2 Milestone:
Module: Platform Keywords:
Cc: sebastian, kzar, greiner Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29513555/

Description (last modified by mjethani)

Background

Multiple notifications can be displayed at the same time, whereas lib/notificationHelper.js only supports a single notification at a time. If multiple notifications are displayed anyway, it fails badly.

Some things that are obviously broken:

  1. activeButtons is updated in showNotification. If later the user clicks on one of the buttons in an older notification widget that is still being displayed, it would either do nothing or, worse, call the wrong handler.
  2. Closing an older notification will clear out the currently active notification.
  3. If the notification is of type question but chrome.notifications is not supported, it will use the confirm API instead, which is synchronous, but display the icon animation and set it to the active notification afterwards after the notification has already been dismissed. Happily this is not a problem in practice as questions do not appear in the popup, but it is still an error in the logic and may manifest as a bug in the future. Moreover, before this even happens, there's an error because activeNotification.onClicked is not yet defined.
  4. If the user closes an open notification using the platform's native UI, it will clear out the active notification but not stop the icon animation.

There may be more.

Also, critical notifications should remain visible until the user dismisses them, but this is not the case (tested on Chrome 59 on macOS 10.12.5).

What to change

Distinguish between the "active" notification and "open" notifications as they are two different concepts, and modify the code accordingly by using the appropriate data structures to manage multiple notifications. Fix any bugs.

Pass the requireInteraction option to notifications.create, with a value of true for critical notifications and false otherwise.

Change History (5)

comment:1 Changed 10 months ago by mjethani

This work is required to get the extension working decently on Firefox for Android (WebExtensions).

comment:2 Changed 10 months ago by mjethani

  • Review URL(s) modified (diff)

comment:3 Changed 10 months ago by mjethani

  • Description modified (diff)

comment:4 Changed 10 months ago by kzar

  • Cc greiner added
  • Priority changed from Unknown to P2
  • Ready set

(Adding Thomas to Cc since this relates to the UI.)

Last edited 10 months ago by kzar (previous) (diff)

comment:5 Changed 10 months ago by mjethani

  • Description modified (diff)
Note: See TracTickets for help on using tickets.