NestedScrollView's outer scrollable jumping with BouncingScrollPhysics due to double precision errors

[Michal-MK]

This PR fixes scrolling issues with NestedScrollView using the BouncingScrollPhysics. In one of the steps of the calculation, we can reach a state where the position of the inner scrollable is set to a double value that falls within precisionErrorTolerance of 0 but we were using == with 0 rather than checking for a precision. My posts in the linked issue show the current behavior, and how I reached to conclusion (the code in this PR). This PR only addresses the "jumping" of the outer scrollable.

Fixes #136199

Pre-launch Checklist

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
• I signed the [CLA].
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is [test-exempt].
• All existing and new tests are passing.

I have not finished a test for this since I have never done so and therefore have 0 experience writing tests in Flutter, so any help there would be appreciated. I am also not sure how to test double precision errors in general. I did run all the nested_scroll_view_tests.dart locally and there are no failures.

[goderbauer]

@Michal-MK thanks for the contribution. Looks like this PR is failing some checks. Can you please take a look and fix those up? Thanks!

[Michal-MK]

@goderbauer I can decode the Linux Analyze check, which is clear to me, I have a TODO in tests, but the issue is I do not know how to write tests for this fix. For the other 6 failing checks I have no idea what the output is trying to tell me. I looked at the "undestading a LUCI build failure", but that to me appears to describe something else.

�05:52:02� Test failed.
��������������������������������������������������������
For your convenience, the error messages reported above are repeated here:
  �  �  

[Michal-MK]

Apart from tree-status all tests should pass, I'll wait and see if a test for this will be needed/asked for.

[Piinks]

Thanks for the contribution @Michal-MK!
This will need a test to verify it works as expected, and to prevent us from accidentally undoing this in the future. :)

[Michal-MK]

Okay so I tried something, the issue is that I do not fully understand the test as I just while(true)'d until a set of inputs caused the if to be hit with the faulty values. Then I searched for a place where the wrong value would appear, and it appeared in the outer scrollable's controller.

I replaced another == 0 check that may render the first one obsolete, and added a test that will fail if the outer scroll controller's listener is called. Since that is the place I found manifests the precision error to the user.

This is the output before the fix. (here it passes since I added the assert(false) after)
After the fix it does not trigger the outer controller's callback:

All other tests in NestedScrollView pass.

[Piinks]

Hey @Michal-MK welcome. Thanks for contributing!

[Piinks]

Why? The outer should scroll before the inner, why would the outer not scroll?

[Michal-MK]

The way I understand the test is that we have:
tester.drag(find.byKey(innerKey), const Offset(0, 2000));

Which means dragging the finger down the screen (I believe). So we are immediately overscrolling and triggering the dampening effect of the spring. The total height is 300 + 64 + 800 from the individual components. 1164 < 2000. So we have technically 836 units to work with.

tester.fling(find.byKey(innerKey), const Offset(0, -600), 2000);

After that we immediately fling the finger up the screen. My understanding is that the total distance will be 600 units up and the 2000 just affects how quickly the 600 will happen.

Which equals 836 - 600 = 236. So we still have 236 units to spare before the outer scrollable should start scrolling. What does not fly is the spring does not actually "stretch" to 836 visually, but I have no idea how that is implemented. If this description makes sense I will add it to the test itself, but there is a possibility it is nonsense and I just do not understand what I wrote ;).

[Piinks]

Hmm, it may be easier to understand if you add checks on what the actual position is before and after each gesture. What is the value of position.pixels for the outer and inner each time? For the nested scroll view in general, the inner should not scroll before the outer has fully scrolled to its end.

If we are dragging down first, overscrolling the outer, then flinging up, I would expect the outer to consume some of the distance flinging up before the inner receives any.

[Michal-MK]

"For the nested scroll view in general, the inner should not scroll before the outer has fully scrolled to its end." - True, but since in the default state, the outer is at zero we are immediately overscrolling inner scrollable.

"If we are dragging down first, overscrolling the outer, then flinging up, I would expect the outer to consume some of the distance flinging up before the inner receives any." - the outer is not being overscrolled: This is the default state:

And this is the state after pulling down:

The outer scrollable does not move, and I checked that by adding a listener to the outer scroll view and pulled downwards, and indeed, I got no prints. I.e. the callback was never invoked.

outerController.addListener(() {
  print('outerController: ${outerController.offset}, ${outerController.position}');
});

Since we are not supposed to assign a scroll controller to the inner scrollview I do not have output from that. But I did try a fling up, after overscrolling the inner scrollable, and it can actually be fairly strong and yet it does not trigger anything in the outer scrollable.

