Opened 13 months ago

Closed 13 months ago

Last modified 13 months ago

#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):

https://codereview.adblockplus.org/29948561/diff/29948562/lib/notificationHelper.js

Description (last modified by greiner)

Background

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.

Change History (11)

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

  • Blocking 6936 added

comment:4 Changed 13 months ago by greiner

  • Blocking 7110 added; 6936 removed

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

  • Status changed from new to reviewing

comment:8 Changed 13 months ago by greiner

  • Blocking 7135 added; 7110 removed

comment:9 Changed 13 months ago by abpbot

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

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

  • Blocking 7135 removed
Note: See TracTickets for help on using tickets.