Fix padding for ScrollableList

Previously we had totally wrong behavior. Now we have more correct behavior and
testing.

Fixes #1808

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

@@ -142,13 +142,13 @@ class RenderList extends RenderVirtualViewport<ListParentData> {
       case Axis.vertical:
         itemWidth = math.max(0.0, size.width - (padding == null ? 0.0 : padding.horizontal));
         itemHeight = itemExtent ?? size.height;
-        y = padding != null ? padding.top : 0.0;
+        x = padding != null ? padding.left : 0.0;
         dy = itemHeight;
         break;
       case Axis.horizontal:
         itemWidth = itemExtent ?? size.width;
         itemHeight = math.max(0.0, size.height - (padding == null ? 0.0 : padding.vertical));
-        x = padding != null ? padding.left : 0.0;
+        y = padding != null ? padding.top : 0.0;
         dx = itemWidth;
         break;
     }

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

@@ -79,7 +79,7 @@ class _ScrollableListState extends ScrollableState<ScrollableList> {
   Widget buildContent(BuildContext context) {
     return new ListViewport(
       onExtentsChanged: _handleExtentsChanged,
-      startOffset: scrollOffset,
+      scrollOffset: scrollOffset,
       scrollDirection: config.scrollDirection,
       scrollAnchor: config.scrollAnchor,
       itemExtent: config.itemExtent,
@@ -94,7 +94,7 @@ class _ScrollableListState extends ScrollableState<ScrollableList> {
 class _VirtualListViewport extends VirtualViewport {
   _VirtualListViewport(
     this.onExtentsChanged,
-    this.startOffset,
+    this.scrollOffset,
     this.scrollDirection,
     this.scrollAnchor,
     this.itemExtent,
@@ -107,7 +107,7 @@ class _VirtualListViewport extends VirtualViewport {
   }
 
   final ExtentsChangedCallback onExtentsChanged;
-  final double startOffset;
+  final double scrollOffset;
   final Axis scrollDirection;
   final ViewportAnchor scrollAnchor;
   final double itemExtent;
@@ -115,6 +115,33 @@ class _VirtualListViewport extends VirtualViewport {
   final EdgeDims padding;
   final Painter overlayPainter;
 
+  double get _leadingPadding {
+    switch (scrollDirection) {
+      case Axis.vertical:
+        switch (scrollAnchor) {
+          case ViewportAnchor.start:
+            return padding.top;
+          case ViewportAnchor.end:
+            return padding.bottom;
+        }
+        break;
+      case Axis.horizontal:
+        switch (scrollAnchor) {
+          case ViewportAnchor.start:
+            return padding.left;
+          case ViewportAnchor.end:
+            return padding.right;
+        }
+        break;
+    }
+  }
+
+  double get startOffset {
+    if (padding == null)
+      return scrollOffset;
+    return scrollOffset - _leadingPadding;
+  }
+
   RenderList createRenderObject() => new RenderList(itemExtent: itemExtent);
 
   _VirtualListViewportElement createElement() => new _VirtualListViewportElement(this);
@@ -158,32 +185,15 @@ class _VirtualListViewportElement extends VirtualViewportElement<_VirtualListVie
 
     double containerExtent;
     double contentExtent;
-    double leadingPadding;
 
     switch (widget.scrollDirection) {
       case Axis.vertical:
         containerExtent = containerSize.height;
         contentExtent = length == null ? double.INFINITY : widget.itemExtent * length + padding.vertical;
-        switch (widget.scrollAnchor) {
-          case ViewportAnchor.start:
-            leadingPadding = padding.top;
-            break;
-          case ViewportAnchor.end:
-            leadingPadding = padding.bottom;
-            break;
-        }
         break;
       case Axis.horizontal:
         containerExtent = renderObject.size.width;
         contentExtent = length == null ? double.INFINITY : widget.itemExtent * length + padding.horizontal;
-        switch (widget.scrollAnchor) {
-          case ViewportAnchor.start:
-            leadingPadding = padding.left;
-            break;
-          case ViewportAnchor.end:
-            leadingPadding = padding.right;
-            break;
-        }
         break;
     }
 
@@ -193,8 +203,9 @@ class _VirtualListViewportElement extends VirtualViewportElement<_VirtualListVie
       _startOffsetBase = 0.0;
       _startOffsetLimit = double.INFINITY;
     } else {
-      int startItem = math.max(0, (widget.startOffset - leadingPadding) ~/ itemExtent);
-      int limitItem = math.max(0, ((widget.startOffset - leadingPadding + containerExtent) / itemExtent).ceil());
+      final double startOffset = widget.startOffset;
+      int startItem = math.max(0, startOffset ~/ itemExtent);
+      int limitItem = math.max(0, ((startOffset + containerExtent) / itemExtent).ceil());
 
       if (!widget.itemsWrap && length != null) {
         startItem = math.min(length, startItem);
@@ -234,7 +245,7 @@ class _VirtualListViewportElement extends VirtualViewportElement<_VirtualListVie
 class ListViewport extends _VirtualListViewport with VirtualViewportIterableMixin {
   ListViewport({
     ExtentsChangedCallback onExtentsChanged,
-    double startOffset: 0.0,
+    double scrollOffset: 0.0,
     Axis scrollDirection: Axis.vertical,
     ViewportAnchor scrollAnchor: ViewportAnchor.start,
     double itemExtent,
@@ -244,7 +255,7 @@ class ListViewport extends _VirtualListViewport with VirtualViewportIterableMixi
     this.children
   }) : super(
     onExtentsChanged,
-    startOffset,
+    scrollOffset,
     scrollDirection,
     scrollAnchor,
     itemExtent,
@@ -331,7 +342,7 @@ class _ScrollableLazyListState extends ScrollableState<ScrollableLazyList> {
   Widget buildContent(BuildContext context) {
     return new LazyListViewport(
       onExtentsChanged: _handleExtentsChanged,
-      startOffset: scrollOffset,
+      scrollOffset: scrollOffset,
       scrollDirection: config.scrollDirection,
       scrollAnchor: config.scrollAnchor,
       itemExtent: config.itemExtent,
@@ -346,7 +357,7 @@ class _ScrollableLazyListState extends ScrollableState<ScrollableLazyList> {
 class LazyListViewport extends _VirtualListViewport with VirtualViewportLazyMixin {
   LazyListViewport({
     ExtentsChangedCallback onExtentsChanged,
-    double startOffset: 0.0,
+    double scrollOffset: 0.0,
     Axis scrollDirection: Axis.vertical,
     ViewportAnchor scrollAnchor: ViewportAnchor.start,
     double itemExtent,
@@ -356,7 +367,7 @@ class LazyListViewport extends _VirtualListViewport with VirtualViewportLazyMixi
     this.itemBuilder
   }) : super(
     onExtentsChanged,
-    startOffset,
+    scrollOffset,
     scrollDirection,
     scrollAnchor,
     itemExtent,

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

@@ -102,18 +102,19 @@ abstract class VirtualViewportElement<T extends VirtualViewport> extends RenderO
       // If we don't already need layout, we need to request a layout if the
       // viewport has shifted to expose new children.
       if (!renderObject.needsLayout) {
+        final double startOffset = widget.startOffset;
         bool shouldLayout = false;
         if (startOffsetBase != null) {
-          if (widget.startOffset < startOffsetBase)
+          if (startOffset < startOffsetBase)
             shouldLayout = true;
-          else if (widget.startOffset == startOffsetBase && oldWidget?.startOffset != startOffsetBase)
+          else if (startOffset == startOffsetBase && oldWidget?.startOffset != startOffsetBase)
             shouldLayout = true;
         }
 
         if (startOffsetLimit != null) {
-          if (widget.startOffset > startOffsetLimit)
+          if (startOffset > startOffsetLimit)
             shouldLayout = true;
-          else if (widget.startOffset == startOffsetLimit && oldWidget?.startOffset != startOffsetLimit)
+          else if (startOffset == startOffsetLimit && oldWidget?.startOffset != startOffsetLimit)
             shouldLayout = true;
         }
 

packages/flutter/test/widget/scrollable_list_hit_testing_test.dart

@@ -8,12 +8,11 @@ import 'package:flutter/widgets.dart';
 import 'package:test/test.dart';
 
 const List<int> items = const <int>[0, 1, 2, 3, 4, 5];
-List<int> tapped = <int>[];
 
 void main() {
   test('Tap item after scroll - horizontal', () {
     testWidgets((WidgetTester tester) {
-      tester.pumpWidget(new Container());
+      List<int> tapped = <int>[];
       tester.pumpWidget(new Center(
         child: new Container(
           height: 50.0,
@@ -53,7 +52,7 @@ void main() {
 
   test('Tap item after scroll - vertical', () {
     testWidgets((WidgetTester tester) {
-      tester.pumpWidget(new Container());
+      List<int> tapped = <int>[];
       tester.pumpWidget(new Center(
         child: new Container(
           width: 50.0,
@@ -85,11 +84,80 @@ void main() {
       expect(tester.findText('3'), isNotNull);
       expect(tester.findText('4'), isNull);
       expect(tester.findText('5'), isNull);
-      expect(tapped, equals([2]));
+      expect(tapped, equals([]));
       tester.tap(tester.findText('1'));
-      expect(tapped, equals([2, 1]));
+      expect(tapped, equals([1]));
       tester.tap(tester.findText('3'));
-      expect(tapped, equals([2, 1])); // the center of the third item is off-screen so it shouldn't get hit
+      expect(tapped, equals([1])); // the center of the third item is off-screen so it shouldn't get hit
+    });
+  });
+
+  test('Padding scroll anchor start', () {
+    testWidgets((WidgetTester tester) {
+      List<int> tapped = <int>[];
+
+      tester.pumpWidget(
+        new ScrollableList(
+          key: new GlobalKey(),
+          itemExtent: 290.0,
+          padding: new EdgeDims.TRBL(20.0, 15.0, 10.0, 5.0),
+          children: items.map((int item) {
+            return new Container(
+              child: new GestureDetector(
+                onTap: () { tapped.add(item); },
+                child: new Text('$item')
+              )
+            );
+          })
+        )
+      );
+      tester.tapAt(new Point(200.0, 19.0));
+      expect(tapped, equals([]));
+      tester.tapAt(new Point(200.0, 21.0));
+      expect(tapped, equals([0]));
+      tester.tapAt(new Point(4.0, 400.0));
+      expect(tapped, equals([0]));
+      tester.tapAt(new Point(6.0, 400.0));
+      expect(tapped, equals([0, 1]));
+      tester.tapAt(new Point(800.0 - 14.0, 400.0));
+      expect(tapped, equals([0, 1]));
+      tester.tapAt(new Point(800.0 - 16.0, 400.0));
+      expect(tapped, equals([0, 1, 1]));
+    });
+  });
+
+  test('Padding scroll anchor end', () {
+    testWidgets((WidgetTester tester) {
+      List<int> tapped = <int>[];
+
+      tester.pumpWidget(
+        new ScrollableList(
+          key: new GlobalKey(),
+          itemExtent: 290.0,
+          scrollAnchor: ViewportAnchor.end,
+          padding: new EdgeDims.TRBL(20.0, 15.0, 10.0, 5.0),
+          children: items.map((int item) {
+            return new Container(
+              child: new GestureDetector(
+                onTap: () { tapped.add(item); },
+                child: new Text('$item')
+              )
+            );
+          })
+        )
+      );
+      tester.tapAt(new Point(200.0, 600.0 - 9.0));
+      expect(tapped, equals([]));
+      tester.tapAt(new Point(200.0, 600.0 - 11.0));
+      expect(tapped, equals([5]));
+      tester.tapAt(new Point(4.0, 200.0));
+      expect(tapped, equals([5]));
+      tester.tapAt(new Point(6.0, 200.0));
+      expect(tapped, equals([5, 4]));
+      tester.tapAt(new Point(800.0 - 14.0, 200.0));
+      expect(tapped, equals([5, 4]));
+      tester.tapAt(new Point(800.0 - 16.0, 200.0));
+      expect(tapped, equals([5, 4, 4]));
     });
   });
 }