20231130_185335-small.mp4

The actual positions are in here:
#138319 (comment)
As can be seen, before it fluctuated between "immeasurable delta from 0" and 0 now it stays on 0. I do not know how to test that other than though the scroll controller not firing.

[Piinks]

the outer is not being overscrolled

Ah right I see. The inner is using bouncing scroll physics. Yup. So the inner overscrolls at the top, but then when the test flings the inner up, I expect the outer to consume some of the distance before the inner does.

Since we are not supposed to assign a scroll controller to the inner scrollview I do not have output from that.

The inner scroll controller is accessible through the inner key you have created in the test.

Can you add checks that validate the inner and outer positions at each step to the test?

[Michal-MK]

Sure, I'll try.

[Michal-MK]

Alright, I added more asserts to the test, I did not find a way to obtain the controller from the previous setup so I just borrowed someone else's solution :). The outer scrollable does not move, it may be surprising but it's true (according to the expect() and the .addListener())

[Piinks]

@Michal-MK thanks for the update. It looks like there are a lot of failing tests currently, can you take a look?

[Piinks]

Hmm, it may be easier to understand if you add checks on what the actual position is before and after each gesture. What is the value of position.pixels for the outer and inner each time? For the nested scroll view in general, the inner should not scroll before the outer has fully scrolled to its end.

If we are dragging down first, overscrolling the outer, then flinging up, I would expect the outer to consume some of the distance flinging up before the inner receives any.

[Michal-MK]

Tests are failing randomly, as noted here: #138319 (comment). No idea how to decode the output. I just merged current master into it and it worked again. That was the case even before I added my test so i do not know what is causing it.

[Michal-MK]

So it seems that Linux web_canvaskit_tests_4 and Linux web_tests_0 are now failing consistently, but their output is still illegible: â��â��â��â��â��â��â��â��â�

[Michal-MK]

After spending more time with it I did find the tiny "show more details" button. There I finally got the actual output. Hmm why is GitHub's so broken then...? I modified the test to hopefully pass the two failing cases.

[Piinks]

Rather than this complicated value, you could use the greaterThan, moreOrLessEquals, etc tools that flutter_test provides.

[Michal-MK]

Since. we are essentially testing a double precision error, I included the exact value the controller had after the drag, but I can change it.

[Michal-MK]

On a second thought I like how the math adds up, because later in the code I do: endValue + 600 which would then not work... If I used moreOrLessEquals the delta would have to be really big. I could do moreOrLessEquals(-2000, delta: 100.0) since 2000 is the drag parameter, but I do not like how vague that becomes.

[Piinks]

Ok! We should probably add some context at least in a comment for future wonderers. Style guide has recommendations: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-mysterious-and-magical-numbers-that-lack-a-clear-derivation

[Piinks]

I am not sure I follow here. What was the failure this is working around?

[Michal-MK]

Before I had concrete values at each step, as seen in this commit, I then rewrote it since tests on Web were failing due to the controller reporting different values than seen on native platforms. I can remove the comment if that is intended.

[Piinks]

This change looks good, with added context in the test for that value. I will be OOO on vacation after today, so if you would not like to wait, another reviewer can be requested on the Discord. This change will need two approvals. Thanks!

[Piinks]

Thanks for the update @Michal-MK, and happy new year! This LGTM!

[Michal-MK]

Happy new year to you and all of the Flutter team. Awesome framework, glad I could contribute. :)

[Michal-MK]

@Piinks Is there something on my part that needs doing? The "Google testing" test is now running for more than 5 days, is that intended?

[Piinks]

This still needs a second review, but I have rebased it to kick the Google testing shard. 👍
Following up on the second review in https://discord.com/channels/608014603317936148/804482791517192273/1193960740856545290

[Piinks]

Hmm, I was not able to rebase since there is a merge conflict. Can you rebase with the master branch @Michal-MK?

[Michal-MK]

Struggling with the rebase, give me a minute :)

[Michal-MK]

So, I finally won agains git rebase, unfortunately, the bot added some people for review, that was not intentional. @Piinks I do not see the Discord channel and I have not found any info on how to enable it on what is publicly available.

[Piinks]

Thanks for rebasing! Looks like that channel is restricted to contributors with commit access (members of flutter-hackers): https://github.com/flutter/flutter/wiki/Contributor-access

Sorry for sharing something inaccessible. The channel is for seeking reviewers, so it should get picked up soon. 👍

[goderbauer]

secondary LGTM