Adds a parent scope TraversalEdgeBehavior and fixes modal route to no… (#130841)

…t trap the focus

fixes https://github.com/flutter/flutter/issues/112567

Several things I done:

1. add new enum `parentScope`
2. refactor _sortAllDescendants so that it doesn't recursively looking
into its descendant when it encounter a FocusScopeNode.
3. When the nextFocus try to focus into a FocusScopeNode, it will try to
find the first focusable FocusNode in traversal order and focus it
instead if it doesn't have a first focus.
4. Change the default in Navigator to always be `parentScope`
5. Only the root navigator will use `leaveFlutterView` if platform is
web.


Previously 2 and 3 are not needed because once a focusscope trapped the
focus, there isn't a chance where the parent focusscope have to deal
with next focus.

If we don't do 2 and 3 after the change, it will cause it to stuck in
the current scope again. Because once the focus leave the current scope,
it needs to also remove the first focus in that scope (so that it can
start fresh when focus traversal back to the scope in the future). At
this point the current scope will have the primary focus without the
first focus. The parent scope will then try to find the next focus, and
it will place the focus to the first traversal child in the current
scope again.

## Pre-launch Checklist

- [ ] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ ] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [ ] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [ ] I signed the [CLA].
- [ ] I listed at least one issue that this PR fixes in the description
above.
- [ ] I updated/added relevant documentation (doc comments with `///`).
- [ ] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [ ] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat

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

@@ -1685,6 +1685,7 @@ class _WidgetsAppState extends State<WidgetsApp> with WidgetsBindingObserver {
             },
           onUnknownRoute: _onUnknownRoute,
           observers: widget.navigatorObservers!,
+          routeTraversalEdgeBehavior: kIsWeb ? TraversalEdgeBehavior.leaveFlutterView : TraversalEdgeBehavior.parentScope,
           reportsRouteUpdateToEngine: true,
         ),
       );

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

@@ -120,6 +120,16 @@ enum TraversalEdgeBehavior {
   /// address bar, escape an `iframe`, or focus on HTML elements other than
   /// those managed by Flutter.
   leaveFlutterView,
+
+  /// Allows focus to traverse up to parent scope.
+  ///
+  /// When reaching the edge of the current scope, requesting the next focus
+  /// will look up to the parent scope of the current scope and focus the focus
+  /// node next to the current scope.
+  ///
+  /// If there is no parent scope above the current scope, fallback to
+  /// [closedLoop] behavior.
+  parentScope,
 }
 
 /// Determines how focusable widgets are traversed within a [FocusTraversalGroup].
@@ -186,6 +196,60 @@ abstract class FocusTraversalPolicy with Diagnosticable {
     );
   }
 
+  /// Request focus on a focus node as a result of a tab traversal.
+  ///
+  /// If the `node` is a [FocusScopeNode], this method will recursively find
+  /// the next focus from its descendants until it find a regular [FocusNode].
+  ///
+  /// Returns true if this method focused a new focus node.
+  bool _requestTabTraversalFocus(
+    FocusNode node, {
+    ScrollPositionAlignmentPolicy? alignmentPolicy,
+    double? alignment,
+    Duration? duration,
+    Curve? curve,
+    required bool forward,
+  }) {
+    if (node is FocusScopeNode) {
+      if (node.focusedChild != null) {
+        // Can't stop here as the `focusedChild` may be a focus scope node
+        // without a first focus. The first focus will be picked in the
+        // next iteration.
+        return _requestTabTraversalFocus(
+          node.focusedChild!,
+          alignmentPolicy: alignmentPolicy,
+          alignment: alignment,
+          duration: duration,
+          curve: curve,
+          forward: forward,
+        );
+      }
+      final List<FocusNode> sortedChildren = _sortAllDescendants(node, node);
+      if (sortedChildren.isNotEmpty) {
+        _requestTabTraversalFocus(
+          forward ? sortedChildren.first : sortedChildren.last,
+          alignmentPolicy: alignmentPolicy,
+          alignment: alignment,
+          duration: duration,
+          curve: curve,
+          forward: forward,
+        );
+        // Regardless if _requestTabTraversalFocus return true or false, a first
+        // focus has been picked.
+        return true;
+      }
+    }
+    final bool nodeHadPrimaryFocus = node.hasPrimaryFocus;
+    requestFocusCallback(
+      node,
+      alignmentPolicy: alignmentPolicy,
+      alignment: alignment,
+      duration: duration,
+      curve: curve,
+    );
+    return !nodeHadPrimaryFocus;
+  }
+
   /// Returns the node that should receive focus if focus is traversing
   /// forwards, and there is no current focus.
   ///
