From 4763be745d16635cb7ea16bd4e364bee1edf4d5f Mon Sep 17 00:00:00 2001 From: chunhtai <47866232+chunhtai@users.noreply.github.com> Date: Wed, 19 Jul 2023 18:01:01 -0700 Subject: [PATCH] Can traverse if current focused node skips traversal (#130812) Currently if the focus is on a focusnode that `skipTraversal = true`, the tab won't be able to traverse focus out of the node. this pr fixes it --- .../lib/src/widgets/focus_traversal.dart | 47 +++++++++++----- .../test/material/icon_button_test.dart | 2 +- .../test/material/input_chip_test.dart | 2 +- .../material/raw_material_button_test.dart | 2 +- .../test/material/text_field_test.dart | 2 +- .../flutter/test/widgets/actions_test.dart | 2 + .../test/widgets/focus_traversal_test.dart | 55 +++++++++++++++++++ 7 files changed, 94 insertions(+), 18 deletions(-) diff --git a/packages/flutter/lib/src/widgets/focus_traversal.dart b/packages/flutter/lib/src/widgets/focus_traversal.dart index 33a83b0b7a..9cafbca24d 100644 --- a/packages/flutter/lib/src/widgets/focus_traversal.dart +++ b/packages/flutter/lib/src/widgets/focus_traversal.dart @@ -352,7 +352,7 @@ abstract class FocusTraversalPolicy with Diagnosticable { @protected Iterable sortDescendants(Iterable descendants, FocusNode currentNode); - Map _findGroups(FocusScopeNode scope, _FocusTraversalGroupNode? scopeGroupNode) { + Map _findGroups(FocusScopeNode scope, _FocusTraversalGroupNode? scopeGroupNode, FocusNode currentNode) { final FocusTraversalPolicy defaultPolicy = scopeGroupNode?.policy ?? ReadingOrderTraversalPolicy(); final Map groups = {}; for (final FocusNode node in scope.descendants) { @@ -374,7 +374,10 @@ abstract class FocusTraversalPolicy with Diagnosticable { } // Skip non-focusable and non-traversable nodes in the same way that // FocusScopeNode.traversalDescendants would. - if (node.canRequestFocus && !node.skipTraversal) { + // + // Current focused node needs to be in the group so that the caller can + // find the next traversable node from the current focused node. + if (node == currentNode || (node.canRequestFocus && !node.skipTraversal)) { groups[groupNode] ??= _FocusTraversalGroupInfo(groupNode, members: [], defaultPolicy: defaultPolicy); assert(!groups[groupNode]!.members.contains(node)); groups[groupNode]!.members.add(node); @@ -388,7 +391,7 @@ abstract class FocusTraversalPolicy with Diagnosticable { 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); + final Map groups = _findGroups(scope, scopeGroupNode, currentNode); // Sort the member lists using the individual policy sorts. for (final FocusNode? key in groups.keys) { @@ -397,6 +400,7 @@ abstract class FocusTraversalPolicy with Diagnosticable { groups[key]!.members.addAll(sortedMembers); } + // Traverse the group tree, adding the children of members in the order they // appear in the member lists. final List sortedDescendants = []; @@ -421,17 +425,29 @@ abstract class FocusTraversalPolicy with Diagnosticable { // They were left in above because they were needed to find their members // during sorting. sortedDescendants.removeWhere((FocusNode node) { - return !node.canRequestFocus || node.skipTraversal; + return node != currentNode && (!node.canRequestFocus || node.skipTraversal); }); // Sanity check to make sure that the algorithm above doesn't diverge from // the one in FocusScopeNode.traversalDescendants in terms of which nodes it // finds. - assert( - sortedDescendants.length <= scope.traversalDescendants.length && sortedDescendants.toSet().difference(scope.traversalDescendants.toSet()).isEmpty, - 'Sorted descendants contains different nodes than FocusScopeNode.traversalDescendants would. ' - 'These are the different nodes: ${sortedDescendants.toSet().difference(scope.traversalDescendants.toSet())}', - ); + assert((){ + final Set difference = sortedDescendants.toSet().difference(scope.traversalDescendants.toSet()); + if (currentNode.skipTraversal) { + assert( + difference.length == 1 && difference.contains(currentNode), + 'Sorted descendants contains different nodes than FocusScopeNode.traversalDescendants would. ' + 'These are the different nodes: ${difference.where((FocusNode node) => node != currentNode)}', + ); + return true; + } + assert( + difference.isEmpty, + 'Sorted descendants contains different nodes than FocusScopeNode.traversalDescendants would. ' + 'These are the different nodes: $difference', + ); + return true; + }()); return sortedDescendants; } @@ -453,7 +469,7 @@ abstract class FocusTraversalPolicy with Diagnosticable { bool _moveFocus(FocusNode currentNode, {required bool forward}) { final FocusScopeNode nearestScope = currentNode.nearestScope!; invalidateScopeData(nearestScope); - final FocusNode? focusedChild = nearestScope.focusedChild; + FocusNode? focusedChild = nearestScope.focusedChild; if (focusedChild == null) { final FocusNode? firstFocus = forward ? findFirstFocus(currentNode) : findLastFocus(currentNode); if (firstFocus != null) { @@ -464,8 +480,11 @@ abstract class FocusTraversalPolicy with Diagnosticable { return true; } } - final List sortedNodes = _sortAllDescendants(nearestScope, currentNode); - if (sortedNodes.isEmpty) { + 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; @@ -473,7 +492,7 @@ abstract class FocusTraversalPolicy with Diagnosticable { if (forward && focusedChild == sortedNodes.last) { switch (nearestScope.traversalEdgeBehavior) { case TraversalEdgeBehavior.leaveFlutterView: - focusedChild!.unfocus(); + focusedChild.unfocus(); return false; case TraversalEdgeBehavior.closedLoop: requestFocusCallback(sortedNodes.first, alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd); @@ -483,7 +502,7 @@ abstract class FocusTraversalPolicy with Diagnosticable { if (!forward && focusedChild == sortedNodes.first) { switch (nearestScope.traversalEdgeBehavior) { case TraversalEdgeBehavior.leaveFlutterView: - focusedChild!.unfocus(); + focusedChild.unfocus(); return false; case TraversalEdgeBehavior.closedLoop: requestFocusCallback(sortedNodes.last, alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart); diff --git a/packages/flutter/test/material/icon_button_test.dart b/packages/flutter/test/material/icon_button_test.dart index 949a467b1b..2bbe170f2a 100644 --- a/packages/flutter/test/material/icon_button_test.dart +++ b/packages/flutter/test/material/icon_button_test.dart @@ -812,7 +812,7 @@ void main() { expect(focusNode1.hasPrimaryFocus, isTrue); expect(focusNode2.hasPrimaryFocus, isFalse); - expect(focusNode1.nextFocus(), isTrue); + expect(focusNode1.nextFocus(), isFalse); await tester.pump(); expect(focusNode1.hasPrimaryFocus, isTrue); diff --git a/packages/flutter/test/material/input_chip_test.dart b/packages/flutter/test/material/input_chip_test.dart index 827ded8084..7fec3ace8a 100644 --- a/packages/flutter/test/material/input_chip_test.dart +++ b/packages/flutter/test/material/input_chip_test.dart @@ -271,7 +271,7 @@ void main() { expect(focusNode1.hasPrimaryFocus, isTrue); expect(focusNode2.hasPrimaryFocus, isFalse); - expect(focusNode1.nextFocus(), isTrue); + expect(focusNode1.nextFocus(), isFalse); await tester.pump(); expect(focusNode1.hasPrimaryFocus, isTrue); diff --git a/packages/flutter/test/material/raw_material_button_test.dart b/packages/flutter/test/material/raw_material_button_test.dart index da69556af2..7935a4d3dc 100644 --- a/packages/flutter/test/material/raw_material_button_test.dart +++ b/packages/flutter/test/material/raw_material_button_test.dart @@ -414,7 +414,7 @@ void main() { expect(focusNode1.hasPrimaryFocus, isTrue); expect(focusNode2.hasPrimaryFocus, isFalse); - expect(focusNode1.nextFocus(), isTrue); + expect(focusNode1.nextFocus(), isFalse); await tester.pump(); expect(focusNode1.hasPrimaryFocus, isTrue); diff --git a/packages/flutter/test/material/text_field_test.dart b/packages/flutter/test/material/text_field_test.dart index 09fb36bce0..9b3af744d3 100644 --- a/packages/flutter/test/material/text_field_test.dart +++ b/packages/flutter/test/material/text_field_test.dart @@ -6690,7 +6690,7 @@ void main() { expect(focusNode1.hasPrimaryFocus, isTrue); expect(focusNode2.hasPrimaryFocus, isFalse); - expect(focusNode1.nextFocus(), isTrue); + expect(focusNode1.nextFocus(), isFalse); await tester.pump(); expect(focusNode1.hasPrimaryFocus, isTrue); diff --git a/packages/flutter/test/widgets/actions_test.dart b/packages/flutter/test/widgets/actions_test.dart index d428c994d0..b59f17f8ea 100644 --- a/packages/flutter/test/widgets/actions_test.dart +++ b/packages/flutter/test/widgets/actions_test.dart @@ -908,6 +908,7 @@ void main() { await tester.pumpWidget( MaterialApp( home: FocusableActionDetector( + focusNode: FocusNode(skipTraversal: true), child: Column( children: [ ElevatedButton( @@ -938,6 +939,7 @@ void main() { await tester.pumpWidget( MaterialApp( home: FocusableActionDetector( + focusNode: FocusNode(skipTraversal: true), descendantsAreTraversable: false, child: Column( children: [ diff --git a/packages/flutter/test/widgets/focus_traversal_test.dart b/packages/flutter/test/widgets/focus_traversal_test.dart index e03f77628b..ff0b8c9729 100644 --- a/packages/flutter/test/widgets/focus_traversal_test.dart +++ b/packages/flutter/test/widgets/focus_traversal_test.dart @@ -2090,6 +2090,61 @@ void main() { expect(Focus.of(upperLeftKey.currentContext!).hasPrimaryFocus, isTrue); }, skip: isBrowser, variant: KeySimulatorTransitModeVariant.all()); // https://github.com/flutter/flutter/issues/35347 + testWidgets('Focus traversal actions works when current focus skip traversal', (WidgetTester tester) async { + final GlobalKey key1 = GlobalKey(debugLabel: 'key1'); + final GlobalKey key2 = GlobalKey(debugLabel: 'key2'); + final GlobalKey key3 = GlobalKey(debugLabel: 'key3'); + + await tester.pumpWidget( + WidgetsApp( + color: const Color(0xFFFFFFFF), + onGenerateRoute: (RouteSettings settings) { + return TestRoute( + child: Directionality( + textDirection: TextDirection.ltr, + child: FocusScope( + debugLabel: 'scope', + child: Column( + children: [ + Row( + children: [ + Focus( + autofocus: true, + skipTraversal: true, + debugLabel: '1', + child: SizedBox(width: 100, height: 100, key: key1), + ), + Focus( + debugLabel: '2', + child: SizedBox(width: 100, height: 100, key: key2), + ), + Focus( + debugLabel: '3', + child: SizedBox(width: 100, height: 100, key: key3), + ), + ], + ), + ], + ), + ), + ), + ); + }, + ), + ); + + expect(Focus.of(key1.currentContext!).hasPrimaryFocus, isTrue); + await tester.sendKeyEvent(LogicalKeyboardKey.tab); + expect(Focus.of(key2.currentContext!).hasPrimaryFocus, isTrue); + await tester.sendKeyEvent(LogicalKeyboardKey.tab); + expect(Focus.of(key3.currentContext!).hasPrimaryFocus, isTrue); + await tester.sendKeyEvent(LogicalKeyboardKey.tab); + // Skips key 1 because it skips traversal. + expect(Focus.of(key2.currentContext!).hasPrimaryFocus, isTrue); + await tester.sendKeyEvent(LogicalKeyboardKey.tab); + expect(Focus.of(key3.currentContext!).hasPrimaryFocus, isTrue); + }, skip: isBrowser, variant: KeySimulatorTransitModeVariant.all()); // https://github.com/flutter/flutter/issues/35347 + testWidgets('Focus traversal inside a vertical scrollable scrolls to stay visible.', (WidgetTester tester) async { final List items = List.generate(11, (int index) => index).toList(); final List nodes = List.generate(11, (int index) => FocusNode(debugLabel: 'Item ${index + 1}')).toList();