Skip to content

Commit e4a39fa

Browse files
gspencergooggspencergoog
authored
Add applyFocusChangeIfNeeded, have menus restore focus before activating (#130536)
## Description This modifies the `MenuAnchor` `onPressed` activation to delay until after the current frame is built, and resolve any focus changes before it invokes the `onPressed`, so that actions that operate on the `primaryFocus` can have a chance of working on the focused item they were meant to work on. ## Related Issues - Fixes flutter/flutter#118731 ## Tests - No tests yet (hence draft still)
1 parent d457287 commit e4a39fa

File tree

9 files changed

+140
-50
lines changed

9 files changed

+140
-50
lines changed

‎dev/manual_tests/lib/menu_anchor.dart

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -706,6 +706,12 @@ List<Widget> createTestMenus({
706706
TestMenu.mainMenu3,
707707
menuChildren: <Widget>[
708708
menuItemButton(TestMenu.subMenu8),
709+
MenuItemButton(
710+
onPressed: () {
711+
debugPrint('Focused Item: $primaryFocus');
712+
},
713+
child: const Text('Print Focused Item'),
714+
)
709715
],
710716
),
711717
submenuButton(
@@ -734,7 +740,11 @@ List<Widget> createTestMenus({
734740
submenuButton(
735741
TestMenu.subSubMenu3,
736742
menuChildren: <Widget>[
737-
menuItemButton(TestMenu.subSubSubMenu1),
743+
for (int i=0; i < 100; ++i)
744+
MenuItemButton(
745+
onPressed: () {},
746+
child: Text('Menu Item $i'),
747+
),
738748
],
739749
),
740750
],

‎examples/api/test/material/menu_anchor/checkbox_menu_button.0_test.dart

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,13 @@ void main() {
1313
);
1414

1515
await tester.tap(find.byType(TextButton));
16-
await tester.pump();
16+
await tester.pumpAndSettle();
1717

1818
expect(find.text('Show Message'), findsOneWidget);
1919
expect(find.text(example.MenuApp.kMessage), findsNothing);
2020

2121
await tester.tap(find.text('Show Message'));
22-
await tester.pump();
22+
await tester.pumpAndSettle();
2323

2424
expect(find.text('Show Message'), findsNothing);
2525
expect(find.text(example.MenuApp.kMessage), findsOneWidget);

‎examples/api/test/material/menu_anchor/menu_anchor.0_test.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ void main() {
4141

4242
await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp);
4343
await tester.sendKeyEvent(LogicalKeyboardKey.enter);
44-
await tester.pump();
44+
await tester.pumpAndSettle();
4545

4646
expect(find.text(example.MenuApp.kMessage), findsOneWidget);
4747
expect(find.text('Last Selected: ${example.MenuEntry.showMessage.label}'), findsOneWidget);

‎examples/api/test/material/menu_anchor/menu_anchor.1_test.dart

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ void main() {
2020
await tester.pumpWidget(const example.ContextMenuApp());
2121

2222
await tester.tapAt(const Offset(100, 200), buttons: kSecondaryButton);
23-
await tester.pump();
23+
await tester.pumpAndSettle();
2424
expect(tester.getRect(findMenu()), equals(const Rect.fromLTRB(100.0, 200.0, 433.0, 360.0)));
2525

2626
// Make sure tapping in a different place causes the menu to move.
@@ -46,15 +46,15 @@ void main() {
4646
expect(find.text('Background Color'), findsOneWidget);
4747

4848
await tester.tap(find.text('Background Color'));
49-
await tester.pump();
49+
await tester.pumpAndSettle();
5050

5151
expect(find.text(example.MenuEntry.colorRed.label), findsOneWidget);
5252
expect(find.text(example.MenuEntry.colorGreen.label), findsOneWidget);
5353
expect(find.text(example.MenuEntry.colorBlue.label), findsOneWidget);
5454

5555
await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp);
5656
await tester.sendKeyEvent(LogicalKeyboardKey.enter);
57-
await tester.pump();
57+
await tester.pumpAndSettle();
5858

5959
expect(find.text(example.ContextMenuApp.kMessage), findsOneWidget);
6060
expect(find.text('Last Selected: ${example.MenuEntry.showMessage.label}'), findsOneWidget);

‎examples/api/test/material/menu_anchor/menu_bar.0_test.dart

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ void main() {
2020

2121
final Finder menuButtonFinder = find.byType(SubmenuButton).first;
2222
await tester.tap(menuButtonFinder);
23-
await tester.pump();
23+
await tester.pumpAndSettle();
2424

2525
expect(find.text('About'), findsOneWidget);
2626
expect(find.text('Show Message'), findsOneWidget);
@@ -34,7 +34,7 @@ void main() {
3434
await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
3535
await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
3636
await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
37-
await tester.pump();
37+
await tester.pumpAndSettle();
3838

3939
expect(find.text('About'), findsOneWidget);
4040
expect(find.text('Show Message'), findsOneWidget);
@@ -46,7 +46,7 @@ void main() {
4646

4747
await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp);
4848
await tester.sendKeyEvent(LogicalKeyboardKey.enter);
49-
await tester.pump();
49+
await tester.pumpAndSettle();
5050

5151
expect(find.text(example.MenuBarApp.kMessage), findsOneWidget);
5252
expect(find.text('Last Selected: Show Message'), findsOneWidget);

‎examples/api/test/material/menu_anchor/radio_menu_button.0_test.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ void main() {
2424
expect(tester.widget<Container>(find.byType(Container)).color, equals(Colors.red));
2525

2626
await tester.tap(find.text('Green Background'));
27-
await tester.pump();
27+
await tester.pumpAndSettle();
2828

2929
expect(tester.widget<Container>(find.byType(Container)).color, equals(Colors.green));
3030
});

‎packages/flutter/lib/src/material/menu_anchor.dart

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1099,10 +1099,16 @@ class _MenuItemButtonState extends State<MenuItemButton> {
10991099

11001100
void _handleSelect() {
11011101
assert(_debugMenuInfo('Selected ${widget.child} menu'));
1102-
widget.onPressed?.call();
11031102
if (widget.closeOnActivate) {
11041103
_MenuAnchorState._maybeOf(context)?._root._close();
11051104
}
1105+
// Delay the call to onPressed until post-frame so that the focus is
1106+
// restored to what it was before the menu was opened before the action is
1107+
// executed.
1108+
SchedulerBinding.instance.addPostFrameCallback((Duration _) {
1109+
FocusManager.instance.applyFocusChangesIfNeeded();
1110+
widget.onPressed?.call();
1111+
});
11061112
}
11071113

11081114
void _createInternalFocusNodeIfNeeded() {

‎packages/flutter/lib/src/widgets/focus_manager.dart

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import 'dart:ui';
88
import 'package:flutter/foundation.dart';
99
import 'package:flutter/gestures.dart';
1010
import 'package:flutter/painting.dart';
11+
import 'package:flutter/scheduler.dart';
1112
import 'package:flutter/services.dart';
1213

1314
import 'binding.dart';
@@ -1601,10 +1602,32 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
16011602
return;
16021603
}
16031604
_haveScheduledUpdate = true;
1604-
scheduleMicrotask(_applyFocusChange);
1605+
scheduleMicrotask(applyFocusChangesIfNeeded);
16051606
}
16061607

1607-
void _applyFocusChange() {
1608+
/// Applies any pending focus changes and notifies listeners that the focus
1609+
/// has changed.
1610+
///
1611+
/// Must not be called during the build phase. This method is meant to be
1612+
/// called in a post-frame callback or microtask when the pending focus
1613+
/// changes need to be resolved before something else occurs.
1614+
///
1615+
/// It can't be called during the build phase because not all listeners are
1616+
/// safe to be called with an update during a build.
1617+
///
1618+
/// Typically, this is called automatically by the [FocusManager], but
1619+
/// sometimes it is necessary to ensure that no focus changes are pending
1620+
/// before executing an action. For example, the [MenuAnchor] class uses this
1621+
/// to make sure that the previous focus has been restored before executing a
1622+
/// menu callback when a menu item is selected.
1623+
///
1624+
/// It is safe to call this if no focus changes are pending.
1625+
void applyFocusChangesIfNeeded() {
1626+
assert(
1627+
SchedulerBinding.instance.schedulerPhase != SchedulerPhase.persistentCallbacks,
1628+
'applyFocusChangesIfNeeded() should not be called during the build phase.'
1629+
);
1630+
16081631
_haveScheduledUpdate = false;
16091632
final FocusNode? previousFocus = _primaryFocus;
16101633

‎packages/flutter/test/material/menu_anchor_test.dart

Lines changed: 87 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -486,6 +486,53 @@ void main() {
486486
);
487487
}, variant: TargetPlatformVariant.desktop());
488488

489+
testWidgets('focus is returned to previous focus before invoking onPressed', (WidgetTester tester) async {
490+
final FocusNode buttonFocus = FocusNode(debugLabel: 'Button Focus');
491+
FocusNode? focusInOnPressed;
492+
493+
void onMenuSelected(TestMenu item) {
494+
focusInOnPressed = FocusManager.instance.primaryFocus;
495+
}
496+
497+
await tester.pumpWidget(
498+
MaterialApp(
499+
home: Material(
500+
child: Column(
501+
children: <Widget>[
502+
MenuBar(
503+
controller: controller,
504+
children: createTestMenus(
505+
onPressed: onMenuSelected,
506+
),
507+
),
508+
ElevatedButton(
509+
autofocus: true,
510+
onPressed: () {},
511+
focusNode: buttonFocus,
512+
child: const Text('Press Me'),
513+
),
514+
],
515+
),
516+
),
517+
),
518+
);
519+
520+
await tester.pump();
521+
expect(FocusManager.instance.primaryFocus, equals(buttonFocus));
522+
523+
await tester.tap(find.text(TestMenu.mainMenu1.label));
524+
await tester.pump();
525+
526+
await tester.tap(find.text(TestMenu.subMenu11.label));
527+
await tester.pump();
528+
529+
await tester.tap(find.text(TestMenu.subSubMenu110.label));
530+
await tester.pump();
531+
532+
expect(focusInOnPressed, equals(buttonFocus));
533+
expect(FocusManager.instance.primaryFocus, equals(buttonFocus));
534+
});
535+
489536
group('Menu functions', () {
490537
testWidgets('basic menu structure', (WidgetTester tester) async {
491538
await tester.pumpWidget(
@@ -3064,35 +3111,38 @@ void main() {
30643111
child: Center(
30653112
child: MenuItemButton(
30663113
style: MenuItemButton.styleFrom(fixedSize: const Size(88.0, 36.0)),
3067-
onPressed: () { },
3114+
onPressed: () {},
30683115
child: const Text('ABC'),
30693116
),
30703117
),
30713118
),
30723119
);
30733120

30743121
// The flags should not have SemanticsFlag.isButton
3075-
expect(semantics, hasSemantics(
3076-
TestSemantics.root(
3077-
children: <TestSemantics>[
3078-
TestSemantics.rootChild(
3079-
actions: <SemanticsAction>[
3080-
SemanticsAction.tap,
3081-
],
3082-
label: 'ABC',
3083-
rect: const Rect.fromLTRB(0.0, 0.0, 88.0, 48.0),
3084-
transform: Matrix4.translationValues(356.0, 276.0, 0.0),
3085-
flags: <SemanticsFlag>[
3086-
SemanticsFlag.hasEnabledState,
3087-
SemanticsFlag.isEnabled,
3088-
SemanticsFlag.isFocusable,
3089-
],
3090-
textDirection: TextDirection.ltr,
3091-
),
3092-
],
3122+
expect(
3123+
semantics,
3124+
hasSemantics(
3125+
TestSemantics.root(
3126+
children: <TestSemantics>[
3127+
TestSemantics.rootChild(
3128+
actions: <SemanticsAction>[
3129+
SemanticsAction.tap,
3130+
],
3131+
label: 'ABC',
3132+
rect: const Rect.fromLTRB(0.0, 0.0, 88.0, 48.0),
3133+
transform: Matrix4.translationValues(356.0, 276.0, 0.0),
3134+
flags: <SemanticsFlag>[
3135+
SemanticsFlag.hasEnabledState,
3136+
SemanticsFlag.isEnabled,
3137+
SemanticsFlag.isFocusable,
3138+
],
3139+
textDirection: TextDirection.ltr,
3140+
),
3141+
],
3142+
),
3143+
ignoreId: true,
30933144
),
3094-
ignoreId: true,
3095-
));
3145+
);
30963146

30973147
semantics.dispose();
30983148
});
@@ -3114,22 +3164,23 @@ void main() {
31143164
);
31153165

31163166
// The flags should not have SemanticsFlag.isButton
3117-
expect(semantics, hasSemantics(
3118-
TestSemantics.root(
3119-
children: <TestSemantics>[
3120-
TestSemantics.rootChild(
3121-
label: 'ABC',
3122-
rect: const Rect.fromLTRB(0.0, 0.0, 88.0, 48.0),
3123-
transform: Matrix4.translationValues(356.0, 276.0, 0.0),
3124-
flags: <SemanticsFlag>[
3125-
SemanticsFlag.hasEnabledState,
3126-
],
3127-
textDirection: TextDirection.ltr,
3128-
),
3129-
],
3167+
expect(
3168+
semantics,
3169+
hasSemantics(
3170+
TestSemantics.root(
3171+
children: <TestSemantics>[
3172+
TestSemantics(
3173+
rect: const Rect.fromLTRB(0.0, 0.0, 88.0, 48.0),
3174+
flags: <SemanticsFlag>[SemanticsFlag.hasEnabledState],
3175+
label: 'ABC',
3176+
textDirection: TextDirection.ltr,
3177+
),
3178+
],
3179+
),
3180+
ignoreTransform: true,
3181+
ignoreId: true,
31303182
),
3131-
ignoreId: true,
3132-
));
3183+
);
31333184

31343185
semantics.dispose();
31353186
});

0 commit comments

Comments
 (0)