@@ -352,10 +416,21 @@ abstract class FocusTraversalPolicy with Diagnosticable {
   @protected
   Iterable<FocusNode> sortDescendants(Iterable<FocusNode> descendants, FocusNode currentNode);
 
-  Map<FocusNode?, _FocusTraversalGroupInfo> _findGroups(FocusScopeNode scope, _FocusTraversalGroupNode? scopeGroupNode, FocusNode currentNode) {
+  static Iterable<FocusNode> _getDescendantsWithoutExpandingScope(FocusNode node) {
+    final List<FocusNode> result = <FocusNode>[];
+    for (final FocusNode child in node.children) {
+      if (child is! FocusScopeNode) {
+        result.addAll(_getDescendantsWithoutExpandingScope(child));
+      }
+      result.add(child);
+    }
+    return result;
+  }
+
+  static Map<FocusNode?, _FocusTraversalGroupInfo> _findGroups(FocusScopeNode scope, _FocusTraversalGroupNode? scopeGroupNode, FocusNode currentNode) {
     final FocusTraversalPolicy defaultPolicy = scopeGroupNode?.policy ?? ReadingOrderTraversalPolicy();
     final Map<FocusNode?, _FocusTraversalGroupInfo> groups = <FocusNode?, _FocusTraversalGroupInfo>{};
-    for (final FocusNode node in scope.descendants) {
+    for (final FocusNode node in _getDescendantsWithoutExpandingScope(scope)) {
       final _FocusTraversalGroupNode? groupNode = FocusTraversalGroup._getGroupNode(node);
       // Group nodes need to be added to their parent's node, or to the "null"
       // node if no parent is found. This creates the hierarchy of group nodes
@@ -388,7 +463,7 @@ abstract class FocusTraversalPolicy with Diagnosticable {
 
   // Sort all descendants, taking into account the FocusTraversalGroup
   // that they are each in, and filtering out non-traversable/focusable nodes.
-  List<FocusNode> _sortAllDescendants(FocusScopeNode scope, FocusNode currentNode) {
+  static List<FocusNode> _sortAllDescendants(FocusScopeNode scope, FocusNode currentNode) {
     final _FocusTraversalGroupNode? scopeGroupNode = FocusTraversalGroup._getGroupNode(scope);
     // Build the sorting data structure, separating descendants into groups.
     final Map<FocusNode?, _FocusTraversalGroupInfo> groups = _findGroups(scope, scopeGroupNode, currentNode);
@@ -473,30 +548,42 @@ abstract class FocusTraversalPolicy with Diagnosticable {
     if (focusedChild == null) {
       final FocusNode? firstFocus = forward ? findFirstFocus(currentNode) : findLastFocus(currentNode);
       if (firstFocus != null) {
-        requestFocusCallback(
+        return _requestTabTraversalFocus(
           firstFocus,
           alignmentPolicy: forward ? ScrollPositionAlignmentPolicy.keepVisibleAtEnd : ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+          forward: forward,
         );
-        return true;
       }
     }
     focusedChild ??= nearestScope;
     final List<FocusNode> sortedNodes = _sortAllDescendants(nearestScope, focusedChild);
-
     assert(sortedNodes.contains(focusedChild));
-    if (sortedNodes.length < 2) {
-      // If there are no nodes to traverse to, like when descendantsAreTraversable
-      // is false or skipTraversal for all the nodes is true.
-      return false;
-    }
+
     if (forward && focusedChild == sortedNodes.last) {
       switch (nearestScope.traversalEdgeBehavior) {
         case TraversalEdgeBehavior.leaveFlutterView:
           focusedChild.unfocus();
           return false;
+        case TraversalEdgeBehavior.parentScope:
+          final FocusScopeNode? parentScope = nearestScope.enclosingScope;
+          if (parentScope != null && parentScope != FocusManager.instance.rootScope) {
+            focusedChild.unfocus();
+            parentScope.nextFocus();
+            // Verify the focus really has changed.
+            return focusedChild.enclosingScope?.focusedChild != focusedChild;
+          }
+          // No valid parent scope. Fallback to closed loop behavior.
+          return _requestTabTraversalFocus(
+            sortedNodes.first,
+            alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd,
+            forward: forward,
+          );
         case TraversalEdgeBehavior.closedLoop:
-          requestFocusCallback(sortedNodes.first, alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd);
-          return true;
+          return _requestTabTraversalFocus(
+            sortedNodes.first,
+            alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd,
+            forward: forward,
+          );
       }
     }
     if (!forward && focusedChild == sortedNodes.first) {
@@ -504,9 +591,26 @@ abstract class FocusTraversalPolicy with Diagnosticable {
         case TraversalEdgeBehavior.leaveFlutterView:
           focusedChild.unfocus();
           return false;
+        case TraversalEdgeBehavior.parentScope:
+          final FocusScopeNode? parentScope = nearestScope.enclosingScope;
+          if (parentScope != null && parentScope != FocusManager.instance.rootScope) {
+            focusedChild.unfocus();
+            parentScope.previousFocus();
+            // Verify the focus really has changed.
+            return focusedChild.enclosingScope?.focusedChild != focusedChild;
+          }
+          // No valid parent scope. Fallback to closed loop behavior.
+          return _requestTabTraversalFocus(
+            sortedNodes.last,
+            alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+            forward: forward,
+          );
         case TraversalEdgeBehavior.closedLoop:
-          requestFocusCallback(sortedNodes.last, alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart);
-          return true;
+          return _requestTabTraversalFocus(
+            sortedNodes.last,
+            alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+            forward: forward,
+          );
       }
     }
 
@@ -514,11 +618,11 @@ abstract class FocusTraversalPolicy with Diagnosticable {
     FocusNode? previousNode;
     for (final FocusNode node in maybeFlipped) {
       if (previousNode == focusedChild) {
-        requestFocusCallback(
+        return _requestTabTraversalFocus(
           node,
           alignmentPolicy: forward ? ScrollPositionAlignmentPolicy.keepVisibleAtEnd : ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+          forward: forward,
         );
-        return true;
       }
       previousNode = node;
     }

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

@@ -1146,9 +1146,7 @@ class DefaultTransitionDelegate<T> extends TransitionDelegate<T> {
 /// The default value of [Navigator.routeTraversalEdgeBehavior].
 ///
 /// {@macro flutter.widgets.navigator.routeTraversalEdgeBehavior}
-const TraversalEdgeBehavior kDefaultRouteTraversalEdgeBehavior = kIsWeb
-  ? TraversalEdgeBehavior.leaveFlutterView
-  : TraversalEdgeBehavior.closedLoop;
+const TraversalEdgeBehavior kDefaultRouteTraversalEdgeBehavior = TraversalEdgeBehavior.parentScope;
 
 /// A widget that manages a set of child widgets with a stack discipline.
 ///

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

@@ -834,7 +834,9 @@ class _ModalScopeState<T> extends State<_ModalScope<T>> {
   late Listenable _listenable;
 
   /// The node this scope will use for its root [FocusScope] widget.
-  final FocusScopeNode focusScopeNode = FocusScopeNode(debugLabel: '$_ModalScopeState Focus Scope');
+  final FocusScopeNode focusScopeNode = FocusScopeNode(
+    debugLabel: '$_ModalScopeState Focus Scope',
+  );
   final ScrollController primaryScrollController = ScrollController();
 
   @override
@@ -936,6 +938,8 @@ class _ModalScopeState<T> extends State<_ModalScope<T>> {
                     controller: primaryScrollController,
                     child: FocusScope(
                       node: focusScopeNode, // immutable
+                      // Only top most route can participate in focus traversal.
+                      skipTraversal: !widget.route.isCurrent,
                       child: RepaintBoundary(
                         child: AnimatedBuilder(
                           animation: _listenable, // immutable
@@ -1704,11 +1708,26 @@ abstract class ModalRoute<T> extends TransitionRoute<T> with LocalHistoryRoute<T
     changedInternalState();
   }
 
+  @override
+  void didChangeNext(Route<dynamic>? nextRoute) {
+    super.didChangeNext(nextRoute);
+    changedInternalState();
+  }
+
+  @override
+  void didPopNext(Route<dynamic> nextRoute) {
+    super.didPopNext(nextRoute);
+    changedInternalState();
+  }
+
   @override
   void changedInternalState() {
     super.changedInternalState();
-    setState(() { /* internal state already changed */ });
-    _modalBarrier.markNeedsBuild();
+    // No need to mark dirty if this method is called during build phase.
+    if (SchedulerBinding.instance.schedulerPhase != SchedulerPhase.persistentCallbacks) {
+      setState(() { /* internal state already changed */ });
+      _modalBarrier.markNeedsBuild();
+    }
     _modalScope.maintainState = maintainState;
   }
 

packages/flutter/test/material/icon_button_test.dart

@@ -4,6 +4,7 @@
 
 import 'dart:ui';
 
+import 'package:flutter/foundation.dart';
 import 'package:flutter/material.dart';
 import 'package:flutter/rendering.dart';
 import 'package:flutter_test/flutter_test.dart';
@@ -792,6 +793,10 @@ void main() {
   testWidgetsWithLeakTracking("Disabled IconButton can't be traversed to when disabled.", (WidgetTester tester) async {
     final FocusNode focusNode1 = FocusNode(debugLabel: 'IconButton 1');
     final FocusNode focusNode2 = FocusNode(debugLabel: 'IconButton 2');
+    addTearDown(() {
+      focusNode1.dispose();
+      focusNode2.dispose();
+    });
 
     await tester.pumpWidget(
       wrap(
@@ -821,11 +826,8 @@ void main() {
     expect(focusNode1.nextFocus(), isFalse);
     await tester.pump();
 
-    expect(focusNode1.hasPrimaryFocus, isTrue);
+    expect(focusNode1.hasPrimaryFocus, !kIsWeb);
     expect(focusNode2.hasPrimaryFocus, isFalse);
-
-    focusNode1.dispose();
-    focusNode2.dispose();
   });
 
   group('feedback', () {

packages/flutter/test/widgets/actions_test.dart

@@ -965,7 +965,7 @@ void main() {
         expect(buttonNode2.hasFocus, isFalse);
         primaryFocus!.nextFocus();
         await tester.pump();
-        expect(buttonNode1.hasFocus, isTrue);
+        expect(buttonNode1.hasFocus, isFalse);
         expect(buttonNode2.hasFocus, isFalse);
       },
     );

packages/flutter/test/widgets/focus_traversal_test.dart

@@ -430,6 +430,92 @@ void main() {
 
   });
 
+  testWidgets('Nested navigator does not trap focus', (WidgetTester tester) async {
+    final FocusNode node1 = FocusNode();
+    final FocusNode node2 = FocusNode();
+    final FocusNode node3 = FocusNode();
+    await tester.pumpWidget(
+      Directionality(
+        textDirection: TextDirection.ltr,
+        child: FocusTraversalGroup(
+          policy: ReadingOrderTraversalPolicy(),
+          child: FocusScope(
+            child: Column(
+              children: <Widget>[
+                Focus(
+                  focusNode: node1,
+                  child: const SizedBox(width: 100, height: 100),
+                ),
+                SizedBox(
+                  width: 100,
+                  height: 100,
+                  child: Navigator(
+                    pages: <Page<void>>[
+                      MaterialPage<void>(
+                        child: Focus(
+                          focusNode: node2,
+                          child: const SizedBox(width: 100, height: 100),
+                        ),
+                      ),
+                    ],
+                    onPopPage: (_, __) => false,
+                  ),
+                ),
+                Focus(
+                  focusNode: node3,
+                  child: const SizedBox(width: 100, height: 100),
+                ),
+              ],
+            ),
+          ),
+        ),
+      ),
+    );
+
+    node1.requestFocus();
+    await tester.pump();
+
+    expect(node1.hasFocus, isTrue);
+    expect(node2.hasFocus, isFalse);
+    expect(node3.hasFocus, isFalse);
+
+    node1.nextFocus();
+    await tester.pump();
+    expect(node1.hasFocus, isFalse);
+    expect(node2.hasFocus, isTrue);
+    expect(node3.hasFocus, isFalse);
+
+    node2.nextFocus();
+    await tester.pump();
+    expect(node1.hasFocus, isFalse);
+    expect(node2.hasFocus, isFalse);
+    expect(node3.hasFocus, isTrue);
+
+    node3.nextFocus();
+    await tester.pump();
+    expect(node1.hasFocus, isTrue);
+    expect(node2.hasFocus, isFalse);
+    expect(node3.hasFocus, isFalse);
+
+    node1.previousFocus();
+    await tester.pump();
+    expect(node1.hasFocus, isFalse);
+    expect(node2.hasFocus, isFalse);
+    expect(node3.hasFocus, isTrue);
+
+    node3.previousFocus();
+    await tester.pump();
+    expect(node1.hasFocus, isFalse);
+    expect(node2.hasFocus, isTrue);
+    expect(node3.hasFocus, isFalse);
+
+    node2.previousFocus();
+    await tester.pump();
+    expect(node1.hasFocus, isTrue);
+    expect(node2.hasFocus, isFalse);
+    expect(node3.hasFocus, isFalse);
+  });
+
   group(ReadingOrderTraversalPolicy, () {
     testWidgets('Find the initial focus if there is none yet.', (WidgetTester tester) async {
       final GlobalKey key1 = GlobalKey(debugLabel: '1');

packages/flutter/test/widgets/navigator_test.dart

@@ -1487,29 +1487,34 @@ void main() {
         return result;
       },
     ));
-    expect(log, <String>['building page 1 - false']);
+    final List<String> expected = <String>['building page 1 - false'];
+    expect(log, expected);
     key.currentState!.pushReplacement(PageRouteBuilder<int>(
       pageBuilder: (BuildContext context, Animation<double> animation, Animation<double> secondaryAnimation) {
         log.add('building page 2 - ${ModalRoute.of(context)!.canPop}');
         return const Placeholder();
       },
     ));
-    expect(log, <String>['building page 1 - false']);
+    expect(log, expected);
     await tester.pump();
-    expect(log, <String>['building page 1 - false', 'building page 2 - false']);
+    expected.add('building page 2 - false');
+    expected.add('building page 1 - false'); // page 1 is rebuilt again because isCurrent changed.
+    expect(log, expected);
     await tester.pump(const Duration(milliseconds: 150));
-    expect(log, <String>['building page 1 - false', 'building page 2 - false']);
+    expect(log, expected);
     key.currentState!.pushReplacement(PageRouteBuilder<int>(
       pageBuilder: (BuildContext context, Animation<double> animation, Animation<double> secondaryAnimation) {
         log.add('building page 3 - ${ModalRoute.of(context)!.canPop}');
         return const Placeholder();
       },
     ));
-    expect(log, <String>['building page 1 - false', 'building page 2 - false']);
+    expect(log, expected);
     await tester.pump();
-    expect(log, <String>['building page 1 - false', 'building page 2 - false', 'building page 3 - false']);
+    expected.add('building page 3 - false');
+    expected.add('building page 2 - false'); // page 2 is rebuilt again because isCurrent changed.
+    expect(log, expected);
     await tester.pump(const Duration(milliseconds: 200));
-    expect(log, <String>['building page 1 - false', 'building page 2 - false', 'building page 3 - false']);
+    expect(log, expected);
   });
 
   testWidgets('route semantics', (WidgetTester tester) async {