From 0f3bd90d9b108904cc0390e899c2b3c6ee2e76e0 Mon Sep 17 00:00:00 2001 From: chunhtai <47866232+chunhtai@users.noreply.github.com> Date: Fri, 1 Sep 2023 08:37:38 -0700 Subject: [PATCH] =?UTF-8?q?Adds=20a=20parent=20scope=20TraversalEdgeBehavi?= =?UTF-8?q?or=20and=20fixes=20modal=20route=20to=20no=E2=80=A6=20(#130841)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …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]. [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 | 1 + .../lib/src/widgets/focus_traversal.dart | 138 +++++++++++++++--- .../flutter/lib/src/widgets/navigator.dart | 4 +- packages/flutter/lib/src/widgets/routes.dart | 25 +++- .../test/material/icon_button_test.dart | 10 +- .../flutter/test/widgets/actions_test.dart | 2 +- .../test/widgets/focus_traversal_test.dart | 86 +++++++++++ .../flutter/test/widgets/navigator_test.dart | 19 ++- 8 files changed, 250 insertions(+), 35 deletions(-) diff --git a/packages/flutter/lib/src/widgets/app.dart b/packages/flutter/lib/src/widgets/app.dart index d8d649c103a4..7a524639fb42 100644 --- a/packages/flutter/lib/src/widgets/app.dart +++ b/packages/flutter/lib/src/widgets/app.dart @@ -1685,6 +1685,7 @@ class _WidgetsAppState extends State with WidgetsBindingObserver { }, onUnknownRoute: _onUnknownRoute, observers: widget.navigatorObservers!, + routeTraversalEdgeBehavior: kIsWeb ? TraversalEdgeBehavior.leaveFlutterView : TraversalEdgeBehavior.parentScope, reportsRouteUpdateToEngine: true, ), ); diff --git a/packages/flutter/lib/src/widgets/focus_traversal.dart b/packages/flutter/lib/src/widgets/focus_traversal.dart index 203e13252958..f67080c71a02 100644 --- a/packages/flutter/lib/src/widgets/focus_traversal.dart +++ b/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 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 sortDescendants(Iterable descendants, FocusNode currentNode); - Map _findGroups(FocusScopeNode scope, _FocusTraversalGroupNode? scopeGroupNode, FocusNode currentNode) { + static Iterable _getDescendantsWithoutExpandingScope(FocusNode node) { + final List result = []; + for (final FocusNode child in node.children) { + if (child is! FocusScopeNode) { + result.addAll(_getDescendantsWithoutExpandingScope(child)); + } + result.add(child); + } + return result; + } + + static Map _findGroups(FocusScopeNode scope, _FocusTraversalGroupNode? scopeGroupNode, FocusNode currentNode) { final FocusTraversalPolicy defaultPolicy = scopeGroupNode?.policy ?? ReadingOrderTraversalPolicy(); final Map groups = {}; - 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 _sortAllDescendants(FocusScopeNode scope, FocusNode currentNode) { + static List _sortAllDescendants(FocusScopeNode scope, FocusNode currentNode) { final _FocusTraversalGroupNode? scopeGroupNode = FocusTraversalGroup._getGroupNode(scope); // Build the sorting data structure, separating descendants into groups. final Map 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 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; } diff --git a/packages/flutter/lib/src/widgets/navigator.dart b/packages/flutter/lib/src/widgets/navigator.dart index ccf3fbfb56ad..bc57d8a39198 100644 --- a/packages/flutter/lib/src/widgets/navigator.dart +++ b/packages/flutter/lib/src/widgets/navigator.dart @@ -1146,9 +1146,7 @@ class DefaultTransitionDelegate extends TransitionDelegate { /// 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. /// diff --git a/packages/flutter/lib/src/widgets/routes.dart b/packages/flutter/lib/src/widgets/routes.dart index b51cb4f61c7c..fc954fa0a1a2 100644 --- a/packages/flutter/lib/src/widgets/routes.dart +++ b/packages/flutter/lib/src/widgets/routes.dart @@ -834,7 +834,9 @@ class _ModalScopeState extends State<_ModalScope> { 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 extends State<_ModalScope> { 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 extends TransitionRoute with LocalHistoryRoute? nextRoute) { + super.didChangeNext(nextRoute); + changedInternalState(); + } + + @override + void didPopNext(Route 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; } diff --git a/packages/flutter/test/material/icon_button_test.dart b/packages/flutter/test/material/icon_button_test.dart index fce7855170a5..02c6ad08d427 100644 --- a/packages/flutter/test/material/icon_button_test.dart +++ b/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', () { diff --git a/packages/flutter/test/widgets/actions_test.dart b/packages/flutter/test/widgets/actions_test.dart index b59f17f8eafc..6a246304e3f8 100644 --- a/packages/flutter/test/widgets/actions_test.dart +++ b/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); }, ); diff --git a/packages/flutter/test/widgets/focus_traversal_test.dart b/packages/flutter/test/widgets/focus_traversal_test.dart index ff0b8c972974..04cf497d08da 100644 --- a/packages/flutter/test/widgets/focus_traversal_test.dart +++ b/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: [ + Focus( + focusNode: node1, + child: const SizedBox(width: 100, height: 100), + ), + SizedBox( + width: 100, + height: 100, + child: Navigator( + pages: >[ + MaterialPage( + 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'); diff --git a/packages/flutter/test/widgets/navigator_test.dart b/packages/flutter/test/widgets/navigator_test.dart index 886de4a71269..95111931e34d 100644 --- a/packages/flutter/test/widgets/navigator_test.dart +++ b/packages/flutter/test/widgets/navigator_test.dart @@ -1487,29 +1487,34 @@ void main() { return result; }, )); - expect(log, ['building page 1 - false']); + final List expected = ['building page 1 - false']; + expect(log, expected); key.currentState!.pushReplacement(PageRouteBuilder( pageBuilder: (BuildContext context, Animation animation, Animation secondaryAnimation) { log.add('building page 2 - ${ModalRoute.of(context)!.canPop}'); return const Placeholder(); }, )); - expect(log, ['building page 1 - false']); + expect(log, expected); await tester.pump(); - expect(log, ['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, ['building page 1 - false', 'building page 2 - false']); + expect(log, expected); key.currentState!.pushReplacement(PageRouteBuilder( pageBuilder: (BuildContext context, Animation animation, Animation secondaryAnimation) { log.add('building page 3 - ${ModalRoute.of(context)!.canPop}'); return const Placeholder(); }, )); - expect(log, ['building page 1 - false', 'building page 2 - false']); + expect(log, expected); await tester.pump(); - expect(log, ['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, ['building page 1 - false', 'building page 2 - false', 'building page 3 - false']); + expect(log, expected); }); testWidgets('route semantics', (WidgetTester tester) async {