706684 - Resting your hand on the device causes zoom to freak out

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Patrick Walton (:pcwalton)]

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.

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I think I've seen this.  I got 
"11-30 15:51:38.949: E/GeckoPanZoomController(13127): Received impossible touch end while in NOTHING"

Comment 2

[Patrick Walton (:pcwalton)]

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.

Comment 3

[Patrick Walton (:pcwalton)]

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

[Wesley Johnston (:wesj)]

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

Comment 5

[Patrick Walton (:pcwalton)]

(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.

Comment 6

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

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.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 8

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 2.

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.

Comment 9

[Chris Lord [:cwiiis]]

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.

Comment 10

[Patrick Walton (:pcwalton)]

 > ::: 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

[Chris Lord [:cwiiis]]

(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

[Kartikaya Gupta (email:[e-mail])]

(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

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 14

[Chris Lord [:cwiiis]]

(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

[Kartikaya Gupta (email:[e-mail])]

(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

[Chris Lord [:cwiiis]]

(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.

Comment 17

[Patrick Walton (:pcwalton)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ca4d85ac6bf5

Comment 18

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/ca4d85ac6bf5

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Patch to use LinkedList.add

This crashes on Froyo because LinkedList.push does not exist. The attached patch makes it use LinkedList.add instead which is equivalent.

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: I'm a moron, it should be addFirst().

Comment 21

[James Willcox (:snorp) ([e-mail]) (he/him)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/cd76470c9e6f

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cd76470c9e6f

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

Patrick, please request aurora approval

Comment 25

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 588509 [details] [diff] [review]
Proposed patch, version 2.

[Triage Comment]

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 591093 [details] [diff] [review]
I'm a moron, it should be addFirst().

[Triage Comment]

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-beta/rev/603d4ea03ad3

Comment 28

[Andreea Pod]

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

Comment 29

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 30

[Doug Turner (:dougt)]

per kats please open a new bug with this new behavior.

Comment 31

[Cristian Nicolae (:xti)]

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