Closed Bug 772940 Opened 8 years ago Closed 7 years ago

Swipe to close animation janky

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 19
Tracking Status
firefox15 --- wontfix
firefox16 --- wontfix
firefox17 --- affected
firefox19 --- verified
fennec + ---

People

(Reporter: bnicholson, Assigned: lucasr)

References

Details

(Whiteboard: ui-responsiveness)

Attachments

(7 files)

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.
Wes, is this covered in another bug?
Component: Theme and Visual Design → General
Nope. Please leave this open. Just not crazy high priority to me.
Nom'ing for tracking. I see this pretty frequently, and it really makes our UI feel unpolished.
tracking-fennec: --- → ?
(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.
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.
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.
Assignee: nobody → lucasr.at.mozilla
tracking-fennec: ? → +
Whiteboard: ui-responsiveness
Attachment #667444 - Flags: review?(mark.finkle)
Attachment #667446 - Flags: review?(mark.finkle)
Attachment #667448 - Flags: review?(mark.finkle)
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
Blocks: 797987
Attachment #667442 - Flags: review?(mark.finkle) → review+
Attachment #667443 - Flags: review?(mark.finkle) → review+
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
Attachment #667444 - Flags: review?(mark.finkle) → review+
Attachment #667445 - Flags: review?(mark.finkle) → review+
Attachment #667446 - Flags: review?(mark.finkle) → review+
Attachment #667447 - Flags: review?(mark.finkle) → review+
Attachment #667448 - Flags: review?(mark.finkle) → review+
(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.
I think this or either bug 767980 has resulted in a black flash on startup. See bug 801477.
Status: RESOLVED → VERIFIED
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.
Attachment #667442 - Flags: approval-mozilla-aurora?
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.
Attachment #667443 - Flags: approval-mozilla-aurora?
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.
Attachment #667444 - Flags: approval-mozilla-aurora?
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.
Attachment #667445 - Flags: approval-mozilla-aurora?
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.
Attachment #667446 - Flags: approval-mozilla-aurora?
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.
Attachment #667447 - Flags: approval-mozilla-aurora?
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.
Attachment #667448 - Flags: approval-mozilla-aurora?
(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 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.
Attachment #667442 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #667443 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #667444 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #667445 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #667446 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #667447 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #667448 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.