Opened 2 years ago

Closed 20 months ago

Last modified 20 months ago

#6201 closed defect (fixed)

"Hide targeted messages?" notification not triggering

Reported by: jsonesen Assignee: kzar
Priority: P2 Milestone:
Module: User-Interface Keywords:
Cc: trev, kzar, greiner, sebastian Blocked By:
Blocking: #6208 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29643698/

Description (last modified by kzar)

Environment

Mac OS High Sierra / Ubuntu 16.04
Chrome (63) / Firefox (57)
adblockpluschrome since a5c2164d73ca

How to reproduce

  1. Browse to https://www.nexusmods.com/

Observed behaviour

No "Hide targeted messages?" notification is displayed.

Expected behaviour

The notifications should be displayed.

Notes

Change History (15)

comment:1 Changed 2 years ago by kzar

Which revisions do 1865 and 1866 refer to?

comment:2 Changed 2 years ago by trev

  • Cc sebastian added

Revision 1866 is #5776 which makes #5728 the only possible culprit.

comment:3 Changed 2 years ago by kzar

  • Cc greiner added; sergz removed
  • Description modified (diff)
  • Priority changed from Unknown to P2
  • Ready set
  • Summary changed from antiAdblock Notifications not triggering to "Hide targeted messages?" notification not triggering

Thanks, I can reproduce though I tried reverting #5728 quickly and it didn't seem to help. I've updated the issue, it might be a Platform issue but I've left it as Core for now.

comment:4 Changed 2 years ago by kzar

  • Description modified (diff)

comment:5 Changed 2 years ago by kzar

  • Owner set to kzar

comment:6 Changed 2 years ago by kzar

  • Component changed from Core to User-Interface
  • Priority changed from P2 to Unknown
  • Ready unset
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

Turns out the required change was in adblockplusui, so I've un-triaged this and uploaded a review.

comment:7 Changed 2 years ago by saroyanm

  • Priority changed from Unknown to P2
  • Ready set

comment:8 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 6201 - Fix the "Hide targeted messages?" notification

comment:9 Changed 2 years ago by kzar

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

comment:10 Changed 2 years ago by kzar

  • Blocking 6208 added

comment:11 Changed 22 months ago by Ross

The notification now displays in Chrome 64 but does not in Firefox 58/51. Not sure if it is caused by the following error:

Type error for parameter options (Property "buttons" is unsupported by Firefox) for notifications.create - adblockplus.js:7279

comment:12 Changed 22 months ago by kzar

Yep I think you're right about the cause, see this Firefox bug. Not much we can do here until that's resolved I think.

comment:13 Changed 20 months ago by sebastian

  • Resolution fixed deleted
  • Status changed from closed to reopened

We should at least handle that error, the same way we did for Opera, which doesn't support notifications with buttons either. However, the reason this fix doesn't apply on Firefox seems to be that it throws the error synchronously, while Opera gives an asynchronous error. So we have to adapt our error handling to account for this. Note that otherwise, besides spamming the error log, even notification without buttons would fail, as we still pass buttons: [] in that case.

Furthermore, we might want to consider to not subscribe the "Adblock Warning Removal List" on Firefox and Opera, by default, if the notification isn't shown anyway.

comment:14 Changed 20 months ago by sebastian

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

I filed a separate issue (#6496) for this.

comment:15 Changed 20 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Fixed. Working in Chrome.

ABP 3.0.2.1983
Chrome 49 / 65 / Windows 7

Note: See TracTickets for help on using tickets.