1499941 - Fling occasionally stops abruptly or bounces back when flinging while dynamic toolbar is moving

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[Botond Ballo [:botond]]

This is a follow-up to bug 1498329 to cover the remaining issue described in bug 1498329 comment 10 through bug 1498329 comment 12.

Comment 1

[Botond Ballo [:botond]]

I can reproduce this, intermittently but often enough to debug it. Haven't gotten to the bottom of it yet.

Comment 2

[Botond Ballo [:botond]]

It seems that we have some sign confusion around VelocityTracker::HandleDynamicToolbarMovement().

Moving the finger *upwards* causes the dynamic toolbar to move *upwards* (negative position delta) but causes the page's scroll offset to *increase* (positive scroll offset delta). For the purposes of this discussion, let's call the former "spatial coordinates", and the latter "scroll coordinates".

Coordinates of touch events are in "spatial coordinates". Axis::mVelocity is in "scroll coordinates", and VelocityTracker::ComputeVelocity() accordingly returns "scroll coordinates".

VelocityTracker::AddPosition() expects (and gets) "spatial coordinates". To make this work, somewhere during the velocity calculation, the position deltas have to be negated. This happens here [1] in SimpleVelocityTracker, and here [2] in AndroidVelocityTracker.

Prior to bug 1457586, APZCTreeManager::ProcessDynamicToolbarMovement() was called ProcessTouchVelocity(), and took a velocity in "scroll coordinates". Accordingly, its caller would negate the position delta it used to compute the velocity [3].

In bug 1457586, I refactored that function to accept the position and time delta components of the velocity separately, but without changing any signs. That is, the toolbar animator continued to pass in a negated ("scroll coordinates") delta. SimpleVelocityTracker correctly factored that into its velocity computation without negating it [4], since it was already in "scroll coordinates". AndroidVelocityTracker, however, adds this delta to a delta in "spatial coordinates" [5], producing bogus results.

[1] https://searchfox.org/mozilla-central/rev/72b1e834f384a2ffec6eb4ce405fbd4b5e881109/gfx/layers/apz/src/SimpleVelocityTracker.cpp#60
[2] https://searchfox.org/mozilla-central/rev/72b1e834f384a2ffec6eb4ce405fbd4b5e881109/gfx/layers/apz/src/AndroidVelocityTracker.cpp#303
[3] https://searchfox.org/mozilla-central/rev/f288db20979b4aa6ab808cf6065bbd76f619e049/gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp#645
[4] https://searchfox.org/mozilla-central/rev/72b1e834f384a2ffec6eb4ce405fbd4b5e881109/gfx/layers/apz/src/SimpleVelocityTracker.cpp#81
[5] https://searchfox.org/mozilla-central/rev/72b1e834f384a2ffec6eb4ce405fbd4b5e881109/gfx/layers/apz/src/AndroidVelocityTracker.cpp#64

Comment 3

[Botond Ballo [:botond]]

Attachment: Bug 1499941 - Fix spatial vs. scroll coordinate confusion around APZCTreeManager::ProcessDynamicToolbarMovement() and helpers. r?kats

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74050b668ada
Fix spatial vs. scroll coordinate confusion around APZCTreeManager::ProcessDynamicToolbarMovement() and helpers. r=kats

Comment 5

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/74050b668ada

Comment 6

[Botond Ballo [:botond]]

Mark, could you give the latest nightly a try, and let me know if the issue (as you originally reported it in bug 1498329 comment 10) is resolved? Thanks!

Comment 7

[Mark]

Yes it looks resolved. Flings look great. I am struggling to find anything more to complain about but I'll keep trying ;) Thanks a lot.

Comment 8

[Mark]

Oops, I spoke too soon. Sorry.

bug 1502638.

:)

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 9019846 [details]
Bug 1499941 - Fix spatial vs. scroll coordinate confusion around APZCTreeManager::ProcessDynamicToolbarMovement() and helpers. r?kats

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1457586

User impact if declined: When starting a fling while the toolbar is transition on- or off-screen, sometimes the fling goes in the wrong direction (creating the appearance of a "bounce") due to an incorrect velocity computation

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Small change that's obvious in hindsight, hard to imagine it being wrong or causing a regression
Bug 1502638 was reported as a potential regression, but it's low impact (1 / 20 repro rate according to reporter; I couldn't repro it at all) and I think it's more likely to be a pre-existing issue with bug 1457586 than a regression from this fix

String changes made/needed:

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9019846 [details]
Bug 1499941 - Fix spatial vs. scroll coordinate confusion around APZCTreeManager::ProcessDynamicToolbarMovement() and helpers. r?kats

[Triage Comment]
Simple fix for a new APZ scrolling regression in 64. Approved for 64.0b6.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b5f86619d085

Comment 12

[Andrei Bodea[:andrei]]

Device:
 - Google Pixel C (Android 8.0.0)

Verified as fixed in the latest Beta build 64b.0b6 (2018-11-01) and the latest Nightly build 65.0a1(2018-11-02).