Fix ensureVisible and default focus traversal for reversed scrollables (#128756)

Fixes flutter/flutter#128749

The ScrollPositionAlignmentPolicy does not account for AxisDirection, which meant default focus traversal of reversed scrollables did not work. The policy doesn't know about the axis direction, so this is corrected in the ScrollPosition where all the information is available before calling on the viewport for the new target scroll offset.

This fixes that by flipping the policy (unless explicit) so that focus traversal works.

Author: Piinks

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

@@ -757,6 +757,26 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics {
     context.setSemanticsActions(_semanticActions!);
   }
 
+  ScrollPositionAlignmentPolicy _maybeFlipAlignment(ScrollPositionAlignmentPolicy alignmentPolicy) {
+    return switch (alignmentPolicy) {
+      // Don't flip when explicit.
+      ScrollPositionAlignmentPolicy.explicit => alignmentPolicy,
+      ScrollPositionAlignmentPolicy.keepVisibleAtEnd => ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      ScrollPositionAlignmentPolicy.keepVisibleAtStart => ScrollPositionAlignmentPolicy.keepVisibleAtEnd,
+    };
+  }
+
+  ScrollPositionAlignmentPolicy _applyAxisDirectionToAlignmentPolicy(ScrollPositionAlignmentPolicy alignmentPolicy) {
+    return switch (axisDirection) {
+      // Start and end alignments must account for axis direction.
+      // When focus is requested for example, it knows the directionality of the
+      // keyboard keys initiating traversal, but not the direction of the
+      // Scrollable.
+      AxisDirection.up || AxisDirection.left => _maybeFlipAlignment(alignmentPolicy),
+      AxisDirection.down || AxisDirection.right => alignmentPolicy,
+    };
+  }
+
   /// Animates the position such that the given object is as visible as possible
   /// by just scrolling this position.
   ///
@@ -790,7 +810,7 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics {
     }
 
     double target;
-    switch (alignmentPolicy) {
+    switch (_applyAxisDirectionToAlignmentPolicy(alignmentPolicy)) {
       case ScrollPositionAlignmentPolicy.explicit:
         target = clampDouble(viewport.getOffsetToReveal(object, alignment, rect: targetRect).offset, minScrollExtent, maxScrollExtent);
       case ScrollPositionAlignmentPolicy.keepVisibleAtEnd:

packages/flutter/test/widgets/ensure_visible_test.dart

@@ -8,7 +8,7 @@ import 'package:flutter/rendering.dart';
 import 'package:flutter/widgets.dart';
 import 'package:flutter_test/flutter_test.dart';
 
-Finder findKey(int i) => find.byKey(ValueKey<int>(i));
+Finder findKey(int i) => find.byKey(ValueKey<int>(i), skipOffstage: false);
 
 Widget buildSingleChildScrollView(Axis scrollDirection, { bool reverse = false }) {
   return Directionality(
@@ -147,6 +147,47 @@ void main() {
       await tester.pump();
       await tester.pump(const Duration(milliseconds: 1020));
       expect(tester.getBottomRight(findKey(3)).dy, equals(500.0));
+
+      // Regression test for https://github.com/flutter/flutter/issues/128749
+      // Reset to zero position.
+      tester.state<ScrollableState>(find.byType(Scrollable)).position.jumpTo(0.0);
+      await tester.pump();
+      // 4 is not currently visible as the SingleChildScrollView is contained
+      // within a centered SizedBox.
+      expect(tester.getBottomLeft(findKey(4)).dy, equals(100.0));
+      expect(tester.getBottomLeft(findKey(6)).dy, equals(500.0));
+      Scrollable.ensureVisible(
+        findContext(6),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      Scrollable.ensureVisible(
+        findContext(5),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      // 5 and 6 are already visible beyond the top edge, so no change.
+      expect(tester.getBottomLeft(findKey(4)).dy, equals(100.0));
+      expect(tester.getBottomLeft(findKey(6)).dy, equals(500.0));
+      Scrollable.ensureVisible(
+        findContext(4),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      // Since it is reversed, 4 should have come into view at the top
+      // edge of the scrollable, matching the alignment expectation.
+      expect(tester.getBottomLeft(findKey(4)).dy, equals(300.0));
+      expect(tester.getBottomLeft(findKey(6)).dy, equals(700.0));
+
+      // Bring 6 back into view at the trailing edge, checking the other
+      // alignment.
+      Scrollable.ensureVisible(
+        findContext(6),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd,
+      );
+      await tester.pump();
+      expect(tester.getBottomLeft(findKey(4)).dy, equals(100.0));
+      expect(tester.getBottomLeft(findKey(6)).dy, equals(500.0));
     });
 
     testWidgets('SingleChildScrollView ensureVisible Axis.horizontal reverse', (WidgetTester tester) async {
@@ -174,6 +215,52 @@ void main() {
       await tester.pump();
       await tester.pump(const Duration(milliseconds: 1020));
       expect(tester.getBottomRight(findKey(3)).dx, equals(700.0));
+
+      // Regression test for https://github.com/flutter/flutter/issues/128749
+      // Reset to zero position.
+      tester.state<ScrollableState>(find.byType(Scrollable)).position.jumpTo(0.0);
+      await tester.pump();
+      // 4 is not currently visible as the SingleChildScrollView is contained
+      // within a centered SizedBox.
+      expect(tester.getBottomLeft(findKey(3)).dx, equals(-100.0));
+      expect(tester.getBottomLeft(findKey(6)).dx, equals(500.0));
+      Scrollable.ensureVisible(
+        findContext(6),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      Scrollable.ensureVisible(
+        findContext(5),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      Scrollable.ensureVisible(
+        findContext(4),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      // 4, 5 and 6 are already visible beyond the left edge, so no change.
+      expect(tester.getBottomLeft(findKey(3)).dx, equals(-100.0));
+      expect(tester.getBottomLeft(findKey(6)).dx, equals(500.0));
+      Scrollable.ensureVisible(
+        findContext(3),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      // Since it is reversed, 3 should have come into view at the leading
+      // edge of the scrollable, matching the alignment expectation.
+      expect(tester.getBottomLeft(findKey(3)).dx, equals(100.0));
+      expect(tester.getBottomLeft(findKey(6)).dx, equals(700.0));
+
+      // Bring 6 back into view at the trailing edge, checking the other
+      // alignment.
+      Scrollable.ensureVisible(
+        findContext(6),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd,
+      );
+      await tester.pump();
+      expect(tester.getBottomLeft(findKey(3)).dx, equals(-100.0));
+      expect(tester.getBottomLeft(findKey(6)).dx, equals(500.0));
     });
 
     testWidgets('SingleChildScrollView ensureVisible rotated child', (WidgetTester tester) async {
@@ -407,6 +494,46 @@ void main() {
       await tester.pump();
       await tester.pump(const Duration(milliseconds: 1020));
       expect(tester.getBottomRight(findKey(3)).dy, equals(500.0));
+
+      // Regression test for https://github.com/flutter/flutter/issues/128749
+      // Reset to zero position.
+      await prepare(0.0);
+      // 2 is not currently visible as the ListView is contained
+      // within a centered SizedBox.
+      expect(tester.getBottomLeft(findKey(2)).dy, equals(100.0));
+      expect(tester.getBottomLeft(findKey(0)).dy, equals(500.0));
+      Scrollable.ensureVisible(
+        findContext(0),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      Scrollable.ensureVisible(
+        findContext(1),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      // 0 and 1 are already visible beyond the top edge, so no change.
+      expect(tester.getBottomLeft(findKey(2)).dy, equals(100.0));
+      expect(tester.getBottomLeft(findKey(0)).dy, equals(500.0));
+      Scrollable.ensureVisible(
+        findContext(2),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      // Since it is reversed, 2 should have come into view at the top
+      // edge of the scrollable, matching the alignment expectation.
+      expect(tester.getBottomLeft(findKey(2)).dy, equals(300.0));
+      expect(tester.getBottomLeft(findKey(0)).dy, equals(700.0));
+
+      // Bring 0 back into view at the trailing edge, checking the other
+      // alignment.
+      Scrollable.ensureVisible(
+        findContext(0),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd,
+      );
+      await tester.pump();
+      expect(tester.getBottomLeft(findKey(2)).dy, equals(100.0));
+      expect(tester.getBottomLeft(findKey(0)).dy, equals(500.0));
     });
 
     testWidgets('ListView ensureVisible Axis.horizontal reverse', (WidgetTester tester) async {
@@ -443,6 +570,51 @@ void main() {
       await tester.pump();
       await tester.pump(const Duration(milliseconds: 1020));
       expect(tester.getBottomRight(findKey(3)).dx, equals(700.0));
+
+      // Regression test for https://github.com/flutter/flutter/issues/128749
+      // Reset to zero position.
+      await prepare(0.0);
+      // 3 is not currently visible as the ListView is contained
+      // within a centered SizedBox.
+      expect(tester.getBottomLeft(findKey(3)).dx, equals(-100.0));
+      expect(tester.getBottomLeft(findKey(0)).dx, equals(500.0));
+      Scrollable.ensureVisible(
+        findContext(0),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      Scrollable.ensureVisible(
+        findContext(1),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      Scrollable.ensureVisible(
+        findContext(2),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      // 0, 1 and 2 are already visible beyond the left edge, so no change.
+      expect(tester.getBottomLeft(findKey(3)).dx, equals(-100.0));
+      expect(tester.getBottomLeft(findKey(0)).dx, equals(500.0));
+      Scrollable.ensureVisible(
+        findContext(3),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      // Since it is reversed, 3 should have come into view at the leading
+      // edge of the scrollable, matching the alignment expectation.
+      expect(tester.getBottomLeft(findKey(3)).dx, equals(100.0));
+      expect(tester.getBottomLeft(findKey(0)).dx, equals(700.0));
+
+      // Bring 0 back into view at the trailing edge, checking the other
+      // alignment.
+      Scrollable.ensureVisible(
+        findContext(0),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd,
+      );
+      await tester.pump();
+      expect(tester.getBottomLeft(findKey(3)).dx, equals(-100.0));
+      expect(tester.getBottomLeft(findKey(0)).dx, equals(500.0));
     });
 
     testWidgets('ListView ensureVisible negative child', (WidgetTester tester) async {
@@ -662,6 +834,46 @@ void main() {
       await tester.pump();
       await tester.pump(const Duration(milliseconds: 1020));
       expect(tester.getBottomRight(findKey(3)).dy, equals(500.0));
+
+      // Regression test for https://github.com/flutter/flutter/issues/128749
+      // Reset to zero position.
+      await prepare(0.0);
+      // 2 is not currently visible as the ListView is contained
+      // within a centered SizedBox.
+      expect(tester.getBottomLeft(findKey(2)).dy, equals(100.0));
+      expect(tester.getBottomLeft(findKey(0)).dy, equals(500.0));
+      Scrollable.ensureVisible(
+        findContext(0),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      Scrollable.ensureVisible(
+        findContext(1),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      // 0 and 1 are already visible beyond the top edge, so no change.
+      expect(tester.getBottomLeft(findKey(2)).dy, equals(100.0));
+      expect(tester.getBottomLeft(findKey(0)).dy, equals(500.0));
+      Scrollable.ensureVisible(
+        findContext(2),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      // Since it is reversed, 2 should have come into view at the top
+      // edge of the scrollable, matching the alignment expectation.
+      expect(tester.getBottomLeft(findKey(2)).dy, equals(300.0));
+      expect(tester.getBottomLeft(findKey(0)).dy, equals(700.0));
+
+      // Bring 0 back into view at the trailing edge, checking the other
+      // alignment.
+      Scrollable.ensureVisible(
+        findContext(0),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd,
+      );
+      await tester.pump();
+      expect(tester.getBottomLeft(findKey(2)).dy, equals(100.0));
+      expect(tester.getBottomLeft(findKey(0)).dy, equals(500.0));
     });
 
     testWidgets('ListView ensureVisible Axis.horizontal reverse', (WidgetTester tester) async {
@@ -698,6 +910,51 @@ void main() {
       await tester.pump();
       await tester.pump(const Duration(milliseconds: 1020));
       expect(tester.getBottomRight(findKey(3)).dx, equals(700.0));
+
+      // Regression test for https://github.com/flutter/flutter/issues/128749
+      // Reset to zero position.
+      await prepare(0.0);
+      // 3 is not currently visible as the ListView is contained
+      // within a centered SizedBox.
+      expect(tester.getBottomLeft(findKey(3)).dx, equals(-100.0));
+      expect(tester.getBottomLeft(findKey(0)).dx, equals(500.0));
+      Scrollable.ensureVisible(
+        findContext(0),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      Scrollable.ensureVisible(
+        findContext(1),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      Scrollable.ensureVisible(
+        findContext(2),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      // 0, 1 and 2 are already visible beyond the left edge, so no change.
+      expect(tester.getBottomLeft(findKey(3)).dx, equals(-100.0));
+      expect(tester.getBottomLeft(findKey(0)).dx, equals(500.0));
+      Scrollable.ensureVisible(
+        findContext(3),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtStart,
+      );
+      await tester.pump();
+      // Since it is reversed, 3 should have come into view at the leading
+      // edge of the scrollable, matching the alignment expectation.
+      expect(tester.getBottomLeft(findKey(3)).dx, equals(100.0));
+      expect(tester.getBottomLeft(findKey(0)).dx, equals(700.0));
+
+      // Bring 0 back into view at the trailing edge, checking the other
+      // alignment.
+      Scrollable.ensureVisible(
+        findContext(0),
+        alignmentPolicy: ScrollPositionAlignmentPolicy.keepVisibleAtEnd,
+      );
+      await tester.pump();
+      expect(tester.getBottomLeft(findKey(3)).dx, equals(-100.0));
+      expect(tester.getBottomLeft(findKey(0)).dx, equals(500.0));
     });
   });