It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Update:
I've added the same null check in the CupertinoScrollbar thumb press gesture from @TahaTesser comment in #81277 (comment)

This same check can also be found in the other CupertinoScrollbar gestures:

Thanks for the contribution! Is there a test you can add that shows what issue this fixes (that would have failed before your patch)?

@tvolkert I tried looking into related tests in the framework but I didn't find any. In fact, I removed the checks from all gestures in the scrollbar and tests were still passing.
I'm not sure how to test this situation in particular because it implies a first step of quickly moving the scrollbar so that it appears before doing the gesture that crashes. We cannot use the isAlways shown property here.

@tvolkert I've added some tests.

@Piinks Thanks! Yes, I've ended up figuring out the cause.
The problem here is that is not clear that the Scrollbar always needs a scroll controller to function correctly visually (without interaction).
On our 2 year old app we've been using scrollbars here and there, and when we needed a scroll controller, we only passed it to the listview and co.
Do you think the fix (the early return) is still worthy to be included? I believe it is, given the similar checks that other gestures have and because it is an optional parameter that only affects the interactivity. In many cases one only wants to see the amount scrolled, which works fine even when using different controllers.
I don't really understand how the isAlwaysShown applies here. For this use case I don't mind for it to hide. Do you mean including an assert to prevent this kind of mistake from happening?

It was only until now that we included a crash reporting tool that we found those errors on production code. We've never found those while developing all this time.

@Piinks Thanks! Yes, I've ended up figuring out the cause.
The problem here is that is not clear that the Scrollbar always needs a scroll controller to function correctly visually (without interaction).

Visually, no it does not need a ScrollController, we actually used to assert that a scroll controller was always provided for interaction. We removed that to allow the PrimaryScrollController to be an option and remove some of the churn of always having to wire it up yourself.

This is because we wanted to support multiple options for the Scrollbar. You do not always have to provide a scroll controller to the scrollbar, but in this case you do. There are some examples and documentation updates that have not arrived to the stable branch yet, you can see the master branch documentation here for Scrollbar, there is more under RawScrollbar and CupertinoScrollbar as well.

On our 2 year old app we've been using scrollbars here and there, and when we needed a scroll controller, we only passed it to the listview and co.

We do have a new feature that recently landed that allows you to add a Scrollbar as part of the ScrollBehavior. This might be of benefit to your use case as the ScrollBehavior handles setting everything up for you - including the ScrollController. There is a migration guide available here to show you how to do this using ScrollBehavior.buildScrollbar. This could resolve your problem another way and never require you to wire up ScrollView/Scrollbar yourself. :)

Do you think the fix (the early return) is still worthy to be included? I believe it is, given the similar checks that other gestures have and because it is an optional parameter that only affects the interactivity. In many cases one only wants to see the amount scrolled, which works fine even when using different controllers.

I do not think we want the early returns as it breaks the scrollbar without any explanation to the user.

I don't really understand how the isAlwaysShown applies here. For this use case I don't mind for it to hide. Do you mean including an assert to prevent this kind of mistake from happening?

I don't think this is related to isAlwaysShown, the PR I linked was meant to illustrate the type of error messaging
we have been adding to help users identify this issue. I think we can take a similar approach/error message for this case that informs the user what scroll controller the scrollbar is trying to use.

@Piinks Oh, lots of new things coming to the scrollbar. Thanks for the info!

I do not think we want the early returns as it breaks the scrollbar without any explanation to the user.

I'm looking into the error message improvement. How should we go about it? Currently the getScrollbarDirection() is documented to return null when no position is attached and the gesture handlers look like this:

  void handleThumbPressStart(Offset localPosition) {
    final Axis? direction = getScrollbarDirection();
    if (direction == null) {
      return;
    }
    ....

How do you think it would be better?

1. Throw after the check

  void handleThumbPressStart(Offset localPosition) {
    final Axis? direction = getScrollbarDirection();
    if (direction == null) {
      // Call an assertion that throws the explanation like on the isAlwaysShown situation.
      assertion();
      return;
    }
    ....

2. Change the getScrollbarDirection API to not return null and show the appropriate error message from inside that function.

3. Call a check method at the start of every gesture

  void handleThumbPressStart(Offset localPosition) {
    // Throws the FlutterError internally
    _checkForValidScrollPosition();
    ....

4. Any other suggestion

We could also change the API of getScrollbarDirection to not return null and call the check at the start as in 3)

Something along the lines of 3 sounds good to me! Thank you for continuing to work on this and making the error messaging better! :)

You could probably take most of the error from the isAlwaysShown and put it in a method to reuse here and there.

@Piinks I've updated the PR and tests with the suggestions

I've been thinking a bit more about this. Do you think we should have this assertion check when building the scrollbar for the first time?. In the same place as the isAlwaysShown check, but unconditionally.

I think that sounds like a great idea! If RawScrollbar.interactive is true, we could definitely surface that error sooner for gestures. You can have a scrollbar that is not interactive, but isAlwaysShown, so I think those would be two separate checks still.

@Piinks Any update on this?

I've included 2 tests that check that the controllers used internally are different because the primary one can't be attached to multiple scroll views at the same time.

@Piinks Thanks to you!

It looks like this PR introduced issues for flutter_gallery. For example, just running flutter_gallery ends up triggering asserts introduced by this PR now.

I thould add that this seems to only happen on macOS desktop.

@dnfield I've just tested the flutter_gallery on Linux and I also get the asserts. But I'd say it is a legitimate warning. If you disable the asserts you will notice that scrolling with a mouse press in the main scroll in the home screen doesn't work.
From what I debugged that is because the PrimaryScrollController is being shared by multiple pieces of the UI at the same time, in particular:

• The main scroll in the home page
• The scroll inside the settings
• Each of the 3 categories scrolls (Material, Cupertino and "Styles & Other")

The fact that it is such an "invisible" bug was the main reason I tried to improve the situation with this PR by warning about it. Everything works normally but it fails when interacting with the scrollbar either with the thumb on mobile or with the mouse on desktop.
Maybe @Piinks has some more information about this. The PR doesn't change the previous behavior, rather warns about the situation beforehand.

As an extra note, I think it will be great to further improve the message by including the location of the failing widget. In the case of the Flutter Gallery just in the home screen there are 5-6 active scrolls, so knowing which one is failing is very precious information.
I don't know who in the team has some expertise with how the Flutter errors and diagnostic nodes work so that the message can be improved. I asked on Discord the other day https://discord.com/channels/608014603317936148/608021234516754444/853999114018750484

scrolling with a mouse press in the main scroll in the home screen doesn't work.

@davidmartos96 can you confirm if this means trying to drag the scrollable with the mouse pointer? That is most likely disabled by ScrollBehavior.dragDevices on desktop if that is the case.

I'm filing a bug for this so we can discuss further, will link back in a sec

Also just connecting discussions - this came up here: https://discord.com/channels/608014603317936148/608021234516754444/854798350246215751

Let's follow-up here: flutter/gallery#523

@davidmartos96 can you confirm if this means trying to drag the scrollable with the mouse pointer?

Yes, it is by dragging with the mouse pointer. Running the app on profile (no asserts) on Linux desktop will throw when dragging the thumb.

Could you please also check out this related issue #84735

Could you please also check out this related issue #84735

Followed up there.