742019 - Calling preventDefault when a second finger touches the screen can break panning

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

We currently listen for touchevents, and if the user calls preventDefault on a touchstart event, block java from handling them. This can cause problems if the page doesn't call preventDefault until a second touchstart happens (i.e. a second finger on the screen), and we've already started dispatching some events to Java

1.) The user puts one finger down
2.) User pans the page
3.) User puts a second finger down, page calls preventDefault
4.) User lifts both fingers and then puts the first down and tries to pan again

AR: Page starts pinch zooming
ER: Page pans normally.

We need to either fire a touch cancel in java to end any in progress stuff. Ideally we could keep track of which finger caused the preventDefault and release panning back to java when that finger was released, but because touchevents don't really tie one event-> one touch (in fact, a touch event can correspond to changes in multiple touches), its difficult to keep track of exactly which touch preventDefault was called on.

Comment 1

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

Attachment: (1/2) Cleanup variables

This is the same patch that you r+'d in bug 741565, I just want it to be the first patch that lands :)

Comment 2

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

Attachment: (2/2) Properly handle preventDefault on the non-first touch point

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 612209 [details] [diff] [review]
(2/2) Properly handle preventDefault on the non-first touch point

Review of attachment 612209 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: mobile/android/base/gfx/LayerController.java
@@ +422,5 @@
>      
>              if (aValue) {
>                  mView.clearEventQueue();
> +                long now = SystemClock.uptimeMillis();
> +                mView.processEvent(MotionEvent.obtain(now, now, MotionEvent.ACTION_CANCEL, 0, 0, 0));

Is there some reason we wouldn't just put this in clearEventQueue()? The only other place it is called is on ACTION_DOWN (if we aren't still in the double tap timer range).

::: mobile/android/base/gfx/LayerView.java
@@ +130,5 @@
>          }
>          return processEvent(event);
>      }
>  
> +    boolean processEvent(MotionEvent event) {

I like having public and private explicitly written.

Comment 4

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

(In reply to Wesley Johnston (:wesj) from comment #3)
> 
> Is there some reason we wouldn't just put this in clearEventQueue()? The
> only other place it is called is on ACTION_DOWN (if we aren't still in the
> double tap timer range).

It does kind of make sense to have in clearEventQueue, but I'm not sure we want it to run in that other scenario. That means it would get run before every DOWN event which seems bad, and might end up canceling flings even if the down gets eaten by preventDefault.

> > +    boolean processEvent(MotionEvent event) {
> 
> I like having public and private explicitly written.

This is package scope, I didn't want to make it public to avoid people abusing it by calling it from god-knows-where.

Comment 5

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

Already told wesj on IRC, but putting it here so it's recorded: there's still a bug in the following scenario:
1. A sequence of events like touchdown, move, touchdown in quick succession and they all get queued in java
2. The first touchdown isn't prevented, so then we call preventPanning(false) and process all three events in step 1.
3. The second touchdown *is* prevented and we call preventPanning(true), and it should be thrown away but it has already been processed.

Comment 6

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

Attachment: Rewrite how touch events work

Sorry for snowballing this completely out of proportion.

Also sorry for not splitting this into incremental patches, but it's (I hope) easier to read this way. I suggest first reading through TouchEventHandler.java, and then look at the changes I made to nsWindow.cpp. The rest of it is just glue that holds everything together, and ripping out the old code.

This patch incorporates the other two patches you already r+'d on this bug (but which I haven't landed, and will probably obsolete) as well as the patches from bug 741565.

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 612390 [details] [diff] [review]
Rewrite how touch events work

Review of attachment 612390 [details] [diff] [review]:
-----------------------------------------------------------------

I like this! I wish we weren't doing such a big rewrite this late in the game, but I think its pros outweigh that.

Comment 9

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

Comment on attachment 612206 [details] [diff] [review]
(1/2) Cleanup variables

Obsoleted by attachment 612390 [details] [diff] [review]

Comment 10

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

Comment on attachment 612209 [details] [diff] [review]
(2/2) Properly handle preventDefault on the non-first touch point

Obsoleted by attachment 612390 [details] [diff] [review]

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c81e9412e02a

Comment 13

[Matt Brubeck (:mbrubeck)]

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

Comment 14

[Adrian Tamas (:AdrianT)]

Verified/fixed on:
Nightly Fennec 15.0a1 2012-04-26
Device: HTC Desire (Android 2.2.2)

The initial scenario reported in the bug is no longer reproducible