Opened 3 months ago

Closed 3 months ago

Last modified 6 weeks ago

#6922 closed defect (fixed)

Allows any kind of notification in the new Bubble UI

Reported by: agiammarchi Assignee: agiammarchi
Priority: P1 Milestone:
Module: User-Interface Keywords:
Cc: kzar, greiner, sebastian Blocked By:
Blocking: #6892 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://gitlab.com/eyeo/adblockplus/abpui/adblockplusui/merge_requests/101

Description

While brainstorming of a possible new kind of notifications, as per https://gitlab.com/eyeo/adblockplus/abpui/adblockplusui/issues/179, we've noticed the latest Bubble UI doesn't actually show properly unknown notifications.

Currently, the Bubble UI only shows valid icons and colors for critical or information type of notifications, ignoring everything else or, actually, showing bad UI around other kinds.

Change History (15)

comment:1 Changed 3 months ago by agiammarchi

  • Blocking 6892 added

comment:2 Changed 3 months ago by greiner

  • Owner set to agiammarchi
  • Priority changed from Unknown to P1
  • Ready set

comment:3 Changed 3 months ago by greiner

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

comment:4 Changed 3 months ago by greiner

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:5 Changed 3 months ago by abpbot

A commit referencing this issue has landed:
Issue 6922 - Provide a fallback for unknown notifications type

comment:6 follow-up: Changed 7 weeks ago by Ross

I cannot seem to get this to work.

If I get Chrome to load a normal notification, the desktop notification appears but there is nothing in the Bubble UI.

If I get Chrome to load a critical notification, it also displays but not in the Bubble UI. Also, the ! animation seems to be locked to the tab the user was on when the notification was received. If they switch to another tab, the ABP icon is reset to normal and does not continue to animate. If the user then swtiches back to the tab before, the animations is continued again. This is different to the old behaviour.

comment:7 Changed 7 weeks ago by kzar

  • Cc kzar added

comment:8 Changed 7 weeks ago by greiner

I cannot seem to get this to work.

Can you try a notification that has an undefined type? (e.g. "foo") It is expected that a "normal" type notification doesn't show up in the bubble UI. In case it helps, I'm currently working on updating the notifications spec to clarify the expected behavior of each notification type so feel free to check it out here.

The issues you mentioned in the last paragraph indeed sound like bugs.

comment:9 Changed 7 weeks ago by greiner

  • Cc greiner added

comment:10 in reply to: ↑ 6 Changed 7 weeks ago by kzar

Replying to Ross:

If I get Chrome to load a critical notification, it also displays but not in the Bubble UI. Also, the ! animation seems to be locked to the tab the user was on when the notification was received. If they switch to another tab, the ABP icon is reset to normal and does not continue to animate. If the user then swtiches back to the tab before, the animations is continued again. This is different to the old behaviour.

I have failed to reproduce that on both Chrome 49 and 70 on Debian Testing so far. Would you mind opening an issue, with steps to reproduce? Would you also mind checking to see if this appears to be a regression since the previous release?

FWIW I added this snippet to the end of adblockpluschrome/lib/notificationHelper.js, rebuilt the extension, then navigated to example.com. The icon animations worked OK, even when I switched between tabs. When I dismissed the notification the animation stopped.

require("notification").Notification.addNotification({
  id: new Date() | 0,
  type: "critical",
  title: "Notification title",
  message: "<a>Open contribution page</a>",
  links: ["contribute"],
  urlFilters: ["||example.com^$document"]
});

comment:11 Changed 7 weeks ago by sebastian

  • Cc sebastian added

comment:12 Changed 6 weeks ago by Ross

Ah okay. I think there is still an issue but I haven't narrowed it down yet. Thank you for the new spec link with the tables, it's really helpful. I'm halfway through testing and will have another update soon.

comment:13 Changed 6 weeks ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

I've opened #7077 for the critical icon animation. It's not a regression as I can reproduce in current release.

For the purpose of this ticket, the bubble UI displays notifications of an unknown type as expected (according to spec).

ABP 3.3.2.2176
Chrome 70 / Chrome 49 / Windows 10
Firefox 63 / Firefox 51 / Windows 10
Opera 56 / 36 / Windows 10

comment:14 follow-up: Changed 6 weeks ago by agiammarchi

Thanks Ross. I wonder since #7077 is not a regression if it should be considered a release blocker and, in such case, if there's anything I can do to help (I'm familiar with the notification part but not with the icon, so I need to investigate a bit in case it's needed).

Best Regards

comment:15 in reply to: ↑ 14 Changed 6 weeks ago by kzar

Replying to agiammarchi:

I wonder since #7077 is not a regression if it should be considered a release blocker...

Since this behaviour isn't a regression since the previous release, and isn't too serious I don't think it should be considered a release blocker.

...if there's anything I can do to help...

I'll take a look into this again now, but if I get stuck I might take you up on that. Thanks for the offer!

Note: See TracTickets for help on using tickets.