Opened 2 years ago

Closed 2 years ago

Last modified 11 days ago

#5354 closed defect (fixed)

Notifications with buttons are not supported in Opera

Reported by: Ross Assignee: jsonesen
Priority: P3 Milestone: Adblock-Plus-3.0-for-Firefox
Module: Platform Keywords:
Cc: kzar, greiner, jsonesen, sebastian Blocked By:
Blocking: Platform: Opera
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29583620/

Description (last modified by sebastian)

Environment

ABP 1.13.2.1785
Opera 36 / 49 / Window 7

How to reproduce

  1. Install ABP (to have default/clear preferences).
  2. Visit http://www.nexusmods.com/games
  3. Repeat Steps 2-3 in Chrome.

Observed behaviour

In Chrome, the anti-adblock notification appears as expected.

In Opera it does not and the following error appears in the extensions background page console:

_generated_background_page.html:1 Unchecked runtime.lastError while running notifications.create: Adding buttons to notifications is not supported.

Expected behaviour

If buttons are not supported, the notification should be displayed without buttons, except if the type of the notification is question in which case the notification should not be shown if buttons are not supported. Either way, no error should be logged.

Change History (17)

comment:1 Changed 2 years ago by Ross

I'm not actually 100% sure if the anti-adblock notification should appear in Opera but the consensus seems to be be it should. The error seems to be related to a platform restriction (not able to add buttons to notifications) so I'm not sure if/how it worked if it did before.

comment:2 Changed 2 years ago by sebastian

  • Cc kzar greiner jsonesen sebastian added
  • Component changed from Unknown to Platform
  • Priority changed from Unknown to P3

Yeah, generally, the notification should appear on Opera. But from reading the error message, I agree, it doesn't seem we can have buttons there, and without buttons this notification seems quite useless. Possibly, we could fallback to HTML notifications though. Either way we should get rid of the error.

comment:3 Changed 2 years ago by kzar

  • Ready set

comment:4 Changed 2 years ago by jsonesen

  • Owner set to jsonesen

comment:5 Changed 2 years ago by jsonesen

Is this duplicated in #5759??

comment:6 Changed 2 years ago by sebastian

  • Summary changed from Anti-adblock notification does not work in Opera to Notifications with buttons are not supported in Opera

comment:7 Changed 2 years ago by sebastian

  • Description modified (diff)

comment:8 Changed 2 years ago by jsonesen

Also, it looks like we will need to implement a html notification if we want them to appear on opera. However, deleting the button property does fix the issues with errors being thrown.

comment:9 Changed 2 years ago by sebastian

  • Description modified (diff)

There is already a fallback to HTML notifications, but it seems HTML notifications cannot have buttons either. So we don't use this fallback for notifications of the type question (e.g. the anti-adblock notification), and not if browser.notifications is supported anyway.

But then again, there is no point in showing the anti-adblock notification if we can not have buttons to opt-in or -out of that list. So if buttons aren't supported in browser.notifications and the notification type is question it seems best to not show the notification at all.

comment:10 Changed 2 years ago by jsonesen

  • Review URL(s) modified (diff)

comment:11 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 5354 - Adds handling for notifications with buttons in Opera

comment:12 Changed 2 years ago by jsonesen

  • Resolution set to fixed
  • Status changed from new to closed

comment:13 Changed 2 years ago by sebastian

  • Milestone set to Adblock-Plus-3.0-for-Chrome-Opera-Firefox

comment:14 Changed 7 months ago by Thanos

spam

Last edited 7 months ago by greiner (previous) (diff)

comment:15 Changed 5 months ago by Himanshu0709

spam

Last edited 11 days ago by kzar (previous) (diff)

comment:16 Changed 3 months ago by vedantydv123

spam

Last edited 11 days ago by kzar (previous) (diff)

comment:17 Changed 3 weeks ago by Henshaw34

spam

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