Closed
Bug 979396
Opened 11 years ago
Closed 11 years ago
Homescreen swipes end with canned animation
Categories
(Firefox OS Graveyard :: Gaia::Homescreen, defect, P2)
Tracking
(b2g-v2.0 fixed)
RESOLVED
FIXED
2.0 S1 (9may)
Tracking | Status | |
---|---|---|
b2g-v2.0 | --- | fixed |
People
(Reporter: gbrander, Assigned: Eli)
References
Details
(Keywords: perf, Whiteboard: [c=handeye p=3 s= u=])
Attachments
(1 file)
(deleted),
text/x-github-pull-request
|
vingtetun
:
review+
gbrander
:
ui-review+
vingtetun
:
feedback+
|
Details |
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.
Reporter | ||
Updated•11 years ago
|
Summary: Homescreen swipes interpolate with canned animation → Homescreen swipes end with canned animation
Comment 1•11 years ago
|
||
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.
Status: NEW → ASSIGNED
OS: Mac OS X → Gonk (Firefox OS)
Priority: -- → P2
Hardware: x86 → ARM
Whiteboard: [c=handeye p= s= u=]
Comment 3•11 years ago
|
||
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
Assignee | ||
Comment 4•11 years ago
|
||
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•11 years ago
|
||
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.
Flags: needinfo?(gbrander)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [c=handeye p= s= u=] → [c=handeye p=3 s= u=]
Assignee | ||
Comment 6•11 years ago
|
||
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.
Flags: needinfo?(gbrander)
Assignee | ||
Comment 8•11 years ago
|
||
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.
Flags: needinfo?(gbrander)
Comment 9•11 years ago
|
||
Comment on attachment 8392311 [details]
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.
Attachment #8392311 -
Flags: review?(21)
Attachment #8392311 -
Flags: review-
Attachment #8392311 -
Flags: feedback+
Assignee | ||
Comment 10•11 years ago
|
||
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!
Assignee | ||
Comment 11•11 years ago
|
||
Vivien, you were correct about the |lastGoingPageTimestamp| twice in the code. That was a mistaken duplication on my part.
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 8392311 [details] 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
Attachment #8392311 -
Flags: review- → review?(21)
Comment 13•11 years ago
|
||
Comment on attachment 8392311 [details]
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.
Attachment #8392311 -
Flags: ui-review?(gbrander)
Attachment #8392311 -
Flags: review?(21)
Attachment #8392311 -
Flags: review+
Assignee | ||
Comment 14•11 years ago
|
||
Code updated from the comments. I'll ping Gordon about a UX review. Thanks!
Reporter | ||
Comment 15•11 years ago
|
||
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.
Flags: needinfo?(gbrander)
Assignee | ||
Comment 16•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 17•11 years ago
|
||
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•11 years ago
|
||
This PR can't be merged. Please rebase. Also, Gordon, can you please formally set ui-review+ on the attachment?
Flags: needinfo?(gbrander)
Keywords: checkin-needed
Reporter | ||
Comment 20•11 years ago
|
||
Comment on attachment 8392311 [details] Patch Pull Request Review+. See additional comments https://bugzilla.mozilla.org/show_bug.cgi?id=979396#c15
Attachment #8392311 -
Flags: ui-review?(gbrander) → ui-review+
Flags: needinfo?(gbrander)
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 22•11 years ago
|
||
Yes, the failure was real. Come to find out, velocity evaluates to NaN if swiping using the automated tests. This is now resolved.
Keywords: checkin-needed
Comment 23•11 years ago
|
||
Master: https://github.com/mozilla-b2g/gaia/commit/c6b4c9a672646d702b4895f144a738cc44037803
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-b2g-v2.0:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.5 S1 (9may)
You need to log in
before you can comment on or make changes to this bug.
Description
•