Opened 21 months ago

Closed 18 months ago

Last modified 18 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 21 months ago by kzar

Which revisions do 1865 and 1866 refer to?

comment:2 Changed 21 months ago by trev

  • Cc sebastian added

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

comment:3 Changed 21 months 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 21 months ago by kzar

  • Description modified (diff)

comment:5 Changed 21 months ago by kzar

  • Owner set to kzar

comment:6 Changed 21 months 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 21 months ago by saroyanm

  • Priority changed from Unknown to P2
  • Ready set

comment:8 Changed 21 months ago by abpbot

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

comment:9 Changed 21 months ago by kzar

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

comment:10 Changed 21 months ago by kzar

  • Blocking 6208 added

comment:11 Changed 20 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 19 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 18 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 18 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 18 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.