[System] The animation that shows a notification feels sluggish

RESOLVED FIXED in 2.2 S9 (3apr)

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

({polish})

unspecified
2.2 S9 (3apr)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [systemsfe])

Attachments

(1 attachment, 1 obsolete attachment)

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?
Flags: needinfo?(tshakespeare)
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Keywords: polish
Whiteboard: [systemsfe]
I don't see an issue with the proposal but Eric is going to have final say.
Flags: needinfo?(tshakespeare) → needinfo?(epang)
Comment on attachment 8570877 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master

Here is a proposal if you want to test it.
Attachment #8570877 - Flags: review?(mhenretty)
(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 :).
Flags: needinfo?(epang)
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.
Attachment #8570877 - Flags: review?(mhenretty) → review+
Not blocking.
Assignee: nobody → felash
blocking-b2g: 2.2? → ---
Flags: needinfo?(felash)
Comment on attachment 8570877 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master

Let's do another round of review after these changes.
Flags: needinfo?(felash)
Attachment #8570877 - Flags: ui-review?(epang)
Attachment #8570877 - Flags: review?(mhenretty)
Attachment #8570877 - Flags: review+
Comment on attachment 8570877 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master

Feels better to me. Make sure you squash :)
Attachment #8570877 - Flags: review?(mhenretty) → review+
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!
Attachment #8570877 - Flags: ui-review?(epang) → ui-review+
Thanks guys !

master: 7f1d04fb1ffa72578b75c7ebba64ed221fbc230e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
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
Attachment #8570877 - Flags: approval-gaia-v2.2?
Comment on attachment 8570877 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master

Clearing approval until this re-lands.
Attachment #8570877 - Flags: approval-gaia-v2.2?
Sounds likely.

Too bad we don't have a screenshot for "normal" assertion errors :(
Never failed once locally :(
It doesn't fail on my slow computer either :/ I don't have a clue.
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?
The problem is that the test ends before the "based on Mozilla" overlay is removed. So we can't see anything.
Removing profiling backlog, this isn't a performance issue - the animation just has a long pause defined in it.
No longer blocks: 1094010
Mmm I wonder if we still have the issue now that bug 1062109 landed.

I'm trying to rebase.
Attachment #8570877 - Attachment is obsolete: true
Flags: needinfo?(felash)
Comment on attachment 8578773 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master

carrying over r=mhenretty and ui-r=omega
Attachment #8578773 - Flags: ui-review+
Attachment #8578773 - Flags: review+
Flags: needinfo?(felash)
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.
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 !
Found a failing test (notification_behavior_test.js) but it seems to fail on master too, so merging.
Flags: needinfo?(felash)
Keywords: checkin-needed
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.
Attachment #8578773 - Flags: approval-gaia-v2.2?
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
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.
Attachment #8578773 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
You need to log in before you can comment on or make changes to this bug.