1674279 - Fix APZ test failures introduced by AndroidFlingPhysics and AndroidVelocityTracker, and enable the affected tests

Status: RESOLVED FIXED

(Core :: Panning and Zooming, task)

Description

[Markus Stange [:mstange]]

Broken out from bug 1545234.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1674279 - When determining whether to start a fling, require the velocity to be greater than apz.fling_min_velocity_threshold (change >= to >). r=botond

This only really makes a difference when the pref is set to zero: Now we will no
longer attempt to kick off no-op flings. Many of the APZ gtests are manually
setting the pref to zero, and were unintentionally kicking off no-op flings.
As a result, they were crashing on an assertion in APZFlingPhysics which checks
that flings always start with a non-zero velocity.

When the pref is set to a non-zero value, which is the case for our users, it
doesn't really make a difference whether we check > or >=, because users are
very unlikely to hit the "==" case. So this patch shouldn't affect the user
experience.

As a result of this change, the following test starts passing on Android:

APZCFlingStopTester.FlingStopTap

There are a lot more tests that were hitting this assertion, but those are still
failing for other reasons.

On non-Android, this change causes the following test to fail:

APZCPinchGestureDetectorTester.Panning_TwoFingerFling_ZoomDisabled

That's because the regular velocity tracker determines a velocity of zero, and
the test checks that a fling is started. This test will be fixed in an upcoming
patch in this series.

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1674279 - Reduce time between simulated touch events to 20ms so that the Android velocity tracker doesn't treat the gaps as pauses. r=botond

This makes the following tests pass on Android:

APZCFlingStopTester.FlingStop
APZCFlingStopTester.FlingStopSlowListener
APZCFlingStopTester.FlingStopPreventDefault
APZScrollHandoffTester.SimultaneousFlings
APZScrollHandoffTester.ScrollgrabFling
APZScrollHandoffTester.ScrollgrabFlingAcceleration1
APZScrollHandoffTester.ImmediateHandoffDisallowed_Fling

It cases the following test to start failing on non-Android:

APZScrollHandoffTester.ScrollgrabFlingAcceleration2

That's because the reduced time between the pan events causes a higher scroll
speed, so the fling goes faster and farther, and hits overscroll. This will be
fixed in a later patch in this series.

Depends on D95246

Comment 3

[Markus Stange [:mstange]]

Attachment: Bug 1674279 - Break the pan gesture into three steps, so that the velocity tracker has enough samples to compute a velocity. r=botond

This makes the following tests pass on Android:

APZCBasicTester.Fling
APZScrollHandoffTester.PartialFlingHandoff

Depends on D95247

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1674279 - Break the "moving pinch" gesture into three events, so that the velocity trackers can determine a non-zero velocity. r=botond

This fixes the following test:

APZCPinchGestureDetectorTester.Panning_TwoFingerFling_ZoomDisabled

Depends on D95248

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1674279 - Increase scrollable range of the nested scroll frame so that the accelerated fling does not hit the end of the range before TestFlingAcceleration() can read the velocity. r=botond

This fixes APZScrollHandoffTester.ScrollgrabFlingAcceleration2.

Depends on D95249

Comment 6

[Markus Stange [:mstange]]

Attachment: Bug 1674279 - Only call OnPan from OnPanEnd if the pan end event comes with a displacement. r=botond

On macOS, the pan end events usually don't come with a displacement, so the call
to OnPan is unnecessary.

This change fixes the test APZCSnappingOnMomentumTester.Snap_On_Momentum when
the Android velocity tracker is used. The test was failing in the following way:
The test executes a pan gesture where the pan end event has a zero delta.
The pan end event is dispatched after a delay after the last delta-ful pan.
Calling OnPon for the zero-delta event at the end added an additional data point
to the velocity tracker's history, making it look like the pan had come to an
abrupt stop.
After the pan end event, the test dispatches a pan momentum start event, again
after a small delay. This event kicks off the scroll snap.
The scroll snap computes a velocity at this timestamp. Compared to the last
position in the velocity tracker's history, this timestamp is "in the future",
so the tracker has to extrapolate. It fits a quadratic polynomial to the data
points it has. After the fast pan down and the abrupt stop, it now predicts a
reversal of the velocity.
This reversal causes the scroll snap to be initiated in the wrong direction.
We snap upwards instead of downwards, and the test fails.

