Opened 5 months ago

Closed 5 months ago

Last modified 5 weeks ago

#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.

Change History (13)

comment:1 Changed 5 months ago by greiner

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.

comment:2 Changed 5 months ago 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 5 months ago 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 5 months ago 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?)

Last edited 5 months ago by kzar (previous) (diff)

comment:5 Changed 5 months ago 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 5 months ago 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 5 months ago by greiner

  • Priority changed from Unknown to P2
  • Ready set

comment:8 Changed 5 months ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:9 Changed 5 months ago 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 5 months ago by greiner

  • Blocking 7416 added

comment:11 Changed 5 months ago by abpbot

A commit referencing this issue has landed:
Issue 7408 - Remove window.confirm question notification fallback

comment:12 Changed 5 months ago 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 5 weeks ago 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

Note: See TracTickets for help on using tickets.