Last Comment Bug 701292 - clicks are transmitted when panning
: clicks are transmitted when panning
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: P1 major (vote)
: ---
Assigned To: [:fabrice] Fabrice Desré
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-09 23:11 PST by [:fabrice] Fabrice Desré
Modified: 2012-01-09 11:24 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
patch (2.26 KB, patch)
2011-11-09 23:13 PST, [:fabrice] Fabrice Desré
pwalton: review-
Details | Diff | Review
Temp fix for click while panning (1.22 KB, patch)
2011-11-10 08:55 PST, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
Details | Diff | Review
Add a pan thresold (3.15 KB, patch)
2011-11-14 07:11 PST, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
Details | Diff | Review

Description [:fabrice] Fabrice Desré 2011-11-09 23:11:44 PST
That makes browsing quite difficult ;)
Comment 1 [:fabrice] Fabrice Desré 2011-11-09 23:13:01 PST
Created attachment 573429 [details] [diff] [review]
patch

That seems to work fine here, but the constants at least probably need some tweaking.
Comment 2 Patrick Walton (:pcwalton) 2011-11-10 07:04:01 PST
Comment on attachment 573429 [details] [diff] [review]
patch

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

::: embedding/android/gfx/LayerController.java
@@ +298,5 @@
> +            Log.i("Gecko", "Setting mLastEvent");
> +            mLastX = event.getRawX();
> +            mLastY = event.getRawY();
> +            mLastTime = event.getEventTime();
> +        }

This duplicates some logic that the pan/zoom controller already has. I think it'd be better to move this logic to a new method PanZoomController.shouldCallTouchListener(). It can possibly be as simple as:

return mState == PANNING || mState == PANNING_HOLD || mState == PINCHING;

Then just have the LayerController call that method on mPanZoomController to determine whether to call the touch listener.
Comment 3 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2011-11-10 08:55:55 PST
Created attachment 573532 [details] [diff] [review]
Temp fix for click while panning

Was discussing this somewhat with wesj yesterday. I had a patch for this that I was using locally that I think is a good temporary fix for this issue, until wesj revisits this code for touch events.
Comment 4 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2011-11-10 08:59:03 PST
(In case it's not clear why the patch works, it's because the PanZoomController's onTouchEnd returns true after a pan. This effectively eats the touch-up event, and so what gecko gets is just the touch-down, which is not sufficient for a click. Without a pan, however, it gets both touch-up and touch-down and so triggers a click).
Comment 5 Patrick Walton (:pcwalton) 2011-11-10 08:59:27 PST
Comment on attachment 573532 [details] [diff] [review]
Temp fix for click while panning

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

Clever!

r+ with a comment that explains that this is temporary.
Comment 6 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2011-11-11 13:40:39 PST
Just as an update, there is an issue with my patch which makes it really hard to click on links sometimes. I believe this happens because even a one-pixel movement between the touch-down and touch-up will send the PanZoomController into pan mode and kill the click. The best way to fix this (and which would be good in general) is to have a movement threshold in the PanZoomController below which a pan doesn't actually get started (e.g. you need to drag 3-4 pixels before it starts panning). I can do this in a separate patch.
Comment 7 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2011-11-14 07:11:35 PST
Created attachment 574290 [details] [diff] [review]
Add a pan thresold
Comment 8 Patrick Walton (:pcwalton) 2011-11-14 12:42:24 PST
Comment on attachment 574290 [details] [diff] [review]
Add a pan thresold

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

LGTM
Comment 9 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2011-11-14 12:58:07 PST
http://hg.mozilla.org/projects/birch/rev/ce4ebd1d2c9f
http://hg.mozilla.org/projects/birch/rev/6da7470b607c
Comment 10 Cristian Nicolae (:xti) 2011-11-16 06:58:41 PST
Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:11.0a1)Gecko/20111116
Firefox/11.0a1 Fennec/11.0a1
Devices: Motorola Droid 2
OS: Android 2.3.3

Note You need to log in before you can comment on or make changes to this bug.