1137613 - [System] The animation that shows a notification feels sluggish

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

STR:
1. receive a SMS (or take a screenshot)

Total animation time is 1.4sec.
Look at the animation:

1. first .36s: the ambient indicator appears progressively, it takes 86px only in the center of the screen.
2. next .36s: nothing happens, this is "pause".
3. next .48s: the ambient indicator width progressively increases until taking the whole available width.
4. next .2s: the notification box is progressively displayed.


This is a very long time, but I don't object this, as we usually don't need to see notifications right away. However, the pause at step 2 makes the whole animation feels sluggish. We think the phone is lagging. Tiffanie things the same in bug 988530 comment 25. 

I think we should change this in v2.2. Yes it's polish, but it's not risky at all because it's all done in CSS.

Proposal:
* get rid of the pause (step 3).
* makes the other various steps shorter. .2s for steps 1 and 3 should be more than enough. We need to look at how it looks like on the device because animating the width _can_ be sluggish though.

NI Tiffanie: what do you think?

Comment 1

[Tiffanie Shakespeare [:tif]]

I don't see an issue with the proposal but Eric is going to have final say.

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] julienw:1137613-notification-animation > mozilla-b2g:master

Comment 3

[Julien Wajsberg [:julienw]]

Comment on attachment 8570877 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master

Here is a proposal if you want to test it.

Comment 4

[Eric Pang [:epang] UX]

(In reply to Tiffanie Shakespeare from comment #1)
> I don't see an issue with the proposal but Eric is going to have final say.

Thanks for flagging me on this Tiff!  This is fine by me, thanks Julien for making a patch :).

Comment 5

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

Comment on attachment 8570877 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master

For the record, I don't think the old animation felt sluggish, since even during the pause it was animating the background color of the "nub". I think it's sad that we can't see that color transition  anymore, I thought it was a nice effect.

That said, if everyone else is on board with this change I won't block it. I left a comment on github about css properties, but code looks fine overall.

Comment 6

[Gregor Wagner [:gwagner]]

Not blocking.

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8570877 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master

Let's do another round of review after these changes.

Comment 8

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

Comment on attachment 8570877 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master

Feels better to me. Make sure you squash :)

Comment 9

[Eric Pang [:epang] UX]

Comment on attachment 8570877 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master

Amazing what a fraction of a second does.  Looks good to me R+
Thanks for making the change Julien!

Comment 10

[Julien Wajsberg [:julienw]]

Thanks guys !

master: 7f1d04fb1ffa72578b75c7ebba64ed221fbc230e

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8570877 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1042713
[User impact] if declined: the animation looks sluggish and the phone seems to be slow.
[Testing completed]: yes, mostly manually
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none

Comment 12

[Kevin Grandon :kgrandon]

Backed out for likely causing Gij failures when landing: https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=b47bc9275d63

Backout commit: https://github.com/mozilla-b2g/gaia/commit/3fc0ac309f5fb0c1fe82c12223b955a4efce27e6

Logs: http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/b2g-inbound-linux64_gecko/1425414827/b2g-inbound_ubuntu64_vm-b2gdt_test-gaia-js-integration-7-bm67-tests1-linux64-build46.txt.gz

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8570877 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master

Clearing approval until this re-lands.

Comment 14

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=b2g-inbound&job_id=1441058
repository: b2g-inbound
start_time: 2015-03-03T13:05:36
who: kgrandon[at]mozilla[dot]com
machine: tst-linux64-spot-118
buildname: b2g_ubuntu64_vm b2g-inbound opt test gaia-js-integration-7
revision: b47bc9275d63

TEST-UNEXPECTED-FAIL | apps/system/test/marionette/notification_test.js | notification tests swipe up should hide the toast
AssertionError: true == false

Comment 15

[Julien Wajsberg [:julienw]]

Sounds likely.

Too bad we don't have a screenshot for "normal" assertion errors :(

Comment 16

[Julien Wajsberg [:julienw]]

Never failed once locally :(

Comment 17

[Julien Wajsberg [:julienw]]

It doesn't fail on my slow computer either :/ I don't have a clue.

Comment 18

[Kevin Grandon :kgrandon]

Have you tried adding a screenshot to the test when it runs on treeherder? The test seems pretty simple, so based on the patch I wonder if we're too slow when trying to close the notification now? Does it hide any earlier than before?

Comment 19

[Julien Wajsberg [:julienw]]

The problem is that the test ends before the "based on Mozilla" overlay is removed. So we can't see anything.

Comment 20

[Chris Lord [:cwiiis]]

Removing profiling backlog, this isn't a performance issue - the animation just has a long pause defined in it.

Comment 21

[Julien Wajsberg [:julienw]]

Mmm I wonder if we still have the issue now that bug 1062109 landed.

I'm trying to rebase.

Comment 22

[GitHub Autolander Bot]

Attachment: [gaia] julienw:1137613-notification-animation > mozilla-b2g:master

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 8578773 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master

carrying over r=mhenretty and ui-r=omega

Comment 24

[Julien Wajsberg [:julienw]]

Thanks to bug 1062109, the test is no more intermittent and is consistently failing with the patch. This makes things easier :)

So I think I found the issue: because the element is always displayed after the patch (I removed the visibility easing), ".displayed()" returns before the notification is displayed, and the "slide up" flick does nothing. The "right" way would be to wait the end of the animation before doing anything in the marionette test, but I don't really know how to do this.

So instead, I've put back the visibility easing, which makes marionette's "displayed()" behave like we want it to. I'd need to check on a device how this looks like before merging, but I hope that it looks right now that bug 1137203 is fixed.

I updated the PR and will check tomorrow.

Comment 25

[Julien Wajsberg [:julienw]]

I checked on a device and it looks great. I updated the PR too, with some comments to explain some bits to a possible reader.

Now I want to wait for a good gaia try, but because gaia try is lying, I'll wait until tomorrow instead ;) I hope I'll still be able to uplift to 2.2 !

Comment 26

[Julien Wajsberg [:julienw]]

Found a failing test (notification_behavior_test.js) but it seems to fail on master too, so merging.

Comment 27

[Julien Wajsberg [:julienw]]

Comment on attachment 8578773 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master

[Approval Request Comment]
See comment 11.

This is a low risk patch, that's why I'm asking for approval for v2.2. The issues we had were solely test issues, not code issues.

Comment 28

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/1aec7e584f1b6d6bfe5b1c2afe1985b3e64c84ef

Comment 29

[bhavana bajaj [:bajaj]]

Comment on attachment 8578773 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master

We should backout if there are any fallouts due to this, rather than forward fixing things.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/50d70c79035fc47b8a96d73013fa0985f2f38a12