Closed
Bug 706684
Opened 13 years ago
Closed 13 years ago
Resting your hand on the device causes zoom to freak out
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(firefox11 fixed, firefox12 fixed, firefox13 verified, blocking-fennec1.0 +, fennec11+)
VERIFIED
FIXED
Firefox 12
People
(Reporter: pcwalton, Assigned: pcwalton)
References
Details
Attachments
(2 files, 2 obsolete files)
21.87 KB,
patch
|
cwiiis
:
review+
kats
:
feedback+
blassey
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.12 KB,
patch
|
kats
:
review+
blassey
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
When there are more than two touches, the PanZoomController gets very confused, and the end result is frequently that your page turns into a single pixel and you have to restart the browser. This is rather suboptimal.
I think I've seen this. I got
"11-30 15:51:38.949: E/GeckoPanZoomController(13127): Received impossible touch end while in NOTHING"
Updated•13 years ago
|
Priority: -- → P2
Updated•13 years ago
|
Assignee: nobody → pwalton
Assignee | ||
Comment 2•13 years ago
|
||
I see this at least once daily and I have to restart the browser when it happens, because I lose the page. Requesting that this be bumped to a P1.
Updated•13 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 3•13 years ago
|
||
As far as I can tell this is some sort of issue in the ScaleGestureDetector. I thought it might be that we aren't passing all touch events to the gesture detector, but changing LayerView.onTouchEvent() so that it unconditionally passes touch events through doesn't help. We may just need to reinvent the scale gesture detector...
Comment 4•13 years ago
|
||
I've seen some of this when playing with multitouch (i.e. resting your hand can result in multiple touches. They're usually pretty large touch surfaces though. Maybe we can try just not send touches with a size greater than .25 to the gesture detector?
http://developer.android.com/reference/android/view/MotionEvent.html#getSize%28int%29
Assignee | ||
Comment 5•13 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #4)
> I've seen some of this when playing with multitouch (i.e. resting your hand
> can result in multiple touches. They're usually pretty large touch surfaces
> though. Maybe we can try just not send touches with a size greater than .25
> to the gesture detector?
>
> http://developer.android.com/reference/android/view/MotionEvent.
> html#getSize%28int%29
Instead of trying to hack around the GestureDetector's limitations I'd rather just reinvent the ScaleGestureDetector. Trying hacks like that is really relying on the internal implementation of the gesture detector, which Google (or the manufacturers) could change under our feet.
Updated•13 years ago
|
tracking-fennec: --- → 11+
Assignee | ||
Comment 6•13 years ago
|
||
I'll be testing this more thoroughly on various devices tomorrow, but I'd like to go ahead and get review in case I don't find any major issues. Preliminary testing suggests that this bug doesn't happen anymore on a Droid X, a Droid RAZR, or a Galaxy Tab.
This patch implements the "SimpleScaleGestureDetector", a hopefully more reliable replacement for the built-in Android scale gesture detector. Copy-and-pasting from the comments at the top of the file, I believe it's more reliable because:
* It doesn't assume that pointer IDs are numbered 0 and 1.
* It doesn't attempt to correct for "slop" when resting one's hand on the device. On some devices (e.g. the Droid X) this can cause the ScaleGestureDetector to lose track of how many pointers are down, with disastrous results (this bug).
* Cancelling a zoom into a pan is handled correctly.
* It doesn't take pressure into account, which results in smoother scaling.
Attachment #588311 -
Flags: review?(bugmail.mozilla)
Comment 7•13 years ago
|
||
Comment on attachment 588311 [details] [diff] [review]
Proposed patch.
Review of attachment 588311 [details] [diff] [review]:
-----------------------------------------------------------------
Overall this looks pretty good, and I like how you implemented the SimpleScaleGestureDetector - it's very clean and easy to follow. The one thing I was thinking about - do we want to handle more than two touch points? With the current code, if you put down fingers 1, 2, and 3, and then lift finger 1, you won't be able to pinch with fingers 2 and 3. I haven't checked if this works with the system detector, but if it works there then we should probably add support for it. If it doesn't work there then I could go either way; it would be nice but I think would complicate the code more. Thoughts?
(Also I'm PTO for the next few days and may not be online, so if you want to get Cwiiis to steal the review that's fine by me, I don't see any problems with the code other than the few nits.)
::: mobile/android/base/ui/PanZoomController.java
@@ +359,5 @@
> return FloatMath.sqrt(dx * dx + dy * dy);
> }
>
> private void track(float x, float y, long time) {
> + Log.e(LOGTAG, "### track " + x + " " + y);
Do you want the log lines checked in, or were they debugging-only? There's this one and a few in onScale/onScaleEnd.
@@ +403,4 @@
> track(event.getHistoricalX(0, i),
> event.getHistoricalY(0, i),
> event.getHistoricalEventTime(i));
> + }*/
Why take these out? Are they messing with things?
::: mobile/android/base/ui/SimpleScaleGestureDetector.java
@@ +14,5 @@
> + *
> + * The Original Code is Mozilla Android code.
> + *
> + * The Initial Developer of the Original Code is Mozilla Foundation.
> + * Portions created by the Initial Developer are Copyright (C) 2009-2010
Fix copyright year.
Assignee | ||
Comment 8•13 years ago
|
||
Here's a new version that removes the debug logging that crept in. It also supports starting a gesture by placing >2 fingers on the device and lifting fingers until 2 remain. It didn't make the code significantly more complex; in fact, it arguably made it simpler. Thanks for the catch.
Attachment #588311 -
Attachment is obsolete: true
Attachment #588311 -
Flags: review?(bugmail.mozilla)
Attachment #588509 -
Flags: review?(chrislord.net)
Comment 9•13 years ago
|
||
Comment on attachment 588509 [details] [diff] [review]
Proposed patch, version 2.
Review of attachment 588509 [details] [diff] [review]:
-----------------------------------------------------------------
Like it - r+ with either using PointF.length or an explanation for the addition of PointUtils.distance, and removal of toJSON from PointerInfo.toString.
::: mobile/android/base/gfx/PointUtils.java
@@ +81,5 @@
> + /** Computes the scalar distance between two points. */
> + public static float distance(PointF one, PointF two) {
> + float deltaX = one.x - two.x, deltaY = one.y - two.y;
> + return FloatMath.sqrt(deltaX * deltaX + deltaY * deltaY);
> + }
Is there a reason not to use PointF.length(float x, float y) for this?
::: mobile/android/base/ui/SimpleScaleGestureDetector.java
@@ +305,5 @@
> + String prevString;
> + if (mPrevious == null) {
> + prevString = "n/a";
> + } else {
> + prevString = PointUtils.toJSON(mPrevious).toString();
It's a bit heavy-weight to use toJSON here isn't it? Why not just use the PointF.toString()?
@@ +309,5 @@
> + prevString = PointUtils.toJSON(mPrevious).toString();
> + }
> +
> + // The current position should always be non-null.
> + String currentString = PointUtils.toJSON(mCurrent).toString();
Same here.
Attachment #588509 -
Flags: review?(chrislord.net) → review+
Assignee | ||
Comment 10•13 years ago
|
||
> ::: mobile/android/base/gfx/PointUtils.java
> @@ +81,5 @@
> > + /** Computes the scalar distance between two points. */
> > + public static float distance(PointF one, PointF two) {
> > + float deltaX = one.x - two.x, deltaY = one.y - two.y;
> > + return FloatMath.sqrt(deltaX * deltaX + deltaY * deltaY);
> > + }
>
> Is there a reason not to use PointF.length(float x, float y) for this?
No legitimate reason. I didn't know about that function :) Google didn't use it themselves in ScaleGestureDetector, another strike against that class...
>
> ::: mobile/android/base/ui/SimpleScaleGestureDetector.java
> @@ +305,5 @@
> > + String prevString;
> > + if (mPrevious == null) {
> > + prevString = "n/a";
> > + } else {
> > + prevString = PointUtils.toJSON(mPrevious).toString();
>
> It's a bit heavy-weight to use toJSON here isn't it? Why not just use the
> PointF.toString()?
>
> @@ +309,5 @@
> > + prevString = PointUtils.toJSON(mPrevious).toString();
> > + }
> > +
> > + // The current position should always be non-null.
> > + String currentString = PointUtils.toJSON(mCurrent).toString();
>
> Same here.
Sadly PointF doesn't have a reasonable implementation for toString(). (Point does IIRC. It's an oversight in the Android libraries.) But I can write one in PointUtils if you'd like.
Comment 11•13 years ago
|
||
(In reply to Patrick Walton (:pcwalton) from comment #10)
> Sadly PointF doesn't have a reasonable implementation for toString(). (Point
> does IIRC. It's an oversight in the Android libraries.) But I can write one
> in PointUtils if you'd like.
heh, typical Android :) If we ever have debugging that calls this a lot, I think we'll want to not take such a round-about route to get a string, so it's worth doing. Would remove the need for the try/catch block there too. Your call though, I'm easy either way.
Comment 12•13 years ago
|
||
(In reply to Patrick Walton (:pcwalton) from comment #10)
> > ::: mobile/android/base/gfx/PointUtils.java
> > @@ +81,5 @@
> > > + /** Computes the scalar distance between two points. */
> > > + public static float distance(PointF one, PointF two) {
> > > + float deltaX = one.x - two.x, deltaY = one.y - two.y;
> > > + return FloatMath.sqrt(deltaX * deltaX + deltaY * deltaY);
> > > + }
> >
> > Is there a reason not to use PointF.length(float x, float y) for this?
>
> No legitimate reason. I didn't know about that function :) Google didn't use
> it themselves in ScaleGestureDetector, another strike against that class...
>
Despite what you'd expect, PointF.length(float x, float y) is a static method and returns sqrt(x*x + y*y), so you can't actually use that for this purpose. At least not without creating a new PointF(one.x - two.x, one.y - two.y) which seems silly.
Comment 13•13 years ago
|
||
Comment on attachment 588509 [details] [diff] [review]
Proposed patch, version 2.
Your updated patch looks fine, btw, although I would have used an ArrayList instead of a LinkedList since I don't think there's much advantage to using LinkedList, and it will be marginally slower.
Also I would wrap the sPointerInfoFreeList in a SoftReference so the VM can reclaim the space if it wants to; however the space will be small enough that it won't make a significant difference either way. Really I would like to see a generalized ObjectCache class to handle recycling different object types, particularly ViewportMetrics and point/rect classes, since we create a metric-ton of those on any UI event.
Attachment #588509 -
Flags: feedback+
Comment 14•13 years ago
|
||
(In reply to Kartikaya Gupta (:kats) from comment #12)
> (In reply to Patrick Walton (:pcwalton) from comment #10)
> > > ::: mobile/android/base/gfx/PointUtils.java
> > > @@ +81,5 @@
> > > > + /** Computes the scalar distance between two points. */
> > > > + public static float distance(PointF one, PointF two) {
> > > > + float deltaX = one.x - two.x, deltaY = one.y - two.y;
> > > > + return FloatMath.sqrt(deltaX * deltaX + deltaY * deltaY);
> > > > + }
> > >
> > > Is there a reason not to use PointF.length(float x, float y) for this?
> >
> > No legitimate reason. I didn't know about that function :) Google didn't use
> > it themselves in ScaleGestureDetector, another strike against that class...
> >
>
> Despite what you'd expect, PointF.length(float x, float y) is a static
> method and returns sqrt(x*x + y*y), so you can't actually use that for this
> purpose. At least not without creating a new PointF(one.x - two.x, one.y -
> two.y) which seems silly.
You could do PointF.length(one.x - two.x, one.y - two.y)? Why would you need to create a new PointF?
Comment 15•13 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #14)
> You could do PointF.length(one.x - two.x, one.y - two.y)? Why would you need
> to create a new PointF?
Oh, true. I think I still prefer PointUtils.distance for being more readable, but you're right, it could be done with PointF.length as well.
Comment 16•13 years ago
|
||
(In reply to Kartikaya Gupta (:kats) from comment #15)
> (In reply to Chris Lord [:cwiiis] from comment #14)
> > You could do PointF.length(one.x - two.x, one.y - two.y)? Why would you need
> > to create a new PointF?
>
> Oh, true. I think I still prefer PointUtils.distance for being more
> readable, but you're right, it could be done with PointF.length as well.
I don't mind it being there as a utility function that uses PointF.length internally I suppose, though I think it'd be a bit extravagant. Where we can, we should try and stick to the stock class libraries.
Assignee | ||
Comment 17•13 years ago
|
||
Comment 18•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Updated•13 years ago
|
status-firefox11:
--- → affected
status-firefox12:
--- → fixed
Comment 19•13 years ago
|
||
This crashes on Froyo because LinkedList.push does not exist. The attached patch makes it use LinkedList.add instead which is equivalent.
Attachment #591090 -
Flags: review?(bugmail.mozilla)
Comment 20•13 years ago
|
||
Attachment #591090 -
Attachment is obsolete: true
Attachment #591090 -
Flags: review?(bugmail.mozilla)
Updated•13 years ago
|
Attachment #591093 -
Flags: review+
Comment 21•13 years ago
|
||
Comment 22•13 years ago
|
||
Comment 23•13 years ago
|
||
Patrick, please request aurora approval
Comment 25•13 years ago
|
||
Comment on attachment 588509 [details] [diff] [review]
Proposed patch, version 2.
[Triage Comment]
Attachment #588509 -
Flags: approval-mozilla-beta+
Comment 26•13 years ago
|
||
Comment on attachment 591093 [details] [diff] [review]
I'm a moron, it should be addFirst().
[Triage Comment]
Attachment #591093 -
Flags: approval-mozilla-beta+
Comment 27•13 years ago
|
||
Comment 28•13 years ago
|
||
Zoom is still going wild when touching the screen on a larger area than the touch of a finger, please see video: http://www.youtube.com/watch?v=1vGEBn251I4&feature=plcp&context=C3b4f1d5UDOEgsToPDskJ4vODsGN4DS13W6dxwebXG.
This is reproducible on:
Firefox 11 (tinderbox build): 1328844077/09-Feb-2012 20:42
20120209192117
Firefox 12: Firefox 12.0a2 (2012-02-09)
Firefox 13: Firefox 13.0a1 (2012-02-09)
Device: LG Optimus 2X (Android 2.2.2)
Reopening the bug
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 29•13 years ago
|
||
This behaviour isn't the same as that described in comment #0. In fact it is a much more "expected" behaviour in that it leaves the browser looking normal and still usable. If you agree, please close this bug and file a new one for this behaviour.
Updated•13 years ago
|
Keywords: fennecnative-releaseblocker
Comment 30•13 years ago
|
||
per kats please open a new bug with this new behavior.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
blocking-fennec1.0: --- → +
Comment 31•13 years ago
|
||
Verified fixed on:
Firefox 13.0a1 (2012-03-01)
20120301031135
http://hg.mozilla.org/mozilla-central/rev/1c3b291d0830
--
Device: Samsung Galaxy S2
OS: Android 2.3.4
Status: RESOLVED → VERIFIED
status-firefox13:
--- → verified
Updated•4 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
•