Opened on 03/26/2019 at 03:48:32 PM
Closed on 03/28/2019 at 01:09:50 PM
Last modified on 07/21/2019 at 09:01:08 PM
#7408 closed change (fixed)
Remove window.confirm question notification fallback
| Reported by: | kzar | Assignee: | kzar |
|---|---|---|---|
| Priority: | P2 | Milestone: | Adblock-Plus-for-Chrome-Opera-Firefox-next |
| Module: | User-Interface | Keywords: | manifestv3 |
| Cc: | greiner, sebastian | Blocked By: | |
| Blocking: | #7338, #7416 | Platform: | Unknown / Cross platform |
| Ready: | yes | Confidential: | no |
| Tester: | Ross | Verified working: | yes |
| Review URL(s): |
https://gitlab.com/eyeo/adblockplus/adblockpluschrome/merge_requests/52 |
||
Description (last modified by kzar)
Background
With #7338 we are forced to stop using certain APIs from the background page. One such API is confirm which we use as a fallback for question notifications in the situation where browser.notifications isn't available for use (on Microsoft Edge.)
What to change
Remove the clause containing confirm in adblockpluschrome/lib/notificationHelper.js entirely.
Hints for testers
We no longer expect notifications of type "question" to be displayed on Edge.
Attachments (0)
Change History (13)
comment:1 Changed on 03/26/2019 at 03:59:32 PM by greiner
comment:2 Changed on 03/26/2019 at 04:05:50 PM by kzar
Well, up to you. So long as we stop using window.confirm I'm happy. (To be honest there's no objection from me to nuke notifications entirely, the code is rather a mess, but I figure you guys might object :p)
comment:3 Changed on 03/26/2019 at 04:54:18 PM by greiner
It is a mess and I'd also be happy to get rid of it but there are lots of dependencies. The best offer I have is to not make the upcoming new notification system backwards-compatible so that we can truly start from scratch. While that suggestion has already been rejected by Felix, I'll try pushing for it again to at least keep the client-side logic at a minimum.
comment:4 Changed on 03/26/2019 at 05:01:48 PM by kzar
In the mean time, what do you think to just removing this clause. So notifications of type "question" just no longer get displayed on Edge, instead of using this window.confirm API as a workaround?
(Perhaps it's for the best anyway, it seems to me that we display the notification_with_buttons string there which is "Click 'OK' to open all links in this notification." but in actually it should be more like "Click 'OK' to agree" surely?)
comment:5 Changed on 03/26/2019 at 05:05:42 PM by greiner
Replying to kzar:
In the mean time, what do you think to just removing this clause. So notifications of type "question" just no longer get displayed on Edge, instead of using this window.confirm API as a workaround?
That'd be fine with me.
comment:6 Changed on 03/26/2019 at 05:29:56 PM by kzar
- Description modified (diff)
- Owner set to kzar
- Summary changed from Replace window.confirm question notification fallback to Remove window.confirm question notification fallback
Cool OK, I'll go ahead with that then.
comment:7 Changed on 03/26/2019 at 05:37:26 PM by greiner
- Priority changed from Unknown to P2
- Ready set
comment:8 Changed on 03/27/2019 at 12:30:00 PM by kzar
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:9 Changed on 03/27/2019 at 07:56:03 PM by sebastian
For reference, the Anti-Adblock filter list isn't subscribed on Microsoft Edge in the first place. So the only way to hit the code patch using confirm() is by manually subscribing it.
comment:10 Changed on 03/28/2019 at 12:58:04 PM by greiner
- Blocking 7416 added
comment:11 Changed on 03/28/2019 at 01:08:40 PM by abpbot
A commit referencing this issue has landed:
Issue 7408 - Remove window.confirm question notification fallback
comment:12 Changed on 03/28/2019 at 01:09:50 PM by kzar
- Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
- Resolution set to fixed
- Status changed from reviewing to closed
comment:13 Changed on 07/21/2019 at 09:01:08 PM by Ross
- Tester changed from Unknown to Ross
- Verified working set
Done. Working as described.
ABP 3.5.2.2339
Microsoft Edge 44.17763.1.0
We intend to remove the anti-adblock notification (see private ticket ui#43) but we haven't gotten around to do so yet and we were waiting for various stakeholders. But having said that, I'd rather remove that entire thing than add more workarounds.