979396 - Homescreen swipes end with canned animation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect, P2)

Description

[Gordon Brander :gordonb]

Home screen animates distance between release point and resting point using a fixed-time CSS animation.

This causes the performance of the home screen to appear significantly worse than it is in actual fact. Why? When I swipe quickly, there is a sudden slow down as soon as I release.

Steps to reproduce:

1. Go to home screen
2. Swipe screen slowly, release
3. Swipe screen quickly, release

What happens: ending animation is same velocity in both cases.
* Home screen tracks your finger 1:1 while dragging (touchmove). The code mutates a CSS property to do this.
* When you release (touchend), the code switches over to a CSS animation with a fixed velocity. This velocity does not match the velocity of your swipe, causing the appearance of poor responsiveness.

What should happen:
Animation after touchend should match velocity of swipe. A heavy easing curve should be used to make physics appear realistic.

We saw this problem play out painfully during MWC. Many users would try quickly swiping the homescreen as a gauge of Firefox OS' responsiveness. Most appeared disappointed.

This is likely a low-effort, high-impact improvement.

Related bugs:

#903773 caused by the fact that CSS animations will rip out frames when the system is memory-bound. requestAnimationFrame and property mutation might actually yield better results for this case.

#925540 also a frequent frustration for our MWC users. The threshold for "bounce back" should be smaller.

Comment 1

[Mike Lee [:mlee]]

Eli,

Once you've got your development environment setup. Take a look at this issue and see if Gordon's proposal in comment 0 is the solution we want to pursue. We haven't set a timeframe for delivering this fix so use this as an introduction to our bug tracking system, code repos, build, commit, and code review processes. Let me know if you have any questions.

Mike

fyi: This sentence hyperlinks bug 903773 and bug 925540 referenced in comment 0.

Comment 2

[Mike Lee [:mlee]]

Eli, please see comment 1.

Comment 3

[Cristian Rodriguez (:crdlc)]

Please see this code

https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/grid.js#L496

If the swipe time is less than 300ms -> animation time will be the swipe time
If the swipe time is greater than 300ms -> animation time will be 300ms

Comment 4

[:Eli Perelman]

Gordon,

I am having a little bit of difficulty in determining what the expected behavior of this should be, and differentiating it from the problem. Would you happen to be able to demo for me the problem and give a more detailed explanation of a resolution?

Eli

Comment 5

[Mike Lee [:mlee]]

Gordon, please see Eli's question in comment 4.

Eli, you can use the "Need more information from ..." checkbox on a bug's page to request information/comment from someone. It is usually a good way to notify someone that you need their response.

Comment 6

[:Eli Perelman]

After studying the problem some more, I think I have a better understanding of what is trying to be achieved here. I will work on this patch and submit for review shortly.

Comment 7

[:Eli Perelman]

Attachment: Patch Pull Request

Comment 8

[:Eli Perelman]

Gordon, I currently have a pending-review pull request open for this, but was wondering if you would be willing to give a UX review on it? Please let me know what I need to do to get the ball rolling on this.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8392311 [details] [review]
Patch Pull Request

I think the curve needs to be redesigned. Sometimes by panning super slowly the animation is super fast, and in many cases the animation is way faster than my pan gesture.

So I tend to think that Math.ceil is too aggressive here and results into creating artificial steps.

Any reasons to not simply: |var velocity = Math.max(1.0, Math.abs(panningResolver.getVelocity()));| ?

Also I see |lastGoingPageTimestamp += delay| twice in the code, which makes me feels a bit uncomfortable as delay sometimes results into a negative value and that's likely what results into fast transition for slow moves.

So I'm pretty sure the extra |lastGoingPageTimestamp| needs to be removed.

Lastly you probably want a constant for the |100|ms value, sitting next to kPageTransitionDuration and called kPageMinTransitionDuration.

That said, this patch based on the patches I have landed yesterday to reduce the latency when panning between panels seems good.

Comment 10

[:Eli Perelman]

Vivien, thanks for the review. I will pull your latest changes and see what I can do to incorporate your details into the implementation. Thanks!

Comment 11

[:Eli Perelman]

Vivien, you were correct about the |lastGoingPageTimestamp| twice in the code. That was a mistaken duplication on my part.

Comment 12

[:Eli Perelman]

Comment on attachment 8392311 [details] [review]
Patch Pull Request

This is ready to review again. The latest changes from master have been merged in, so the relevant changes are located here:

https://github.com/mozilla-b2g/gaia/pull/17250/files#diff-1cdbd1790a1033ed3a6efde03856200aR496

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8392311 [details] [review]
Patch Pull Request

I added a few comments. The code looks good overall but please fix the comment before trying to merge. Also a UX review is definitively needed.

Comment 14

[:Eli Perelman]

Code updated from the comments. I'll ping Gordon about a UX review. Thanks!

Comment 15

[Gordon Brander :gordonb]

The end result of the patch makes several improvements:

* End velocity maps to swipe velocity more closely -- no longer feels like you're fighting the physics.
* The distance you must swipe to go to next/prev panel has been reduced (I think this is Vivien's work).

However, the homescreen still has some other "uncanny valley" hand-eye coordination issues, listed below.

* Use velocity over certain threshold instead of swipe distance to trigger panel direction change. This allows for effortless "small swipes" with high velocity and prevents the issue we saw at MWC of users not swiping far enough to get where they want. See https://bugzilla.mozilla.org/show_bug.cgi?id=925540#c1.
* Lower the minimum animation speed (increase the time) to get more realistic response to slow swipes.
* Make finishing animation curve a function of velocity -- higher velocities should have heavier easing curves. This makes the velocity and slowdown look realistic -- no sudden jerky stops.

These issues are interrelated. Whether we try to tackle them in this patch or future patches, I defer to you.

Comment 16

[:Eli Perelman]

In my opinion, I think the original issue of the canned homescreen animation has been solved, and I would like to see these other issues be separate bugs for us to track and resolve.

Comment 17

[Cristian Rodriguez (:crdlc)]

I want to take a look to the patch just out of curiosity but I can see several files changed in the pr, am I wrong? Thanks a lot

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This PR can't be merged. Please rebase.

Also, Gordon, can you please formally set ui-review+ on the attachment?

Comment 19

[:Eli Perelman]

Ryan, my apologies. This should now be resolved in the PR.

Comment 20

[Gordon Brander :gordonb]

Comment on attachment 8392311 [details] [review]
Patch Pull Request

Review+. See additional comments https://bugzilla.mozilla.org/show_bug.cgi?id=979396#c15

Comment 21

[Ryan VanderMeulen [:RyanVM]]

The Travis failures look real?

Comment 22

[:Eli Perelman]

Yes, the failure was real. Come to find out, velocity evaluates to NaN if swiping using the automated tests. This is now resolved.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/c6b4c9a672646d702b4895f144a738cc44037803