showOnScreen doesn't trigger scroll if item is already fully on screen (#17729)

packages/flutter/lib/src/material/app_bar.dart

@@ -920,7 +920,7 @@ class SliverAppBar extends StatefulWidget {
 
   /// Whether the app bar should remain visible at the start of the scroll view.
   ///
-  /// The app bar can still expand an contract as the user scrolls, but it will
+  /// The app bar can still expand and contract as the user scrolls, but it will
   /// remain visible rather than being scrolled out of view.
   final bool pinned;
 

packages/flutter/lib/src/rendering/viewport.dart

@@ -784,24 +784,65 @@ abstract class RenderViewportBase<ParentDataClass extends ContainerParentDataMix
 
   @override
   void showOnScreen([RenderObject child]) {
-    // Logic duplicated in [_RenderSingleChildViewport.showOnScreen].
+    RenderViewportBase.showInViewport(child: child, viewport: this, offset: offset);
-    if (child != null) {
-      // TODO(goderbauer): Don't scroll if it is already visible.
-      // TODO(goderbauer): Don't guess if we need to align at leading or trailing edge.
-      // Move viewport the smallest distance to bring [child] on screen.
-      final double leadingEdgeOffset = getOffsetToReveal(child, 0.0);
-      final double trailingEdgeOffset = getOffsetToReveal(child, 1.0);
-      final double currentOffset = offset.pixels;
-      // TODO(goderbauer): Don't scroll if that puts us outside of viewport's bounds.
-      if ((currentOffset - leadingEdgeOffset).abs() < (currentOffset - trailingEdgeOffset).abs()) {
-        offset.jumpTo(leadingEdgeOffset);
-      } else {
-        offset.jumpTo(trailingEdgeOffset);
-      }
-    }
     // Make sure the viewport itself is on screen.
     super.showOnScreen();
   }
+
+  /// Make the given `child` of the given `viewport` fully visible in the
+  /// `viewport` by manipulating the provided [ViewportOffset] `offset`.
+  ///
+  /// The parameters `viewport` and `offset` are required and cannot be null.
+  /// If `child` is null this is a no-op.
+  static void showInViewport({
+    RenderObject child,
+    @required RenderAbstractViewport viewport,
+    @required ViewportOffset offset,
+  }) {
+    assert(viewport != null);
+    assert(offset != null);
+    if (child == null) {
+      return;
+    }
+    final double leadingEdgeOffset = viewport.getOffsetToReveal(child, 0.0);
+    final double trailingEdgeOffset = viewport.getOffsetToReveal(child, 1.0);
+    final double currentOffset = offset.pixels;
+
+    //        scrollOffset
+    //                    0 +---------+
+    //                      |         |
+    //                    _ |         |
+    // viewport position |  |         |
+    //   with `child` at |  |         | _
+    //     trailing edge |_ | xxxxxxx |  | viewport position
+    //                      |         |  | with `child` at
+    //                      |         | _| leading edge
+    //                      |         |
+    //                  800 +---------+
+    //
+    // `trailingEdgeOffset`: Distance from scrollOffset 0 to the start of the
+    //                       viewport on the left in image above.
+    // `leadingEdgeOffset`: Distance from scrollOffset 0 to the start of the
+    //                      viewport on the right in image above.
+    //
+    // The viewport position on the left is achieved by setting `offset.pixels`
+    // to `trailingEdgeOffset`, the one on the right by setting it to
+    // `leadingEdgeOffset`.
+
+    assert(leadingEdgeOffset >= trailingEdgeOffset);
+
+    if (currentOffset > leadingEdgeOffset) {
+      // `child` currently starts above the leading edge and can be shown fully
+      // on screen by scrolling down (which means: moving viewport up).
+      offset.jumpTo(leadingEdgeOffset);
+    } else if (currentOffset < trailingEdgeOffset ) {
+      // `child currently ends below the trailing edge and can be shown fully
+      // on screen by scrolling up (which means: moving viewport down)
+      offset.jumpTo(trailingEdgeOffset);
+    }
+    // else: `child` is between leading and trailing edge and hence already
+    //     fully shown on screen. No action necessary.
+  }
 }
 
 /// A render object that is bigger on the inside.

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

