Make it possible to remove nodes from traversal sort. (#58621)

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

@@ -167,7 +167,7 @@ abstract class FocusTraversalPolicy with Diagnosticable {
     final FocusScopeNode scope = currentNode.nearestScope;
     FocusNode candidate = scope.focusedChild;
     if (candidate == null && scope.descendants.isNotEmpty) {
-      final Iterable<FocusNode> sorted = _sortAllDescendants(scope);
+      final Iterable<FocusNode> sorted = _sortAllDescendants(scope, currentNode);
       if (sorted.isEmpty) {
         candidate = null;
       } else {
@@ -254,10 +254,15 @@ abstract class FocusTraversalPolicy with Diagnosticable {
   /// Subclasses should override this to implement a different sort for [next]
   /// and [previous] to use in their ordering. If the returned iterable omits a
   /// node that is a descendant of the given scope, then the user will be unable
-  /// to use next/previous keyboard traversal to reach that node, and if that
-  /// node is used as the originator of a call to next/previous (i.e. supplied
-  /// as the argument to [next] or [previous]), then the next or previous node
-  /// will not be able to be determined and the focus will not change.
+  /// to use next/previous keyboard traversal to reach that node.
+  ///
+  /// The node used to initiate the traversal (the one passed to [next] or
+  /// [previous]) is passed as `currentNode`.
+  ///
+  /// Having the current node in the list is what allows the algorithm to
+  /// determine which nodes are adjacent to the current node. If the
+  /// `currentNode` is removed from the list, then the focus will be unchanged
+  /// when [next] or [previous] are called, and they will return false.
   ///
   /// This is not used for directional focus ([inDirection]), only for
   /// determining the focus order for [next] and [previous].
@@ -268,7 +273,7 @@ abstract class FocusTraversalPolicy with Diagnosticable {
   /// can appear in arbitrary order, and change positions between sorts), whereas
   /// [mergeSort] is stable.
   @protected
-  Iterable<FocusNode> sortDescendants(Iterable<FocusNode> descendants);
+  Iterable<FocusNode> sortDescendants(Iterable<FocusNode> descendants, FocusNode currentNode);
 
   _FocusTraversalGroupMarker _getMarker(BuildContext context) {
     return context?.getElementForInheritedWidgetOfExactType<_FocusTraversalGroupMarker>()?.widget as _FocusTraversalGroupMarker;
@@ -276,7 +281,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) {
+  List<FocusNode> _sortAllDescendants(FocusScopeNode scope, FocusNode currentNode) {
     assert(scope != null);
     final _FocusTraversalGroupMarker scopeGroupMarker = _getMarker(scope.context);
     final FocusTraversalPolicy defaultPolicy = scopeGroupMarker?.policy ?? ReadingOrderTraversalPolicy();
@@ -314,7 +319,7 @@ abstract class FocusTraversalPolicy with Diagnosticable {
     // Sort the member lists using the individual policy sorts.
     final Set<FocusNode> groupKeys = groups.keys.toSet();
     for (final FocusNode key in groups.keys) {
-      final List<FocusNode> sortedMembers = groups[key].policy.sortDescendants(groups[key].members).toList();
+      final List<FocusNode> sortedMembers = groups[key].policy.sortDescendants(groups[key].members, currentNode).toList();
       groups[key].members.clear();
       groups[key].members.addAll(sortedMembers);
     }
@@ -336,13 +341,9 @@ abstract class FocusTraversalPolicy with Diagnosticable {
 
     visitGroups(groups[scopeGroupMarker?.focusNode]);
     assert(
-      sortedDescendants.toSet().difference(scope.traversalDescendants.toSet()).isEmpty,
+      sortedDescendants.length <= scope.traversalDescendants.length && sortedDescendants.toSet().difference(scope.traversalDescendants.toSet()).isEmpty,
       'sorted descendants contains more nodes than it should: (${sortedDescendants.toSet().difference(scope.traversalDescendants.toSet())})'
     );
-    assert(
-      scope.traversalDescendants.toSet().difference(sortedDescendants.toSet()).isEmpty,
-      'sorted descendants are missing some nodes: (${scope.traversalDescendants.toSet().difference(sortedDescendants.toSet())})'
-    );
     return sortedDescendants;
   }
 
@@ -379,7 +380,7 @@ abstract class FocusTraversalPolicy with Diagnosticable {
         return true;
       }
     }
-    final List<FocusNode> sortedNodes = _sortAllDescendants(nearestScope);
+    final List<FocusNode> sortedNodes = _sortAllDescendants(nearestScope, currentNode);
     if (forward && focusedChild == sortedNodes.last) {
       _focusAndEnsureVisible(sortedNodes.first, alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd);
       return true;
@@ -830,7 +831,7 @@ mixin DirectionalFocusTraversalPolicyMixin on FocusTraversalPolicy {
 ///    explicitly using [FocusTraversalOrder] widgets.
 class WidgetOrderTraversalPolicy extends FocusTraversalPolicy with DirectionalFocusTraversalPolicyMixin {
   @override
-  Iterable<FocusNode> sortDescendants(Iterable<FocusNode> descendants) => descendants;
+  Iterable<FocusNode> sortDescendants(Iterable<FocusNode> descendants, FocusNode currentNode) => descendants;
 }
 
 // This class exists mainly for efficiency reasons: the rect is copied out of
@@ -1084,7 +1085,7 @@ class ReadingOrderTraversalPolicy extends FocusTraversalPolicy with DirectionalF
   // Sorts the list of nodes based on their geometry into the desired reading
   // order based on the directionality of the context for each node.
   @override
-  Iterable<FocusNode> sortDescendants(Iterable<FocusNode> descendants) {
+  Iterable<FocusNode> sortDescendants(Iterable<FocusNode> descendants, FocusNode currentNode) {
     assert(descendants != null);
     if (descendants.length <= 1) {
       return descendants;
@@ -1368,9 +1369,9 @@ class OrderedTraversalPolicy extends FocusTraversalPolicy with DirectionalFocusT
   final FocusTraversalPolicy secondary;
 
   @override
-  Iterable<FocusNode> sortDescendants(Iterable<FocusNode> descendants) {
+  Iterable<FocusNode> sortDescendants(Iterable<FocusNode> descendants, FocusNode currentNode) {
     final FocusTraversalPolicy secondaryPolicy = secondary ?? ReadingOrderTraversalPolicy();
-    final Iterable<FocusNode> sortedDescendants = secondaryPolicy.sortDescendants(descendants);
+    final Iterable<FocusNode> sortedDescendants = secondaryPolicy.sortDescendants(descendants, currentNode);
     final List<FocusNode> unordered = <FocusNode>[];
     final List<_OrderedFocusInfo> ordered = <_OrderedFocusInfo>[];
     for (final FocusNode node in sortedDescendants) {

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

@@ -275,6 +275,7 @@ class ShortcutManager extends ChangeNotifier with Diagnosticable {
   Map<LogicalKeySet, Intent> get shortcuts => _shortcuts;
   Map<LogicalKeySet, Intent> _shortcuts;
   set shortcuts(Map<LogicalKeySet, Intent> value) {
+    assert(value != null);
     if (!mapEquals<LogicalKeySet, Intent>(_shortcuts, value)) {
       _shortcuts = value;
       notifyListeners();
@@ -349,16 +350,18 @@ class ShortcutManager extends ChangeNotifier with Diagnosticable {
 ///    invoked.
 ///  * [Action], a class for defining an invocation of a user action.
 class Shortcuts extends StatefulWidget {
-  /// Creates a ActionManager object.
+  /// Creates a const [Shortcuts] widget.
   ///
-  /// The [child] argument must not be null.
+  /// The [child] and [shortcuts] arguments are required and must not be null.
   const Shortcuts({
     Key key,
     this.manager,
-    this.shortcuts,
-    this.child,
+    @required this.shortcuts,
+    @required this.child,
     this.debugLabel,
-  }) : super(key: key);
+  }) : assert(shortcuts != null),
+       assert(child != null),
+       super(key: key);
 
   /// The [ShortcutManager] that will manage the mapping between key
   /// combinations and [Action]s.

packages/flutter/test/widgets/focus_traversal_test.dart

@@ -13,6 +13,18 @@ import 'package:flutter/widgets.dart';
 
 import 'semantics_tester.dart';
 
+/// Used to test removal of nodes while sorting.
+class SkipAllButFirstAndLastPolicy extends FocusTraversalPolicy with DirectionalFocusTraversalPolicyMixin {
+  @override
+  Iterable<FocusNode> sortDescendants(Iterable<FocusNode> descendants, FocusNode currentNode) {
+    return <FocusNode>[
+      descendants.first,
+      if (currentNode != descendants.first && currentNode != descendants.last) currentNode,
+      descendants.last,
+    ];
+  }
+}
+
 void main() {
   group(WidgetOrderTraversalPolicy, () {
     testWidgets('Find the initial focus if there is none yet.', (WidgetTester tester) async {
@@ -288,6 +300,40 @@ void main() {
       expect(scope.hasFocus, isTrue);
     });
 
+    testWidgets('Move focus to next/previous node while skipping nodes in policy', (WidgetTester tester) async {
+      final List<FocusNode> nodes =
+      List<FocusNode>.generate(7, (int index) => FocusNode(debugLabel: 'Node $index'));
+      await tester.pumpWidget(
+        FocusTraversalGroup(
+          policy: SkipAllButFirstAndLastPolicy(),
+          child: Column(
+            children: List<Widget>.generate(
+              nodes.length,
+              (int index) => Focus(
+                focusNode: nodes[index],
+                child: const SizedBox(),
+              ),
+            ),
+          ),
+        ),
+      );
+
+      nodes[2].requestFocus();
+      await tester.pump();
+
+      expect(nodes[2].hasPrimaryFocus, isTrue);
+
+      primaryFocus.nextFocus();
+      await tester.pump();
+
+      expect(nodes[6].hasPrimaryFocus, isTrue);
+
+      primaryFocus.previousFocus();
+      await tester.pump();
+
+      expect(nodes[0].hasPrimaryFocus, isTrue);
+    });
+
     testWidgets('Find the initial focus when a route is pushed or popped.', (WidgetTester tester) async {
       final GlobalKey key1 = GlobalKey(debugLabel: '1');
       final GlobalKey key2 = GlobalKey(debugLabel: '2');
@@ -1299,7 +1345,7 @@ void main() {
       expect(scope.hasFocus, isTrue);
     });
 
-    testWidgets('Directional focus avoids hysterisis.', (WidgetTester tester) async {
+    testWidgets('Directional focus avoids hysteresis.', (WidgetTester tester) async {
       final List<GlobalKey> keys = <GlobalKey>[
         GlobalKey(debugLabel: 'row 1:1'),
         GlobalKey(debugLabel: 'row 2:1'),

packages/flutter/test/widgets/shortcuts_test.dart

@@ -191,18 +191,24 @@ void main() {
         LogicalKeyboardKey.keyB,
       ).debugFillProperties(builder);
 
-      final List<String> description = builder.properties
-          .where((DiagnosticsNode node) {
-            return !node.isFiltered(DiagnosticLevel.info);
-          })
-          .map((DiagnosticsNode node) => node.toString())
-          .toList();
+      final List<String> description = builder.properties.where((DiagnosticsNode node) {
+        return !node.isFiltered(DiagnosticLevel.info);
+      }).map((DiagnosticsNode node) => node.toString()).toList();
 
       expect(description.length, equals(1));
       expect(description[0], equals('keys: Key A + Key B'));
     });
   });
   group(Shortcuts, () {
+    testWidgets('Default constructed Shortcuts has empty shortcuts', (WidgetTester tester) async {
+      final ShortcutManager manager = ShortcutManager();
+      expect(manager.shortcuts, isNotNull);
+      expect(manager.shortcuts, isEmpty);
+      const Shortcuts shortcuts = Shortcuts(shortcuts: <LogicalKeySet, Intent>{}, child: SizedBox());
+      await tester.pumpWidget(shortcuts);
+      expect(shortcuts.shortcuts, isNotNull);
+      expect(shortcuts.shortcuts, isEmpty);
+    });
     testWidgets('ShortcutManager handles shortcuts', (WidgetTester tester) async {
       final GlobalKey containerKey = GlobalKey();
       final List<LogicalKeyboardKey> pressedKeys = <LogicalKeyboardKey>[];
@@ -317,21 +323,23 @@ void main() {
     test('Shortcuts diagnostics work.', () {
       final DiagnosticPropertiesBuilder builder = DiagnosticPropertiesBuilder();
 
-      Shortcuts(shortcuts: <LogicalKeySet, Intent>{LogicalKeySet(
-        LogicalKeyboardKey.shift,
-        LogicalKeyboardKey.keyA,
-      ) : const ActivateIntent(),
-        LogicalKeySet(
-        LogicalKeyboardKey.shift,
-        LogicalKeyboardKey.arrowRight,
-      ) : const DirectionalFocusIntent(TraversalDirection.right)}).debugFillProperties(builder);
+      Shortcuts(
+        shortcuts: <LogicalKeySet, Intent>{
+          LogicalKeySet(
+            LogicalKeyboardKey.shift,
+            LogicalKeyboardKey.keyA,
+          ): const ActivateIntent(),
+          LogicalKeySet(
+            LogicalKeyboardKey.shift,
+            LogicalKeyboardKey.arrowRight,
+          ): const DirectionalFocusIntent(TraversalDirection.right)
+        },
+        child: const SizedBox(),
+      ).debugFillProperties(builder);
 
-      final List<String> description = builder.properties
-          .where((DiagnosticsNode node) {
+      final List<String> description = builder.properties.where((DiagnosticsNode node) {
         return !node.isFiltered(DiagnosticLevel.info);
-      })
-          .map((DiagnosticsNode node) => node.toString())
-          .toList();
+      }).map((DiagnosticsNode node) => node.toString()).toList();
 
       expect(description.length, equals(1));
       expect(
@@ -350,14 +358,12 @@ void main() {
             LogicalKeyboardKey.keyB,
           ): const ActivateIntent(),
         },
+        child: const SizedBox(),
       ).debugFillProperties(builder);
 
-      final List<String> description = builder.properties
-          .where((DiagnosticsNode node) {
+      final List<String> description = builder.properties.where((DiagnosticsNode node) {
         return !node.isFiltered(DiagnosticLevel.info);
-      })
-          .map((DiagnosticsNode node) => node.toString())
-          .toList();
+      }).map((DiagnosticsNode node) => node.toString()).toList();
 
       expect(description.length, equals(1));
       expect(description[0], equals('shortcuts: <Debug Label>'));
@@ -373,14 +379,12 @@ void main() {
             LogicalKeyboardKey.keyB,
           ): const ActivateIntent(),
         },
+        child: const SizedBox(),
       ).debugFillProperties(builder);
 
-      final List<String> description = builder.properties
-          .where((DiagnosticsNode node) {
+      final List<String> description = builder.properties.where((DiagnosticsNode node) {
         return !node.isFiltered(DiagnosticLevel.info);
-      })
-          .map((DiagnosticsNode node) => node.toString())
-          .toList();
+      }).map((DiagnosticsNode node) => node.toString()).toList();
 
       expect(description.length, equals(2));
       expect(description[0], equalsIgnoringHashCodes('manager: ShortcutManager#00000(shortcuts: {})'));