Opened on 03/19/2017 at 04:54:33 AM
Closed on 03/19/2017 at 12:24:26 PM
Last modified on 03/21/2017 at 12:30:36 PM
#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): | |||
Description (last modified by sebastian)
Environment
Adblock Plus 1.13.1
Chrome 56
How to reproduce
- 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.
- 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:
- Issue 4762 - Added "relentless" notification that shows up in intervals
- Fixing previous commit, consider dependency update importing issue 3672 changes properly
Related discussions:
Attachments (0)
Change History (18)
comment:1 Changed on 03/19/2017 at 06:41:09 AM by kzar
- Cc fhd added
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:2 Changed on 03/19/2017 at 06:42:11 AM by kzar
- Cc greiner added
(Adding Thomas to Cc since this relates to adblockplusui.)
comment:3 Changed on 03/19/2017 at 07:05:45 AM 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 on 03/19/2017 at 09:45:48 AM 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 on 03/19/2017 at 10:06:10 AM by kzar
- Review URL(s) modified (diff)
Cool that works, uploaded a new review.
comment:6 Changed on 03/19/2017 at 12:20:39 PM by abpbot
A commit referencing this issue has landed:
Issue 5014 - Fix broken "Hide targeted messages?" notifications
comment:7 Changed on 03/19/2017 at 12:24:26 PM 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 on 03/20/2017 at 04:56:01 PM 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: ↓ 12 Changed on 03/20/2017 at 05:05:08 PM 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.
comment:10 Changed on 03/20/2017 at 05:22:07 PM 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 on 03/20/2017 at 05:35:45 PM 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: ↓ 14 Changed on 03/20/2017 at 05:41:27 PM 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 on 03/20/2017 at 07:42:22 PM 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: ↓ 16 Changed on 03/21/2017 at 10:51:39 AM 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 on 03/21/2017 at 11:04:21 AM by sebastian
- Description modified (diff)
comment:16 in reply to: ↑ 14 ; follow-up: ↓ 17 Changed on 03/21/2017 at 11:20:28 AM 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 on 03/21/2017 at 12:18:03 PM 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 on 03/21/2017 at 12:30:36 PM 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
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.