Opened on 12/15/2017 at 12:47:50 PM
Closed on 03/18/2018 at 11:08:39 PM
Last modified on 03/20/2018 at 09:39:36 AM
#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): | |||
Description (last modified by kzar)
Environment
Mac OS High Sierra / Ubuntu 16.04
Chrome (63) / Firefox (57)
adblockpluschrome since a5c2164d73ca
How to reproduce
- Browse to https://www.nexusmods.com/
Observed behaviour
No "Hide targeted messages?" notification is displayed.
Expected behaviour
The notifications should be displayed.
Notes
- This is a regression since the adblockpluscore dependency update for #5776.
- Reverting the suspected culprit #5728 doesn't seem to help, though I've not carefully checked through those changes. So perhaps one of the other included changes are to blame, or perhaps one of the other included changes are to blame.
Attachments (0)
Change History (15)
comment:1 Changed on 12/18/2017 at 12:35:17 PM by kzar
comment:2 Changed on 12/18/2017 at 12:40:19 PM by trev
- Cc sebastian added
comment:3 Changed on 12/18/2017 at 01:01:00 PM 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:5 Changed on 12/18/2017 at 03:29:31 PM by kzar
- Owner set to kzar
comment:6 Changed on 12/18/2017 at 03:34:18 PM 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 on 12/20/2017 at 01:01:05 PM by saroyanm
- Priority changed from Unknown to P2
- Ready set
comment:8 Changed on 12/20/2017 at 01:35:17 PM by abpbot
A commit referencing this issue has landed:
Issue 6201 - Fix the "Hide targeted messages?" notification
comment:9 Changed on 12/20/2017 at 01:35:50 PM by kzar
- Resolution set to fixed
- Status changed from reviewing to closed
comment:10 Changed on 12/20/2017 at 01:54:44 PM by kzar
- Blocking 6208 added
comment:11 Changed on 02/07/2018 at 05:19:39 PM 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 on 02/13/2018 at 05:48:29 PM 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 on 03/18/2018 at 09:08:12 PM 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 on 03/18/2018 at 11:08:39 PM by sebastian
- Resolution set to fixed
- Status changed from reopened to closed
I filed a separate issue (#6496) for this.
comment:15 Changed on 03/20/2018 at 09:39:36 AM 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
Which revisions do 1865 and 1866 refer to?