772940 - Swipe to close animation janky

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

Swipe to close animation can sometimes appear janky/spazzy. This happens if the tab title/thumbnail is updated during the animation.

STR:
1) Open a new tab for http://people.mozilla.com/~bnicholson/test/interval.html
2) Swipe to close the new tab

I've seen this several times during normal use, but it's difficult to reproduce it reliably without using this test case.

Comment 1

[Aaron Train [:aaronmt]]

Wes, is this covered in another bug?

Comment 2

[Wesley Johnston (:wesj)]

Nope. Please leave this open. Just not crazy high priority to me.

Comment 3

[Brian Nicholson (:bnicholson)]

Nom'ing for tracking. I see this pretty frequently, and it really makes our UI feel unpolished.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Brian Nicholson (:bnicholson) from comment #3)
> Nom'ing for tracking. I see this pretty frequently, and it really makes our
> UI feel unpolished.

I think we need to prepare a patch to turn off "swipe to close" if this animation is unacceptable. Swipe-to-close is not high priority, and neither would be a patch to fix this issue.

Comment 5

[Brian Nicholson (:bnicholson)]

I think disabling the animation is a good idea if we don't want to spend time fixing it. It doesn't seem like this problem is reproducible on all phones, but it looks pretty bad on my RAZR.

Comment 6

[Chris Peterson [:cpeterson]]

I see these janky tab animations on my Galaxy Nexus nearly all the time. It seems like most of the animation frames are not painted. The jank seems more apparent when swiping the tab to the right side.

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: (1/7) Add support for setting/getting alpha in AnimatorProxy

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: (2/7) Add support for alpha animations with PropertyAnimator

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: (3/7) Re-implement tab swipe for more robustness

Comment 10

[Lucas Rocha (:lucasr)]

Attachment: (4/7) Move scroll method up to AnimatorProxy as they are always the same

Comment 11

[Lucas Rocha (:lucasr)]

Attachment: (5/7) Add height setter/getter to AnimatorProxy

Comment 12

[Lucas Rocha (:lucasr)]

Attachment: (6/7) Add support for height animations with PropertyAnimator

Comment 13

[Lucas Rocha (:lucasr)]

Attachment: (7/7) Slide tab rows to fill gap of removed tab

Comment 14

[Lucas Rocha (:lucasr)]

These patches re-implement tab swipe logic entirely. Here are some core points:

