From e9542ec646513af9fe07d08f4836a5b8d7069d7a Mon Sep 17 00:00:00 2001 From: David Martos Date: Thu, 10 Jun 2021 21:54:03 +0200 Subject: [PATCH] Assert for valid ScrollController in Scrollbar gestures (#81278) --- .../flutter/lib/src/cupertino/dialog.dart | 22 ++- .../flutter/lib/src/widgets/scrollbar.dart | 165 ++++++++++-------- .../test/cupertino/action_sheet_test.dart | 30 ++++ .../flutter/test/cupertino/dialog_test.dart | 26 +++ .../test/cupertino/scrollbar_test.dart | 35 ++++ .../flutter/test/material/scrollbar_test.dart | 5 +- .../flutter/test/widgets/scrollbar_test.dart | 35 ++++ 7 files changed, 243 insertions(+), 75 deletions(-) diff --git a/packages/flutter/lib/src/cupertino/dialog.dart b/packages/flutter/lib/src/cupertino/dialog.dart index c519c4c41e..87af1ae867 100644 --- a/packages/flutter/lib/src/cupertino/dialog.dart +++ b/packages/flutter/lib/src/cupertino/dialog.dart @@ -278,6 +278,9 @@ class CupertinoAlertDialog extends StatelessWidget { /// section when there are many actions. final ScrollController? scrollController; + ScrollController get _effectiveScrollController => + scrollController ?? ScrollController(); + /// A scroll controller that can be used to control the scrolling of the /// actions in the dialog. /// @@ -289,6 +292,9 @@ class CupertinoAlertDialog extends StatelessWidget { /// section when it is long. final ScrollController? actionScrollController; + ScrollController get _effectiveActionScrollController => + actionScrollController ?? ScrollController(); + /// {@macro flutter.material.dialog.insetAnimationDuration} final Duration insetAnimationDuration; @@ -305,7 +311,7 @@ class CupertinoAlertDialog extends StatelessWidget { child: _CupertinoAlertContentSection( title: title, message: content, - scrollController: scrollController, + scrollController: _effectiveScrollController, titlePadding: EdgeInsets.only( left: _kDialogEdgePadding, right: _kDialogEdgePadding, @@ -344,7 +350,7 @@ class CupertinoAlertDialog extends StatelessWidget { ); if (actions.isNotEmpty) { actionSection = _CupertinoAlertActionSection( - scrollController: actionScrollController, + scrollController: _effectiveActionScrollController, isActionSheet: false, children: actions, ); @@ -591,12 +597,18 @@ class CupertinoActionSheet extends StatelessWidget { /// short. final ScrollController? messageScrollController; + ScrollController get _effectiveMessageScrollController => + messageScrollController ?? ScrollController(); + /// A scroll controller that can be used to control the scrolling of the /// [actions] in the action sheet. /// /// This attribute is typically not needed. final ScrollController? actionScrollController; + ScrollController get _effectiveActionScrollController => + actionScrollController ?? ScrollController(); + /// The optional cancel button that is grouped separately from the other /// actions. /// @@ -609,7 +621,7 @@ class CupertinoActionSheet extends StatelessWidget { final Widget titleSection = _CupertinoAlertContentSection( title: title, message: message, - scrollController: messageScrollController, + scrollController: _effectiveMessageScrollController, titlePadding: const EdgeInsets.only( left: _kActionSheetContentHorizontalPadding, right: _kActionSheetContentHorizontalPadding, @@ -650,7 +662,7 @@ class CupertinoActionSheet extends StatelessWidget { ); } return _CupertinoAlertActionSection( - scrollController: actionScrollController, + scrollController: _effectiveActionScrollController, hasCancelButton: cancelButton != null, isActionSheet: true, children: actions!, @@ -1501,6 +1513,7 @@ class _CupertinoAlertContentSection extends StatelessWidget { } return CupertinoScrollbar( + controller: scrollController, child: SingleChildScrollView( controller: scrollController, child: Column( @@ -1565,6 +1578,7 @@ class _CupertinoAlertActionSectionState } return CupertinoScrollbar( + controller: widget.scrollController, child: SingleChildScrollView( controller: widget.scrollController, child: _CupertinoDialogActionsRenderWidget( diff --git a/packages/flutter/lib/src/widgets/scrollbar.dart b/packages/flutter/lib/src/widgets/scrollbar.dart index a64c98680d..00a3a8cca1 100644 --- a/packages/flutter/lib/src/widgets/scrollbar.dart +++ b/packages/flutter/lib/src/widgets/scrollbar.dart @@ -1026,84 +1026,105 @@ class RawScrollbarState extends State with TickerProv // A scroll event is required in order to paint the thumb. void _maybeTriggerScrollbar() { WidgetsBinding.instance!.addPostFrameCallback((Duration duration) { + final ScrollController? scrollController = widget.controller ?? PrimaryScrollController.of(context); if (showScrollbar) { _fadeoutTimer?.cancel(); // Wait one frame and cause an empty scroll event. This allows the // thumb to show immediately when isAlwaysShown is true. A scroll // event is required in order to paint the thumb. - final ScrollController? scrollController = widget.controller ?? PrimaryScrollController.of(context); - final bool tryPrimary = widget.controller == null; - final String controllerForError = tryPrimary - ? 'provided ScrollController' - : 'PrimaryScrollController'; - assert( - scrollController != null, - 'A ScrollController is required when Scrollbar.isAlwaysShown is true. ' - '${tryPrimary ? 'The Scrollbar was not provided a ScrollController, ' - 'and attempted to use the PrimaryScrollController, but none was found.' :''}', - ); - assert (() { - if (!scrollController!.hasClients) { - throw FlutterError.fromParts([ - ErrorSummary( - "The Scrollbar's ScrollController has no ScrollPosition attached.", - ), - ErrorDescription( - 'A Scrollbar cannot be painted without a ScrollPosition. ', - ), - ErrorHint( - 'The Scrollbar attempted to use the $controllerForError. This ' - 'ScrollController should be associated with the ScrollView that ' - 'the Scrollbar is being applied to. ' - '${tryPrimary - ? 'A ScrollView with an Axis.vertical ' - 'ScrollDirection will automatically use the ' - 'PrimaryScrollController if the user has not provided a ' - 'ScrollController, but a ScrollDirection of Axis.horizontal will ' - 'not. To use the PrimaryScrollController explicitly, set ScrollView.primary ' - 'to true for the Scrollable widget.' - : 'When providing your own ScrollController, ensure both the ' - 'Scrollbar and the Scrollable widget use the same one.' - }', - ), - ]); - } - return true; - }()); - assert (() { - try { - scrollController!.position; - } catch (_) { - throw FlutterError.fromParts([ - ErrorSummary( - 'The $controllerForError is currently attached to more than one ' - 'ScrollPosition.', - ), - ErrorDescription( - 'The Scrollbar requires a single ScrollPosition in order to be painted.', - ), - ErrorHint( - 'When Scrollbar.isAlwaysShown is true, the associated Scrollable ' - 'widgets must have unique ScrollControllers. ' - '${tryPrimary - ? 'The PrimaryScrollController is used by default for ' - 'ScrollViews with an Axis.vertical ScrollDirection, ' - 'unless the ScrollView has been provided its own ' - 'ScrollController. More than one Scrollable may have tried ' - 'to use the PrimaryScrollController of the current context.' - : 'The provided ScrollController must be unique to a ' - 'Scrollable widget.' - }', - ), - ]); - } - return true; - }()); + _checkForValidScrollPosition(); scrollController!.position.didUpdateScrollPositionBy(0); } + + // Interactive scrollbars need to be properly configured. + // If there is no scroll controller, there will not be gestures at all. + if (scrollController != null && enableGestures) { + _checkForValidScrollPosition(); + } }); } + void _checkForValidScrollPosition() { + final ScrollController? scrollController = widget.controller ?? PrimaryScrollController.of(context); + final bool tryPrimary = widget.controller == null; + final String controllerForError = tryPrimary + ? 'provided ScrollController' + : 'PrimaryScrollController'; + + String when = ''; + if (showScrollbar) { + when = 'Scrollbar.isAlwaysShown is true'; + } else if (enableGestures) { + when = 'the scrollbar is interactive'; + } else { + when = 'using the Scrollbar'; + } + + assert( + scrollController != null, + 'A ScrollController is required when $when. ' + '${tryPrimary ? 'The Scrollbar was not provided a ScrollController, ' + 'and attempted to use the PrimaryScrollController, but none was found.' :''}', + ); + assert (() { + if (!scrollController!.hasClients) { + throw FlutterError.fromParts([ + ErrorSummary( + "The Scrollbar's ScrollController has no ScrollPosition attached.", + ), + ErrorDescription( + 'A Scrollbar cannot be painted without a ScrollPosition. ', + ), + ErrorHint( + 'The Scrollbar attempted to use the $controllerForError. This ' + 'ScrollController should be associated with the ScrollView that ' + 'the Scrollbar is being applied to. ' + '${tryPrimary + ? 'A ScrollView with an Axis.vertical ' + 'ScrollDirection will automatically use the ' + 'PrimaryScrollController if the user has not provided a ' + 'ScrollController, but a ScrollDirection of Axis.horizontal will ' + 'not. To use the PrimaryScrollController explicitly, set ScrollView.primary ' + 'to true for the Scrollable widget.' + : 'When providing your own ScrollController, ensure both the ' + 'Scrollbar and the Scrollable widget use the same one.' + }', + ), + ]); + } + return true; + }()); + assert (() { + try { + scrollController!.position; + } catch (_) { + throw FlutterError.fromParts([ + ErrorSummary( + 'The $controllerForError is currently attached to more than one ' + 'ScrollPosition.', + ), + ErrorDescription( + 'The Scrollbar requires a single ScrollPosition in order to be painted.', + ), + ErrorHint( + 'When $when, the associated Scrollable ' + 'widgets must have unique ScrollControllers. ' + '${tryPrimary + ? 'The PrimaryScrollController is used by default for ' + 'ScrollViews with an Axis.vertical ScrollDirection, ' + 'unless the ScrollView has been provided its own ' + 'ScrollController. More than one Scrollable may have tried ' + 'to use the PrimaryScrollController of the current context.' + : 'The provided ScrollController must be unique to a ' + 'Scrollable widget.' + }', + ), + ]); + } + return true; + }()); + } + /// This method is responsible for configuring the [scrollbarPainter] /// according to the [widget]'s properties and any inherited widgets the /// painter depends on, like [Directionality] and [MediaQuery]. @@ -1191,6 +1212,7 @@ class RawScrollbarState extends State with TickerProv @protected @mustCallSuper void handleThumbPress() { + _checkForValidScrollPosition(); if (getScrollbarDirection() == null) { return; } @@ -1203,6 +1225,7 @@ class RawScrollbarState extends State with TickerProv @protected @mustCallSuper void handleThumbPressStart(Offset localPosition) { + _checkForValidScrollPosition(); _currentController = widget.controller ?? PrimaryScrollController.of(context); final Axis? direction = getScrollbarDirection(); if (direction == null) { @@ -1219,6 +1242,7 @@ class RawScrollbarState extends State with TickerProv @protected @mustCallSuper void handleThumbPressUpdate(Offset localPosition) { + _checkForValidScrollPosition(); final Axis? direction = getScrollbarDirection(); if (direction == null) { return; @@ -1231,9 +1255,11 @@ class RawScrollbarState extends State with TickerProv @protected @mustCallSuper void handleThumbPressEnd(Offset localPosition, Velocity velocity) { + _checkForValidScrollPosition(); final Axis? direction = getScrollbarDirection(); - if (direction == null) + if (direction == null) { return; + } _maybeStartFadeoutTimer(); _dragScrollbarAxisOffset = null; _currentController = null; @@ -1241,6 +1267,7 @@ class RawScrollbarState extends State with TickerProv void _handleTrackTapDown(TapDownDetails details) { // The Scrollbar should page towards the position of the tap on the track. + _checkForValidScrollPosition(); _currentController = widget.controller ?? PrimaryScrollController.of(context); double scrollIncrement; diff --git a/packages/flutter/test/cupertino/action_sheet_test.dart b/packages/flutter/test/cupertino/action_sheet_test.dart index c7b42696b5..1f46897bc6 100644 --- a/packages/flutter/test/cupertino/action_sheet_test.dart +++ b/packages/flutter/test/cupertino/action_sheet_test.dart @@ -389,6 +389,36 @@ void main() { expect(tester.getSize(find.byType(CupertinoActionSheet)).height, screenHeight); }); + testWidgets('CupertinoActionSheet scrollbars controllers should be different', (WidgetTester tester) async { + // https://github.com/flutter/flutter/pull/81278 + await tester.pumpWidget( + createAppWithButtonThatLaunchesActionSheet( + CupertinoActionSheet( + title: const Text('The title'), + message: Text('Very long content' * 200), + actions: [ + CupertinoActionSheetAction( + child: const Text('One'), + onPressed: () { }, + ), + ], + ) + ), + ); + + await tester.tap(find.text('Go')); + await tester.pump(); + + final List scrollbars = + find.descendant( + of: find.byType(CupertinoActionSheet), + matching: find.byType(CupertinoScrollbar), + ).evaluate().map((Element e) => e.widget as CupertinoScrollbar).toList(); + + expect(scrollbars.length, 2); + expect(scrollbars[0].controller != scrollbars[1].controller, isTrue); + }); + testWidgets('Tap on button calls onPressed', (WidgetTester tester) async { bool wasPressed = false; await tester.pumpWidget( diff --git a/packages/flutter/test/cupertino/dialog_test.dart b/packages/flutter/test/cupertino/dialog_test.dart index 41bc8a2505..ef9dfb0541 100644 --- a/packages/flutter/test/cupertino/dialog_test.dart +++ b/packages/flutter/test/cupertino/dialog_test.dart @@ -1286,6 +1286,32 @@ void main() { // one for the content. expect(find.byType(CupertinoScrollbar), findsNWidgets(2)); }, variant: TargetPlatformVariant.all()); + + testWidgets('CupertinoAlertDialog scrollbars controllers should be different', (WidgetTester tester) async { + // https://github.com/flutter/flutter/pull/81278 + await tester.pumpWidget( + const MaterialApp( + home: MediaQuery( + data: MediaQueryData(viewInsets: EdgeInsets.zero), + child: CupertinoAlertDialog( + actions: [ + CupertinoDialogAction(child: Text('OK')), + ], + content: Placeholder(fallbackHeight: 200.0), + ), + ), + ), + ); + + final List scrollbars = + find.descendant( + of: find.byType(CupertinoAlertDialog), + matching: find.byType(CupertinoScrollbar), + ).evaluate().map((Element e) => e.widget as CupertinoScrollbar).toList(); + + expect(scrollbars.length, 2); + expect(scrollbars[0].controller != scrollbars[1].controller, isTrue); + }); } RenderBox findActionButtonRenderBoxByTitle(WidgetTester tester, String title) { diff --git a/packages/flutter/test/cupertino/scrollbar_test.dart b/packages/flutter/test/cupertino/scrollbar_test.dart index bbedb7290c..b9aeee8b59 100644 --- a/packages/flutter/test/cupertino/scrollbar_test.dart +++ b/packages/flutter/test/cupertino/scrollbar_test.dart @@ -934,6 +934,41 @@ void main() { ); }); + testWidgets('Interactive scrollbars should have a valid scroll controller', (WidgetTester tester) async { + final ScrollController primaryScrollController = ScrollController(); + final ScrollController scrollController = ScrollController(); + + await tester.pumpWidget( + Directionality( + textDirection: TextDirection.ltr, + child: MediaQuery( + data: const MediaQueryData(), + child: PrimaryScrollController( + controller: primaryScrollController, + child: CupertinoScrollbar( + child: SingleChildScrollView( + controller: scrollController, + child: const SizedBox( + height: 1000.0, + width: 1000.0, + ), + ), + ), + ), + ), + ), + ); + + await tester.pumpAndSettle(); + + final dynamic exception = tester.takeException(); + expect(exception, isAssertionError); + expect( + (exception as AssertionError).message, + contains("The Scrollbar's ScrollController has no ScrollPosition attached."), + ); + }); + testWidgets('Simultaneous dragging and pointer scrolling does not cause a crash', (WidgetTester tester) async { // Regression test for https://github.com/flutter/flutter/issues/70105 final ScrollController scrollController = ScrollController(); diff --git a/packages/flutter/test/material/scrollbar_test.dart b/packages/flutter/test/material/scrollbar_test.dart index bc09413398..9e36f25c80 100644 --- a/packages/flutter/test/material/scrollbar_test.dart +++ b/packages/flutter/test/material/scrollbar_test.dart @@ -1051,8 +1051,9 @@ void main() { ), child: Scrollbar( controller: controller, - child: const SingleChildScrollView( - child: SizedBox(width: 4000.0, height: 4000.0), + child: SingleChildScrollView( + controller: controller, + child: const SizedBox(width: 4000.0, height: 4000.0), ), ), ), diff --git a/packages/flutter/test/widgets/scrollbar_test.dart b/packages/flutter/test/widgets/scrollbar_test.dart index e84dd973ff..bef7cd826c 100644 --- a/packages/flutter/test/widgets/scrollbar_test.dart +++ b/packages/flutter/test/widgets/scrollbar_test.dart @@ -1171,6 +1171,41 @@ void main() { ); }); + testWidgets('Interactive scrollbars should have a valid scroll controller', (WidgetTester tester) async { + final ScrollController primaryScrollController = ScrollController(); + final ScrollController scrollController = ScrollController(); + + await tester.pumpWidget( + Directionality( + textDirection: TextDirection.ltr, + child: MediaQuery( + data: const MediaQueryData(), + child: PrimaryScrollController( + controller: primaryScrollController, + child: RawScrollbar( + child: SingleChildScrollView( + controller: scrollController, + child: const SizedBox( + height: 1000.0, + width: 1000.0, + ), + ), + ), + ), + ), + ), + ); + + await tester.pumpAndSettle(); + + final dynamic exception = tester.takeException(); + expect(exception, isAssertionError); + expect( + (exception as AssertionError).message, + contains("The Scrollbar's ScrollController has no ScrollPosition attached."), + ); + }); + testWidgets('Simultaneous dragging and pointer scrolling does not cause a crash', (WidgetTester tester) async { // Regression test for https://github.com/flutter/flutter/issues/70105 final ScrollController scrollController = ScrollController();