Closed Bug 1499941 Opened Last year Closed Last year

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

Categories

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

All
Android
defect

Tracking

()

VERIFIED FIXED
mozilla65
Tracking Status
firefox62 --- unaffected
firefox63 --- unaffected
firefox64 --- verified
firefox65 --- verified

People

(Reporter: botond, Assigned: botond)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

This is a follow-up to bug 1498329 to cover the remaining issue described in bug 1498329 comment 10 through bug 1498329 comment 12.
See Also: → 1498329
I can reproduce this, intermittently but often enough to debug it. Haven't gotten to the bottom of it yet.
Assignee: nobody → 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
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/74050b668ada
Fix spatial vs. scroll coordinate confusion around APZCTreeManager::ProcessDynamicToolbarMovement() and helpers. r=kats
https://hg.mozilla.org/mozilla-central/rev/74050b668ada
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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!
Flags: needinfo?(mark.paxman99)
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.
Flags: needinfo?(mark.paxman99)
Oops, I spoke too soon. Sorry.

bug 1502638.

:)
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:
Attachment #9019846 - Flags: approval-mozilla-beta?
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.
Attachment #9019846 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.