flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
165.33k stars 27.28k forks source link

Rewrite the NestedScrollView #102003

Open Piinks opened 2 years ago

Piinks commented 2 years ago

I have a few ideas for this, although I will not be able to pursue them now.

Ideally, a re-vamped NestedScrollView would solve these issues (among others, theses are just a few of several that stand out):

Lastly, it would be awesome if we could do this without breaking anyone. :)

tenhobi commented 1 year ago

Not sure if it is a different issue, but similar to #102001 that is describing issues with inner horizontal scrollviews because of PSC, if user tries to add (or by default have on web and desktop) scrollbars into a NestedScrollView with a TabBarView, because of shared PSC, exceptions are raised too, so some NestedScrollController would be very helpful. 🙏 Could you take a look at #118713 and confirm that these issues are known and tracked? :)

And also that scrollbars in such configuration are hidden when scrolled to top with pinned AppBar etc.

maRci002 commented 1 year ago

I think #36419 should be also considered while rewriting NestedScrollView since each CustomScrollView / ListView children might have some state which shouldn't be destroyed ( for instance a ListView(scrollDirection: Axis.vertical) might have multiple ListView(scrollDirection: Axis.horizontal) children or StatefulWidget children for instance Switch Widget ) so PageStorageKey is not enough to retain a whole Widget tree.

36419 tries to scroll every attached scroll views which means Scrollbar will throw exceptions ( just like #118713 which uses PageStorageKey ).

Which means _NestedScrollCoordinator._innerPositions should return only one ScrollPosition otherwise multiple scroll views will be scrolled together.

Current implementation: https://github.com/flutter/flutter/blob/15cd00f1ed126e5b5c036e2cb6a7373b2df5ba52/packages/flutter/lib/src/widgets/nested_scroll_view.dart#L597-L599

This could be easily avoided by wrapping each CustomScrollView / ListView by an InheritedWidget ( in my example NestedScrollViewIsScrollViewActiveProvider ).

Expected implementation:

  Iterable<_NestedScrollPosition> get _innerPositions {
    return _innerController.nestedPositions.where((p) {
      // return true by default so there isn't any breaking change
      return NestedScrollViewIsScrollViewActiveProvider.of(p.context.storageContext)?.isActive ?? true;      
    });    
  }
class MyStatelessWidget extends StatelessWidget {
  const MyStatelessWidget({super.key});

  @override
  Widget build(BuildContext context) {
    final List<String> tabs = <String>['Tab 1', 'Tab 2'];
    return DefaultTabController(
      length: tabs.length,
      child: Scaffold(
        body: NestedScrollView(
          headerSliverBuilder: (BuildContext context, bool innerBoxIsScrolled) => [...],
          body: TabBarView(
            children: tabs.map((String name) {
              return Builder(
                builder: (BuildContext context) {
                  // use some state manager to determine `isActive` by current tab `name`
                  final isActive = ...;

                  // Provider `isActive` flag
                  return NestedScrollViewIsScrollViewActiveProvider(
                    isActive: isActive,
                    child: CustomScrollView(
                      slivers: [
                        // ...
                      ],
                    ),
                  );
                },
              );
            }).toList(),
          ),
        ),
      ),
    );
  }
}

ScrollBar issues:

AsturaPhoenix commented 1 year ago

I wonder if, as part of this kind of rewrite, it'd be worthwhile to consider DraggableScrollableSheet use cases as well. I haven't looked too deeply into NestedScrollView itself, but DraggableScrollableSheet is also coordinating between inner and outer scrollables, where it's subclassing ScrollPosition to actuate the outer sheet in some cases and the inner scrollable (via super) in others. Right now because both DraggableScrollableSheet and NestedScrollView expect their own subclasses of ScrollPosition, they can't be used in the same context, but I think DraggableScrollableSheet ends up doing a lot of the legwork coordinating between scrollables anyway, and I think it may be just as much in need of a rewrite. I suspect it may be possible to factor out a scroll controller (coordinator?) that coordinates between an outer scrollable and inner scrollables based on some (possibly complex) policy.

Tracking using DraggableScrollableSheet with multiple inner scrollables at #119966.

(Of similar concern, but I believe architected a different way: linked_scroll_controller)

If we wanted to scope creep further, I get the impression it might be worth re-examining the architectural distinction between ScrollControllers and ScrollPositions (and maybe other pieces), because they seem to be bleeding together somewhat and contributing to the grief surrounding scrollbars not being able to work with ScrollControllers with more than one ScrollPosition (e.g. #118713).

erikas-taroza commented 1 year ago

I want to use NestedScrollView but it has too many issues that prevents a perfect UX. Looking forward to the rewrite :)

olof-dev commented 1 year ago

In a rewrite, the center property of ScrollView could also be exposed. With the current code, it seems trivial to add, as long as one is OK with the center key being placed on one of the header widgets of the NestedScrollView. (By 'trivial' here I mean that there's only 5 places that one needs to put super.center or make sure it's passed along.)

Reprevise commented 1 year ago

Personally, I think this should get reprioritized. There are too many issues with NestedScrollView that are affecting apps and there are little to no good temporarily workarounds in order to get the same behavior. Many of these NestedScrollView issues have been around for years and they need to get resolved.

rusyaev-dk commented 2 months ago

So, eventually I found the correct solution!

That work perfectly with any additional package (e.g. auto_route) or you can use just classical TabBarView.

Dear Flutter team, please just make this fix <3

P.S. if you use this on iOS device, it won't work as expected. To fix it read this: https://github.com/flutter/flutter/issues/17649#issuecomment-2279644392 - so you need to catch overscrolling notification. If you have questions, write down below