522494 - Tapping or double tapping pans a small amount

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect, P1)

Description

[Benjamin Stover (:stechz)]

Implement a threshold so that panning does not occur until we are relatively sure that the user is not tapping.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: Fix the jiggle

The drag radius determines whether or not to call dragMove yet.  In practice on the N900, it works pretty well.  Seeing a pan implies not clicking something accidentally.  Double tapping to zoom feels a lot sturdier, and it's obvious in the cases where you dragged too much (because then content pans).

I did a little refactoring.  For your reviewing pleasure, the refactors are listed below.  If you don't think a change improved clarity, it's easily removed.

> -    this._movedOutOfRadius = false;

I moved drag radius responsibility completely to the drag handler, since it already had the code for checking the distance and contains the dragging radius.
 
> -  isPointOutsideRadius: function isPointOutsideRadius(sX, sY) {
> +  /** Returns true if drag should pan scrollboxes.*/
> +  checkPan: function checkPan(sX, sY) {

checkPan returns true if the coordinates are outside the click radius, and keeps returning true for the remainder of that drag.  When the mouse module needs to know if the drag should result in panning, it calls this instead of using isOutsideOfRadius.

> -    this.alreadyLocked = false;
> +    this.locked = false;

Just a variable naming nit.

> -  _doClick: function _doClick(movedOutOfRadius) {

doClick seemed poorly named, and looking at mouseup I kept forgetting what the function was about.  Since it is used only once, I inlined it and added some comments.

> +    if (dragData.dragging)
> +      this._doDragStop(sX, sY, !dragData.checkPan(sX, sY));

Not a refactor, but used the third parameter to prevent tiny kinetic scrolls when nothing was panned.

Comment 2

[Benjamin Stover (:stechz)]

Attachment: Bit rot

Comment 3

[Ben Combee (:bcombee)]

Comment on attachment 407146 [details] [diff] [review]
Bit rot

You accidentally got the previous regression fix into this patch, but that can be fixed on commit.

Comment 4

[Ben Combee (:bcombee)]

Comment on attachment 407146 [details] [diff] [review]
Bit rot

Code looks good, no divides that are suspicious that could generate NaNs, and tested on Win32 desktop.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
https://hg.mozilla.org/mobile-browser/rev/2b2c1ac25f9b

Comment 6

[Aakash Desai [:aakashd]]

cool beans, verified FIXED On builds:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.2b1pre) Gecko/20091022 Fennec/1.0a4pre

and

Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/20091022
Fennec/1.0b5pre

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a1pre) Gecko/20091022
Fennec/1.0b5pre

We need to add this to our screen navigation BFT to check for things that happen (well, should not happen) when tapping and/or double-tapping for zoom.

Comment 7

[Ben Combee (:bcombee)]

stuart reports kinetic pans aren't working well with this patch, because we're not factoring in movement before the mousedown which is needed to really get good moves on the n810/n900

Comment 8

[Aakash Desai [:aakashd]]

litmus testcase 7583 was updated to regression test this bug.