Skip to content
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

Improved ios 13 scrollbar fidelity #41799

Merged

Conversation

@Kavantix
Copy link
Contributor

Kavantix commented Oct 2, 2019

Description

Drag from the right is no more
Longpress is now only 100ms instead of 500
Added optional duration field to longpressgesturerecognizer
Added controller field to material scrollbar api
Haptic feedback only triggers when scrollbar is fully expanded
Added haptic feedback when releasing the scrollbar after dragging it

Related Issues

Fixes #37893
Fixes#37892

Tests

I added the following tests:

Drag from the right test removed
Adjusted drag test to use the new (correct) behaviour
Longpress test adjusted for new timing

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). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.
@Kavantix

This comment has been minimized.

Copy link
Contributor Author

Kavantix commented Oct 2, 2019

@justinmc FYI

Copy link
Contributor

justinmc left a comment

This is a very high quality and high impact PR, I really appreciate you taking the time!

The only major changes I'm requesting below are documentation and maybe a test. Let me know if something doesn't make sense or you need help with anything.

I also want to understand why you added the draggable behavior to Material's Scrollbar on iOS. In what case are you thinking it will be desirable? I think I'm on board but I want to talk it over.

}

ScrollController get _controller => widget.controller ?? PrimaryScrollController.of(context);

This comment has been minimized.

Copy link
@justinmc

justinmc Oct 2, 2019

Contributor

I should have done this from the start, good call. We'll need to update the documentation to reflect this change.

Documentation changes for CupertinoScrollbar:

/// ...
/// To add a scrollbar to a [ScrollView], simply wrap the scroll view widget in
/// a [CupertinoScrollbar] widget.
///
/// By default, the CupertinoScrollbar will be draggable (a feature introduced
/// in iOS 13) if it uses the PrimaryScrollController. For multiple scrollbars, or
/// other more complicated situations, see the [controller] parameter.
///
/// See also:
/// ...

And the docs for controller. Notice that this example doesn't work unless you explicitly pass the controller.

/// ...
/// introduced in iOS 13.
///
/// If nothing is passed to controller, the default behavior is to automatically
/// enable scrollbar dragging on the nearest ScrollController using
/// [PrimaryScrollController.of].
///
/// If a ScrollController is passed, then scrollbar dragging will be enabled on
/// the given ScrollController. A stateful ancestor of this CupertinoScrollbar
/// needs to manage the ScrollController and either pass it to a scrollable
/// descendant or use a PrimaryScrollController to share it.
///
/// Here is an example of using the `controller` parameter to enable
/// scrollbar dragging for multiple independent ListViews:
///
/// {@tool sample}
///
/// ```dart
/// build(BuildContext context) {
/// final ScrollController controllerOne = ScrollController();
/// final ScrollController controllerTwo = ScrollController();
///
/// return Column(
///   children: <Widget>[
///     Container(
///        height: 200,
///        child: CupertinoScrollbar(
///          controller: controllerOne,
///          child: ListView.builder(
///            controller: controllerOne,
///            itemCount: 120,
///            itemBuilder: (BuildContext context, int index) => Text('item $index'),
///          ),
///        ),
///      ),
///      Container(
///        height: 200,
///        child: CupertinoScrollbar(
///          controller: controllerTwo,
///          child: ListView.builder(
///            controller: controllerTwo,
///            itemCount: 120,
///            itemBuilder: (BuildContext context, int index) => Text('list 2 item $index');
///          ),
///        ),
///      ),
///    ],
///   );
/// }
/// {@end-tool}
/// ```

This comment has been minimized.

Copy link
@justinmc

justinmc Oct 2, 2019

Contributor

Any performance problems with potentially calling PrimaryScrollController.of(context) repeatedly @HansMuller?

This comment has been minimized.

Copy link
@HansMuller

HansMuller Oct 7, 2019

Contributor

InheritedWidget lookups like that are constant time (just a hashtable lookup), so it shouldn't be a problem.

bool _checkVertical() {
try {
return _controller.position.axis == Axis.vertical;
} catch (_) {
// Disable the gesture if we cannot determine the direction.
return false;
}
Comment on lines 214 to 244

This comment has been minimized.

Copy link
@justinmc

justinmc Oct 2, 2019

Contributor

Good call, we don't want to assert now that scroll dragging is enabled by default.

}

