1061854 - Notification Toaster: there is no animation when swiping a notification

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Robert Bindar]

Trying to swipe-close a notification when the toaster appears doesn't produce any animation, but the notification is closed.

Comment 1

[Michael Henretty [:mikehenrty][:mhenretty]]

[Blocking Requested - why for this release]: This is a regression from bug 1042713. It should be simple to fix.

Comment 2

[Alberto Pastor [:albertopq]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23728

According to :epang, the swipe-left gesture is no longer valid. This PR implements dismissing the notification by swipping up.

Comment 3

[Eric Pang [:epang] UX]

Comment on attachment 8484244 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23728

Looks good to me R+
I've added a ui-review for Rob to make sure the interaction is what he expected.

Thanks!

Comment 4

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8484244 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23728

This isn't working yet. A couple of things:

1.) If I swipe close a notification and there are no unread notifications in the tray, the ambient indicator should go away.

2.) The breaks the swipe to close notification animation we had in the utility tray. See bug 1062321 for why we had the cancelSwipe for the Y SCROLL_THRESHOLD path. We need to support both scrolling and swiping in the tray.

Comment 5

[Alberto Pastor [:albertopq]]

That makes sense. Thanks for the feedback! Working on it :)

Comment 6

[Rob MacDonald [:robmac]]

(In reply to Michael Henretty [:mhenretty] from comment #4)
> 
> 1.) If I swipe close a notification and there are no unread notifications in
> the tray, the ambient indicator should go away.
> 
> 2.) The breaks the swipe to close notification animation we had in the
> utility tray. See bug 1062321 for why we had the cancelSwipe for the Y
> SCROLL_THRESHOLD path. We need to support both scrolling and swiping in the
> tray.

Hi Alberto...

I'm having problems applying patches this evening unfortunately and may require some tech support tomorrow. In the meantime, though, I wanted to add to the comments above and apologize for the gap in the spec.

Here's my proposal for the intended behaviour for an open notification:

- Left / Right swipe clears the notification and is the same as the left / right swipe in the utility tray. As Michael stated, if the left right swipe is clearing the only notification, the ambient indicator should fade away.

- Swiping up on the notification hides the notification but does not clear it. This allows the user to continue their task and review the notification at a later time within the utility tray. The ambient indicator in this place is displayed.

Hopefully this makes sense and is feasible but NI or IRC me with any questions. I'll also minus my review flag until you have a chance to implement the changes.

Thanks!
Rob

Comment 7

[Rob MacDonald [:robmac]]

Comment on attachment 8484244 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23728

As per previous comment.

Comment 8

[Alberto Pastor [:albertopq]]

Hi Rob,

Thanks for the clarifications. I'm still not sure what would be the visual solution for when swiping left/right and you have unread notifications. Should the ambient indicator stay in the top of the screen while swiping? Should we just remove the ambient indicator and then fade it in after removing the notification? Not sure this behaviour is going to be understood by the user, though :S. But is just an opinion :)

Comment 9

[Alberto Pastor [:albertopq]]

After talking to Rob, we are going to remove the swipe left/right gesture for the toast. We keep it in the tray.

When swiping up the toast, it hides it but doesn't clear the ambient indicator.

Comment 10

[Michael Henretty [:mikehenrty][:mhenretty]]

I'll wait for the UI review to make sure we have the right functionality before doing code review.

Comment 11

[Rob MacDonald [:robmac]]

Comment on attachment 8484244 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23728

Iterated with Alberto on the patch this afternoon and it looks good. Thanks Alberto!

Comment 12

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8484244 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23728

Ok, I did a first pass on github, and left some implementation comments.

Beyond those, I think we have a problem with how we are dealing with unread notifications. It's not a glaring issue, but it can cause some strange behavior. Right now, we keep track of unread notifications using a count. But we never decrement this count, we only reset it back to zero. However, an app might close a notification without the user interacting with the utility tray. This could result in the ambient notification indicator being displayed even when there are no notifications in the tray. Sorry, I should have caught that earlier. We don't have to fix that in this bug, but if we don't we should file a follow-up now and fix it for 2.1 for sure.

Also, we need at least a unit test for this.

Comment 13

[Alberto Pastor [:albertopq]]

Thanks for the review, Michael.

I just submitted a new commit ammending your comments.
Regarding unit tests, you mean we need to add them about this bug (dismissing the toast) or about the ambient indicator itself? In the second case, there are already some tests (not added in this bug).
In the first case, wouldn't make more sense a UI test?

Thanks!

Comment 14

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Alberto Pastor [:albertopq] from comment #13)
> Thanks for the review, Michael.
> 
> I just submitted a new commit ammending your comments.
> Regarding unit tests, you mean we need to add them about this bug
> (dismissing the toast) or about the ambient indicator itself? In the second
> case, there are already some tests (not added in this bug).
> In the first case, wouldn't make more sense a UI test?

I was speaking about this bug. Integration tests (marionette-js) would be ideal, but that is harder and we have a bug 922431 to do a bunch of notification integration test work. Unit test is eaiser here, but if you want to take a stab at marionette js tests, I can definitely help out.

Comment 15

[Alberto Pastor [:albertopq]]

Added some UI tests. Please, let me know when you have time to take a look :)

Comment 16

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8484244 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23728

Alright, this is looking really good! And thank you for the integration tests. We've needed those.

One problem left. If I get a download notification (unread=1), then read it (unread=0), then I get an email notification (unread=1), and then the system removes the download notification (decExternalNotifications, unread=0), the ambient indicator will go away even though I have an unread email notification. Instead of a count, what we need is a list of unread notificationId's. Then when we add or remove notifications, we also add remove from this list, and we use this list for the unread count.

The other thing we need to make sure of is when we close a notification we remove it from the unread list.

Comment 17

[Alberto Pastor [:albertopq]]

Thanks for the review Michael.
Just created a follow up on Bug 1065519 for the cases you are presenting here, in order to close the scope of this bug to the swipe up gesture. Does that make sense?

Thanks!

Comment 18

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8484244 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23728

Looking good! Left a couple of small comments on github. Thanks for fixing this and filing the followup.

Comment 19

[Alberto Pastor [:albertopq]]

master: https://github.com/mozilla-b2g/gaia/commit/2758aee22e84b1fea731c4dec0e3f3bd80f4d916

Comment 20

[Alberto Pastor [:albertopq]]

Comment on attachment 8484244 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23728

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 1042713
[User impact] if declined: User is unable to dismiss notifications + if tries to swipe left/right the toast, a weird delayed animation plays.
[Testing completed]: Manual testing + added UI tests.
[Risk to taking this patch] (and alternatives if risky): Given the UI tests added, the risk is pretty low
[String changes made]: -

Comment 21

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/d4de4123c678b198692af256254a26507e9c9a08

Comment 22

[KTucker [:KTucker][Inactive 3/4/2016]]

This is verified fixed on the Flame 2.1 and the Flame 2.2. 

No weird animation happens from swiping left or right on a notification toast and the when the user swipes up on a notification toast it slides away smoothly. 

Flame 2.1 

Device: Flame 2.1 KK (319mb) (Full Flash)
BuildID: 20141012001201
Gaia: d18e130216cd3960cd327179364d9f71e42debda
Gecko: 610ee0e6a776
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Flame 2.2 

Device: Flame 2.2 Master  KK (319mb) (Full Flash)
BuildID: 20141012040203
Gaia: 717ad4e8b7fc10ab8248500d00ba5ba0977fa8ab
Gecko: 44168a7af20d
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0