Opened on 11/02/2018 at 05:38:27 PM

Closed on 11/22/2018 at 12:16:03 PM

Last modified on 11/23/2018 at 11:08:08 AM

#7093 closed change (fixed)

Don't animate icon for notifications with URL filters

Reported by: greiner Assignee: greiner
Priority: P2 Milestone: Adblock-Plus-3.5-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: sebastian, kzar, wspee Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by greiner)


In ui#197 we are planning to add a notification targeted to a website the user visits.

We already have "urlFilters" for achieving that for the anti-adblock notification but we want to show a less obtrusive message in the icon popup instead. That means that both the icon and the message inside the icon popup should remain unchanged on tabs that are unrelated to the active notification.

While we can easily determine whether to show the message in the icon popup when opening it, the icon animation is global which means that we'd either have to ensure that it's only shown when a page is shown that matches any of the URL filters or that we don't animate it for any notifications with URL filters.
Looking through the code, I'd say that the latter seems far simpler.

What to change

Don't animate the toolbar icon if the active notification contains URL filters.

Attachments (0)

Change History (11)

comment:1 Changed on 11/02/2018 at 07:46:06 PM by sebastian

I wonder whether it makes sense to bind the appearance of the notification and the condition under which it is shown (e.g. urlFilters) together, or whether we should rather introduce a new severity (or a separate flag) for notifications that should not animate the icon?

comment:2 Changed on 11/05/2018 at 12:33:21 PM by greiner

  • Description modified (diff)

Each of those would be viable options and, admittedly, it's not ideal to make such behavior implicit which is why I'd like to disconnect the various display methods from notification types during the upcoming notification system revamp.

We could add a flag such as "sticky" to indicate that the notification should stick to any matching pages. However, since such a flag would only be relevant for notifications which have URL filters and since all such notifications are targeted to specific pages, it would seem more appropriate to make the behavior dependent on whether there are URL filters.

But please keep in mind that the main reason for disabling the icon animation is not because we don't want to animate the icon but because we can't animate the icon only for tabs with matching pages.
In comparison, if we were to just add a "animateIcon" flag, we may end up with the icon animating for a tab who doesn't have a message in the icon popup.

comment:3 Changed on 11/07/2018 at 05:03:03 PM by greiner

  • Blocking 6936 added

comment:4 Changed on 11/07/2018 at 05:12:53 PM by greiner

  • Blocking 7110 added; 6936 removed

comment:5 Changed on 11/20/2018 at 05:56:17 PM by greiner

  • Cc wspee added
  • Owner set to greiner
  • Review URL(s) modified (diff)

I've attached a review for the change I'm proposing. Admittedly, this is merely a workaround so I'd be happy to revisit this change soon after when we start tackling the notification system as a whole.

comment:6 Changed on 11/20/2018 at 08:12:22 PM by kzar

  • Priority changed from Unknown to P2
  • Ready set

Sounds pragmatic to me, especially if you guys plan to rewrite much of this logic anyway.

comment:7 Changed on 11/21/2018 at 11:06:45 AM by greiner

  • Status changed from new to reviewing

comment:8 Changed on 11/21/2018 at 12:08:09 PM by greiner

  • Blocking 7135 added; 7110 removed

comment:9 Changed on 11/22/2018 at 12:15:10 PM by abpbot

A commit referencing this issue has landed:
Issue 7093 - Only animate icon for global notifications

comment:10 Changed on 11/22/2018 at 12:16:03 PM by greiner

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:11 Changed on 11/23/2018 at 11:08:08 AM by greiner

  • Blocking 7135 removed

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from greiner.
Note: See TracTickets for help on using tickets.