[System] The animation that shows a notification feels sluggish

RESOLVED FIXED in Firefox OS v2.2

Status

Firefox OS
Gaia::System
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

({polish})

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

Firefox Tracking Flags

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

Details

(Whiteboard: [systemsfe])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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)
(Assignee)

Updated

3 years ago
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM

Updated

3 years ago
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)
Created attachment 8570877 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master
(Assignee)

Comment 3

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

Here is a proposal if you want to test it.
(Assignee)

Updated

3 years ago
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? → ---
(Assignee)

Updated

3 years ago
Flags: needinfo?(felash)
(Assignee)

Comment 7

3 years ago
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+
(Assignee)

Comment 10

3 years ago
Thanks guys !

master: 7f1d04fb1ffa72578b75c7ebba64ed221fbc230e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

3 years ago
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?
Comment hidden (Treeherder Robot)
(Assignee)

Comment 15

3 years ago
Sounds likely.

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

Comment 16

3 years ago
Never failed once locally :(
(Assignee)

Comment 17

3 years ago
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?
(Assignee)

Comment 19

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

Updated

3 years ago
Blocks: 1094010
Removing profiling backlog, this isn't a performance issue - the animation just has a long pause defined in it.
No longer blocks: 1094010
(Assignee)

Comment 21

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

I'm trying to rebase.
Created attachment 8578773 [details] [review]
[gaia] julienw:1137613-notification-animation > mozilla-b2g:master
(Assignee)

Updated

3 years ago
Attachment #8570877 - Attachment is obsolete: true
Flags: needinfo?(felash)
(Assignee)

Comment 23

3 years ago
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+
(Assignee)

Updated

3 years ago
Flags: needinfo?(felash)
(Assignee)

Comment 24

3 years ago
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.
(Assignee)

Comment 25

3 years ago
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 !
(Assignee)

Comment 26

3 years ago
Found a failing test (notification_behavior_test.js) but it seems to fail on master too, so merging.
Flags: needinfo?(felash)
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 27

3 years ago
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?

Updated

3 years ago
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 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+
v2.2: https://github.com/mozilla-b2g/gaia/commit/50d70c79035fc47b8a96d73013fa0985f2f38a12
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
Target Milestone: --- → 2.2 S9 (3apr)
You need to log in before you can comment on or make changes to this bug.