Skip to content

Fix draggable scrollable sheet scroll notification #45083

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

itome
Copy link
Contributor

@itome itome commented Nov 18, 2019

Description

ScrollEndNotification for DraggableScrollable's child ScrollView doesn't dispatched when inertia scroll ended in its extent. And then, it is dispatched when next drag started.

import 'package:flutter/material.dart';

void main() => runApp(SampleApp());

class SampleApp extends StatefulWidget {
  @override
  SampleAppState createState() => SampleAppState();
}

class SampleAppState extends State<SampleApp> {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        body: Center(
          child: Builder(
            builder: (context) {
              return RaisedButton(
                child: Text('Show bottom sheet'),
                onPressed: () {
                  showModalBottomSheet(
                    isScrollControlled: true,
                    backgroundColor: Colors.transparent,
                    context: context,
                    builder: (context) {
                      return DraggableScrollableSheet(
                        initialChildSize: 0.5,
                        minChildSize: 0.25,
                        maxChildSize: 1.0,
                        builder: (context, controller) {
                          return Container(
                            color: Colors.white,
                            child: NotificationListener<ScrollNotification>(
                              onNotification: (notification) {
                                print(notification);
                                return false;
                              },
                              child: SingleChildScrollView(
                                controller: controller,
                                padding: EdgeInsets.symmetric(
                                    horizontal: 24, vertical: 8),
                                child: Text(
                                  '''
Hello and welcome to another stable release of Flutter. So far this year, we’ve been right on target with one stable release each quarter, as per our plan (well, less of a plan and more of a goal, but still, it’s been working out pretty well so far…). This release is our biggest yet, with 1,548 Pull Requests merged from 108 contributors. As always, the interesting PRs are listed below. And there are lots of interesting things to discuss in this release, including:

One regression fixed but also one added
Some breaking API changes
Some severe issues caught and fixed
Support for macOS Catalina and iOS 13
A number of new features
And more!
And to be clear, when I say “we,” I mean the Flutter community as a whole. The Flutter team couldn’t possibly continue to scale as we have without all of our contributors, no matter who your employer is. Thanks everyone for your contributions!
                                  ''' *
                                      10,
                                ),
                              ),
                            ),
                          );
                        },
                      );
                    },
                  );
                },
              );
            },
          ),
        ),
      ),
    );
  }
}
master this PR
ezgif com-video-to-gif (3) ezgif com-video-to-gif (4)

To fix this, I added super.goBallastic(0) in _DraggableScrollableSheetScrollPosition.goBallastic() when inertia scroll ended.

Related Issues

Replace this paragraph with a list of issues related to this PR from our [issue database]. Indicate, which of these issues are resolved or fixed by this PR.

Tests

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read [Handling breaking changes]).
    ScrollEndNotification is dispatched at the right time
  • No, this is not a breaking change.

@fluttergithubbot
Copy link
Contributor

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.

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

@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Nov 18, 2019
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@itome
Copy link
Contributor Author

itome commented Nov 18, 2019

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@gspencergoog
Copy link
Contributor

Hi @itome, thanks for the PR!

Could you please add a test that fails on master, and succeeds with your PR added? That way we can prevent a regression of your change in the future.

@itome itome force-pushed the fix-draggable-scrollable-bottom-sheet-listener branch from c3a3dc3 to 768732e Compare November 19, 2019 06:07
@itome
Copy link
Contributor Author

itome commented Nov 19, 2019

@gspencergoog I added tests!


final List<Type> types = <Type>[
ScrollStartNotification,
ScrollEndNotification,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gspencergoog Do you think UserScrollNotification and ScrollUpdateNotification should also be dispatched while dragging without covering its container?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, it seems like in this case we could file a seaprate bug for that and fix it in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(With a TODO here for it)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added TODO comment. And I will send another PR for this bug. Do I need to post issue before sending PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issuebis helpful to make sure we don't lose track

@goderbauer goderbauer requested a review from dnfield November 20, 2019 18:56
Comment on lines 301 to 309
expect(notificationTypes.removeAt(0), ScrollStartNotification);
expect(notificationTypes.removeAt(0), UserScrollNotification);
expect(notificationTypes.removeLast(), UserScrollNotification);
expect(notificationTypes.removeLast(), ScrollEndNotification);
expect(
notificationTypes.every((Type t) => t == ScrollUpdateNotification),
true,
);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How many notifications are we getting from this? Can we just assert we get the exact expected number and order of notifications instead of this logic? Maybe something like:

final List<Type> expectedNotificationTypes = [
  ScrollStartNotification,
  UserScrollNotification,
  ...List<Type>.filled(expectedNumberOfUpdates, ScrollUpdateNotifcation),
  UserScrollNotification,
  ScrollEndNotification,
];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I fixed test code.

@itome itome force-pushed the fix-draggable-scrollable-bottom-sheet-listener branch from c1abbcf to 2dce8d1 Compare November 22, 2019 02:20
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!


final List<Type> types = <Type>[
ScrollStartNotification,
ScrollEndNotification,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issuebis helpful to make sure we don't lose track

@itome
Copy link
Contributor Author

itome commented Nov 22, 2019

@dnfield CI failing bug it seems to be not related to my changes. Can you rerun it?

@gspencergoog
Copy link
Contributor

Restarted.

@gspencergoog
Copy link
Contributor

So, I did restart it, but it isn't flake. Can you try syncing to HEAD and pushing again? You might just be synced to a bad build.

@itome
Copy link
Contributor Author

itome commented Nov 23, 2019

Thanks, CI passed.

@dnfield dnfield merged commit a3eeb51 into flutter:master Nov 23, 2019
@itome itome deleted the fix-draggable-scrollable-bottom-sheet-listener branch November 23, 2019 05:49
@iampopal
Copy link

I need to dismiss the draggable bottom sheet when clicking on the background of the bottom sheet model.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants