Closed
Bug 765069
Opened 12 years ago
Closed 12 years ago
Close by swipe velocity checks are wrong
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(firefox15 fixed)
RESOLVED
FIXED
Firefox 16
Tracking | Status | |
---|---|---|
firefox15 | --- | fixed |
People
(Reporter: wesj, Unassigned)
References
Details
Attachments
(1 file)
2.54 KB,
patch
|
mbrubeck
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•12 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 2•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2448a35540d2
Comment 4•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2448a35540d2
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Reporter | ||
Comment 6•12 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•12 years ago
|
||
Approved for Aurora 15. We'll back the new tabs out if they pose significant issues before Beta.
Updated•12 years ago
|
Attachment #633305 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reporter | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/7bb8c28d8879
Updated•12 years ago
|
status-firefox15:
--- → fixed
Assignee | ||
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•