@@ -586,19 +586,7 @@ class _RenderSingleChildViewport extends RenderBox with RenderObjectWithChildMix
 
   @override
   void showOnScreen([RenderObject child]) {
-    // Logic duplicated in [RenderViewportBase.showOnScreen].
+    RenderViewportBase.showInViewport(child: child, viewport: this, offset: offset);
-    if (child != null) {
-      // Move viewport the smallest distance to bring [child] on screen.
-      final double leadingEdgeOffset = getOffsetToReveal(child, 0.0);
-      final double trailingEdgeOffset = getOffsetToReveal(child, 1.0);
-      final double currentOffset = offset.pixels;
-      if ((currentOffset - leadingEdgeOffset).abs() < (currentOffset - trailingEdgeOffset).abs()) {
-        offset.jumpTo(leadingEdgeOffset);
-      } else {
-        offset.jumpTo(trailingEdgeOffset);
-      }
-    }
-
     // Make sure the viewport itself is on screen.
     super.showOnScreen();
   }

packages/flutter/test/widgets/scrollable_semantics_test.dart

@@ -137,12 +137,6 @@ void main() {
     await tester.pump(const Duration(seconds: 5));
     expect(tester.getTopLeft(find.byWidget(containers.first)).dy, kExpandedAppBarHeight);
 
-    final int secondContainerId = tester.renderObject(find.byWidget(containers[1])).debugSemantics.id;
-    tester.binding.pipelineOwner.semanticsOwner.performAction(secondContainerId, SemanticsAction.showOnScreen);
-    await tester.pump();
-    await tester.pump(const Duration(seconds: 5));
-    expect(tester.getTopLeft(find.byWidget(containers[1])).dy, kExpandedAppBarHeight);
-
     semantics.dispose();
   });
 
@@ -168,7 +162,7 @@ void main() {
     });
 
     final ScrollController scrollController = new ScrollController(
-      initialScrollOffset: kItemHeight / 2,
+      initialScrollOffset: 2.5 * kItemHeight,
     );
 
     await tester.pumpWidget(new Directionality(
@@ -195,7 +189,7 @@ void main() {
       ),
     ));
 
-    expect(scrollController.offset, kItemHeight / 2);
+    expect(scrollController.offset, 2.5 * kItemHeight);
 
     final int id0 = tester.renderObject(find.byWidget(children[0])).debugSemantics.id;
     tester.binding.pipelineOwner.semanticsOwner.performAction(id0, SemanticsAction.showOnScreen);
@@ -203,12 +197,6 @@ void main() {
     await tester.pump(const Duration(seconds: 5));
     expect(tester.getTopLeft(find.byWidget(children[0])).dy, kToolbarHeight);
 
-    final int id1 = tester.renderObject(find.byWidget(children[1])).debugSemantics.id;
-    tester.binding.pipelineOwner.semanticsOwner.performAction(id1, SemanticsAction.showOnScreen);
-    await tester.pump();
-    await tester.pump(const Duration(seconds: 5));
-    expect(tester.getTopLeft(find.byWidget(children[1])).dy, kToolbarHeight);
-
     semantics.dispose();
   });
 
@@ -399,6 +387,200 @@ void main() {
 
     semantics.dispose();
   });
