1755044 - [Bug]: Smooth scrolling feels heavy for small flicks

Status: RESOLVED FIXED

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

Description

[Kevin Brosnan [Ex-Mozilla]]

From github: https://github.com/mozilla-mobile/fenix/issues/23687.

Steps to reproduce

Scroll any website with small and light flicks.

Expected behaviour

Ideally page should move exactly with the finger. Scrolling is smoother and feels lighter on chrome or samsung internet. Can also compare with scrolling with small flicks in WhatsApp, it is much much smoother.

Actual behaviour

Webpage feels heavy and sticky. It takes effort to move the page with small flicks.

Device name

No response

Android version

Android 11

Firefox release type

Firefox Beta

Firefox version

97.0.0-beta.6

Device logs

No response

Additional information

No response

Change performed by the Move to Bugzilla add-on.

Comment 1

[Botond Ballo [:botond]]

One theory is that this is caused by the touch start tolerance, which we could consider revising to be lower (we last did that in bug 1230077). I reached out to the original bug reporter on Github to ask if tweaking that setting helps.

Also cc'ing Markus, I'm curious if you've noticed this being an issue.

Comment 2

[Markus Stange [:mstange]]

Yes I have. One of the problems I remember is that we don't only absorb the touch tolerance amount; we absorb the entire pan distance of the threshold-breaking event, even if the event's pan distance is much larger than the touch tolerance threshold.

Comment 3

[Botond Ballo [:botond]]

