The default bug view has changed. See this FAQ.

Close by swipe velocity checks are wrong

RESOLVED FIXED in Firefox 15

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: wesj, Unassigned)

Tracking

Trunk
Firefox 16
Points:
---

Firefox Tracking Flags

(firefox15 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Created attachment 633305 [details] [diff] [review]
Patch

So when I first wrote close by swipe I threw in a min velocity of 1000px/sec based on some testing on my end. Then, I changed that to be dpi independent, but forgot to update the value. So right now, you have to fling at > 1000 in/sec for the fling to trigger slide closed (releasing your finger on the opposite side you started also triggers it, so you don't notice this is broken.

This fixes that use a min velocity of 5 in/sec, which feels good to me (its hard to go much slower without feeling like an idiot). It also fixes some problems with the speed calculation I didn't notice because this wasn't working.
(Reporter)

Comment 1

5 years ago
Comment on attachment 633305 [details] [diff] [review]
Patch

I haven't had a good mbrubeck review in awhile. You want to look at this?
Attachment #633305 - Attachment is patch: true
Attachment #633305 - Flags: review?(mbrubeck)
Comment on attachment 633305 [details] [diff] [review]
Patch

>+    private static final int SWIPE_CLOSE_VELOCITY = 5;
>     private static final int MAX_ANIMATION_TIME = 250;

Please add units for both of these, either in the identifiers themselves or in comments right here.

>             if (Math.abs(velocityX)/GeckoAppShell.getDpi() > SWIPE_CLOSE_VELOCITY) {
>+                // is this is a swipe, we want to continue the row moving at the swipe velocity
>                 float d = (velocityX > 0 ? 1 : -1) * mView.getWidth();
>+                animateTo(mView, (int)d, (int)((d + mView.getScrollX())*1000/velocityX));

So velocityX is in px/s, and the 1000 is because animateTo takes an a duration in ms, right?  Could you add comments to onFling and animateTo noting the units of those arguments?
Attachment #633305 - Flags: review?(mbrubeck) → review+
(Reporter)

Comment 3

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2448a35540d2
https://hg.mozilla.org/mozilla-central/rev/2448a35540d2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
(Reporter)

Updated

5 years ago
Duplicate of this bug: 765165
(Reporter)

Comment 6

5 years ago
Comment on attachment 633305 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 713450
User impact if declined: Close animation ain't right. Polish
Testing completed (on m-c, etc.): Landed on central last week
Risk to taking this patch (and alternatives if risky): Low risk fix if we take bug 13450.
String or UUID changes made by this patch: None.
Attachment #633305 - Flags: approval-mozilla-aurora?

Comment 7

5 years ago
Approved for Aurora 15. We'll back the new tabs out if they pose significant issues before Beta.

Updated

5 years ago
Attachment #633305 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Comment 8

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/7bb8c28d8879

Updated

5 years ago
status-firefox15: --- → fixed
You need to log in before you can comment on or make changes to this bug.