Opened 2 years ago

Closed 3 months ago

Last modified 6 weeks ago

#5512 closed change (rejected)

Add support for multiple notifications in lib/notificationHelper.js

Reported by: mjethani Assignee: mjethani
Priority: P2 Milestone:
Module: Platform Keywords: closed-in-favor-of-gitlab
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 (10)

comment:1 Changed 2 years ago by mjethani

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

comment:2 Changed 2 years ago by mjethani

  • Review URL(s) modified (diff)

comment:3 Changed 2 years ago by mjethani

  • Description modified (diff)

comment:4 Changed 2 years 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 2 years ago by kzar (previous) (diff)

comment:5 Changed 2 years ago by mjethani

  • Description modified (diff)

comment:6 Changed 11 months ago by johnson83

spam

Last edited 6 weeks ago by kzar (previous) (diff)

comment:7 Changed 5 months ago by slotslotonline

spam

Last edited 6 weeks ago by kzar (previous) (diff)

comment:8 Changed 3 months ago by sebastian

  • Keywords closed-in-favor-of-gitlab added
  • Resolution set to rejected
  • Status changed from new to closed

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the new issue tracker.

comment:9 Changed 3 months ago by slotslotonline

spam

Last edited 6 weeks ago by kzar (previous) (diff)

comment:10 Changed 3 months ago by slotslotonline

spam

Last edited 6 weeks ago by kzar (previous) (diff)
Note: See TracTickets for help on using tickets.