Unverified Commit e4a39fa2 authored by Greg Spencer's avatar Greg Spencer Committed by GitHub

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 https://github.com/flutter/flutter/issues/118731

## Tests
 - No tests yet (hence draft still)
parent d457287f
...@@ -706,6 +706,12 @@ List<Widget> createTestMenus({ ...@@ -706,6 +706,12 @@ List<Widget> createTestMenus({
TestMenu.mainMenu3, TestMenu.mainMenu3,
menuChildren: <Widget>[ menuChildren: <Widget>[
menuItemButton(TestMenu.subMenu8), menuItemButton(TestMenu.subMenu8),
MenuItemButton(
onPressed: () {
debugPrint('Focused Item: $primaryFocus');
},
child: const Text('Print Focused Item'),
)
], ],
), ),
submenuButton( submenuButton(
...@@ -734,7 +740,11 @@ List<Widget> createTestMenus({ ...@@ -734,7 +740,11 @@ List<Widget> createTestMenus({
submenuButton( submenuButton(
TestMenu.subSubMenu3, TestMenu.subSubMenu3,
menuChildren: <Widget>[ menuChildren: <Widget>[
menuItemButton(TestMenu.subSubSubMenu1), for (int i=0; i < 100; ++i)
MenuItemButton(
onPressed: () {},
child: Text('Menu Item $i'),
),
], ],
), ),
], ],
......
...@@ -13,13 +13,13 @@ void main() { ...@@ -13,13 +13,13 @@ void main() {
); );
await tester.tap(find.byType(TextButton)); await tester.tap(find.byType(TextButton));
await tester.pump(); await tester.pumpAndSettle();
expect(find.text('Show Message'), findsOneWidget); expect(find.text('Show Message'), findsOneWidget);
expect(find.text(example.MenuApp.kMessage), findsNothing); expect(find.text(example.MenuApp.kMessage), findsNothing);
await tester.tap(find.text('Show Message')); await tester.tap(find.text('Show Message'));
await tester.pump(); await tester.pumpAndSettle();
expect(find.text('Show Message'), findsNothing); expect(find.text('Show Message'), findsNothing);
expect(find.text(example.MenuApp.kMessage), findsOneWidget); expect(find.text(example.MenuApp.kMessage), findsOneWidget);
......
...@@ -41,7 +41,7 @@ void main() { ...@@ -41,7 +41,7 @@ void main() {
await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp); await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp);
await tester.sendKeyEvent(LogicalKeyboardKey.enter); await tester.sendKeyEvent(LogicalKeyboardKey.enter);
await tester.pump(); await tester.pumpAndSettle();
expect(find.text(example.MenuApp.kMessage), findsOneWidget); expect(find.text(example.MenuApp.kMessage), findsOneWidget);
expect(find.text('Last Selected: ${example.MenuEntry.showMessage.label}'), findsOneWidget); expect(find.text('Last Selected: ${example.MenuEntry.showMessage.label}'), findsOneWidget);
......
...@@ -20,7 +20,7 @@ void main() { ...@@ -20,7 +20,7 @@ void main() {
await tester.pumpWidget(const example.ContextMenuApp()); await tester.pumpWidget(const example.ContextMenuApp());
await tester.tapAt(const Offset(100, 200), buttons: kSecondaryButton); await tester.tapAt(const Offset(100, 200), buttons: kSecondaryButton);
await tester.pump(); await tester.pumpAndSettle();
expect(tester.getRect(findMenu()), equals(const Rect.fromLTRB(100.0, 200.0, 433.0, 360.0))); expect(tester.getRect(findMenu()), equals(const Rect.fromLTRB(100.0, 200.0, 433.0, 360.0)));
// Make sure tapping in a different place causes the menu to move. // Make sure tapping in a different place causes the menu to move.
...@@ -46,7 +46,7 @@ void main() { ...@@ -46,7 +46,7 @@ void main() {
expect(find.text('Background Color'), findsOneWidget); expect(find.text('Background Color'), findsOneWidget);
await tester.tap(find.text('Background Color')); await tester.tap(find.text('Background Color'));
await tester.pump(); await tester.pumpAndSettle();
expect(find.text(example.MenuEntry.colorRed.label), findsOneWidget); expect(find.text(example.MenuEntry.colorRed.label), findsOneWidget);
expect(find.text(example.MenuEntry.colorGreen.label), findsOneWidget); expect(find.text(example.MenuEntry.colorGreen.label), findsOneWidget);
...@@ -54,7 +54,7 @@ void main() { ...@@ -54,7 +54,7 @@ void main() {
await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp); await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp);
await tester.sendKeyEvent(LogicalKeyboardKey.enter); await tester.sendKeyEvent(LogicalKeyboardKey.enter);
await tester.pump(); await tester.pumpAndSettle();
expect(find.text(example.ContextMenuApp.kMessage), findsOneWidget); expect(find.text(example.ContextMenuApp.kMessage), findsOneWidget);
expect(find.text('Last Selected: ${example.MenuEntry.showMessage.label}'), findsOneWidget); expect(find.text('Last Selected: ${example.MenuEntry.showMessage.label}'), findsOneWidget);
......
...@@ -20,7 +20,7 @@ void main() { ...@@ -20,7 +20,7 @@ void main() {
final Finder menuButtonFinder = find.byType(SubmenuButton).first; final Finder menuButtonFinder = find.byType(SubmenuButton).first;
await tester.tap(menuButtonFinder); await tester.tap(menuButtonFinder);
await tester.pump(); await tester.pumpAndSettle();
expect(find.text('About'), findsOneWidget); expect(find.text('About'), findsOneWidget);
expect(find.text('Show Message'), findsOneWidget); expect(find.text('Show Message'), findsOneWidget);
...@@ -34,7 +34,7 @@ void main() { ...@@ -34,7 +34,7 @@ void main() {
await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown); await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown); await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown); await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
await tester.pump(); await tester.pumpAndSettle();
expect(find.text('About'), findsOneWidget); expect(find.text('About'), findsOneWidget);
expect(find.text('Show Message'), findsOneWidget); expect(find.text('Show Message'), findsOneWidget);
...@@ -46,7 +46,7 @@ void main() { ...@@ -46,7 +46,7 @@ void main() {
await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp); await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp);
await tester.sendKeyEvent(LogicalKeyboardKey.enter); await tester.sendKeyEvent(LogicalKeyboardKey.enter);
await tester.pump(); await tester.pumpAndSettle();
expect(find.text(example.MenuBarApp.kMessage), findsOneWidget); expect(find.text(example.MenuBarApp.kMessage), findsOneWidget);
expect(find.text('Last Selected: Show Message'), findsOneWidget); expect(find.text('Last Selected: Show Message'), findsOneWidget);
......
...@@ -24,7 +24,7 @@ void main() { ...@@ -24,7 +24,7 @@ void main() {
expect(tester.widget<Container>(find.byType(Container)).color, equals(Colors.red)); expect(tester.widget<Container>(find.byType(Container)).color, equals(Colors.red));
await tester.tap(find.text('Green Background')); await tester.tap(find.text('Green Background'));
await tester.pump(); await tester.pumpAndSettle();
expect(tester.widget<Container>(find.byType(Container)).color, equals(Colors.green)); expect(tester.widget<Container>(find.byType(Container)).color, equals(Colors.green));
}); });
......
...@@ -1099,10 +1099,16 @@ class _MenuItemButtonState extends State<MenuItemButton> { ...@@ -1099,10 +1099,16 @@ class _MenuItemButtonState extends State<MenuItemButton> {
void _handleSelect() { void _handleSelect() {
assert(_debugMenuInfo('Selected ${widget.child} menu')); assert(_debugMenuInfo('Selected ${widget.child} menu'));
widget.onPressed?.call();
if (widget.closeOnActivate) { if (widget.closeOnActivate) {
_MenuAnchorState._maybeOf(context)?._root._close(); _MenuAnchorState._maybeOf(context)?._root._close();
} }
// Delay the call to onPressed until post-frame so that the focus is
// restored to what it was before the menu was opened before the action is
// executed.
SchedulerBinding.instance.addPostFrameCallback((Duration _) {
FocusManager.instance.applyFocusChangesIfNeeded();
widget.onPressed?.call();
});
} }
void _createInternalFocusNodeIfNeeded() { void _createInternalFocusNodeIfNeeded() {
......
...@@ -8,6 +8,7 @@ import 'dart:ui'; ...@@ -8,6 +8,7 @@ import 'dart:ui';
import 'package:flutter/foundation.dart'; import 'package:flutter/foundation.dart';
import 'package:flutter/gestures.dart'; import 'package:flutter/gestures.dart';
import 'package:flutter/painting.dart'; import 'package:flutter/painting.dart';
import 'package:flutter/scheduler.dart';
import 'package:flutter/services.dart'; import 'package:flutter/services.dart';
import 'binding.dart'; import 'binding.dart';
...@@ -1601,10 +1602,32 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -1601,10 +1602,32 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
return; return;
} }
_haveScheduledUpdate = true; _haveScheduledUpdate = true;
scheduleMicrotask(_applyFocusChange); scheduleMicrotask(applyFocusChangesIfNeeded);
} }
void _applyFocusChange() { /// Applies any pending focus changes and notifies listeners that the focus
/// has changed.
///
/// Must not be called during the build phase. This method is meant to be
/// called in a post-frame callback or microtask when the pending focus
/// changes need to be resolved before something else occurs.
///
/// It can't be called during the build phase because not all listeners are
/// safe to be called with an update during a build.
///
/// Typically, this is called automatically by the [FocusManager], but
/// sometimes it is necessary to ensure that no focus changes are pending
/// before executing an action. For example, the [MenuAnchor] class uses this
/// to make sure that the previous focus has been restored before executing a
/// menu callback when a menu item is selected.
///
/// It is safe to call this if no focus changes are pending.
void applyFocusChangesIfNeeded() {
assert(
SchedulerBinding.instance.schedulerPhase != SchedulerPhase.persistentCallbacks,
'applyFocusChangesIfNeeded() should not be called during the build phase.'
);
_haveScheduledUpdate = false; _haveScheduledUpdate = false;
final FocusNode? previousFocus = _primaryFocus; final FocusNode? previousFocus = _primaryFocus;
......
...@@ -486,6 +486,53 @@ void main() { ...@@ -486,6 +486,53 @@ void main() {
); );
}, variant: TargetPlatformVariant.desktop()); }, variant: TargetPlatformVariant.desktop());
testWidgets('focus is returned to previous focus before invoking onPressed', (WidgetTester tester) async {
final FocusNode buttonFocus = FocusNode(debugLabel: 'Button Focus');
FocusNode? focusInOnPressed;
void onMenuSelected(TestMenu item) {
focusInOnPressed = FocusManager.instance.primaryFocus;
}
await tester.pumpWidget(
MaterialApp(
home: Material(
child: Column(
children: <Widget>[
MenuBar(
controller: controller,
children: createTestMenus(
onPressed: onMenuSelected,
),
),
ElevatedButton(
autofocus: true,
onPressed: () {},
focusNode: buttonFocus,
child: const Text('Press Me'),
),
],
),
),
),
);
await tester.pump();
expect(FocusManager.instance.primaryFocus, equals(buttonFocus));
await tester.tap(find.text(TestMenu.mainMenu1.label));
await tester.pump();
await tester.tap(find.text(TestMenu.subMenu11.label));
await tester.pump();
await tester.tap(find.text(TestMenu.subSubMenu110.label));
await tester.pump();
expect(focusInOnPressed, equals(buttonFocus));
expect(FocusManager.instance.primaryFocus, equals(buttonFocus));
});
group('Menu functions', () { group('Menu functions', () {
testWidgets('basic menu structure', (WidgetTester tester) async { testWidgets('basic menu structure', (WidgetTester tester) async {
await tester.pumpWidget( await tester.pumpWidget(
...@@ -3064,7 +3111,7 @@ void main() { ...@@ -3064,7 +3111,7 @@ void main() {
child: Center( child: Center(
child: MenuItemButton( child: MenuItemButton(
style: MenuItemButton.styleFrom(fixedSize: const Size(88.0, 36.0)), style: MenuItemButton.styleFrom(fixedSize: const Size(88.0, 36.0)),
onPressed: () { }, onPressed: () {},
child: const Text('ABC'), child: const Text('ABC'),
), ),
), ),
...@@ -3072,27 +3119,30 @@ void main() { ...@@ -3072,27 +3119,30 @@ void main() {
); );
// The flags should not have SemanticsFlag.isButton // The flags should not have SemanticsFlag.isButton
expect(semantics, hasSemantics( expect(
TestSemantics.root( semantics,
children: <TestSemantics>[ hasSemantics(
TestSemantics.rootChild( TestSemantics.root(
actions: <SemanticsAction>[ children: <TestSemantics>[
SemanticsAction.tap, TestSemantics.rootChild(
], actions: <SemanticsAction>[
label: 'ABC', SemanticsAction.tap,
rect: const Rect.fromLTRB(0.0, 0.0, 88.0, 48.0), ],
transform: Matrix4.translationValues(356.0, 276.0, 0.0), label: 'ABC',
flags: <SemanticsFlag>[ rect: const Rect.fromLTRB(0.0, 0.0, 88.0, 48.0),
SemanticsFlag.hasEnabledState, transform: Matrix4.translationValues(356.0, 276.0, 0.0),
SemanticsFlag.isEnabled, flags: <SemanticsFlag>[
SemanticsFlag.isFocusable, SemanticsFlag.hasEnabledState,
], SemanticsFlag.isEnabled,
textDirection: TextDirection.ltr, SemanticsFlag.isFocusable,
), ],
], textDirection: TextDirection.ltr,
),
],
),
ignoreId: true,
), ),
ignoreId: true, );
));
semantics.dispose(); semantics.dispose();
}); });
...@@ -3114,22 +3164,23 @@ void main() { ...@@ -3114,22 +3164,23 @@ void main() {
); );
// The flags should not have SemanticsFlag.isButton // The flags should not have SemanticsFlag.isButton
expect(semantics, hasSemantics( expect(
TestSemantics.root( semantics,
children: <TestSemantics>[ hasSemantics(
TestSemantics.rootChild( TestSemantics.root(
label: 'ABC', children: <TestSemantics>[
rect: const Rect.fromLTRB(0.0, 0.0, 88.0, 48.0), TestSemantics(
transform: Matrix4.translationValues(356.0, 276.0, 0.0), rect: const Rect.fromLTRB(0.0, 0.0, 88.0, 48.0),
flags: <SemanticsFlag>[ flags: <SemanticsFlag>[SemanticsFlag.hasEnabledState],
SemanticsFlag.hasEnabledState, label: 'ABC',
], textDirection: TextDirection.ltr,
textDirection: TextDirection.ltr, ),
), ],
], ),
ignoreTransform: true,
ignoreId: true,
), ),
ignoreId: true, );
));
semantics.dispose(); semantics.dispose();
}); });
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment