Scroll scrollable to keep focused control visible. (#44965)

Before this change, it was possible to move the focus onto a control that was no longer in the view using focus traversal. This changes that, so that when a control is focused, it makes sure that if it is the child of a scrollable, that the scrollable attempts to keep it in view. If it is already in view, then nothing scrolls.

When asked to move in a direction, the focus traversal code tries to find a control to move to inside the scrollable first, and then looks for things outside of the scrollable only once the scrollable has reached its limit in that direction.

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

@@ -97,7 +97,6 @@ abstract class Action extends Diagnosticable {
   /// null `node`. If the information available from a focus node is
   /// needed in the action, use [ActionDispatcher.invokeFocusedAction] instead.
   @protected
-  @mustCallSuper
   void invoke(FocusNode node, covariant Intent intent);
 
   @override

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

@@ -12,6 +12,8 @@ import 'basic.dart';
 import 'editable_text.dart';
 import 'focus_manager.dart';
 import 'framework.dart';
+import 'scroll_position.dart';
+import 'scrollable.dart';
 
 /// A direction along either the horizontal or vertical axes.
 ///
@@ -146,6 +148,12 @@ abstract class FocusTraversalPolicy {
   bool inDirection(FocusNode currentNode, TraversalDirection direction);
 }
 
+@protected
+void _focusAndEnsureVisible(FocusNode node, {ScrollPositionAlignmentPolicy alignmentPolicy = ScrollPositionAlignmentPolicy.explicit}) {
+  node.requestFocus();
+  Scrollable.ensureVisible(node.context, alignment: 1.0, alignmentPolicy: alignmentPolicy);
+}
+
 /// A policy data object for use by the [DirectionalFocusTraversalPolicyMixin]
 class _DirectionalPolicyDataEntry {
   const _DirectionalPolicyDataEntry({@required this.direction, @required this.node})
@@ -327,6 +335,32 @@ mixin DirectionalFocusTraversalPolicyMixin on FocusTraversalPolicy {
         invalidateScopeData(nearestScope);
         return false;
       }
+
+      // Returns true if successfully popped the history.
+      bool popOrInvalidate(TraversalDirection direction) {
+        final FocusNode lastNode = policyData.history.removeLast().node;
+        if (Scrollable.of(lastNode.context) != Scrollable.of(primaryFocus.context)) {
+          invalidateScopeData(nearestScope);
+          return false;
+        }
+        ScrollPositionAlignmentPolicy alignmentPolicy;
+        switch(direction) {
+          case TraversalDirection.up:
+          case TraversalDirection.left:
+            alignmentPolicy = ScrollPositionAlignmentPolicy.keepVisibleAtStart;
+            break;
+          case TraversalDirection.right:
+          case TraversalDirection.down:
+          alignmentPolicy = ScrollPositionAlignmentPolicy.keepVisibleAtEnd;
+            break;
+        }
+        _focusAndEnsureVisible(
+          lastNode,
+          alignmentPolicy: alignmentPolicy,
+        );
+        return true;
+      }
+
       switch (direction) {
         case TraversalDirection.down:
         case TraversalDirection.up:
@@ -338,17 +372,21 @@ mixin DirectionalFocusTraversalPolicyMixin on FocusTraversalPolicy {
               break;
             case TraversalDirection.up:
             case TraversalDirection.down:
-              policyData.history.removeLast().node.requestFocus();
+              if (popOrInvalidate(direction)) {
                 return true;
               }
               break;
+          }
+          break;
         case TraversalDirection.left:
         case TraversalDirection.right:
           switch (policyData.history.first.direction) {
             case TraversalDirection.left:
             case TraversalDirection.right:
-              policyData.history.removeLast().node.requestFocus();
+              if (popOrInvalidate(direction)) {
                 return true;
+              }
+              break;
             case TraversalDirection.up:
             case TraversalDirection.down:
               // Reset the policy data if we change directions.
@@ -358,7 +396,6 @@ mixin DirectionalFocusTraversalPolicyMixin on FocusTraversalPolicy {
       }
     }
     if (policyData != null && policyData.history.isEmpty) {
-      // Reset the policy data if we change directions.
       invalidateScopeData(nearestScope);
     }
     return false;
@@ -400,22 +437,44 @@ mixin DirectionalFocusTraversalPolicyMixin on FocusTraversalPolicy {
     final FocusScopeNode nearestScope = currentNode.nearestScope;
     final FocusNode focusedChild = nearestScope.focusedChild;
     if (focusedChild == null) {
-      final FocusNode firstFocus = findFirstFocusInDirection(currentNode, direction);
-      (firstFocus ?? currentNode).requestFocus();
+      final FocusNode firstFocus = findFirstFocusInDirection(currentNode, direction) ?? currentNode;
+      switch (direction) {
+        case TraversalDirection.up:
+        case TraversalDirection.left:
+          _focusAndEnsureVisible(
+            firstFocus,
+            alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+          );
+          break;
+        case TraversalDirection.right:
+        case TraversalDirection.down:
+          _focusAndEnsureVisible(
+            firstFocus,
+            alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd,
+          );
+          break;
+      }
       return true;
     }
     if (_popPolicyDataIfNeeded(direction, nearestScope, focusedChild)) {
       return true;
     }
     FocusNode found;
+    final ScrollableState focusedScrollable = Scrollable.of(focusedChild.context);
     switch (direction) {
       case TraversalDirection.down:
       case TraversalDirection.up:
-        final Iterable<FocusNode> eligibleNodes = _sortAndFilterVertically(
+        Iterable<FocusNode> eligibleNodes = _sortAndFilterVertically(
           direction,
           focusedChild.rect,
           nearestScope.traversalDescendants,
         );
+        if (focusedScrollable != null && !focusedScrollable.position.atEdge) {
+          final Iterable<FocusNode> filteredEligibleNodes = eligibleNodes.where((FocusNode node) => Scrollable.of(node.context) == focusedScrollable);
+          if (filteredEligibleNodes.isNotEmpty) {
+            eligibleNodes = filteredEligibleNodes;
+          }
+        }
         if (eligibleNodes.isEmpty) {
           break;
         }
@@ -439,7 +498,13 @@ mixin DirectionalFocusTraversalPolicyMixin on FocusTraversalPolicy {
         break;
       case TraversalDirection.right:
       case TraversalDirection.left:
-        final Iterable<FocusNode> eligibleNodes = _sortAndFilterHorizontally(direction, focusedChild.rect, nearestScope);
+        Iterable<FocusNode> eligibleNodes = _sortAndFilterHorizontally(direction, focusedChild.rect, nearestScope);
+        if (focusedScrollable != null && !focusedScrollable.position.atEdge) {
+          final Iterable<FocusNode> filteredEligibleNodes = eligibleNodes.where((FocusNode node) => Scrollable.of(node.context) == focusedScrollable);
+          if (filteredEligibleNodes.isNotEmpty) {
+            eligibleNodes = filteredEligibleNodes;
+          }
+        }
         if (eligibleNodes.isEmpty) {
           break;
         }
@@ -464,7 +529,22 @@ mixin DirectionalFocusTraversalPolicyMixin on FocusTraversalPolicy {
     }
     if (found != null) {
       _pushPolicyData(direction, nearestScope, focusedChild);
-      found.requestFocus();
+      switch (direction) {
+        case TraversalDirection.up:
+        case TraversalDirection.left:
+          _focusAndEnsureVisible(
+            found,
+            alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+          );
+          break;
+        case TraversalDirection.down:
+        case TraversalDirection.right:
+        _focusAndEnsureVisible(
+          found,
+          alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd,
+        );
+          break;
+      }
       return true;
     }
     return false;
@@ -525,7 +605,12 @@ class WidgetOrderFocusTraversalPolicy extends FocusTraversalPolicy with Directio
     if (focusedChild == null) {
       final FocusNode firstFocus = findFirstFocus(currentNode);
       if (firstFocus != null) {
-        firstFocus.requestFocus();
+        _focusAndEnsureVisible(
+            firstFocus,
+            alignmentPolicy: forward
+                ? ScrollPositionAlignmentPolicy.keepVisibleAtEnd
+                : ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+        );
         return true;
       }
     }
@@ -540,12 +625,12 @@ class WidgetOrderFocusTraversalPolicy extends FocusTraversalPolicy with Directio
         }
         if (forward) {
           if (previousNode == focusedChild) {
-            visited.requestFocus();
+            _focusAndEnsureVisible(visited, alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd);
             return false; // short circuit the traversal.
           }
         } else {
           if (previousNode != null && visited == focusedChild) {
-            previousNode.requestFocus();
+            _focusAndEnsureVisible(previousNode, alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart);
             return false; // short circuit the traversal.
           }
         }
@@ -558,12 +643,12 @@ class WidgetOrderFocusTraversalPolicy extends FocusTraversalPolicy with Directio
     if (visit(nearestScope)) {
       if (forward) {
         if (firstNode != null) {
-          firstNode.requestFocus();
+          _focusAndEnsureVisible(firstNode, alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd);
           return true;
         }
       } else {
         if (lastNode != null) {
-          lastNode.requestFocus();
+          _focusAndEnsureVisible(lastNode, alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart);
           return true;
         }
       }
@@ -694,17 +779,22 @@ class ReadingOrderTraversalPolicy extends FocusTraversalPolicy with DirectionalF
     if (focusedChild == null) {
       final FocusNode firstFocus = findFirstFocus(currentNode);
       if (firstFocus != null) {
-        firstFocus.requestFocus();
+        _focusAndEnsureVisible(
+          firstFocus,
+          alignmentPolicy: forward
+              ? ScrollPositionAlignmentPolicy.keepVisibleAtEnd
+              : ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+        );
         return true;
       }
     }
     final List<FocusNode> sortedNodes = _sortByGeometry(nearestScope).toList();
     if (forward && focusedChild == sortedNodes.last) {
-      sortedNodes.first.requestFocus();
+      _focusAndEnsureVisible(sortedNodes.first, alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd);
       return true;
     }
     if (!forward && focusedChild == sortedNodes.first) {
-      sortedNodes.last.requestFocus();
+      _focusAndEnsureVisible(sortedNodes.last, alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart);
       return true;
     }
 
@@ -712,7 +802,12 @@ class ReadingOrderTraversalPolicy extends FocusTraversalPolicy with DirectionalF
     FocusNode previousNode;
     for (FocusNode node in maybeFlipped) {
       if (previousNode == focusedChild) {
-        node.requestFocus();
+        _focusAndEnsureVisible(
+          node,
+          alignmentPolicy: forward
+            ? ScrollPositionAlignmentPolicy.keepVisibleAtEnd
+            : ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+        );
         return true;
       }
       previousNode = node;
@@ -846,10 +941,7 @@ class RequestFocusAction extends _RequestFocusActionBase {
   static const LocalKey key = ValueKey<Type>(RequestFocusAction);
 
   @override
-  void invoke(FocusNode node, Intent intent) {
-    super.invoke(node, intent);
-    node.requestFocus();
-  }
+  void invoke(FocusNode node, Intent intent) => _focusAndEnsureVisible(node);
 }
 
 /// An [Action] that moves the focus to the next focusable node in the focus
@@ -865,10 +957,7 @@ class NextFocusAction extends _RequestFocusActionBase {
   static const LocalKey key = ValueKey<Type>(NextFocusAction);
 
   @override
-  void invoke(FocusNode node, Intent intent) {
-    super.invoke(node, intent);
-    node.nextFocus();
-  }
+  void invoke(FocusNode node, Intent intent) => node.nextFocus();
 }
 
 /// An [Action] that moves the focus to the previous focusable node in the focus
@@ -885,10 +974,7 @@ class PreviousFocusAction extends _RequestFocusActionBase {
   static const LocalKey key = ValueKey<Type>(PreviousFocusAction);
 
   @override
-  void invoke(FocusNode node, Intent intent) {
-    super.invoke(node, intent);
-    node.previousFocus();
-  }
+  void invoke(FocusNode node, Intent intent) => node.previousFocus();
 }
 
 /// An [Intent] that represents moving to the next focusable node in the given
@@ -935,7 +1021,6 @@ class DirectionalFocusAction extends _RequestFocusActionBase {
 
   @override
   void invoke(FocusNode node, DirectionalFocusIntent intent) {
-    super.invoke(node, intent);
     if (!intent.ignoreTextFields || node.context.widget is! EditableText) {
       node.focusInDirection(intent.direction);
     }

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

@@ -22,6 +22,32 @@ import 'scroll_physics.dart';
 
 export 'scroll_activity.dart' show ScrollHoldController;
 
+/// The policy to use when applying the `alignment` parameter of
+/// [ScrollPosition.ensureVisible].
+enum ScrollPositionAlignmentPolicy {
+  /// Use the `alignment` property of [ScrollPosition.ensureVisible] to decide
+  /// where to align the visible object.
+  explicit,
+
+  /// Find the bottom edge of the scroll container, and scroll the container, if
+  /// necessary, to show the bottom of the object.
+  ///
+  /// For example, find the bottom edge of the scroll container. If the bottom
+  /// edge of the item is below the bottom edge of the scroll container, scroll
+  /// the item so that the bottom of the item is just visible. If the entire
+  /// item is already visible, then do nothing.
+  keepVisibleAtEnd,
+
+  /// Find the top edge of the scroll container, and scroll the container if
+  /// necessary to show the top of the object.
+  ///
+  /// For example, find the top edge of the scroll container. If the top edge of
+  /// the item is above the top edge of the scroll container, scroll the item so
+  /// that the top of the item is just visible. If the entire item is already
+  /// visible, then do nothing.
+  keepVisibleAtStart,
+}
+
 /// Determines which portion of the content is visible in a scroll view.
 ///
 /// The [pixels] value determines the scroll offset that the scroll view uses to
@@ -497,17 +523,41 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics {
 
   /// Animates the position such that the given object is as visible as possible
   /// by just scrolling this position.
+  ///
+  /// See also:
+  ///
+  /// * [ScrollPositionAlignmentPolicy] for the way in which `alignment` is
+  ///   applied, and the way the given `object` is aligned.
   Future<void> ensureVisible(
     RenderObject object, {
     double alignment = 0.0,
     Duration duration = Duration.zero,
     Curve curve = Curves.ease,
+    ScrollPositionAlignmentPolicy alignmentPolicy = ScrollPositionAlignmentPolicy.explicit,
   }) {
+    assert(alignmentPolicy != null);
     assert(object.attached);
     final RenderAbstractViewport viewport = RenderAbstractViewport.of(object);
     assert(viewport != null);
 
-    final double target = viewport.getOffsetToReveal(object, alignment).offset.clamp(minScrollExtent, maxScrollExtent);
+    double target;
+    switch (alignmentPolicy) {
+      case ScrollPositionAlignmentPolicy.explicit:
+        target = viewport.getOffsetToReveal(object, alignment).offset.clamp(minScrollExtent, maxScrollExtent);
+        break;
+      case ScrollPositionAlignmentPolicy.keepVisibleAtEnd:
+        target = viewport.getOffsetToReveal(object, 1.0).offset.clamp(minScrollExtent, maxScrollExtent);
+        if (target < pixels) {
+          target = pixels;
+        }
+        break;
+      case ScrollPositionAlignmentPolicy.keepVisibleAtStart:
+        target = viewport.getOffsetToReveal(object, 0.0).offset.clamp(minScrollExtent, maxScrollExtent);
+        if (target > pixels) {
+          target = pixels;
+        }
+        break;
+    }
 
     if (target == pixels)
       return Future<void>.value();

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

@@ -240,6 +240,7 @@ class Scrollable extends StatefulWidget {
     double alignment = 0.0,
     Duration duration = Duration.zero,
     Curve curve = Curves.ease,
+    ScrollPositionAlignmentPolicy alignmentPolicy = ScrollPositionAlignmentPolicy.explicit,
   }) {
     final List<Future<void>> futures = <Future<void>>[];
 
@@ -250,6 +251,7 @@ class Scrollable extends StatefulWidget {
         alignment: alignment,
         duration: duration,
         curve: curve,
+        alignmentPolicy: alignmentPolicy,
       ));
       context = scrollable.context;
       scrollable = Scrollable.of(context);

packages/flutter/test/widgets/focus_traversal_test.dart

@@ -50,6 +50,7 @@ void main() {
       expect(secondFocusNode.hasFocus, isFalse);
       expect(scope.hasFocus, isTrue);
     });
+
     testWidgets('Move focus to next node.', (WidgetTester tester) async {
       final GlobalKey key1 = GlobalKey(debugLabel: '1');
       final GlobalKey key2 = GlobalKey(debugLabel: '2');
@@ -166,6 +167,7 @@ void main() {
       expect(secondFocusNode.hasFocus, isTrue);
       expect(scope.hasFocus, isTrue);
     });
+
     testWidgets('Move focus to previous node.', (WidgetTester tester) async {
       final GlobalKey key1 = GlobalKey(debugLabel: '1');
       final GlobalKey key2 = GlobalKey(debugLabel: '2');
@@ -239,6 +241,7 @@ void main() {
       expect(secondFocusNode.hasFocus, isFalse);
       expect(scope.hasFocus, 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');
@@ -305,6 +308,7 @@ void main() {
       expect(scope.hasFocus, isTrue);
     });
   });
+
   group(ReadingOrderTraversalPolicy, () {
     testWidgets('Find the initial focus if there is none yet.', (WidgetTester tester) async {
       final GlobalKey key1 = GlobalKey(debugLabel: '1');
@@ -344,6 +348,7 @@ void main() {
       expect(secondFocusNode.hasFocus, isFalse);
       expect(scope.hasFocus, isTrue);
     });
+
     testWidgets('Move reading focus to next node.', (WidgetTester tester) async {
       final GlobalKey key1 = GlobalKey(debugLabel: '1');
       final GlobalKey key2 = GlobalKey(debugLabel: '2');
@@ -458,6 +463,7 @@ void main() {
       expect(secondFocusNode.hasFocus, isTrue);
       expect(scope.hasFocus, isTrue);
     });
+
     testWidgets('Move reading focus to previous node.', (WidgetTester tester) async {
       final GlobalKey key1 = GlobalKey(debugLabel: '1');
       final GlobalKey key2 = GlobalKey(debugLabel: '2');
@@ -532,6 +538,7 @@ void main() {
       expect(scope.hasFocus, isTrue);
     });
   });
+
   group(DirectionalFocusTraversalPolicyMixin, () {
     testWidgets('Move focus in all directions.', (WidgetTester tester) async {
       final GlobalKey upperLeftKey = GlobalKey(debugLabel: 'upperLeftKey');
@@ -672,6 +679,7 @@ void main() {
       expect(lowerRightNode.hasFocus, isFalse);
       expect(scope.hasFocus, isTrue);
     });
+
     testWidgets('Directional focus avoids hysterisis.', (WidgetTester tester) async {
       final List<GlobalKey> keys = <GlobalKey>[
         GlobalKey(debugLabel: 'row 1:1'),
@@ -809,6 +817,7 @@ void main() {
       expectState(<bool>[null, false, null, true, null, null]);
       clear();
     });
+
     testWidgets('Can find first focus in all directions.', (WidgetTester tester) async {
       final GlobalKey upperLeftKey = GlobalKey(debugLabel: 'upperLeftKey');
       final GlobalKey upperRightKey = GlobalKey(debugLabel: 'upperRightKey');
@@ -868,6 +877,7 @@ void main() {
       expect(policy.findFirstFocusInDirection(scope, TraversalDirection.left), equals(upperRightNode));
       expect(policy.findFirstFocusInDirection(scope, TraversalDirection.right), equals(upperLeftNode));
     });
+
     testWidgets('Can find focus when policy data dirty', (WidgetTester tester) async {
       final FocusNode focusTop = FocusNode(debugLabel: 'top');
       final FocusNode focusCenter = FocusNode(debugLabel: 'center');
@@ -918,6 +928,7 @@ void main() {
       expect(focusCenter.hasFocus, isFalse);
       expect(focusTop.hasFocus, isTrue);
     });
+
     testWidgets('Focus traversal actions are invoked when shortcuts are used.', (WidgetTester tester) async {
       final GlobalKey upperLeftKey = GlobalKey(debugLabel: 'upperLeftKey');
       final GlobalKey upperRightKey = GlobalKey(debugLabel: 'upperRightKey');
@@ -1006,6 +1017,203 @@ void main() {
       await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp);
       expect(Focus.of(upperLeftKey.currentContext).hasPrimaryFocus, isTrue);
     }, skip: kIsWeb);
+
+    testWidgets('Focus traversal inside a vertical scrollable scrolls to stay visible.', (WidgetTester tester) async {
+      final List<int> items = List<int>.generate(11, (int index) => index).toList();
+      final List<FocusNode> nodes = List<FocusNode>.generate(11, (int index) => FocusNode(debugLabel: 'Item ${index + 1}')).toList();
+      final FocusNode topNode = FocusNode(debugLabel: 'Header');
+      final FocusNode bottomNode = FocusNode(debugLabel: 'Footer');
+      final ScrollController controller = ScrollController();
+      await tester.pumpWidget(
+        MaterialApp(
+          home: Column(
+            children: <Widget>[
+              Focus(focusNode: topNode, child: Container(height: 100)),
+              Expanded(
+                child: ListView(
+                  scrollDirection: Axis.vertical,
+                  controller: controller,
+                  children: items.map<Widget>((int item) {
+                    return Focus(
+                      focusNode: nodes[item],
+                      child: Container(height: 100),
+                    );
+                  }).toList(),
+                ),
+              ),
+              Focus(focusNode: bottomNode, child: Container(height: 100)),
+            ],
+          ),
+        ),
+      );
+
+      // Start at the top
+      expect(controller.offset, equals(0.0));
+      await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
+      await tester.pump();
+      expect(topNode.hasPrimaryFocus, isTrue);
+      expect(controller.offset, equals(0.0));
+
+      // Enter the list.
+      await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
+      await tester.pump();
+      expect(nodes[0].hasPrimaryFocus, isTrue);
+      expect(controller.offset, equals(0.0));
+
+      // Go down until we hit the bottom of the visible area.
+      for (int i = 1; i <= 4; ++i) {
+        await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
+        await tester.pump();
+        expect(controller.offset, equals(0.0), reason: 'Focusing item $i caused a scroll');
+      }
+
+      // Now keep going down, and the scrollable should scroll automatically.
+      for (int i = 5; i <= 10; ++i) {
+        await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
+        await tester.pump();
+        final double expectedOffset = 100.0 * (i - 5) + 200.0;
+        expect(controller.offset, equals(expectedOffset), reason: "Focusing item $i didn't cause a scroll to $expectedOffset");
+      }
+
+      // Now go one more, and see that the footer gets focused.
+
+      await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
+      await tester.pump();
+      expect(bottomNode.hasPrimaryFocus, isTrue);
+      expect(controller.offset, equals(100.0 * (10 - 5) + 200.0));
+
+      await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp);
+      await tester.pump();
+      expect(nodes[10].hasPrimaryFocus, isTrue);
+      expect(controller.offset, equals(100.0 * (10 - 5) + 200.0));
+
+      // Now reverse directions and go back to the top.
+
+      // These should not cause a scroll.
+      final double lowestOffset = controller.offset;
+      for (int i = 10; i >= 8; --i) {
+        await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp);
+        await tester.pump();
+        expect(controller.offset, equals(lowestOffset), reason: 'Focusing item $i caused a scroll');
+      }
+
+      // These should all cause a scroll.
+      for (int i = 7; i >= 1; --i) {
+        await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp);
+        await tester.pump();
+        final double expectedOffset = 100.0 * (i - 1);
+        expect(controller.offset, equals(expectedOffset), reason: "Focusing item $i didn't cause a scroll");
+      }
+
+      // Back at the top.
+      expect(nodes[0].hasPrimaryFocus, isTrue);
+      expect(controller.offset, equals(0.0));
+
+      // Now we jump to the header.
+      await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp);
+      await tester.pump();
+      expect(topNode.hasPrimaryFocus, isTrue);
+      expect(controller.offset, equals(0.0));
+    }, skip: kIsWeb);
+
+    testWidgets('Focus traversal inside a horizontal scrollable scrolls to stay visible.', (WidgetTester tester) async {
+      final List<int> items = List<int>.generate(11, (int index) => index).toList();
+      final List<FocusNode> nodes = List<FocusNode>.generate(11, (int index) => FocusNode(debugLabel: 'Item ${index + 1}')).toList();
+      final FocusNode leftNode = FocusNode(debugLabel: 'Left Side');
+      final FocusNode rightNode = FocusNode(debugLabel: 'Right Side');
+      final ScrollController controller = ScrollController();
+      await tester.pumpWidget(
+        MaterialApp(
+          home: Row(
+            children: <Widget>[
+              Focus(focusNode: leftNode, child: Container(width: 100)),
+              Expanded(
+                child: ListView(
+                  scrollDirection: Axis.horizontal,
+                  controller: controller,
+                  children: items.map<Widget>((int item) {
+                    return Focus(
+                      focusNode: nodes[item],
+                      child: Container(width: 100),
+                    );
+                  }).toList(),
+                ),
+              ),
+              Focus(focusNode: rightNode, child: Container(width: 100)),
+            ],
+          ),
+        ),
+      );
+
+      // Start at the right
+      expect(controller.offset, equals(0.0));
+      await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight);
+      await tester.pump();
+      expect(leftNode.hasPrimaryFocus, isTrue);
+      expect(controller.offset, equals(0.0));
+
+      // Enter the list.
+      await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight);
+      await tester.pump();
+      expect(nodes[0].hasPrimaryFocus, isTrue);
+      expect(controller.offset, equals(0.0));
+
+      // Go right until we hit the right of the visible area.
+      for (int i = 1; i <= 6; ++i) {
+        await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight);
+        await tester.pump();
+        expect(controller.offset, equals(0.0), reason: 'Focusing item $i caused a scroll');
+      }
+
+      // Now keep going right, and the scrollable should scroll automatically.
+      for (int i = 7; i <= 10; ++i) {
+        await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight);
+        await tester.pump();
+        final double expectedOffset = 100.0 * (i - 5);
+        expect(controller.offset, equals(expectedOffset), reason: "Focusing item $i didn't cause a scroll to $expectedOffset");
+      }
+
+      // Now go one more, and see that the right edge gets focused.
+
+      await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight);
+      await tester.pump();
+      expect(rightNode.hasPrimaryFocus, isTrue);
+      expect(controller.offset, equals(100.0 * 5));
+
+      await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft);
+      await tester.pump();
+      expect(nodes[10].hasPrimaryFocus, isTrue);
+      expect(controller.offset, equals(100.0 * 5));
+
+      // Now reverse directions and go back to the left.
+
+      // These should not cause a scroll.
+      final double lowestOffset = controller.offset;
+      for (int i = 10; i >= 7; --i) {
+        await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft);
+        await tester.pump();
+        expect(controller.offset, equals(lowestOffset), reason: 'Focusing item $i caused a scroll');
+      }
+
+      // These should all cause a scroll.
+      for (int i = 6; i >= 1; --i) {
+        await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft);
+        await tester.pump();
+        final double expectedOffset = 100.0 * (i - 1);
+        expect(controller.offset, equals(expectedOffset), reason: "Focusing item $i didn't cause a scroll");
+      }
+
+      // Back at the left side of the scrollable.
+      expect(nodes[0].hasPrimaryFocus, isTrue);
+      expect(controller.offset, equals(0.0));
+
+      // Now we jump to the left edge of the app.
+      await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft);
+      await tester.pump();
+      expect(leftNode.hasPrimaryFocus, isTrue);
+      expect(controller.offset, equals(0.0));
+    }, skip: kIsWeb);
+
     testWidgets('Arrow focus traversal actions can be re-enabled for text fields.', (WidgetTester tester) async {
       final GlobalKey upperLeftKey = GlobalKey(debugLabel: 'upperLeftKey');
       final GlobalKey upperRightKey = GlobalKey(debugLabel: 'upperRightKey');
@@ -1128,13 +1336,36 @@ void main() {
       await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp);
       expect(focusNodeUpperLeft.hasPrimaryFocus, isTrue);
     });
+
     testWidgets('Focus traversal does not break when no focusable is available on a MaterialApp', (WidgetTester tester) async {
       final List<RawKeyEvent> events = <RawKeyEvent>[];
 
+      await tester.pumpWidget(MaterialApp(home: Container()));
+
+      RawKeyboard.instance.addListener((RawKeyEvent event) {
+        events.add(event);
+      });
+
+      await tester.idle();
+      await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight);
+      await tester.idle();
+
+      expect(events.length, 2);
+    });
+
+    testWidgets('Focus traversal does not break when no focusable is available on a WidgetsApp', (WidgetTester tester) async {
+      final List<RawKeyEvent> events = <RawKeyEvent>[];
+
       await tester.pumpWidget(
-        MaterialApp(
-          home: Container()
-        )
+        WidgetsApp(
+          color: Colors.white,
+          onGenerateRoute: (RouteSettings settings) => PageRouteBuilder<void>(
+            settings: settings,
+            pageBuilder: (BuildContext context, Animation<double> animation1, Animation<double> animation2) {
+              return const Placeholder();
+            },
+          ),
+        ),
       );
 
       RawKeyboard.instance.addListener((RawKeyEvent event) {

packages/flutter/test/widgets/remember_scroll_position_test.dart → packages/flutter/test/widgets/scroll_position_test.dart

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 import 'dart:async';
+import 'dart:ui';
 
 import 'package:meta/meta.dart';
 import 'package:flutter_test/flutter_test.dart';
@@ -13,7 +14,7 @@ ScrollController _controller = ScrollController(
 );
 
 class ThePositiveNumbers extends StatelessWidget {
-  const ThePositiveNumbers({ @required this.from });
+  const ThePositiveNumbers({@required this.from});
   final int from;
   @override
   Widget build(BuildContext context) {
@@ -57,17 +58,17 @@ Future<void> performTest(WidgetTester tester, bool maintainState) async {
 
   // we're 600 pixels high, each item is 100 pixels high, scroll position is
   // 110.0, so we should have 7 items, 1..7.
-  expect(find.text('0'), findsNothing);
-  expect(find.text('1'), findsOneWidget);
-  expect(find.text('2'), findsOneWidget);
-  expect(find.text('3'), findsOneWidget);
-  expect(find.text('4'), findsOneWidget);
-  expect(find.text('5'), findsOneWidget);
-  expect(find.text('6'), findsOneWidget);
-  expect(find.text('7'), findsOneWidget);
-  expect(find.text('8'), findsNothing);
-  expect(find.text('10'), findsNothing);
-  expect(find.text('100'), findsNothing);
+  expect(find.text('0'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('1'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('2'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('3'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('4'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('5'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('6'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('7'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('8'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('10'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('100'), findsNothing, reason: 'with maintainState: $maintainState');
 
   tester.state<ScrollableState>(find.byType(Scrollable)).position.jumpTo(1000.0);
   await tester.pump(const Duration(seconds: 1));
@@ -75,39 +76,39 @@ Future<void> performTest(WidgetTester tester, bool maintainState) async {
   // we're 600 pixels high, each item is 100 pixels high, scroll position is
   // 1000, so we should have exactly 6 items, 10..15.
 
-  expect(find.text('0'), findsNothing);
-  expect(find.text('1'), findsNothing);
-  expect(find.text('8'), findsNothing);
-  expect(find.text('9'), findsNothing);
-  expect(find.text('10'), findsOneWidget);
-  expect(find.text('11'), findsOneWidget);
-  expect(find.text('12'), findsOneWidget);
-  expect(find.text('13'), findsOneWidget);
-  expect(find.text('14'), findsOneWidget);
-  expect(find.text('15'), findsOneWidget);
-  expect(find.text('16'), findsNothing);
-  expect(find.text('100'), findsNothing);
+  expect(find.text('0'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('1'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('8'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('9'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('10'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('11'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('12'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('13'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('14'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('15'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('16'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('100'), findsNothing, reason: 'with maintainState: $maintainState');
 
   navigatorKey.currentState.pushNamed('/second');
   await tester.pump(); // navigating always takes two frames, one to start...
   await tester.pump(const Duration(seconds: 1)); // ...and one to end the transition
 
   // the second list is now visible, starting at 10001
-  expect(find.text('0'), findsNothing);
-  expect(find.text('1'), findsNothing);
-  expect(find.text('10'), findsNothing);
-  expect(find.text('11'), findsNothing);
-  expect(find.text('10000'), findsNothing);
-  expect(find.text('10001'), findsOneWidget);
-  expect(find.text('10002'), findsOneWidget);
-  expect(find.text('10003'), findsOneWidget);
-  expect(find.text('10004'), findsOneWidget);
-  expect(find.text('10005'), findsOneWidget);
-  expect(find.text('10006'), findsOneWidget);
-  expect(find.text('10007'), findsOneWidget);
-  expect(find.text('10008'), findsNothing);
-  expect(find.text('10010'), findsNothing);
-  expect(find.text('10100'), findsNothing);
+  expect(find.text('0'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('1'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('10'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('11'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('10000'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('10001'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('10002'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('10003'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('10004'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('10005'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('10006'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('10007'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('10008'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('10010'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('10100'), findsNothing, reason: 'with maintainState: $maintainState');
 
   navigatorKey.currentState.pop();
   await tester.pump(); // again, navigating always takes two frames
@@ -115,25 +116,25 @@ Future<void> performTest(WidgetTester tester, bool maintainState) async {
   // Ensure we don't clamp the scroll offset even during the navigation.
   // https://github.com/flutter/flutter/issues/4883
   final ScrollableState state = tester.state(find.byType(Scrollable).first);
-  expect(state.position.pixels, equals(1000.0));
+  expect(state.position.pixels, equals(1000.0), reason: 'with maintainState: $maintainState');
 
   await tester.pump(const Duration(seconds: 1));
 
   // we're 600 pixels high, each item is 100 pixels high, scroll position is
   // 1000, so we should have exactly 6 items, 10..15.
 
-  expect(find.text('0'), findsNothing);
-  expect(find.text('1'), findsNothing);
-  expect(find.text('8'), findsNothing);
-  expect(find.text('9'), findsNothing);
-  expect(find.text('10'), findsOneWidget);
-  expect(find.text('11'), findsOneWidget);
-  expect(find.text('12'), findsOneWidget);
-  expect(find.text('13'), findsOneWidget);
-  expect(find.text('14'), findsOneWidget);
-  expect(find.text('15'), findsOneWidget);
-  expect(find.text('16'), findsNothing);
-  expect(find.text('100'), findsNothing);
+  expect(find.text('0'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('1'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('8'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('9'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('10'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('11'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('12'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('13'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('14'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('15'), findsOneWidget, reason: 'with maintainState: $maintainState');
+  expect(find.text('16'), findsNothing, reason: 'with maintainState: $maintainState');
+  expect(find.text('100'), findsNothing, reason: 'with maintainState: $maintainState');
 }
 
 void main() {
@@ -141,4 +142,67 @@ void main() {
     await performTest(tester, true);
     await performTest(tester, false);
   });
+
+  testWidgets('scroll alignment is honored by ensureVisible', (WidgetTester tester) async {
+    final List<int> items = List<int>.generate(11, (int index) => index).toList();
+    final List<FocusNode> nodes = List<FocusNode>.generate(11, (int index) => FocusNode(debugLabel: 'Item ${index + 1}')).toList();
+    final ScrollController controller = ScrollController();
+    await tester.pumpWidget(
+      MaterialApp(
+        home: ListView(
+          scrollDirection: Axis.vertical,
+          controller: controller,
+          children: items.map<Widget>((int item) {
+            return Focus(
+              key: ValueKey<int>(item),
+              focusNode: nodes[item],
+              child: Container(height: 110),
+            );
+          }).toList(),
+        ),
+      ),
+    );
+
+    controller.position.ensureVisible(
+      tester.renderObject(find.byKey(const ValueKey<int>(0))),
+      alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+    );
+    expect(controller.position.pixels, equals(0.0));
+
+    controller.position.ensureVisible(
+      tester.renderObject(find.byKey(const ValueKey<int>(1))),
+      alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+    );
+    expect(controller.position.pixels, equals(0.0));
+
+    controller.position.ensureVisible(
+      tester.renderObject(find.byKey(const ValueKey<int>(1))),
+      alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd,
+    );
+    expect(controller.position.pixels, equals(0.0));
+
+    controller.position.ensureVisible(
+      tester.renderObject(find.byKey(const ValueKey<int>(4))),
+      alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+    );
+    expect(controller.position.pixels, equals(0.0));
+
+    controller.position.ensureVisible(
+      tester.renderObject(find.byKey(const ValueKey<int>(5))),
+      alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+    );
+    expect(controller.position.pixels, equals(0.0));
+
+    controller.position.ensureVisible(
+      tester.renderObject(find.byKey(const ValueKey<int>(5))),
+      alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd,
+    );
+    expect(controller.position.pixels, equals(60.0));
+
+    controller.position.ensureVisible(
+      tester.renderObject(find.byKey(const ValueKey<int>(0))),
+      alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+    );
+    expect(controller.position.pixels, equals(0.0));
+  });
 }