+
+  group('showOnScreen', () {
+
+    const double kItemHeight = 100.0;
+
+    List<Widget> children;
+    ScrollController scrollController;
+    Widget widgetUnderTest;
+
+    setUp(() {
+      children = new List<Widget>.generate(10, (int i) {
+        return new MergeSemantics(
+          child: new Container(
+            height: kItemHeight,
+            child: new Text('container $i'),
+          ),
+        );
+      });
+
+      scrollController = new ScrollController(
+        initialScrollOffset: kItemHeight / 2,
+      );
+
+      widgetUnderTest = new Directionality(
+        textDirection: TextDirection.ltr,
+        child: new Center(
+          child: new Container(
+            height: 2 * kItemHeight,
+            child: new ListView(
+              controller: scrollController,
+              children: children,
+            ),
+          ),
+        ),
+      );
+
+    });
+
+    testWidgets('brings item above leading edge to leading edge', (WidgetTester tester) async {
+      semantics = new SemanticsTester(tester); // enables semantics tree generation
+
+      await tester.pumpWidget(widgetUnderTest);
+
+      expect(scrollController.offset, kItemHeight / 2);
+
+      final int firstContainerId = tester.renderObject(find.byWidget(children.first)).debugSemantics.id;
+      tester.binding.pipelineOwner.semanticsOwner.performAction(firstContainerId, SemanticsAction.showOnScreen);
+      await tester.pumpAndSettle();
+
+      expect(scrollController.offset, 0.0);
+
+      semantics.dispose();
+    });
+
+    testWidgets('brings item below trailing edge to trailing edge', (WidgetTester tester) async {
+      semantics = new SemanticsTester(tester); // enables semantics tree generation
+
+      await tester.pumpWidget(widgetUnderTest);
+
+      expect(scrollController.offset, kItemHeight / 2);
+
+      final int firstContainerId = tester.renderObject(find.byWidget(children[2])).debugSemantics.id;
+      tester.binding.pipelineOwner.semanticsOwner.performAction(firstContainerId, SemanticsAction.showOnScreen);
+      await tester.pumpAndSettle();
+
+      expect(scrollController.offset, kItemHeight);
+
+      semantics.dispose();
+    });
+
+    testWidgets('does not change position of items already fully on-screen', (WidgetTester tester) async {
+      semantics = new SemanticsTester(tester); // enables semantics tree generation
+
+      await tester.pumpWidget(widgetUnderTest);
+
+      expect(scrollController.offset, kItemHeight / 2);
+
+      final int firstContainerId = tester.renderObject(find.byWidget(children[1])).debugSemantics.id;
+      tester.binding.pipelineOwner.semanticsOwner.performAction(firstContainerId, SemanticsAction.showOnScreen);
+      await tester.pumpAndSettle();
+
+      expect(scrollController.offset, kItemHeight / 2);
+
+      semantics.dispose();
+    });
+  });
+
+  group('showOnScreen with negative children', () {
+    const double kItemHeight = 100.0;
+
+    List<Widget> children;
+    ScrollController scrollController;
+    Widget widgetUnderTest;
+
+    setUp(() {
+      final Key center = new GlobalKey();
+
+      children = new List<Widget>.generate(10, (int i) {
+        return new SliverToBoxAdapter(
+          key: i == 5 ? center : null,
+          child: new MergeSemantics(
+            key: new ValueKey<int>(i),
+            child: new Container(
+              height: kItemHeight,
+              child: new Text('container $i'),
+            ),
+          ),
+        );
+      });
+
+      scrollController = new ScrollController(
+        initialScrollOffset: -2.5 * kItemHeight,
+      );
+
+      // 'container 0' is at offset -500
+      // 'container 1' is at offset -400
+      // 'container 2' is at offset -300
+      // 'container 3' is at offset -200
+      // 'container 4' is at offset -100
+      // 'container 5' is at offset 0
+
+      widgetUnderTest = new Directionality(
+        textDirection: TextDirection.ltr,
+        child: new Center(
+          child: new Container(
+            height: 2 * kItemHeight,
+            child: new Scrollable(
+              controller: scrollController,
+              viewportBuilder: (BuildContext context, ViewportOffset offset) {
+                return new Viewport(
+                  cacheExtent: 0.0,
+                  offset: offset,
+                  center: center,
+                  slivers: children
+                );
+              },
+            ),
+          ),
+        ),
+      );
+
+    });
+
+    testWidgets('brings item above leading edge to leading edge', (WidgetTester tester) async {
+      semantics = new SemanticsTester(tester); // enables semantics tree generation
+
+      await tester.pumpWidget(widgetUnderTest);
+
+      expect(scrollController.offset, -250.0);
+
+      final int firstContainerId = tester.renderObject(find.byKey(const ValueKey<int>(2))).debugSemantics.id;
+      tester.binding.pipelineOwner.semanticsOwner.performAction(firstContainerId, SemanticsAction.showOnScreen);
+      await tester.pumpAndSettle();
+
+      expect(scrollController.offset, -300.0);
+
+      semantics.dispose();
+    });
+
+    testWidgets('brings item below trailing edge to trailing edge', (WidgetTester tester) async {
+      semantics = new SemanticsTester(tester); // enables semantics tree generation
+
+      await tester.pumpWidget(widgetUnderTest);
+
+      expect(scrollController.offset, -250.0);
+
+      final int firstContainerId = tester.renderObject(find.byKey(const ValueKey<int>(4))).debugSemantics.id;
+      tester.binding.pipelineOwner.semanticsOwner.performAction(firstContainerId, SemanticsAction.showOnScreen);
+      await tester.pumpAndSettle();
+
+      expect(scrollController.offset, -200.0);
+
+      semantics.dispose();
+    });
+
+    testWidgets('does not change position of items already fully on-screen', (WidgetTester tester) async {
+      semantics = new SemanticsTester(tester); // enables semantics tree generation
+
+      await tester.pumpWidget(widgetUnderTest);
+
+      expect(scrollController.offset, -250.0);
+
+      final int firstContainerId = tester.renderObject(find.byKey(const ValueKey<int>(3))).debugSemantics.id;
+      tester.binding.pipelineOwner.semanticsOwner.performAction(firstContainerId, SemanticsAction.showOnScreen);
+      await tester.pumpAndSettle();
+
+      expect(scrollController.offset, -250.0);
+
+      semantics.dispose();
+    });
+
+  });
+
+
 }
 
 Future<Null> flingUp(WidgetTester tester, { int repetitions: 1 }) => fling(tester, const Offset(0.0, -200.0), repetitions);