1169689 - An APZC can have a nonzero velocity even when panning is disabled due to touch-action

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Some backstory at https://bugzilla.mozilla.org/show_bug.cgi?id=1167721#c17 - I ran into a scenario with a couple of the gtests which I think is actually a bug in the APZC code that manifests if touch-action is enabled.

Consider the case where touch-action is enabled and prevents vertical panning of a scrollable frame. The user puts their finger down and moves. On the move event, we will run the code at [1] which actually sets up a velocity in the Axes based on the location of the touchstart and touchmove. Later we call StartPanning which can decide that no panning will happen, and puts us back into state NONE. However, the velocity will stick around. This feels wrong, and I think should be corrected somehow. It also causes the same two gtests to fail if the OVERCOME_TOUCH_TOLERANCE value is increased beyond 1 or if the TIME_BETWEEN_TOUCH_EVENT is reduced below 100ms. (These magic values result in a velocity of 0.01 which is treated as zero by the IsZero function, but just barely).

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=21393b5c6963#1112

Comment 1

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

Attachment: Patch

Comment 2

[Botond Ballo [:botond]]

Comment on attachment 8612967 [details] [diff] [review]
Patch

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1973,5 @@
>    float dx = mX.PanDistance(point.x);
>    float dy = mY.PanDistance(point.y);
>  
> +  // When the touch move breaks through the pan threshold, reposition the touch
> +  // down origin so the page won't jump when we start panning. Reset the

Do you understand this (pre-existing) comment? What "jumping" happens if we don't call StartTouch() here?

Comment 3

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

So let's say there's a pan threshold of 100 pixels - so you put your finger down and then you have to move by 100 pixels to actually start panning. Now let's say that after that, you move your finger by an additional 5 pixels. The comment says that the page will actually move by 5 pixels, rather than 105 pixels. i.e. the page will track your finger from where it broke through the threshold, rather than where you originally put your finger down. Does that make sense?

Comment 4

[Botond Ballo [:botond]]

It makes sense, but I don't think it reflects reality. 

  - TrackTouch() determines the scroll delta based on (mX.GetPos(), mY.GetPos()) 
    as the starting point and the touch-move event's location as the ending point [1].

  - If we're in StartPanning(), OnTouchMove() has already called 
    UpdateWithTouchAtDevicePoint() [2], which updates the Axis::mPos values.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=21393b5c6963#2240
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=21393b5c6963#1112

Comment 5

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

Attachment: Patch v2

How about this instead?

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8613121 [details] [diff] [review]
Patch v2

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

Maybe mention in the commit message that we're also removing a no-longer-necessary call to Axis::StartTouch()? (I'd normally ask for a comment, but since we're removing the call, there isn't really a place to put the comment :)).

Comment 7

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

Updated commit message and landed.

Comment 8

[Pulsebot]

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

Comment 9

[Carsten Book [:Tomcat]]

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