double _pressStartY = 0.0;

This comment has been minimized.

Copy link
@justinmc

justinmc Oct 2, 2019

Contributor

I'd usually move this declaration up to the top of the class, but it's not a strict style requirement.

..onLongPressEnd = _handleLongPressEnd;
},
);
gestures[_ThumbHorizontalDragGestureRecognizer] =

This comment has been minimized.

Copy link
@justinmc

justinmc Oct 2, 2019

Contributor

I'm on board with removing the "drag from side" behavior. I probably should have removed this in my last PR.

LongPressGestureRecognizer({
Duration duration,

This comment has been minimized.

Copy link
@justinmc

justinmc Oct 2, 2019

Contributor

Maybe name this deadline to keep it consistent with the super class?

This comment has been minimized.

Copy link
@Kavantix

Kavantix Oct 2, 2019

Author Contributor

I originally called it deadline, but it seems a bit off to expose the inner workings by using that name. So I called it duration instead since it will be clear to anyone using it what it means.

This comment has been minimized.

Copy link
@justinmc

justinmc Oct 3, 2019

Contributor

Got it, that makes sense.

/// );
/// }
/// ```
/// {@end-tool}

This comment has been minimized.

Copy link
@justinmc

justinmc Oct 2, 2019

Contributor

If this is a verbatim copy of the docs for controller in CupertinoScrollbar, it should be referenced using @macro and @template. See obscureText in EditableText and TextField, for example.


longPress.dispose();
});

This comment has been minimized.

Copy link
@justinmc

justinmc Oct 2, 2019

Contributor

There should probably be a test that covers the controller field now in Material's Scrollbar.

@Kavantix

This comment has been minimized.

Copy link
Contributor Author

Kavantix commented Oct 2, 2019

Thanks for the great feedback justin!

The point about the performance of calling PrimaryScrollController.of often is a good one, perhaps we should store it in a field. Maybe retrieve it on longpress start and empty it on end.

As for adding the functionality to the material scrollbar. Most people will use that scrollbar I feel since they will be making an app for both platforms. And they then expect it to feel native on both iOS and Android.
In my current projects I am using a wrapper that uses the material scrollbar on android an the cupertino on iOS with the primaryScrollController so this just removes that step.

@Kavantix Kavantix force-pushed the Kavantix:draggable-scrollbar-fidelity-improvement branch from ae106c3 to 60b46df Oct 4, 2019
@justinmc

This comment has been minimized.

Copy link
Contributor

justinmc commented Oct 7, 2019

Thanks for making those changes. I misread the Scrollbar thing, that totally makes sense to pass it through automatically. Also it looks like PrimaryScrollController.of is not a traversal and just a hash table lookup, so that should not be a performance problem.

I think this is just about good to go. There are some failures in the analyze step of the tests that you should fix. It seems to be just code formatting stuff. Then I think we can ship this.

@Kavantix Kavantix force-pushed the Kavantix:draggable-scrollbar-fidelity-improvement branch from 60b46df to 80befd0 Oct 8, 2019
@Kavantix Kavantix force-pushed the Kavantix:draggable-scrollbar-fidelity-improvement branch from 80befd0 to 7aeb639 Oct 26, 2019
@Kavantix Kavantix requested a review from justinmc Oct 26, 2019
@justinmc

This comment has been minimized.

Copy link
Contributor

justinmc commented Oct 28, 2019

@Kavantix Looks like there's a legitimate test failure here:

