From 87845c0b9385a878c20034efa572b61581674b25 Mon Sep 17 00:00:00 2001 From: chunhtai <47866232+chunhtai@users.noreply.github.com> Date: Mon, 9 Oct 2023 09:10:23 -0700 Subject: [PATCH] =?UTF-8?q?Reland=20"Adds=20a=20parent=20scope=20Traversal?= =?UTF-8?q?EdgeBehavior=20and=20fixes=20modal=20rou=E2=80=A6=20(#134554)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …… (#134550)" fixes https://github.com/flutter/flutter/issues/112567 This reverts commit 5900c4baa751aff8f05e820287a02b60cdd62dfa. The internal test needs migration. cl/564746935 This is the same of original pr, no new change ## 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 | 90 ++++++++++++ .../flutter/test/widgets/navigator_test.dart | 19 ++- 8 files changed, 254 insertions(+), 35 deletions(-) diff --git a/packages/flutter/lib/src/widgets/app.dart b/packages/flutter/lib/src/widgets/app.dart index b6b4b9647bfcd..2f34a38762c30 100644 --- a/packages/flutter/lib/src/widgets/app.dart +++ b/packages/flutter/lib/src/widgets/app.dart @@ -1683,6 +1683,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 1212e27c8247a..6ecb5c789183e 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. /// @@ -340,10 +404,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 @@ -376,7 +451,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); @@ -463,30 +538,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) { @@ -494,9 +581,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, + ); } } @@ -504,11 +608,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 3b78bd7387d7b..7394a5260d8cd 100644 --- a/packages/flutter/lib/src/widgets/navigator.dart +++ b/packages/flutter/lib/src/widgets/navigator.dart @@ -1144,9 +1144,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 6edc488d03765..7d8729e661c26 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 44da15a62e6ec..611a62cdfef85 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 b0ee37f40828c..38f548ee210b0 100644 --- a/packages/flutter/test/widgets/actions_test.dart +++ b/packages/flutter/test/widgets/actions_test.dart @@ -981,7 +981,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 5b2abf4cf25ea..e26a3200bc5d8 100644 --- a/packages/flutter/test/widgets/focus_traversal_test.dart +++ b/packages/flutter/test/widgets/focus_traversal_test.dart @@ -441,6 +441,96 @@ void main() { }); + testWidgetsWithLeakTracking('Nested navigator does not trap focus', (WidgetTester tester) async { + final FocusNode node1 = FocusNode(); + addTearDown(node1.dispose); + final FocusNode node2 = FocusNode(); + addTearDown(node2.dispose); + final FocusNode node3 = FocusNode(); + addTearDown(node3.dispose); + + 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, () { testWidgetsWithLeakTracking('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 9b28b0923afa9..e01d3aa280039 100644 --- a/packages/flutter/test/widgets/navigator_test.dart +++ b/packages/flutter/test/widgets/navigator_test.dart @@ -1489,29 +1489,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); }); testWidgetsWithLeakTracking('route semantics', (WidgetTester tester) async {