Added proper focus handling when pushing and popping routes (#40166)

The proposed change will change focus handling when pushing and popping routes so that the FocusScopeNode for the route receives focus when pushed, and that the FocusScopeNode in the navigator receives focus when the route is popped.

This means that the last setFirstFocus call on the scope is used to determine which control actually receives focus. When the focus scope receives focus, it traverses its children, trying to find a non-scope node that is the "first focus" of itself or a child node.

This is a breaking change, because the focus behavior has changed. If you push a route after this change, and had a 'first focus' set on a widget via FocusScopeNode.setFirstFocus, it won't currently receive focus immediately, but after this change it will. Similarly, if you pop a route after this change, the focus will go back to where it was before the route was pushed, which is correct, but different from what happens now.

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

@@ -81,9 +81,12 @@ class FocusAttachment {
     assert(_node != null);
     assert(_focusDebug('Detaching node:', <String>[_node.toString(), 'With enclosing scope ${_node.enclosingScope}']));
     if (isAttached) {
-      if (_node.hasPrimaryFocus) {
-        _node.unfocus();
+      if (_node.hasPrimaryFocus || (_node._manager != null && _node._manager._nextFocus == _node)) {
+        _node.unfocus(focusPrevious: true);
       }
+      assert(_node._manager?._nextFocus != _node);
+      assert(!_node.hasPrimaryFocus);
+      _node._manager?._dirtyNodes?.remove(_node);
       _node._parent?._removeChild(_node);
       _node._attachment = null;
     }
@@ -418,14 +421,15 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
   ///     its descendants.
   ///   - [FocusTraversalPolicy], a class that can be extended to describe a
   ///     traversal policy.
-  bool get canRequestFocus => _canRequestFocus;
+  bool get canRequestFocus => _canRequestFocus && (enclosingScope == null || enclosingScope.canRequestFocus);
   bool _canRequestFocus;
+  @mustCallSuper
   set canRequestFocus(bool value) {
     if (value != _canRequestFocus) {
-      _canRequestFocus = value;
-      if (!_canRequestFocus) {
-        unfocus();
+      if (!value) {
+        unfocus(focusPrevious: true);
       }
+      _canRequestFocus = value;
       _manager?._dirtyNodes?.add(this);
       _manager?._markNeedsUpdate();
     }
@@ -463,8 +467,11 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
   /// An iterator over the children that are allowed to be traversed by the
   /// [FocusTraversalPolicy].
   Iterable<FocusNode> get traversalChildren {
+    if (!canRequestFocus) {
+      return const <FocusNode>[];
+    }
     return children.where(
-      (FocusNode node) => !node.skipTraversal && node.canRequestFocus,
+      (FocusNode node) => !node.skipTraversal && node._canRequestFocus,
     );
   }
 
@@ -625,18 +632,28 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
   ///
   /// Has no effect on nodes that return true from [hasFocus], but false from
   /// [hasPrimaryFocus].
-  void unfocus() {
-    if (hasPrimaryFocus) {
-      final FocusScopeNode scope = enclosingScope;
-      assert(scope != null, 'Node has primary focus, but no enclosingScope.');
-      scope._focusedChildren.remove(this);
-      _manager?._willUnfocusNode(this);
+  ///
+  /// if [focusPrevious] is true, then rather than losing all focus, the focus
+  /// will be moved to the node that the [enclosingScope] thinks should have it,
+  /// based on its history of nodes that were set as first focus on it using
+  /// [FocusScopeNode.setFirstFocus].
+  void unfocus({ bool focusPrevious = false }) {
+    assert(focusPrevious != null);
+    if (!hasFocus && (_manager != null && _manager._nextFocus != this)) {
       return;
     }
-    if (hasFocus) {
+    if (!hasPrimaryFocus) {
       // If we are in the focus chain, but not the primary focus, then unfocus
       // the primary instead.
-      _manager.primaryFocus.unfocus();
+      _manager?.primaryFocus?.unfocus(focusPrevious: focusPrevious);
+    }
+    _manager?._willUnfocusNode(this);
+    final FocusScopeNode scope = enclosingScope;
+    if (scope != null) {
+      scope._focusedChildren.remove(this);
+      if (focusPrevious) {
+        scope._doRequestFocus();
+      }
     }
   }
 
@@ -667,8 +684,8 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
   void _markAsDirty({FocusNode newFocus}) {
     if (_manager != null) {
       // If we have a manager, then let it handle the focus change.
-      _manager._dirtyNodes?.add(this);
       _manager._markNeedsUpdate(newFocus: newFocus);
+      _manager._dirtyNodes?.add(this);
     } else {
       // If we don't have a manager, then change the focus locally.
       newFocus?._setAsFocusedChild();
@@ -748,7 +765,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
 
   @override
   void dispose() {
-    _manager?._willDisposeFocusNode(this);
+    // Detaching will also unfocus and clean up the manager's data structures.
     _attachment?.detach();
     super.dispose();
   }
@@ -966,10 +983,6 @@ class FocusScopeNode extends FocusNode {
 
   @override
   void _doRequestFocus() {
-    if (!canRequestFocus) {
-      return;
-    }
-
     // Start with the primary focus as the focused child of this scope, if there
     // is one. Otherwise start with this node itself.
     FocusNode primaryFocus = focusedChild ?? this;
@@ -980,22 +993,31 @@ class FocusScopeNode extends FocusNode {
       final FocusScopeNode scope = primaryFocus;
       primaryFocus = scope.focusedChild;
     }
-    if (primaryFocus is FocusScopeNode) {
-      // We didn't find a FocusNode at the leaf, so we're focusing the scope.
-      _setAsFocusedChild();
-      _markAsDirty(newFocus: primaryFocus);
+    if (identical(primaryFocus, this)) {
+      // We didn't find a FocusNode at the leaf, so we're focusing the scope, if
+      // allowed.
+      if (primaryFocus.canRequestFocus) {
+        _setAsFocusedChild();
+        _markAsDirty(newFocus: this);
+      }
     } else {
       // We found a FocusScope at the leaf, so ask it to focus itself instead of
       // this scope. That will cause this scope to return true from hasFocus,
       // but false from hasPrimaryFocus.
-      primaryFocus.requestFocus();
+      primaryFocus._doRequestFocus();
     }
   }
 
   @override
   void debugFillProperties(DiagnosticPropertiesBuilder properties) {
     super.debugFillProperties(properties);
-    properties.add(DiagnosticsProperty<FocusNode>('focusedChild', focusedChild, defaultValue: null));
+    if (_focusedChildren.isEmpty) {
+      return;
+    }
+    final List<String> childList = _focusedChildren.reversed.map<String>((FocusNode child) {
+      return '${describeIdentity(child)}${child.debugLabel != null && child.debugLabel.isNotEmpty ? '(${child.debugLabel})' : ''}';
+    }).toList();
+    properties.add(IterableProperty<String>('focusedChildren', childList, defaultValue: <String>[]));
   }
 }
 
@@ -1225,6 +1247,7 @@ class FocusManager with DiagnosticableTreeMixin {
       _updateHighlightMode();
     }
 
+    assert(_focusDebug('Received key event ${event.logicalKey}'));
     // Walk the current focus from the leaf to the root, calling each one's
     // onKey on the way up, and if one responds that they handled it, stop.
     if (_primaryFocus == null) {
@@ -1255,26 +1278,18 @@ class FocusManager with DiagnosticableTreeMixin {
   // update.
   final Set<FocusNode> _dirtyNodes = <FocusNode>{};
 
-  // Called to indicate that the given node is being disposed.
-  void _willDisposeFocusNode(FocusNode node) {
-    assert(node != null);
-    assert(_focusDebug('Disposing of node:', <String>[node.toString(), 'with enclosing scope ${node.enclosingScope}']));
-    _willUnfocusNode(node);
-    _dirtyNodes.remove(node);
-  }
-
   // Called to indicate that the given node is being unfocused, and that any
   // pending request to be focused should be canceled.
   void _willUnfocusNode(FocusNode node) {
     assert(node != null);
     assert(_focusDebug('Unfocusing node $node'));
-    if (_primaryFocus == node) {
-      _primaryFocus = null;
-      _dirtyNodes.add(node);
-      _markNeedsUpdate();
-    }
-    if (_nextFocus == node) {
-      _nextFocus = null;
+    if (_primaryFocus == node || _nextFocus == node) {
+      if (_primaryFocus == node) {
+        _primaryFocus = null;
+      }
+      if (_nextFocus == node) {
+        _nextFocus = null;
+      }
       _dirtyNodes.add(node);
       _markNeedsUpdate();
     }
@@ -1350,6 +1365,7 @@ class FocusManager with DiagnosticableTreeMixin {
   void debugFillProperties(DiagnosticPropertiesBuilder properties) {
     properties.add(FlagProperty('haveScheduledUpdate', value: _haveScheduledUpdate, ifTrue: 'UPDATE SCHEDULED'));
     properties.add(DiagnosticsProperty<FocusNode>('primaryFocus', primaryFocus, defaultValue: null));
+    properties.add(DiagnosticsProperty<FocusNode>('nextFocus', _nextFocus, defaultValue: null));
     final Element element = primaryFocus?.context;
     if (element != null) {
       properties.add(DiagnosticsProperty<String>('primaryFocusCreator', element.debugGetCreatorChain(20)));

packages/flutter/lib/src/widgets/focus_scope.dart

@@ -339,9 +339,9 @@ class _FocusState extends State<Focus> {
       // _createNode is overridden in _FocusScopeState.
       _internalNode ??= _createNode();
     }
+    _focusAttachment = focusNode.attach(context, onKey: widget.onKey);
     focusNode.skipTraversal = widget.skipTraversal ?? focusNode.skipTraversal;
     focusNode.canRequestFocus = widget.canRequestFocus ?? focusNode.canRequestFocus;
-    _focusAttachment = focusNode.attach(context, onKey: widget.onKey);
     _hasFocus = focusNode.hasFocus;
 
     // Add listener even if the _internalNode existed before, since it should

packages/flutter/lib/src/widgets/navigator.dart

@@ -133,7 +133,12 @@ abstract class Route<T> {
   /// The [didChangeNext] and [didChangePrevious] methods are typically called
   /// immediately after this method is called.
   @protected
-  TickerFuture didPush() => TickerFuture.complete();
+  @mustCallSuper
+  TickerFuture didPush() {
+    return TickerFuture.complete()..then<void>((void _) {
+      navigator.focusScopeNode.requestFocus();
+    });
+  }
 
   /// Called after [install] when the route replaced another in the navigator.
   ///

packages/flutter/lib/src/widgets/routes.dart

@@ -185,6 +185,7 @@ abstract class TransitionRoute<T> extends OverlayRoute<T> {
     assert(_controller != null, '$runtimeType.didPush called before calling install() or after calling dispose().');
     assert(!_transitionCompleter.isCompleted, 'Cannot reuse a $runtimeType after disposing it.');
     _animation.addStatusListener(_handleStatusChanged);
+    super.didPush();
     return _controller.forward();
   }
 
@@ -629,6 +630,9 @@ class _ModalScopeState<T> extends State<_ModalScope<T>> {
   // This should be called to wrap any changes to route.isCurrent, route.canPop,
   // and route.offstage.
   void _routeSetState(VoidCallback fn) {
+    if (widget.route.isCurrent) {
+      widget.route.navigator.focusScopeNode.setFirstFocus(focusScopeNode);
+    }
     setState(fn);
   }
 
@@ -654,13 +658,16 @@ class _ModalScopeState<T> extends State<_ModalScope<T>> {
                     widget.route.secondaryAnimation,
                     // This additional AnimatedBuilder is include because if the
                     // value of the userGestureInProgressNotifier changes, it's
-                    // only necessary to rebuild the IgnorePointer widget.
+                    // only necessary to rebuild the IgnorePointer widget and set
+                    // the focus node's ability to focus.
                     AnimatedBuilder(
                       animation: widget.route.navigator.userGestureInProgressNotifier,
                       builder: (BuildContext context, Widget child) {
+                        final bool ignoreEvents = widget.route.animation?.status == AnimationStatus.reverse ||
+                            widget.route.navigator.userGestureInProgress;
+                        focusScopeNode.canRequestFocus = !ignoreEvents;
                         return IgnorePointer(
-                          ignoring: widget.route.navigator.userGestureInProgress
-                            || widget.route.animation?.status == AnimationStatus.reverse,
+                          ignoring: ignoreEvents,
                           child: child,
                         );
                       },

packages/flutter/test/material/page_test.dart

@@ -342,7 +342,7 @@ void main() {
     expect(helloPosition3.dy, helloPosition4.dy);
     await gesture.moveBy(const Offset(500.0, 0.0));
     await gesture.up();
-    expect(await tester.pumpAndSettle(const Duration(minutes: 1)), 2);
+    expect(await tester.pumpAndSettle(const Duration(minutes: 1)), 3);
     expect(find.text('PUSH'), findsOneWidget);
     expect(find.text('HELLO'), findsNothing);
   });

packages/flutter/test/material/text_field_focus_test.dart

@@ -25,6 +25,7 @@ void main() {
     );
 
     expect(tester.testTextInput.isVisible, isTrue);
+    expect(focusNode.hasPrimaryFocus, isTrue);
 
     final BuildContext context = tester.element(find.byType(TextField));
 
@@ -40,11 +41,7 @@ void main() {
     Navigator.of(tester.element(find.text('Dialog'))).pop();
     await tester.pump();
 
-    expect(tester.testTextInput.isVisible, isFalse);
-
-    await tester.tap(find.byType(TextField));
-    await tester.idle();
-
+    expect(focusNode.hasPrimaryFocus, isTrue);
     expect(tester.testTextInput.isVisible, isTrue);
 
     await tester.pumpWidget(Container());

packages/flutter/test/widgets/focus_manager_test.dart

@@ -561,7 +561,7 @@ void main() {
           ' └─rootScope: FocusScopeNode#00000\n'
           '   │ FOCUSED\n'
           '   │ debugLabel: "Root Focus Scope"\n'
-          '   │ focusedChild: FocusScopeNode#00000\n'
+          '   │ focusedChildren: FocusScopeNode#00000\n'
           '   │\n'
           '   ├─Child 1: FocusScopeNode#00000\n'
           '   │ │ context: Container-[GlobalKey#00000]\n'
@@ -581,7 +581,7 @@ void main() {
           '   └─Child 2: FocusScopeNode#00000\n'
           '     │ context: Container-[GlobalKey#00000]\n'
           '     │ FOCUSED\n'
-          '     │ focusedChild: FocusNode#00000\n'
+          '     │ focusedChildren: FocusNode#00000(Child 4)\n'
           '     │\n'
           '     └─Child 1: FocusNode#00000\n'
           '       │ context: Container-[GlobalKey#00000]\n'

packages/flutter/test/widgets/focus_scope_test.dart

@@ -226,7 +226,7 @@ void main() {
             ' │ context: FocusScope\n'
             ' │ FOCUSED\n'
             ' │ debugLabel: "Parent Scope Node"\n'
-            ' │ focusedChild: FocusNode#00000\n'
+            ' │ focusedChildren: FocusNode#00000(Child)\n'
             ' │\n'
             ' └─Child 1: FocusNode#00000\n'
             '     context: Focus\n'
@@ -240,13 +240,13 @@ void main() {
         equalsIgnoringHashCodes('FocusScopeNode#00000\n'
             ' │ FOCUSED\n'
             ' │ debugLabel: "Root Focus Scope"\n'
-            ' │ focusedChild: FocusScopeNode#00000\n'
+            ' │ focusedChildren: FocusScopeNode#00000(Parent Scope Node)\n'
             ' │\n'
             ' └─Child 1: FocusScopeNode#00000\n'
             '   │ context: FocusScope\n'
             '   │ FOCUSED\n'
             '   │ debugLabel: "Parent Scope Node"\n'
-            '   │ focusedChild: FocusNode#00000\n'
+            '   │ focusedChildren: FocusNode#00000(Child)\n'
             '   │\n'
             '   └─Child 1: FocusNode#00000\n'
             '       context: Focus\n'
@@ -733,8 +733,8 @@ void main() {
 
       await tester.pump();
 
-      expect(keyB.currentState.focusNode.hasFocus, isFalse);
-      expect(find.text('b'), findsOneWidget);
+      expect(keyB.currentState.focusNode.hasFocus, isTrue);
+      expect(find.text('B FOCUSED'), findsOneWidget);
     });
 
     testWidgets("Removing unpinned focused scope doesn't move focus to focused widget within next FocusScope", (WidgetTester tester) async {
@@ -814,8 +814,8 @@ void main() {
       );
       await tester.pump();
 
-      expect(keyB.currentState.focusNode.hasFocus, isFalse);
-      expect(find.text('b'), findsOneWidget);
+      expect(keyB.currentState.focusNode.hasFocus, isTrue);
+      expect(find.text('B FOCUSED'), findsOneWidget);
     });
 
     testWidgets('Moving widget from one scope to another retains focus', (WidgetTester tester) async {

packages/flutter/test/widgets/focus_traversal_test.dart

@@ -235,6 +235,71 @@ void main() {
       expect(secondFocusNode.hasFocus, isFalse);
       expect(scope.hasFocus, isTrue);
     });
+    testWidgets('Find the initial focus when a route is pushed or popped.', (WidgetTester tester) async {
+      final GlobalKey key1 = GlobalKey(debugLabel: '1');
+      final GlobalKey key2 = GlobalKey(debugLabel: '2');
+      final FocusNode testNode1 = FocusNode(debugLabel: 'First Focus Node');
+      final FocusNode testNode2 = FocusNode(debugLabel: 'Second Focus Node');
+      await tester.pumpWidget(
+        MaterialApp(
+          home: DefaultFocusTraversal(
+            policy: WidgetOrderFocusTraversalPolicy(),
+            child: Center(
+              child: Builder(builder: (BuildContext context) {
+                return MaterialButton(
+                  key: key1,
+                  focusNode: testNode1,
+                  autofocus: true,
+                  onPressed: () {
+                    Navigator.of(context).push<void>(
+                      MaterialPageRoute<void>(
+                        builder: (BuildContext context) {
+                          return Center(
+                            child: MaterialButton(
+                              key: key2,
+                              focusNode: testNode2,
+                              autofocus: true,
+                              onPressed: () {
+                                Navigator.of(context).pop();
+                              },
+                              child: const Text('Go Back'),
+                            ),
+                          );
+                        },
+                      ),
+                    );
+                  },
+                  child: const Text('Go Forward'),
+                );
+              }),
+            ),
+          ),
+        ),
+      );
+
+      final Element firstChild = tester.element(find.text('Go Forward'));
+      final FocusNode firstFocusNode = Focus.of(firstChild);
+      final FocusNode scope = Focus.of(firstChild).enclosingScope;
+      await tester.pump();
+
+      expect(firstFocusNode.hasFocus, isTrue);
+      expect(scope.hasFocus, isTrue);
+
+      await tester.tap(find.text('Go Forward'));
+      await tester.pumpAndSettle();
+
+      final Element secondChild = tester.element(find.text('Go Back'));
+      final FocusNode secondFocusNode = Focus.of(secondChild);
+
+      expect(firstFocusNode.hasFocus, isFalse);
+      expect(secondFocusNode.hasFocus, isTrue);
+
+      await tester.tap(find.text('Go Back'));
+      await tester.pumpAndSettle();
+
+      expect(firstFocusNode.hasFocus, isTrue);
+      expect(scope.hasFocus, isTrue);
+    });
   });
   group(ReadingOrderTraversalPolicy, () {
     testWidgets('Find the initial focus if there is none yet.', (WidgetTester tester) async {