Opened 6 weeks ago

Closed 6 weeks ago

Last modified 5 weeks ago

#5014 closed defect (fixed)

Broken "Hide targeted messages?" notifications

Reported by: kzar Assignee: kzar
Priority: P1 Milestone: Adblock-Plus-1.13.2-for-Chrome-Opera
Module: Platform Keywords:
Cc: wspee, trev, sebastian, mapx, fhd, greiner, Ross, scheer, rraceanu Blocked By:
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29388608/

Description (last modified by sebastian)

Environment

Adblock Plus 1.13.1
Chrome 56

How to reproduce

  1. Make sure the Adblock Warning Removal List is subscribed but disabled, has already been downloaded before Adlock Plus was loaded (see #5019), and that the related notification hasn't been dismissed before, e.g. by (re)installing Adblock Plus and restarting the browser after confirming that the subscriptions have been successfully downloaded.
  2. Browse to https://forums.nexusmods.com/index.php?c=411,419

Observed behaviour

The notification popup is displayed with no text at all.

Expected behaviour

The notification should have the "Hide targeted messages? ..." message as it did with the previous 1.12.4 release.

Notes

This appears to be a regression with one or both of these commits, which apparently weren't tested by QA:

Related discussions:

Change History (18)

comment:1 Changed 6 weeks ago by kzar

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

I am not familiar with the localisation code but it appears the problem is caused by the ext.i18n.getMessage calls in antiadblockInit.js. For example ext.i18n.getMessage("notification_antiadblock_title") instead of ext.i18n.getMessage("global_notification_antiadblock_title"). Switching the calls to use Utils.getString instead, for example Utils.getString("notification_antiadblock_title"), seems to work for both adblockplus and adblockpluschrome. (I used my eslint-test branch for testing adblockplus.)

Unfortunately I think this problem might warrant a second emergency release. Worse still the ESLint changes already landed in adblockplusui which we definitely shouldn't include in such a release. So we will need to release using an earlier version of adblockplusui with the fix applied. I'll get a second review up now for that change to help speed things up.

comment:2 Changed 6 weeks ago by kzar

  • Cc greiner added

(Adding Thomas to Cc since this relates to adblockplusui.)

comment:3 Changed 6 weeks ago by kzar

  • Review URL(s) modified (diff)

As promised I've backported those changes to apply to 144b2198215e and tested they work for adblockpluschrome.

comment:4 Changed 6 weeks ago by trev

I think a proper fix in this case will be easier than the backout currently under review. We can import these strings without changing the string names, the IDs need to be prefixed with = in metadata.common for that.

comment:5 Changed 6 weeks ago by kzar

  • Review URL(s) modified (diff)

Cool that works, uploaded a new review.

comment:6 Changed 6 weeks ago by abpbot

A commit referencing this issue has landed:
Issue 5014 - Fix broken "Hide targeted messages?" notifications

comment:7 Changed 6 weeks ago by kzar

  • Cc Ross scheer rraceanu added
  • Milestone set to Adblock-Plus-for-Chrome-Opera-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

Guys please could you test this fix, we're probably going to have to an emergency release for it.

comment:8 Changed 5 weeks ago by Ross

I now get no notification at all when browsing that forum. Is that the intended fix?

ABP 1.13.1749
Chrome 56 / Windows 10

comment:9 follow-up: Changed 5 weeks ago by mapx

I tested right now: the notification is ON for me, but:

  • if I select another tab the box is still there. ABP should read the focused tab and remove the notification box.

Is this the intended behaviour ? it's confusing ..

  • other issue:

"Adblock Warning Removal List" is disabled in my ABP but I still get the notification.

Last edited 5 weeks ago by mapx (previous) (diff)

comment:10 Changed 5 weeks ago by sebastian

Yes, that notification is only supposed to show up, if you have the Adblock Warning Removal List subscribed but disabled, which it should be after a fresh installation of Adblock Plus.

mapx, did the behavior you are expecting have been working with any past version of Adblock Plus for Chrome? (In case you want to check with Adblock Plus 1.12.4, you can download it here.)

comment:11 Changed 5 weeks ago by mapx

yeah, I think you're right, but in 1.12.4 nothing works for me:

  • the list is disabled, no notification appears
  • I enabled the list ...and the notification appears
  • I repeated the tests: now the notification does not appear anymore (the list ON / OFF)

So, it's all pretty confusing. As I said in some other place, it's a "feature" which serves no useful case / need.

comment:12 in reply to: ↑ 9 ; follow-up: Changed 5 weeks ago by greiner

Replying to Ross:

I now get no notification at all when browsing that forum. Is that the intended fix?

I can reproduce that on a fresh installation but it does work after disabling and reenabling the extension. Does this match your observations as well?

Replying to mapx:

  • if I select another tab the box is still there. ABP should read the focused tab and remove the notification box.

Is this the intended behaviour ? it's confusing ..

Since the notification UI is not bound to the tab it's tricky to say what the expected behavior is supposed to be.

"Adblock Warning Removal List" is disabled in my ABP but I still get the notification.

We only disable the notification when you disable, enable or remove the filter list. The reason for that is that those clearly state the user's intention so there's no need for showing the notification anymore.

comment:13 Changed 5 weeks ago by greiner

FYI: I created #5019 for the issue regarding the notification not being added on fresh installations.

comment:14 in reply to: ↑ 12 ; follow-up: Changed 5 weeks ago by Ross

Replying to greiner:

I can reproduce that on a fresh installation but it does work after disabling and reenabling the extension. Does this match your observations as well?

I still cannot see the notification after various actions like disable/reenable the extension, the list or the browser. I'm thinking retesting as soon as the fix for #5019 in too would be the best action?

comment:15 Changed 5 weeks ago by sebastian

  • Description modified (diff)

comment:16 in reply to: ↑ 14 ; follow-up: Changed 5 weeks ago by greiner

Replying to Ross:

I still cannot see the notification after various actions like disable/reenable the extension, the list or the browser. I'm thinking retesting as soon as the fix for #5019 in too would be the best action?

I'm not sure whether #5019 will help you with that. Did you check whether the filter list download succeeded or whether any other errors occured during initialization?

comment:17 in reply to: ↑ 16 Changed 5 weeks ago by Ross

Replying to greiner:

Replying to Ross:

I still cannot see the notification after various actions like disable/reenable the extension, the list or the browser. I'm thinking retesting as soon as the fix for #5019 in too would be the best action?

I'm not sure whether #5019 will help you with that. Did you check whether the filter list download succeeded or whether any other errors occured during initialization?

It did succeed - I think (I've tried both just letting it download and using [Update now]) and there appear to be no errors in the console for the popup or background page. I'll try it again in a different VM/Chrome and locally.

comment:18 Changed 5 weeks ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Okay, it works fine in a local install and on the VM's. I was testing in the VM and could not see the notification because in Windows 10 it doesn't appear attached to the browser, it appears attached to the system tray in the bottom right (which was offscreen).

ABP 1.13.1.1749
Chrome 56 / Ubuntu 16.04
Chrome 49 / 56 / Windows 10

Note: See TracTickets for help on using tickets.