524543 - Fast double taps sometimes cause clicks

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Benjamin Stover (:stechz)]

This is possibly a regression from fixing accidental content clicks.  If one of the clicks has not been parsed as a click (<X msecs have occurred), there will only be a single click.

Comment 1

[Aakash Desai [:aakashd]]

Can we get an actual steps to reproduce here?

Comment 2

[Benjamin Stover (:stechz)]

Double tap a link on the N900 really fast.  Sometimes it will do nothing, other times it will open the link.

Comment 3

[Aakash Desai [:aakashd]]

Yep, I'm seeing this too on the n900 on today's nightly build:

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


I dont think this is a regression as I couldn't find any bugs on file for this issue.

Comment 4

[Benjamin Stover (:stechz)]

Attachment: Changes to dragData

This patch includes some refactoring, as any time we change how axis locking works or what constitutes a pan or a click, a lot of code is being touched outside of DragData.  This patch localizes the responsible code into DragData.  This patch feeds information into DragData (much like our kinetic controller) and then ask it questions like: is it a pan? what are the current panning coordinates?

Here's a quick guide to the changes:
> _onMouseUp: function _onMouseUp(evInfo) {
> -    let [sX, sY] = dragData.lockAxis(evInfo.event.screenX, evInfo.event.screenY);
> -
> -    if (dragData.dragging)
> -      this._doDragStop(sX, sY, !dragData.checkPan(sX, sY));
> +    if (dragData.dragging) {
> +      dragData.setDragPosition(evInfo.event.screenX, evInfo.event.screenY);
> +      let [sX, sY] = dragData.panPosition();
> +      this._doDragStop(sX, sY, !dragData.isPan());
> +    }

Lock axis is no longer used.  Instead, when mouse movement data is received, setDragPosition is called with the screen coordinates.  Then, dragData.isPan() and dragData.isClick() tells the event handler how to handle this drag.  The pan and click checks no longer take coordinates as arguments because setDragPosition now has that logic and sets appropriate object state.

>   _doDragMove: function _doDragMove(sX, sY) {
>     let dragData = this._dragData;
> -    let dX = dragData.sX - sX;
> -    let dY = dragData.sY - sY;
> +    let dX = dragData.prevPanX - sX;
> +    let dY = dragData.prevPanY - sY;

Previous pan coordinates are stored by dragData so that a delta can be calculated for the dragger.

setDragPosition now contains the needed logic for when the drag locks, how it locks, and what constitutes a pan.  The functional difference from before is that double taps no longer have to have clicks that take over 50 milliseconds.

madhava and I have extensively tested this patch by panning, double tapping, clicking on chrome things, and looking for accidental content clicks.  If this patch is too scary, I can hack together something where the functional changes from before are more obvious.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 408887 [details] [diff] [review]
Changes to dragData

I like the refactoring part, and I'm not sure why but panning seems a bit faster.

This also seems to correct a long remaining bug that I'm seeing sometimes with quick quadruple click.

Very nice work!

Comment 6

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

Good refactor! Good result!

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

Comment 7

[Aakash Desai [:aakashd]]

verified FIXED on tinderbox build:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b2pre) Gecko/20091029 Namoroka/3.6b2pre Fennec/1.0b5

Comment 8

[Aakash Desai [:aakashd]]

This would be better suited for a browser chrome test.