From 14cd5fa30af9bb416605b71cfa55e322e471712f Mon Sep 17 00:00:00 2001 From: yim Date: Fri, 9 Aug 2024 00:09:18 +0800 Subject: [PATCH] The `PopupMenuButton` should not steal focus from the TextField when it appears. (#150568) Fixes: #24843 Fixes: #50567 --- packages/flutter/lib/src/cupertino/route.dart | 3 ++ .../lib/src/material/bottom_sheet.dart | 1 + packages/flutter/lib/src/material/dialog.dart | 1 + packages/flutter/lib/src/material/page.dart | 1 + .../flutter/lib/src/material/popup_menu.dart | 13 +++++++ .../flutter/lib/src/widgets/navigator.dart | 22 ++++++++++-- packages/flutter/lib/src/widgets/pages.dart | 2 ++ packages/flutter/lib/src/widgets/routes.dart | 7 +++- .../test/material/popup_menu_test.dart | 34 +++++++++++++++++++ 9 files changed, 80 insertions(+), 4 deletions(-) diff --git a/packages/flutter/lib/src/cupertino/route.dart b/packages/flutter/lib/src/cupertino/route.dart index 83d915a044c..d1eac56638a 100644 --- a/packages/flutter/lib/src/cupertino/route.dart +++ b/packages/flutter/lib/src/cupertino/route.dart @@ -282,6 +282,7 @@ class CupertinoPageRoute extends PageRoute with CupertinoRouteTransitionMi required this.builder, this.title, super.settings, + super.requestFocus, this.maintainState = true, super.fullscreenDialog, super.allowSnapshotting = true, @@ -1076,6 +1077,7 @@ class CupertinoModalPopupRoute extends PopupRoute { bool semanticsDismissible = false, super.filter, super.settings, + super.requestFocus, this.anchorPoint, }) : _barrierDismissible = barrierDismissible, _semanticsDismissible = semanticsDismissible; @@ -1387,6 +1389,7 @@ class CupertinoDialogRoute extends RawDialogRoute { super.transitionDuration = const Duration(milliseconds: 250), this.transitionBuilder, super.settings, + super.requestFocus, super.anchorPoint, }) : super( pageBuilder: (BuildContext context, Animation animation, Animation secondaryAnimation) { diff --git a/packages/flutter/lib/src/material/bottom_sheet.dart b/packages/flutter/lib/src/material/bottom_sheet.dart index 25421090771..9dfeec84cfb 100644 --- a/packages/flutter/lib/src/material/bottom_sheet.dart +++ b/packages/flutter/lib/src/material/bottom_sheet.dart @@ -868,6 +868,7 @@ class ModalBottomSheetRoute extends PopupRoute { required this.isScrollControlled, this.scrollControlDisabledMaxHeightRatio = _defaultScrollControlDisabledMaxHeightRatio, super.settings, + super.requestFocus, this.transitionAnimationController, this.anchorPoint, this.useSafeArea = false, diff --git a/packages/flutter/lib/src/material/dialog.dart b/packages/flutter/lib/src/material/dialog.dart index aa1db99d7e0..813b330aff4 100644 --- a/packages/flutter/lib/src/material/dialog.dart +++ b/packages/flutter/lib/src/material/dialog.dart @@ -1580,6 +1580,7 @@ class DialogRoute extends RawDialogRoute { String? barrierLabel, bool useSafeArea = true, super.settings, + super.requestFocus, super.anchorPoint, super.traversalEdgeBehavior, }) : super( diff --git a/packages/flutter/lib/src/material/page.dart b/packages/flutter/lib/src/material/page.dart index a2a23a42503..57fb860f519 100644 --- a/packages/flutter/lib/src/material/page.dart +++ b/packages/flutter/lib/src/material/page.dart @@ -37,6 +37,7 @@ class MaterialPageRoute extends PageRoute with MaterialRouteTransitionMixi MaterialPageRoute({ required this.builder, super.settings, + super.requestFocus, this.maintainState = true, super.fullscreenDialog, super.allowSnapshotting = true, diff --git a/packages/flutter/lib/src/material/popup_menu.dart b/packages/flutter/lib/src/material/popup_menu.dart index d542632cdb2..e16689cfc0b 100644 --- a/packages/flutter/lib/src/material/popup_menu.dart +++ b/packages/flutter/lib/src/material/popup_menu.dart @@ -859,6 +859,7 @@ class _PopupMenuRoute extends PopupRoute { this.constraints, required this.clipBehavior, super.settings, + super.requestFocus, this.popUpAnimationStyle, }) : itemSizes = List.filled(items.length, null), // Menus always cycle focus through their items irrespective of the @@ -1023,6 +1024,9 @@ class _PopupMenuRoute extends PopupRoute { /// The `clipBehavior` argument is used to clip the shape of the menu. Defaults to /// [Clip.none]. /// +/// The `requestFocus` argument specifies whether the menu should request focus +/// when it appears. If it is null, [Navigator.requestFocus] is used instead. +/// /// See also: /// /// * [PopupMenuItem], a popup menu entry for a single value. @@ -1049,6 +1053,7 @@ Future showMenu({ Clip clipBehavior = Clip.none, RouteSettings? routeSettings, AnimationStyle? popUpAnimationStyle, + bool? requestFocus, }) { assert(items.isNotEmpty); assert(debugCheckHasMaterialLocalizations(context)); @@ -1084,6 +1089,7 @@ Future showMenu({ clipBehavior: clipBehavior, settings: routeSettings, popUpAnimationStyle: popUpAnimationStyle, + requestFocus: requestFocus, )); } @@ -1217,6 +1223,7 @@ class PopupMenuButton extends StatefulWidget { this.popUpAnimationStyle, this.routeSettings, this.style, + this.requestFocus, }) : assert( !(child != null && icon != null), 'You can only pass [child] or [icon], not both.', @@ -1438,6 +1445,11 @@ class PopupMenuButton extends StatefulWidget { /// Null by default. final ButtonStyle? style; + /// Whether to request focus when the menu appears. + /// + /// If null, [Navigator.requestFocus] will be used instead. + final bool? requestFocus; + @override PopupMenuButtonState createState() => PopupMenuButtonState(); } @@ -1501,6 +1513,7 @@ class PopupMenuButtonState extends State> { useRootNavigator: widget.useRootNavigator, popUpAnimationStyle: widget.popUpAnimationStyle, routeSettings: widget.routeSettings, + requestFocus: widget.requestFocus, ) .then((T? newValue) { if (!mounted) { diff --git a/packages/flutter/lib/src/widgets/navigator.dart b/packages/flutter/lib/src/widgets/navigator.dart index ba7dcec7e37..23e99279105 100644 --- a/packages/flutter/lib/src/widgets/navigator.dart +++ b/packages/flutter/lib/src/widgets/navigator.dart @@ -162,7 +162,14 @@ abstract class Route extends _RoutePlaceholder { /// /// If the [settings] are not provided, an empty [RouteSettings] object is /// used instead. - Route({ RouteSettings? settings }) : _settings = settings ?? const RouteSettings() { + /// + /// If [requestFocus] is not provided, the value of [Navigator.requestFocus] is + /// used instead. + Route({ + RouteSettings? settings, + bool? requestFocus, + }) : _settings = settings ?? const RouteSettings(), + _requestFocus = requestFocus { if (kFlutterMemoryAllocationsEnabled) { FlutterMemoryAllocations.instance.dispatchObjectCreated( library: 'package:flutter/widgets.dart', @@ -172,6 +179,12 @@ abstract class Route extends _RoutePlaceholder { } } + /// When the route state is updated, request focus if the current route is at the top. + /// + /// If not provided in the constructor, [Navigator.requestFocus] is used instead. + bool get requestFocus => _requestFocus ?? navigator?.widget.requestFocus ?? false; + final bool? _requestFocus; + /// The navigator that the route is in, if any. NavigatorState? get navigator => _navigator; NavigatorState? _navigator; @@ -256,7 +269,7 @@ abstract class Route extends _RoutePlaceholder { @mustCallSuper TickerFuture didPush() { return TickerFuture.complete()..then((void _) { - if (navigator?.widget.requestFocus ?? false) { + if (requestFocus) { navigator!.focusNode.enclosingScope?.requestFocus(); } }); @@ -272,7 +285,7 @@ abstract class Route extends _RoutePlaceholder { @protected @mustCallSuper void didAdd() { - if (navigator?.widget.requestFocus ?? false) { + if (requestFocus) { // This TickerFuture serves two purposes. First, we want to make sure that // animations triggered by other operations will finish before focusing // the navigator. Second, navigator.focusNode might acquire more focused @@ -1760,6 +1773,9 @@ class Navigator extends StatefulWidget { /// Whether or not the navigator and it's new topmost route should request focus /// when the new route is pushed onto the navigator. /// + /// If [Route.requestFocus] is set on the topmost route, that will take precedence + /// over this value. + /// /// Defaults to true. final bool requestFocus; diff --git a/packages/flutter/lib/src/widgets/pages.dart b/packages/flutter/lib/src/widgets/pages.dart index 3ad1079dd3e..cca590c7626 100644 --- a/packages/flutter/lib/src/widgets/pages.dart +++ b/packages/flutter/lib/src/widgets/pages.dart @@ -24,6 +24,7 @@ abstract class PageRoute extends ModalRoute { /// Creates a modal route that replaces the entire screen. PageRoute({ super.settings, + super.requestFocus, this.fullscreenDialog = false, this.allowSnapshotting = true, bool barrierDismissible = false, @@ -81,6 +82,7 @@ class PageRouteBuilder extends PageRoute { /// Creates a route that delegates to builder callbacks. PageRouteBuilder({ super.settings, + super.requestFocus, required this.pageBuilder, this.transitionsBuilder = _defaultTransitionsBuilder, this.transitionDuration = const Duration(milliseconds: 300), diff --git a/packages/flutter/lib/src/widgets/routes.dart b/packages/flutter/lib/src/widgets/routes.dart index 184c158d21c..0a21b567f0b 100644 --- a/packages/flutter/lib/src/widgets/routes.dart +++ b/packages/flutter/lib/src/widgets/routes.dart @@ -57,6 +57,7 @@ abstract class OverlayRoute extends Route { /// Creates a route that knows how to interact with an [Overlay]. OverlayRoute({ super.settings, + super.requestFocus, }); /// Subclasses should override this getter to return the builders for the overlay. @@ -115,6 +116,7 @@ abstract class TransitionRoute extends OverlayRoute implements PredictiveB /// Creates a route that animates itself when it is pushed or popped. TransitionRoute({ super.settings, + super.requestFocus, }); /// This future completes only once the transition itself has finished, after @@ -1028,7 +1030,7 @@ class _ModalScopeState extends State<_ModalScope> { } bool get _shouldRequestFocus { - return widget.route.navigator!.widget.requestFocus; + return widget.route.requestFocus; } // This should be called to wrap any changes to route.isCurrent, route.canPop, @@ -1140,6 +1142,7 @@ abstract class ModalRoute extends TransitionRoute with LocalHistoryRoute extends ModalRoute { /// Initializes the [PopupRoute]. PopupRoute({ super.settings, + super.requestFocus, super.filter, super.traversalEdgeBehavior, }); @@ -2294,6 +2298,7 @@ class RawDialogRoute extends PopupRoute { Duration transitionDuration = const Duration(milliseconds: 200), RouteTransitionsBuilder? transitionBuilder, super.settings, + super.requestFocus, this.anchorPoint, super.traversalEdgeBehavior, }) : _pageBuilder = pageBuilder, diff --git a/packages/flutter/test/material/popup_menu_test.dart b/packages/flutter/test/material/popup_menu_test.dart index 4758f6e7e3f..40ff853f1fd 100644 --- a/packages/flutter/test/material/popup_menu_test.dart +++ b/packages/flutter/test/material/popup_menu_test.dart @@ -4285,6 +4285,40 @@ void main() { inkWell = tester.widget(find.byType(InkWell)); expect(inkWell.borderRadius, borderRadius); }); + + testWidgets('If requestFocus is false, the original focus should be preserved upon menu appearance.', (WidgetTester tester) async { + final FocusNode fieldFocusNode = FocusNode(); + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Column( + children: [ + TextField(focusNode: fieldFocusNode, autofocus: true), + PopupMenuButton( + style: const ButtonStyle( + iconColor: MaterialStatePropertyAll(Colors.red), + ), + itemBuilder: (BuildContext context) { + return >[ + const PopupMenuItem( + value: 1, + child: Text('One'), + ), + ]; + }, + requestFocus: false, + child: const Text('click here'), + ) + ], + ), + ), + ), + ); + expect(fieldFocusNode.hasFocus, isTrue); + await tester.tap(find.text('click here')); + await tester.pump(); + expect(fieldFocusNode.hasFocus, isTrue); + }); } Matcher overlaps(Rect other) => OverlapsMatcher(other);