From 5900c4baa751aff8f05e820287a02b60cdd62dfa Mon Sep 17 00:00:00 2001 From: chunhtai <47866232+chunhtai@users.noreply.github.com> Date: Tue, 12 Sep 2023 09:18:40 -0700 Subject: [PATCH] =?UTF-8?q?Revert=20"Adds=20a=20parent=20scope=20Traversal?= =?UTF-8?q?EdgeBehavior=20and=20fixes=20modal=20rou=E2=80=A6=20(#134550)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …te to no… (#130841)" This reverts commit 0f3bd90d9b108904cc0390e899c2b3c6ee2e76e0. Internal test needs migration ## 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, 35 insertions(+), 254 deletions(-) diff --git a/packages/flutter/lib/src/widgets/app.dart b/packages/flutter/lib/src/widgets/app.dart index 7a524639fb4..d8d649c103a 100644 --- a/packages/flutter/lib/src/widgets/app.dart +++ b/packages/flutter/lib/src/widgets/app.dart @@ -1685,7 +1685,6 @@ 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 f67080c71a0..203e1325295 100644 --- a/packages/flutter/lib/src/widgets/focus_traversal.dart +++ b/packages/flutter/lib/src/widgets/focus_traversal.dart @@ -120,16 +120,6 @@ 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]. @@ -196,60 +186,6 @@ 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. /// @@ -416,21 +352,10 @@ abstract class FocusTraversalPolicy with Diagnosticable { @protected Iterable sortDescendants(Iterable descendants, 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) { + Map _findGroups(FocusScopeNode scope, _FocusTraversalGroupNode? scopeGroupNode, FocusNode currentNode) { final FocusTraversalPolicy defaultPolicy = scopeGroupNode?.policy ?? ReadingOrderTraversalPolicy(); final Map groups = {}; - for (final FocusNode node in _getDescendantsWithoutExpandingScope(scope)) { + for (final FocusNode node in scope.descendants) { 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 @@ -463,7 +388,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. - static List _sortAllDescendants(FocusScopeNode scope, FocusNode currentNode) { + 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); @@ -548,42 +473,30 @@ abstract class FocusTraversalPolicy with Diagnosticable { if (focusedChild == null) { final FocusNode? firstFocus = forward ? findFirstFocus(currentNode) : findLastFocus(currentNode); if (firstFocus != null) { - return _requestTabTraversalFocus( + requestFocusCallback( firstFocus, alignmentPolicy: forward ? ScrollPositionAlignmentPolicy.keepVisibleAtEnd : ScrollPositionAlignmentPolicy.keepVisibleAtStart, - forward: forward, ); + return true; } } focusedChild ??= nearestScope; final List sortedNodes = _sortAllDescendants(nearestScope, focusedChild); - assert(sortedNodes.contains(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: - return _requestTabTraversalFocus( - sortedNodes.first, - alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd, - forward: forward, - ); + requestFocusCallback(sortedNodes.first, alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd); + return true; } } if (!forward && focusedChild == sortedNodes.first) { @@ -591,26 +504,9 @@ 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: - return _requestTabTraversalFocus( - sortedNodes.last, - alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart, - forward: forward, - ); + requestFocusCallback(sortedNodes.last, alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart); + return true; } } @@ -618,11 +514,11 @@ abstract class FocusTraversalPolicy with Diagnosticable { FocusNode? previousNode; for (final FocusNode node in maybeFlipped) { if (previousNode == focusedChild) { - return _requestTabTraversalFocus( + requestFocusCallback( 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 bc57d8a3919..ccf3fbfb56a 100644 --- a/packages/flutter/lib/src/widgets/navigator.dart +++ b/packages/flutter/lib/src/widgets/navigator.dart @@ -1146,7 +1146,9 @@ class DefaultTransitionDelegate extends TransitionDelegate { /// The default value of [Navigator.routeTraversalEdgeBehavior]. /// /// {@macro flutter.widgets.navigator.routeTraversalEdgeBehavior} -const TraversalEdgeBehavior kDefaultRouteTraversalEdgeBehavior = TraversalEdgeBehavior.parentScope; +const TraversalEdgeBehavior kDefaultRouteTraversalEdgeBehavior = kIsWeb + ? TraversalEdgeBehavior.leaveFlutterView + : TraversalEdgeBehavior.closedLoop; /// 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 fc954fa0a1a..b51cb4f61c7 100644 --- a/packages/flutter/lib/src/widgets/routes.dart +++ b/packages/flutter/lib/src/widgets/routes.dart @@ -834,9 +834,7 @@ 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 @@ -938,8 +936,6 @@ 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 @@ -1708,26 +1704,11 @@ 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(); - // 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(); - } + 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 611a62cdfef..44da15a62e6 100644 --- a/packages/flutter/test/material/icon_button_test.dart +++ b/packages/flutter/test/material/icon_button_test.dart @@ -4,7 +4,6 @@ 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'; @@ -793,10 +792,6 @@ 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( @@ -826,8 +821,11 @@ void main() { expect(focusNode1.nextFocus(), isFalse); await tester.pump(); - expect(focusNode1.hasPrimaryFocus, !kIsWeb); + expect(focusNode1.hasPrimaryFocus, isTrue); 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 38f548ee210..b0ee37f4082 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, isFalse); + expect(buttonNode1.hasFocus, isTrue); 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 2ce49be74e9..d8ccbbc8389 100644 --- a/packages/flutter/test/widgets/focus_traversal_test.dart +++ b/packages/flutter/test/widgets/focus_traversal_test.dart @@ -441,96 +441,6 @@ 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 95111931e34..886de4a7126 100644 --- a/packages/flutter/test/widgets/navigator_test.dart +++ b/packages/flutter/test/widgets/navigator_test.dart @@ -1487,34 +1487,29 @@ void main() { return result; }, )); - final List expected = ['building page 1 - false']; - expect(log, expected); + expect(log, ['building page 1 - false']); 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, expected); + expect(log, ['building page 1 - false']); await tester.pump(); - expected.add('building page 2 - false'); - expected.add('building page 1 - false'); // page 1 is rebuilt again because isCurrent changed. - expect(log, expected); + expect(log, ['building page 1 - false', 'building page 2 - false']); await tester.pump(const Duration(milliseconds: 150)); - expect(log, expected); + expect(log, ['building page 1 - false', 'building page 2 - false']); 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, expected); + expect(log, ['building page 1 - false', 'building page 2 - false']); await tester.pump(); - expected.add('building page 3 - false'); - expected.add('building page 2 - false'); // page 2 is rebuilt again because isCurrent changed. - expect(log, expected); + expect(log, ['building page 1 - false', 'building page 2 - false', 'building page 3 - false']); await tester.pump(const Duration(milliseconds: 200)); - expect(log, expected); + expect(log, ['building page 1 - false', 'building page 2 - false', 'building page 3 - false']); }); testWidgets('route semantics', (WidgetTester tester) async {