expect(find.byType(CupertinoScrollbar), paints..rrect(

Maybe something with the duration change? Let me know if you need help figuring out how to fix it.

Drag from the right is no more
Longpress is now only 100ms instead of 500
Added optional duration field to longpressgesturerecognizer
Added controller field to material scrollbar api
Haptic feedback only triggers when scrollbar is fully expanded
Added haptic feedback when releasing the scrollbar after dragging it

Fixes #37893
Fixes#37892
@Kavantix Kavantix force-pushed the Kavantix:draggable-scrollbar-fidelity-improvement branch from 7aeb639 to 8fa0627 Oct 28, 2019
@Kavantix

This comment has been minimized.

Copy link
Contributor Author

Kavantix commented Oct 28, 2019

@justinmc I accidentally changed the wrong value in the test which caused the scrollbar to not have the correct color yet.
It should all work now

Copy link
Contributor

justinmc left a comment

LGTM

Thanks again for taking the time to write this PR! Big improvement for the draggable scrollbar. I'll go ahead and merge this in a minute.

@justinmc justinmc merged commit 0120c41 into flutter:master Oct 29, 2019
62 checks passed
62 checks passed
WIP Ready for review
Details
analyze-linux Task Summary
Details
analyze-linux
Details
build_tests-linux Task Summary
Details
build_tests-linux
Details
customer_testing-linux Task Summary
Details
customer_testing-linux
Details
customer_testing-macos Task Summary
Details
customer_testing-macos
Details
customer_testing-windows Task Summary
Details
customer_testing-windows
Details
deploy_gallery-linux Task Summary
Details
deploy_gallery-linux
Details
deploy_gallery-macos Task Summary
Details
deploy_gallery-macos
Details
docs-linux Task Summary
Details
docs-linux
Details
firebase_test_lab_tests-linux Task Summary
Details
firebase_test_lab_tests-linux
Details
flutter-build
Details
framework_tests-libraries-linux Task Summary
Details
framework_tests-libraries-linux
Details
framework_tests-libraries-macos Task Summary
Details
framework_tests-libraries-macos
Details
framework_tests-libraries-windows Task Summary
Details
framework_tests-libraries-windows
Details
framework_tests-misc-linux Task Summary
Details
framework_tests-misc-linux
Details
framework_tests-misc-macos Task Summary
Details
framework_tests-misc-macos
Details
framework_tests-misc-windows Task Summary
Details
framework_tests-misc-windows
Details
framework_tests-widgets-linux Task Summary
Details
framework_tests-widgets-linux
Details
framework_tests-widgets-macos Task Summary
Details
framework_tests-widgets-macos
Details
framework_tests-widgets-windows Task Summary
Details
framework_tests-widgets-windows
Details
hostonly_devicelab_tests-0-linux Task Summary
Details
hostonly_devicelab_tests-0-linux
Details
hostonly_devicelab_tests-1-linux Task Summary
Details
hostonly_devicelab_tests-1-linux
Details
hostonly_devicelab_tests-2-linux Task Summary
Details
hostonly_devicelab_tests-2-linux
Details
hostonly_devicelab_tests-3_last-linux Task Summary
Details
hostonly_devicelab_tests-3_last-linux
Details
web_tests-0-linux Task Summary
Details
web_tests-0-linux
Details
web_tests-1-linux Task Summary
Details
web_tests-1-linux
Details
web_tests-2-linux Task Summary
Details
web_tests-2-linux
Details
web_tests-3-linux Task Summary
Details
web_tests-3-linux
Details
web_tests-4-linux Task Summary
Details
web_tests-4-linux
Details
web_tests-5-linux Task Summary
Details
web_tests-5-linux
Details
web_tests-6-linux Task Summary
Details
web_tests-6-linux
Details
web_tests-7_last-linux Task Summary
Details
web_tests-7_last-linux
Details
sahandevs added a commit to sahandevs/flutter that referenced this pull request Nov 15, 2019
Drag from the right is no more
Longpress is now only 100ms instead of 500
Added optional duration field to longpressgesturerecognizer
Added controller field to material scrollbar api
Haptic feedback only triggers when scrollbar is fully expanded
Added haptic feedback when releasing the scrollbar after dragging it
sahandevs added a commit to sahandevs/flutter that referenced this pull request Nov 15, 2019
Drag from the right is no more
Longpress is now only 100ms instead of 500
Added optional duration field to longpressgesturerecognizer
Added controller field to material scrollbar api
Haptic feedback only triggers when scrollbar is fully expanded
Added haptic feedback when releasing the scrollbar after dragging it
sahandevs added a commit to sahandevs/flutter that referenced this pull request Nov 15, 2019
Inconnu08 added a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
Drag from the right is no more
Longpress is now only 100ms instead of 500
Added optional duration field to longpressgesturerecognizer
Added controller field to material scrollbar api
Haptic feedback only triggers when scrollbar is fully expanded
Added haptic feedback when releasing the scrollbar after dragging it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.