From 3d93f24c05d750d4cd724a249cdce36246ea6464 Mon Sep 17 00:00:00 2001 From: Darren Austin Date: Wed, 15 May 2019 15:50:39 -0700 Subject: [PATCH] Tapping a modal bottom sheet should not dismiss it by default (#32528) Removed the GestureDetector from the modal bottom sheet that dismissed it on tap and updated several tests to accommodate this change. --- .../material/modal_bottom_sheet_demo.dart | 2 +- .../lib/src/material/bottom_sheet.dart | 63 +++++++++---------- ...sheet_test.dart => bottom_sheet_test.dart} | 35 +++++++---- .../persistent_bottom_sheet_test.dart | 4 +- 4 files changed, 55 insertions(+), 49 deletions(-) rename packages/flutter/test/material/{modal_bottom_sheet_test.dart => bottom_sheet_test.dart} (93%) diff --git a/examples/flutter_gallery/lib/demo/material/modal_bottom_sheet_demo.dart b/examples/flutter_gallery/lib/demo/material/modal_bottom_sheet_demo.dart index 29a0647aac..578f52637e 100644 --- a/examples/flutter_gallery/lib/demo/material/modal_bottom_sheet_demo.dart +++ b/examples/flutter_gallery/lib/demo/material/modal_bottom_sheet_demo.dart @@ -24,7 +24,7 @@ class ModalBottomSheetDemo extends StatelessWidget { return Container( child: Padding( padding: const EdgeInsets.all(32.0), - child: Text('This is the modal bottom sheet. Tap anywhere to dismiss.', + child: Text('This is the modal bottom sheet. Slide down to dismiss.', textAlign: TextAlign.center, style: TextStyle( color: Theme.of(context).accentColor, diff --git a/packages/flutter/lib/src/material/bottom_sheet.dart b/packages/flutter/lib/src/material/bottom_sheet.dart index 63b7d1169d..8f9ca2aae7 100644 --- a/packages/flutter/lib/src/material/bottom_sheet.dart +++ b/packages/flutter/lib/src/material/bottom_sheet.dart @@ -281,43 +281,36 @@ class _ModalBottomSheetState extends State<_ModalBottomSheet> { final MaterialLocalizations localizations = MaterialLocalizations.of(context); final String routeLabel = _getRouteLabel(localizations); - return GestureDetector( - excludeFromSemantics: true, - onTap: () { - if (widget.route.isCurrent) - Navigator.pop(context); - }, - child: AnimatedBuilder( - animation: widget.route.animation, - builder: (BuildContext context, Widget child) { - // Disable the initial animation when accessible navigation is on so - // that the semantics are added to the tree at the correct time. - final double animationValue = mediaQuery.accessibleNavigation ? 1.0 : widget.route.animation.value; - return Semantics( - scopesRoute: true, - namesRoute: true, - label: routeLabel, - explicitChildNodes: true, - child: ClipRect( - child: CustomSingleChildLayout( - delegate: _ModalBottomSheetLayout(animationValue, widget.isScrollControlled), - child: BottomSheet( - animationController: widget.route._animationController, - onClosing: () { - if (widget.route.isCurrent) { - Navigator.pop(context); - } - }, - builder: widget.route.builder, - backgroundColor: widget.backgroundColor, - elevation: widget.elevation, - shape: widget.shape, - ), + return AnimatedBuilder( + animation: widget.route.animation, + builder: (BuildContext context, Widget child) { + // Disable the initial animation when accessible navigation is on so + // that the semantics are added to the tree at the correct time. + final double animationValue = mediaQuery.accessibleNavigation ? 1.0 : widget.route.animation.value; + return Semantics( + scopesRoute: true, + namesRoute: true, + label: routeLabel, + explicitChildNodes: true, + child: ClipRect( + child: CustomSingleChildLayout( + delegate: _ModalBottomSheetLayout(animationValue, widget.isScrollControlled), + child: BottomSheet( + animationController: widget.route._animationController, + onClosing: () { + if (widget.route.isCurrent) { + Navigator.pop(context); + } + }, + builder: widget.route.builder, + backgroundColor: widget.backgroundColor, + elevation: widget.elevation, + shape: widget.shape, ), ), - ); - }, - ), + ), + ); + }, ); } } diff --git a/packages/flutter/test/material/modal_bottom_sheet_test.dart b/packages/flutter/test/material/bottom_sheet_test.dart similarity index 93% rename from packages/flutter/test/material/modal_bottom_sheet_test.dart rename to packages/flutter/test/material/bottom_sheet_test.dart index 406a1527fd..35ac31b1cc 100644 --- a/packages/flutter/test/material/modal_bottom_sheet_test.dart +++ b/packages/flutter/test/material/bottom_sheet_test.dart @@ -10,7 +10,7 @@ import 'package:flutter/gestures.dart'; import '../widgets/semantics_tester.dart'; void main() { - testWidgets('Verify that a tap dismisses a modal BottomSheet', (WidgetTester tester) async { + testWidgets('Tapping on a modal BottomSheet should not dismiss it', (WidgetTester tester) async { BuildContext savedContext; await tester.pumpWidget(MaterialApp( @@ -33,28 +33,41 @@ void main() { showBottomSheetThenCalled = true; }); - await tester.pump(); // bottom sheet show animation starts - await tester.pump(const Duration(seconds: 1)); // animation done + await tester.pumpAndSettle(); expect(find.text('BottomSheet'), findsOneWidget); expect(showBottomSheetThenCalled, isFalse); - // Tap on the bottom sheet itself to dismiss it. + // Tap on the bottom sheet itself, it should not be dismissed await tester.tap(find.text('BottomSheet')); - await tester.pump(); // bottom sheet dismiss animation starts - expect(showBottomSheetThenCalled, isTrue); - await tester.pump(const Duration(seconds: 1)); // last frame of animation (sheet is entirely off-screen, but still present) - await tester.pump(const Duration(seconds: 1)); // frame after the animation (sheet has been removed) + await tester.pumpAndSettle(); + expect(find.text('BottomSheet'), findsOneWidget); + expect(showBottomSheetThenCalled, isFalse); + }); + + testWidgets('Tapping outside a modal BottomSheet should dismiss it', (WidgetTester tester) async { + BuildContext savedContext; + + await tester.pumpWidget(MaterialApp( + home: Builder( + builder: (BuildContext context) { + savedContext = context; + return Container(); + } + ), + )); + + await tester.pump(); expect(find.text('BottomSheet'), findsNothing); - showBottomSheetThenCalled = false; + bool showBottomSheetThenCalled = false; showModalBottomSheet( context: savedContext, builder: (BuildContext context) => const Text('BottomSheet'), ).then((void value) { showBottomSheetThenCalled = true; }); - await tester.pump(); // bottom sheet show animation starts - await tester.pump(const Duration(seconds: 1)); // animation done + + await tester.pumpAndSettle(); expect(find.text('BottomSheet'), findsOneWidget); expect(showBottomSheetThenCalled, isFalse); diff --git a/packages/flutter/test/material/persistent_bottom_sheet_test.dart b/packages/flutter/test/material/persistent_bottom_sheet_test.dart index 21e0fad565..f3f0fd1d32 100644 --- a/packages/flutter/test/material/persistent_bottom_sheet_test.dart +++ b/packages/flutter/test/material/persistent_bottom_sheet_test.dart @@ -406,8 +406,8 @@ void main() { await tester.pumpAndSettle(); expect(find.text('modal bottom sheet'), findsOneWidget); - // Dismiss the modal bottomSheet - await tester.tap(find.text('modal bottom sheet')); + // Dismiss the modal bottomSheet by tapping above the sheet + await tester.tapAt(const Offset(20.0, 20.0)); await tester.pumpAndSettle(); expect(find.text('modal bottom sheet'), findsNothing); expect(find.text('showModalBottomSheet'), findsOneWidget);