(In reply to Markus Stange [:mstange] from comment #2)

Yes I have. One of the problems I remember is that we don't only absorb the touch tolerance amount; we absorb the entire pan distance of the threshold-breaking event, even if the event's pan distance is much larger than the touch tolerance threshold.

Ah, interesting. That shouldn't be too difficult to fix.

Comment 4

[Razvan Cojocaru]

If this is something you feel I can do justice to with a Linux laptop touchpad, I'm up for writing a patch to fix it.
Any suggestions (other than looking at the code calling GetTouchStartTolerance())? Thanks!

Comment 5

[Botond Ballo [:botond]]

The codepath in question is specific to touchscreen input. The best platform to reproduce the issue is probably Android, though a desktop touchscreen could work as well.

If you don't have access to a touchscreen device, you could still work on it without reproducing it, by writing a gtest to exercise the scenario (something we should do anyways) and getting it to pass. (We can additionally ask someone with a touchscreen device, especially someone who is good at noticing subtle changes to scrolling physics such as Markus, to test a try build for us.)

Comment 6

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #5)

by writing a gtest to exercise the scenario (something we should do anyways)

Here's a description of the scenario at issue:

• Suppose the touch-start tolerance is 10 pixels.
• Page receives a touch-start event at some point, say (50, 50).
• Page receives a touch-move event, say at (50, 70), where the distance from the previous point (here, 20 pixels) is larger than the tolerance.
  • Currently, this event successfully overcomes the touch-start tolerance, but doesn't itself cause scrolling.
• Page receives another touch-move event, say at (50, 90), and now scrolls by 20 pixels.

We would like the total distance scrolled to be 30 pixels (the distance the finger moved from (50,50) to (50,90) minus the 10-pixel tolerance), but currently it's only 20 pixels (because the first touch-move event gets discarded completely).

Comment 7

[Razvan Cojocaru]

(In reply to Botond Ballo [:botond] from comment #5)

The codepath in question is specific to touchscreen input. The best platform to reproduce the issue is probably Android, though a desktop touchscreen could work as well.

If you don't have access to a touchscreen device, you could still work on it without reproducing it, by writing a gtest to exercise the scenario (something we should do anyways) and getting it to pass. (We can additionally ask someone with a touchscreen device, especially someone who is good at noticing subtle changes to scrolling physics such as Markus, to test a try build for us.)

I do have an Android smartphone (and an iPad somewhere in a drawer), but I've not really done any Android development so far, which is why I'm usually favouring working on desktop issues, where I'm more at home.

Comment 8

[Razvan Cojocaru]

I've attempted to write the gtest mentioned above, by adding one to TestBasic.cpp, like so:

TEST_F(APZCBasicTester, StartTolerance) {
  // SCOPED_GFX_PREF_FLOAT("apz.touch_start_tolerance", 0.1);
  SCOPED_GFX_PREF_FLOAT("apz.touch_start_tolerance", 0.5);

  tm->SetDPI(100);

  /*
  FrameMetrics fm;
  fm.SetCompositionBounds(ParentLayerRect(0, 0, 100, 100));
  fm.SetScrollableRect(CSSRect(0, 0, 100, 300));
  fm.SetVisualScrollOffset(CSSPoint(0, 50));
  fm.SetIsRootContent(true);
  apzc->SetFrameMetrics(fm);
  */

  TouchDown(apzc, {50, 50}, mcc->Time());

  CSSPoint initialScrollOffset =
      apzc->GetFrameMetrics().GetVisualScrollOffset();

  mcc->AdvanceByMillis(1);
  TouchMove(apzc, {50, 70}, mcc->Time());

  ASSERT_EQ(initialScrollOffset, apzc->GetFrameMetrics().GetVisualScrollOffset());

  mcc->AdvanceByMillis(1);
  TouchMove(apzc, {50, 90}, mcc->Time());

  // This is supposed to fail, just to show the current values until the test is finished.
  ASSERT_EQ(initialScrollOffset.y, apzc->GetFrameMetrics().GetVisualScrollOffset().y);

  // Clean up by ending the touch gesture.
  mcc->AdvanceByMillis(1);
  TouchUp(apzc, {50, 90}, mcc->Time());
}

And I've noticed that apz.touch_start_tolerance resets to 0.1 (which is its default value) between the TouchDown() call and the first TouchMove() call. I'm still debugging to figure out why, but it has dawned on me that perhaps you'd prefer that I put the test somewhere else, or write it in a different fashion. Either way, I thought the pref reset was worth noting (the log is being done in AsyncPanZoomController::GetTouchStartTolerance()):

Note: Google Test filter = APZCBasicTester.StartTolerance
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from APZCBasicTester
[ RUN      ] APZCBasicTester.StartTolerance
StaticPrefs::apz_touch_start_tolerance(): 0.5, GetDPI(): 100
StaticPrefs::apz_touch_start_tolerance(): 0.5, GetDPI(): 100
StaticPrefs::apz_touch_start_tolerance(): 0.5, GetDPI(): 100
got a touch-start in state NOTHING
got a touch-move in state: TOUCHING
StaticPrefs::apz_touch_start_tolerance(): 0.1, GetDPI(): 100
panThreshold: 10
got a touch-move in state: PANNING_LOCKED_Y
[       OK ] APZCBasicTester.StartTolerance (148 ms)
[----------] 1 test from APZCBasicTester (148 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (149 ms total)
[  PASSED  ] 1 test.

Comment 9

[Razvan Cojocaru]

I can, of course, work around the issue by setting the DPI to 100 and keeping apz.touch_start_tolerance at 0.1, but that's a poor choice because, for one, changing the default would break the code, and then there's also another test in TestBasic.cpp that does SCOPED_GFX_PREF_FLOAT("apz.touch_start_tolerance", 1.0f / 1000.0f); - ResumeInterruptedTouchDrag_Bug1592435.

Comment 10

[Razvan Cojocaru]

Later debugging report: turns out that calling SetDefaultAllowedTouchBehavior(apzc, touchBlock);, like the ResumeInterruptedTouchDrag_Bug1592435 test does helps keeping apz.touch_start_tolerance from being reset to 0.1.

Comment 11

[Razvan Cojocaru]

Right, my test now appears to do the right thing. I'm assuming the next step is to modify the UpdateWithTouchAtDevicePoint(aEvent) call corresponding to the TOUCHING case in AsyncPanZoomController::OnTouchMove() to take panThreshold into account somehow?

Comment 12

[Botond Ballo [:botond]]

(In reply to Razvan Cojocaru from comment #10)

Later debugging report: turns out that calling SetDefaultAllowedTouchBehavior(apzc, touchBlock);, like the ResumeInterruptedTouchDrag_Bug1592435 test does helps keeping apz.touch_start_tolerance from being reset to 0.1.

Good find!

The behaviour here is not very obvious, unfortunately. What's happening is:

• When input events are sent to the APZC (e.g. via TouchMove()), they are not handled immediately, but they go into a queue, and may need to wait for certain conditions to be de-queued and processed.
• For touch events, one of these conditions is knowing what the allowed touch-behaviours are for the event.
  • Normally, the allowed touch-behaviours are computed based on the hit test result, but in some cases they need to be provided externally. (In production, this happens via an IPC message to APZ from the browser's main thread.)
  • APZCBasicTester bypasses hit-testing, so in tests using this harness they always need to be provided externally, by calling SetAllowedTouchBehavior()..
    • In some test cases (for example APZCBasicTester.Fling), this happens inside helper functions such as Pan().
• If the touch-behaviors are not provided, the events stay in the queue until the end of the test. At this point, the destructor of the SCOPED_GFX_PREF_FLOAT runs and restores the pref to its default value.
  • The reason the events are subsequently handled is that Teardown() runs delayed tasks, and one of the delayed tasks is a timeout task meant to handle cases where the main thread is too slow to provide information (such as sending that IPC message) and APZ should go ahead and handle waiting events using the information it has. At this point, the events that were waiting are de-queued and processed.

Comment 13

[Razvan Cojocaru]

(In reply to Botond Ballo [:botond] from comment #12)

(In reply to Razvan Cojocaru from comment #10)

Later debugging report: turns out that calling SetDefaultAllowedTouchBehavior(apzc, touchBlock);, like the ResumeInterruptedTouchDrag_Bug1592435 test does helps keeping apz.touch_start_tolerance from being reset to 0.1.

Good find!

The behaviour here is not very obvious, unfortunately. What's happening is:

• When input events are sent to the APZC (e.g. via TouchMove()), they are not handled immediately, but they go into a queue, and may need to wait for certain conditions to be de-queued and processed.
• For touch events, one of these conditions is knowing what the allowed touch-behaviours are for the event.
  • Normally, the allowed touch-behaviours are computed based on the hit test result, but in some cases they need to be provided externally. (In production, this happens via an IPC message to APZ from the browser's main thread.)
  • APZCBasicTester bypasses hit-testing, so in tests using this harness they always need to be provided externally, by calling SetAllowedTouchBehavior()..
    • In some test cases (for example APZCBasicTester.Fling), this happens inside helper functions such as Pan().
• If the touch-behaviors are not provided, the events stay in the queue until the end of the test. At this point, the destructor of the SCOPED_GFX_PREF_FLOAT runs and restores the pref to its default value.
  • The reason the events are subsequently handled is that Teardown() runs delayed tasks, and one of the delayed tasks is a timeout task meant to handle cases where the main thread is too slow to provide information (such as sending that IPC message) and APZ should go ahead and handle waiting events using the information it has. At this point, the events that were waiting are de-queued and processed.

Makes perfect sense. Thanks for taking the time to explain this!

Comment 14

[Botond Ballo [:botond]]

(In reply to Razvan Cojocaru from comment #11)

Right, my test now appears to do the right thing. I'm assuming the next step is to modify the UpdateWithTouchAtDevicePoint(aEvent) call corresponding to the TOUCHING case in AsyncPanZoomController::OnTouchMove() to take panThreshold into account somehow?

Yep, that's definitely the right place to be looking.

Two potential approaches that come to mind:

Easier: Pretend the touch-move event that crosses the pan threshold was at the threshold. In our example, we would pretend the (50,70) event was actually at (50,60). Later, then when (50,90) event arrives, we'll scroll by 30 pixels.

Harder: Split the touch-move event into two, one at the threshold, and one covering the rest of the distance. In our example, we would split the (50,70) event into a (50,60) event and a (50,70) event. The first one would cause us to enter the PANNING state, and the second one can take the TrackTouch() codepath.

The advantage of the second approach would be that we can keep our velocity calculation more accurate because we can assign the invented event an interpolated timestamp. (And also that 10 pixels of scrolling happens a little bit sooner.) However, it's not clear if that small advantage is worth the extra complexity.

Comment 15

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #14)

The advantage of the second approach would be that we can keep our velocity calculation more accurate because we can assign the invented event an interpolated timestamp.

To explain this part a bit: one of the things UpdateWithTouchAtDevicePoint does is passes the current position to the velocity tracker, which is used to compute the velocity of an eventual fling animation.

Now suppose I'm scrolling like this:

t=0.0  pos=(0,0)
t=0.1  pos=(0,100)
t=0.2  pos=(0,200)

Here, I'm scrolling at a constant velocity. But suppose the threshold is 10 and we implement the easier approach. The velocity tracker will see this:

t=0.0  pos=(0,0)
t=0.1  pos=(0,10)
t=0.2  pos=(0,200)

and think I'm actually scrolling at a sharply accelerating a velocity. If I then let go of the screen quickly, it might produce a wildly fast fling :)

With the interpolation approach, we could send it this instead:

t=0.0  pos=(0,0)
t=0.01 pos=(0,10)
t=0.1  pos=(0,100)
t=0.2  pos=(0,200)

which is now again a constant velocity.

Anyways, maybe we can worry about this later.

Comment 16

[Razvan Cojocaru]

I'm fine with taking either approach (it sounds like we might prefer the second approach). Any recommendation on where best to begin?
Is the splitting of the event to be done in AsyncPanZoomController::OnTouchMove()'s TOUCHING case, or above, in some (direct or indirect) caller?

For obvious reasons, I had only thought of the first approach before your replies. :)

Comment 17

[Razvan Cojocaru]

Is there a helper function somewhere that returns a point a certain length between two points, or do I need to implement this from scratch?

Comment 18

[Razvan Cojocaru]

Attachment: Bug 1755044 - Make every pixel of movement past the touch-start tolerance threshold cause scrolling. r?botond,dlrobertson,mstange

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4bdea458a08
Make every pixel of movement past the touch-start tolerance threshold cause scrolling. r=botond

Comment 20

[Norisz Fay [:noriszfay]]

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

Comment 21

[Botond Ballo [:botond]]

Thanks very much for your work on this, Razvan! While it turned out to be more involved than I initially imagined, you stuck with it and we were able to work through the various issues that came up.

Also a shout-out to Markus whose previously-added fling velocity tests (such as this one) caught what might otherwise have been a regression in our scrolling physics from this patch.

Comment 22

[Razvan Cojocaru]

(In reply to Botond Ballo [:botond] from comment #21)

Thanks very much for your work on this, Razvan! While it turned out to be more involved than I initially imagined, you stuck with it and we were able to work through the various issues that came up.

No problem, of course things should be finished once started. Thanks for the prompt and helpful reviews!

Comment 23

[Chris Peterson [:cpeterson]]

This fix will ship in 107. No need to uplift this fix to Beta 106.