woltapp / wolt_modal_sheet

This package provides a responsive modal with multiple pages, motion animation for page transitions, and scrollable content within each page.
MIT License
505 stars 64 forks source link

when child is a listView,scrollbar behaves improperly. #46

Closed leavky closed 1 year ago

leavky commented 1 year ago

Thank you very much for releasing this package, it's been very helpful. I have encountered an issue while using it, specifically with the scrolling behavior. When I set the child to be a ListView or ReorderableListView, the scrollbar behaves improperly. Additionally, when the child is a ReorderableListView, the drag-and-drop sorting becomes very laggy.

ulusoyca commented 1 year ago

Hi, thanks for the feedback. Can you please provide sample code to reproduce and also the screen recording?

leavky commented 1 year ago

code in example, just replace child as ListView.builder.

    WoltModalSheetPage page1(BuildContext modalSheetContext, TextTheme textTheme) {
      return WoltModalSheetPage.withSingleChild(
        stickyActionBar: Padding(
          padding: const EdgeInsets.all(_pagePadding),
          child: Column(
            children: [
              ElevatedButton(
                onPressed: () => Navigator.of(modalSheetContext).pop(),
                child: const SizedBox(
                  height: _buttonHeight,
                  width: double.infinity,
                  child: Center(child: Text('Cancel')),
                ),
              ),
              const SizedBox(height: 8),
              ElevatedButton(
                onPressed: () => pageIndexNotifier.value = pageIndexNotifier.value + 1,
                child: const SizedBox(
                  height: _buttonHeight,
                  width: double.infinity,
                  child: Center(child: Text('Next page')),
                ),
              ),
            ],
          ),
        ),
        topBarTitle: Text('Pagination', style: textTheme.titleSmall),
        isTopBarLayerAlwaysVisible: true,
        leadingNavBarWidget: IconButton(
          padding: const EdgeInsets.all(_pagePadding),
          icon: const Icon(Icons.arrow_back_rounded),
          onPressed: () => pageIndexNotifier.value = pageIndexNotifier.value - 1,
        ),
        trailingNavBarWidget: IconButton(
          padding: const EdgeInsets.all(_pagePadding),
          icon: const Icon(Icons.close),
          onPressed: Navigator.of(modalSheetContext).pop,
        ),
        child: Padding(
          padding: const EdgeInsets.fromLTRB(
            _pagePadding,
            _pagePadding,
            _pagePadding,
            _bottomPaddingForButton,
          ),
          child: SizedBox(
            height: 400,
            child: ListView.builder(
                itemCount: 100,
                itemBuilder: (context, index) {
                  return Text(index.toString());
                }),
          ),
        ),
      );
    }

image

leavky commented 1 year ago

also in coffee_maker, in windows, the scroll hehavior so strangeness, when drag scrollerbar print Exception caught by gestures library image

ulusoyca commented 1 year ago

@leavky First of all, if the main content is scrollable, you should use sliverList factory constructor for the page instead of withSingleChild since sliverList builds the main content items lazily.

Thanks for reporting the issue regarding the extra scroll bar shown in web and desktop apps. This PR (https://github.com/woltapp/wolt_modal_sheet/pull/58) fixes this issue.

@rydmike I can reproduce the exception thrown when scrollbar is dragged in MAC app. It is due to the DragScrollBehavior I use here that I took from FlexColorScheme. Are you familiar with it?

ping @TahaTesser

======== Exception caught by gestures library ======================================================
The following assertion was thrown while handling a pointer data packet:
'package:flutter/src/rendering/proxy_box.dart': Failed assertion: line 2962 pos 12: '!debugNeedsLayout': is not true.

Either the assertion indicates an error in the framework itself, or we should provide substantially more information in this error message to help you determine and fix the underlying cause.
In either case, please report this assertion by filing a bug on GitHub:
  https://github.com/flutter/flutter/issues/new?template=2_bug.md

When the exception was thrown, this was the stack: 
#2      RenderFractionalTranslation.hitTestChildren (package:flutter/src/rendering/proxy_box.dart:2962:12)
rydmike commented 1 year ago

Without looking into the details of this particular case, one issue I have seen on desktop, that seems very related to this, is that on desktop, scrollbars will appear by default where they are not seen on phones/tablets, this may cause issues with multiple scrollbars and where the default scroll controller belongs to both, which will cause a similar assert error. At least that is what I recall from seeing this issue before in the Themes Playground app.

What worked for me, in cases where both scrolling regions were needed, was to totally disable the scrollbar on the widget that should not have one (in the issue image you have two scrollbars for different scrolling regions, you probably only want one) and make sure you have only one scroll controller per scrolling surface, meaning you may have to create a scroll controller manually for them, to make them unique.

You can also try setting the 'primary: true' property on the list view that should be the main one if there are multiple ones, but I honestly don't remember if that helped at all, probably not.

It has been a while since I looked at my similar case, so I am just throwing what I recall from it on the wall. Sorry, I am on a phone now, so I can't look at the details. Without trying the reproduction sample and seeing the assert, and attempting a fix for it, I can't be 100% sure it is the same situation, but it sounds and looks very similar.

Sometimes you can also get rid of the widget causing the 2nd scrolling area and having them in the same scrolling area, depends on the use case of course.

ulusoyca commented 1 year ago

I found out that it is not the scroll bars causing this exceptions but the drag gestures on desktop @rydmike since I did this:

final scrollView = CustomScrollView(
      scrollBehavior: (const DragScrollBehavior()).copyWith(scrollbars: false),
TahaTesser commented 1 year ago

I found out that it is not the scroll bars causing this exceptions but the drag gestures on desktop @rydmike since I did this:

final scrollView = CustomScrollView(
      scrollBehavior: (const DragScrollBehavior()).copyWith(scrollbars: false),

Looks correct. I forgot I added an official Flutter example just for this behavior https://api.flutter.dev/flutter/widgets/RawScrollbar-class.html#cupertino.RawScrollbar.3

ulusoyca commented 1 year ago

I found out that it is not the scroll bars causing this exceptions but the drag gestures on desktop @rydmike since I did this:

final scrollView = CustomScrollView(
      scrollBehavior: (const DragScrollBehavior()).copyWith(scrollbars: false),

Looks correct. I forgot I added an official Flutter example just for this behavior https://api.flutter.dev/flutter/widgets/RawScrollbar-class.html#cupertino.RawScrollbar.3

Ok, however we still receive exceptions caused by drag to scroll gestures on listview in desktop app.

TahaTesser commented 1 year ago

Did you look at https://docs.flutter.dev/release/breaking-changes/default-scroll-behavior-drag?

rydmike commented 1 year ago

Perfect 💙👍

In this case dragscroll is causing extra scrollbars to appear, and you get multiple ones (two in the example issue case), this causes the default scroll controller to get confused about which scrollbar should be using it, it gets very unhappy when two scrollbars both inherit the same one.

Fix is like you did, disable the scrollbar(s) where not needed, only keep default scrollbars in one place. The scrollbar left is then no longer upset that another scrollbar is also using the same default controller. The issue then goes away.

Alternatively use separate scroll controllers for each scroll area and thus scrollbar. This is seldom desirable though (to have two scrollbars), but there are some rare use cases for it.

Both options work, I tried them when dealing with similar issue in the Themes Playground app.

TahaTesser commented 1 year ago

The error thrown in https://github.com/woltapp/wolt_modal_sheet/issues/46#issuecomment-1712503078 is very generic. If you could isolate the issue with a minimal sample It'll help debug the issue.

TahaTesser commented 1 year ago

Also worth testing this issue on a different Flutter branch to make sure it's not an error from Flutter itself e.g. master.

ulusoyca commented 1 year ago

I found out that this is due to shrinkWrap set to true in CustomScrollView

 CustomScrollView(
      scrollBehavior: ScrollConfiguration.of(context).copyWith(
        dragDevices: <PointerDeviceKind>{
          PointerDeviceKind.touch,
          PointerDeviceKind.mouse,
          PointerDeviceKind.trackpad,
          PointerDeviceKind.stylus,
        },
      ),
      shrinkWrap: true,
  /// Shrink wrapping the content of the scroll view is significantly more
  /// expensive than expanding to the maximum allowed size because the content
  /// can expand and contract during scrolling, which means the size of the
  /// scroll view needs to be recomputed whenever the scroll position changes.

ChatGPT says: The error you're seeing, specifically the !debugNeedsLayout assertion failure, suggests that there's a widget in your tree that is being accessed or interacted with (in this case, during a hit test) before its layout phase is complete. This kind of error is often associated with how your widgets are being built, especially if you're doing dynamic or conditional widget construction.

TahaTesser commented 1 year ago

Should avoid using shrinkWrap anywhere in the package. Please see https://github.com/flutter/flutter/issues/130360

ulusoyca commented 1 year ago

flutter/flutter#130360

Ok, this is important, and quite news to me. The whole modal sheet layout calculations rely on shrinkWrap. I need to find a way of getting the implicit height of the main content during rendering to set the layout sheet which is sliver. Let's have a meeting about it when you have time.

TahaTesser commented 1 year ago

Sure thing

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

yunusemrebakir commented 1 year ago

I will create a separate issue to continue shrinkWrap and size calculation discussions.

iyar-avital commented 11 months ago

Same issue, can't solve. What is the actual solution?

If I cancel my scroll by the ScrollBehavior, The other scrollbar not work by touch @ulusoyca