This patch fixes the failure because the velocity tracker no longer sees an
abrupt stop, so it no longer predicts a reversal of the pan direction.

For the regular velocity tracker, we add another delta-ful pan event to the test,
to have enough deltas to compute a velocity from. This test will fail if a zero
velocity is computed, due to a problem described in bug 1674308.

Comment 7

[Markus Stange [:mstange]]

Attachment: Bug 1674279 - Fix reversed logic in this AXIS_LOG message. r=botond

Depends on D95251

Comment 8

[Markus Stange [:mstange]]

Attachment: Bug 1674279 - Compute correct velocities during a pan in AndroidVelocityTracker. r=botond

We were (correctly) negating the velocity in ComputeVelocity, but not in
AddPosition, so the velocity was upside down during the pan and then flipped its
sign at the end of the pan.

Depends on D95252

Comment 9

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/jobs?repo=try&revision=32b0a63cfa4451db569e470498f9c2fb7dc1e691

Comment 10

[Markus Stange [:mstange]]

Attachment: Bug 1674279 - Increase scrollable range of the nested scroll frame so that the accelerated fling does not hit the end of the range before TestFlingAcceleration() can read the velocity. r=botond

This fixes APZScrollHandoffTester.ScrollgrabFlingAcceleration2.

Depends on D95249

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a503b1f051f9
When determining whether to start a fling, require the velocity to be greater than apz.fling_min_velocity_threshold (change >= to >). r=botond
https://hg.mozilla.org/integration/autoland/rev/3791ee43d731
Reduce time between simulated touch events to 20ms so that the Android velocity tracker doesn't treat the gaps as pauses. r=botond
https://hg.mozilla.org/integration/autoland/rev/0a16a7912b76
Break the pan gesture into three steps, so that the velocity tracker has enough samples to compute a velocity. r=botond
https://hg.mozilla.org/integration/autoland/rev/f085fc122290
Break the "moving pinch" gesture into three events, so that the velocity trackers can determine a non-zero velocity. r=botond
https://hg.mozilla.org/integration/autoland/rev/910d09932ee7
Increase scrollable range of the nested scroll frame so that the accelerated fling does not hit the end of the range before TestFlingAcceleration() can read the velocity. r=botond
https://hg.mozilla.org/integration/autoland/rev/d9c73a8e374e
Only call OnPan from OnPanEnd if the pan end event comes with a displacement. r=botond
https://hg.mozilla.org/integration/autoland/rev/0bc86da80d18
Fix reversed logic in this AXIS_LOG message. r=botond
https://hg.mozilla.org/integration/autoland/rev/14c3f00d80bc
Compute correct velocities during a pan in AndroidVelocityTracker. r=botond

Comment 12

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/a503b1f051f9
https://hg.mozilla.org/mozilla-central/rev/3791ee43d731
https://hg.mozilla.org/mozilla-central/rev/0a16a7912b76
https://hg.mozilla.org/mozilla-central/rev/f085fc122290
https://hg.mozilla.org/mozilla-central/rev/910d09932ee7
https://hg.mozilla.org/mozilla-central/rev/d9c73a8e374e
https://hg.mozilla.org/mozilla-central/rev/0bc86da80d18
https://hg.mozilla.org/mozilla-central/rev/14c3f00d80bc

Comment 13

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== Change summary for alert #28035 (as of Thu, 10 Dec 2020 05:25:28 GMT) ==

Regressions:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
55%	about_newtab_with_snippets		windows10-64-shippable-qr	e10s stylo webrender	44.08 -> 68.36

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=28035

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

It's unlikely that this patch for Android tests caused a startup regression on Windows. I suspect this is actually bug 1672517 and should be reassigned to alert 27363 (the improvement should probably be reassigned to alert 27535.

Comment 15

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Done