- Uses proper system-wide values for swipe threshold, minimum and maximum fling velocities, tap timeout, etc.
- Uses hardware accelerated transformations and animations whenever available
- Uses proper API for avoiding scrolling while swiping (see requestDisallowInterceptTouchEvent) instead of the hacky weight-based vertical/horizontal scroll tracking
- Fixes flickering when swiping (pressed state shows and disappears before swipe starts)
- Fade tabs as user swipes to delete (bug 778625 and bug 766710)
- Fixes weird swiping behaviour (bug 787335)
- When the list has only one tab, the swiping "resists" the gesture but still swipes a big (just like Android's notification area)
- Slides remaining tabs to close the gap of the removed tab

These patches are based on the AnimatorProxy/PropertyAnimator work being done in bug 767980.

There's one remaining interaction issue that I'm trying to improve (sometimes it's too easy to trigger a fling) but the patches are ready for review. Here's a test build:

  https://dl.dropbox.com/u/1187037/new-tab-swipe.apk

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 667444 [details] [diff] [review]
(3/7) Re-implement tab swipe for more robustness

I think we could use some simple wiki docs for using the PropertyAnimator

Comment 16

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #15)
> Comment on attachment 667444 [details] [diff] [review]
> (3/7) Re-implement tab swipe for more robustness
> 
> I think we could use some simple wiki docs for using the PropertyAnimator

Good point. Added to my TODO list.

Comment 17

[Lucas Rocha (:lucasr)]

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f6acef4c469
https://hg.mozilla.org/integration/mozilla-inbound/rev/94ea88027451
https://hg.mozilla.org/integration/mozilla-inbound/rev/d15a22572dbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7e92a55d26f
https://hg.mozilla.org/integration/mozilla-inbound/rev/929e6a56f22f
https://hg.mozilla.org/integration/mozilla-inbound/rev/476e094d0e6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d419defdd425

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f6acef4c469
https://hg.mozilla.org/mozilla-central/rev/94ea88027451
https://hg.mozilla.org/mozilla-central/rev/d15a22572dbd
https://hg.mozilla.org/mozilla-central/rev/a7e92a55d26f
https://hg.mozilla.org/mozilla-central/rev/929e6a56f22f
https://hg.mozilla.org/mozilla-central/rev/476e094d0e6b
https://hg.mozilla.org/mozilla-central/rev/d419defdd425

Comment 19

[Aaron Train [:aaronmt]]

I think this or either bug 767980 has resulted in a black flash on startup. See bug 801477.

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 667442 [details] [diff] [review]
(1/7) Add support for setting/getting alpha in AnimatorProxy

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: This patch series reimplement swipe-to-close gesture in the tabs tray in a much more solid and polished way. The UX is greatly improved.
Testing completed (on m-c, etc.): These patches have been in m-c for a couple of weeks now, only issue found was bug 770492, which I've already fixed.
Risk to taking this patch (and alternatives if risky): It's a reimplementation of the feature so, it's not a small change. However, it's been in m-c for some time without any big issues.
String or UUID changes made by this patch: none.

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 667443 [details] [diff] [review]
(2/7) Add support for alpha animations with PropertyAnimator

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: This patch series reimplement swipe-to-close gesture in the tabs tray in a much more solid and polished way. The UX is greatly improved.
Testing completed (on m-c, etc.): These patches have been in m-c for a couple of weeks now, only issue found was bug 770492, which I've already fixed.
Risk to taking this patch (and alternatives if risky): It's a reimplementation of the feature so, it's not a small change. However, it's been in m-c for some time without any big issues.
String or UUID changes made by this patch: none.

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 667444 [details] [diff] [review]
(3/7) Re-implement tab swipe for more robustness

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: This patch series reimplement swipe-to-close gesture in the tabs tray in a much more solid and polished way. The UX is greatly improved.
Testing completed (on m-c, etc.): These patches have been in m-c for a couple of weeks now, only issue found was bug 770492, which I've already fixed.
Risk to taking this patch (and alternatives if risky): It's a reimplementation of the feature so, it's not a small change. However, it's been in m-c for some time without any big issues.
String or UUID changes made by this patch: none.

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 667445 [details] [diff] [review]
(4/7) Move scroll method up to AnimatorProxy as they are always the same

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: This patch series reimplement swipe-to-close gesture in the tabs tray in a much more solid and polished way. The UX is greatly improved.
Testing completed (on m-c, etc.): These patches have been in m-c for a couple of weeks now, only issue found was bug 770492, which I've already fixed.
Risk to taking this patch (and alternatives if risky): It's a reimplementation of the feature so, it's not a small change. However, it's been in m-c for some time without any big issues.
String or UUID changes made by this patch: none.

Comment 24

[Lucas Rocha (:lucasr)]

Comment on attachment 667446 [details] [diff] [review]
(5/7) Add height setter/getter to AnimatorProxy

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: This patch series reimplement swipe-to-close gesture in the tabs tray in a much more solid and polished way. The UX is greatly improved.
Testing completed (on m-c, etc.): These patches have been in m-c for a couple of weeks now, only issue found was bug 770492, which I've already fixed.
Risk to taking this patch (and alternatives if risky): It's a reimplementation of the feature so, it's not a small change. However, it's been in m-c for some time without any big issues.
String or UUID changes made by this patch: none.

Comment 25

[Lucas Rocha (:lucasr)]

Comment on attachment 667447 [details] [diff] [review]
(6/7) Add support for height animations with PropertyAnimator

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: This patch series reimplement swipe-to-close gesture in the tabs tray in a much more solid and polished way. The UX is greatly improved.
Testing completed (on m-c, etc.): These patches have been in m-c for a couple of weeks now, only issue found was bug 770492, which I've already fixed.
Risk to taking this patch (and alternatives if risky): It's a reimplementation of the feature so, it's not a small change. However, it's been in m-c for some time without any big issues.
String or UUID changes made by this patch: none.

Comment 26

[Lucas Rocha (:lucasr)]

Comment on attachment 667448 [details] [diff] [review]
(7/7) Slide tab rows to fill gap of removed tab

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: This patch series reimplement swipe-to-close gesture in the tabs tray in a much more solid and polished way. The UX is greatly improved.
Testing completed (on m-c, etc.): These patches have been in m-c for a couple of weeks now, only issue found was bug 770492, which I've already fixed.
Risk to taking this patch (and alternatives if risky): It's a reimplementation of the feature so, it's not a small change. However, it's been in m-c for some time without any big issues.
String or UUID changes made by this patch: none.

Comment 27

[bhavana bajaj [:bajaj]]

(In reply to Lucas Rocha (:lucasr) from comment #26)
> Comment on attachment 667448 [details] [diff] [review]
> (7/7) Slide tab rows to fill gap of removed tab
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): n/a
> User impact if declined: This patch series reimplement swipe-to-close
> gesture in the tabs tray in a much more solid and polished way. The UX is
> greatly improved.
> Testing completed (on m-c, etc.): These patches have been in m-c for a
> couple of weeks now, only issue found was bug 770492, which I've already
> fixed.
> Risk to taking this patch (and alternatives if risky): It's a
> reimplementation of the feature so, it's not a small change.


Can we maintain the status quo for this release and let these ride the trains instead of having them on aurora ? with so many patches I am concerned about the regressions this can cause once uplifted

 However, it's
> been in m-c for some time without any big issues.
> String or UUID changes made by this patch: none.

Comment 28

[bhavana bajaj [:bajaj]]

Comment on attachment 667442 [details] [diff] [review]
(1/7) Add support for setting/getting alpha in AnimatorProxy

As discussed offline with Lucas and Release Management team, we will have the enitire tab UX improvements ride the trains.