Merge pull request #1401 from Hixie/lists

Make hit testing work in horizontal scrolling list

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

@@ -589,7 +589,7 @@ class PageableList<T> extends ScrollableList<T> {
     ItemBuilder<T> itemBuilder,
     bool itemsWrap: false,
     double itemExtent,
-    PageChangedCallback this.pageChanged,
+    this.onPageChanged,
     EdgeDims padding,
     this.duration: const Duration(milliseconds: 200),
     this.curve: ease
@@ -607,7 +607,7 @@ class PageableList<T> extends ScrollableList<T> {
 
   final Duration duration;
   final Curve curve;
-  final PageChangedCallback pageChanged;
+  final PageChangedCallback onPageChanged;
 
   PageableListState<T> createState() => new PageableListState();
 }
@@ -633,8 +633,8 @@ class PageableListState<T> extends ScrollableListState<T, PageableList<T>> {
   int get currentPage => (scrollOffset / config.itemExtent).floor() % itemCount;
 
   void _notifyPageChanged(_) {
-    if (config.pageChanged != null)
-      config.pageChanged(currentPage);
+    if (config.onPageChanged != null)
+      config.onPageChanged(currentPage);
   }
 
   void settleScrollOffset() {

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

@@ -377,11 +377,17 @@ class RenderBlockViewport extends RenderBlockBase {
 
   void applyPaintTransform(Matrix4 transform) {
     super.applyPaintTransform(transform);
-    transform.translate(0.0, startOffset);
+    if (isVertical)
+      transform.translate(0.0, startOffset);
+    else
+      transform.translate(startOffset, 0.0);
   }
 
   void hitTestChildren(HitTestResult result, { Point position }) {
-    defaultHitTestChildren(result, position: position + new Offset(0.0, -startOffset));
+    if (isVertical)
+      defaultHitTestChildren(result, position: position + new Offset(0.0, -startOffset));
+    else
+      defaultHitTestChildren(result, position: position + new Offset(-startOffset, 0.0));
   }
 
   String debugDescribeSettings(String prefix) => '${super.debugDescribeSettings(prefix)}${prefix}startOffset: ${startOffset}\n';

packages/unit/test/fn3/widget_tester.dart

@@ -25,15 +25,21 @@ class RootComponentState extends State<RootComponent> {
 
 class WidgetTester {
 
+  // See thttps://github.com/flutter/engine/issues/1084 regarding frameTimeMs vs FakeAsync
+
   void pumpFrame(Widget widget, [ double frameTimeMs = 0.0 ]) {
     runApp(widget);
-    scheduler.beginFrame(frameTimeMs); // TODO(ianh): https://github.com/flutter/engine/issues/1084
+    scheduler.beginFrame(frameTimeMs);
   }
 
   void pumpFrameWithoutChange([ double frameTimeMs = 0.0 ]) {
-    scheduler.beginFrame(frameTimeMs); // TODO(ianh): https://github.com/flutter/engine/issues/1084
+    scheduler.beginFrame(frameTimeMs);
   }
 
+  void reset() {
+    runApp(new Container());
+    scheduler.beginFrame(0.0);
+  }
 
   List<Layer> _layers(Layer layer) {
     List<Layer> result = [layer];

packages/unit/test/widget/pageable_list_test.dart

 import 'package:quiver/testing/async.dart';
-import 'package:sky/widgets.dart';
+import 'package:sky/src/fn3.dart';
 import 'package:test/test.dart';
 
-import 'widget_tester.dart';
+import '../fn3/widget_tester.dart';
 
 const Size pageSize = const Size(800.0, 600.0);
 const List<int> pages = const <int>[0, 1, 2, 3, 4, 5];
 int currentPage = null;
 bool itemsWrap = false;
 
-Widget buildPage(int page) {
+Widget buildPage(BuildContext context, int page) {
   return new Container(
     key: new ValueKey<int>(page),
     width: pageSize.width,
@@ -20,14 +20,14 @@ Widget buildPage(int page) {
 
 Widget buildFrame() {
   // The test framework forces the frame (and so the PageableList)
-  // to be 800x600. The pageSize constant reflects as much.
+  // to be 800x600. The pageSize constant reflects this.
   return new PageableList<int>(
     items: pages,
     itemBuilder: buildPage,
     itemsWrap: itemsWrap,
     itemExtent: pageSize.width,
     scrollDirection: ScrollDirection.horizontal,
-    pageChanged: (int page) { currentPage = page; }
+    onPageChanged: (int page) { currentPage = page; }
   );
 }
 
@@ -36,8 +36,8 @@ void page(WidgetTester tester, Offset offset) {
   new FakeAsync().run((async) {
     tester.scroll(tester.findText(itemText), offset);
     // One frame to start the animation, a second to complete it.
-    tester.pumpFrame(buildFrame);
-    tester.pumpFrame(buildFrame, 1000.0);
+    tester.pumpFrameWithoutChange();
+    tester.pumpFrameWithoutChange(1000.0);
     async.elapse(new Duration(seconds: 1));
   });
 }
@@ -57,42 +57,55 @@ void main() {
     WidgetTester tester = new WidgetTester();
     currentPage = null;
     itemsWrap = false;
-    tester.pumpFrame(buildFrame);
+    tester.pumpFrame(buildFrame());
     expect(currentPage, isNull);
     pageLeft(tester);
     expect(currentPage, equals(1));
   });
 
-  test('Underscroll (scroll right), return to page 0', () {
+  test('Scroll right from page 1 to page 0', () {
     WidgetTester tester = new WidgetTester();
-    currentPage = null;
     itemsWrap = false;
-    tester.pumpFrame(buildFrame);
-    expect(currentPage, isNull);
+    tester.pumpFrame(buildFrame());
+    expect(currentPage, equals(1));
     pageRight(tester);
     expect(currentPage, equals(0));
   });
 
-  // PageableList with itemsWrap: true
+  test('Scroll right from page 0 does nothing (underscroll)', () {
+    WidgetTester tester = new WidgetTester();
+    itemsWrap = false;
+    tester.pumpFrame(buildFrame());
+    expect(currentPage, equals(0));
+    pageRight(tester);
+    expect(currentPage, equals(0));
+  });
 
-  itemsWrap = true;
+  // PageableList with itemsWrap: true
 
   test('Scroll left page 0 to page 1, itemsWrap: true', () {
     WidgetTester tester = new WidgetTester();
+    tester.reset();
     currentPage = null;
     itemsWrap = true;
-    tester.pumpFrame(buildFrame);
+    tester.pumpFrame(buildFrame());
     expect(currentPage, isNull);
     pageLeft(tester);
     expect(currentPage, equals(1));
   });
 
-  test('Scroll right from page 0 to page 5, itemsWrap: true', () {
+  test('Scroll right from page 1 to page 0, itemsWrap: true', () {
     WidgetTester tester = new WidgetTester();
-    currentPage = null;
-    itemsWrap = true;
-    tester.pumpFrame(buildFrame);
-    expect(currentPage, isNull);
+    tester.pumpFrame(buildFrame());
+    expect(currentPage, equals(1));
+    pageRight(tester);
+    expect(currentPage, equals(0));
+  });
+
+  test('Scroll right from page 0 to page 5, itemsWrap: true (underscroll)', () {
+    WidgetTester tester = new WidgetTester();
+    tester.pumpFrame(buildFrame());
+    expect(currentPage, equals(0));
     pageRight(tester);
     expect(currentPage, equals(5));
   });

packages/unit/test/widget/scrollable_list_hit_testing_test.dart

+import 'package:quiver/testing/async.dart';
+import 'package:sky/rendering.dart';
+import 'package:sky/src/fn3.dart';
+import 'package:test/test.dart';
+
+import '../fn3/widget_tester.dart';
+
+const List<int> items = const <int>[0, 1, 2, 3, 4, 5];
+List<int> tapped = <int>[];
+
+Widget buildFrame() {
+  return ;
+}
+
+void main() {
+  double t = 0.0;
+  WidgetTester tester = new WidgetTester();
+
+  test('Tap item after scroll - horizontal', () {
+    tester.pumpFrame(new Center(
+      child: new Container(
+        height: 50.0,
+        child: new ScrollableList<int>(
+          key: new GlobalKey(),
+          items: items,
+          itemBuilder: (BuildContext context, int item) {
+            return new Container(
+              key: new ValueKey<int>(item),
+              child: new GestureDetector(
+                onTap: () { tapped.add(item); },
+                child: new Text('$item')
+              )
+            );
+          },
+          itemExtent: 290.0,
+          scrollDirection: ScrollDirection.horizontal
+        )
+      )
+    ), t);
+    tester.scroll(tester.findText('2'), const Offset(-280.0, 0.0));
+    tester.pumpFrameWithoutChange(t += 1000.0);
+    // screen is 800px wide, and has the following items:
+    //  -280..10  = 0
+    //    10..300 = 1
+    //   300..590 = 2
+    //   590..880 = 3
+    expect(tester.findText('0'), isNotNull);
+    expect(tester.findText('1'), isNotNull);
+    expect(tester.findText('2'), isNotNull);
+    expect(tester.findText('3'), isNotNull);
+    expect(tester.findText('4'), isNull);
+    expect(tester.findText('5'), isNull);
+    expect(tapped, equals([]));
+    tester.tap(tester.findText('2'));
+    expect(tapped, equals([2]));
+  });
+
+  test('Tap item after scroll - vertical', () {
+    tester.pumpFrame(new Center(
+      child: new Container(
+        width: 50.0,
+        child: new ScrollableList<int>(
+          key: new GlobalKey(),
+          items: items,
+          itemBuilder: (BuildContext context, int item) {
+            return new Container(
+              key: new ValueKey<int>(item),
+              child: new GestureDetector(
+                onTap: () { tapped.add(item); },
+                child: new Text('$item')
+              )
+            );
+          },
+          itemExtent: 290.0,
+          scrollDirection: ScrollDirection.vertical
+        )
+      )
+    ), t);
+    tester.scroll(tester.findText('1'), const Offset(0.0, -280.0));
+    tester.pumpFrameWithoutChange(t += 1000.0);
+    // screen is 600px tall, and has the following items:
+    //  -280..10  = 0
+    //    10..300 = 1
+    //   300..590 = 2
+    //   590..880 = 3
+    expect(tester.findText('0'), isNotNull);
+    expect(tester.findText('1'), isNotNull);
+    expect(tester.findText('2'), isNotNull);
+    expect(tester.findText('3'), isNotNull);
+    expect(tester.findText('4'), isNull);
+    expect(tester.findText('5'), isNull);
+    expect(tapped, equals([2]));
+    tester.tap(tester.findText('1'));
+    expect(tapped, equals([2, 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
+  });
+
+}

packages/unit/test/widget/scrollable_list_horizontal_test.dart

+import 'package:quiver/testing/async.dart';
+import 'package:sky/rendering.dart';
+import 'package:sky/src/fn3.dart';
+import 'package:test/test.dart';
+
+import '../fn3/widget_tester.dart';
+
+const List<int> items = const <int>[0, 1, 2, 3, 4, 5];
+
+Widget buildFrame() {
+  return new Center(
+    child: new Container(
+      height: 50.0,
+      child: new ScrollableList<int>(
+        items: items,
+        itemBuilder: (BuildContext context, int item) {
+          return new Container(
+            key: new ValueKey<int>(item),
+            child: new Text('$item')
+          );
+        },
+        itemExtent: 290.0,
+        scrollDirection: ScrollDirection.horizontal
+      )
+    )
+  );
+}
+
+void main() {
+  double t = 0.0;
+  WidgetTester tester = new WidgetTester();
+  tester.pumpFrame(buildFrame());
+
+  test('Drag to the left using item 1', () {
+    tester.pumpFrameWithoutChange(t += 1000.0);
+    tester.scroll(tester.findText('1'), const Offset(-300.0, 0.0));
+    tester.pumpFrameWithoutChange(t += 1000.0);
+    // screen is 800px wide, and has the following items:
+    //   -10..280 = 1
+    //   280..570 = 2
+    //   570..860 = 3
+    expect(tester.findText('0'), isNull);
+    expect(tester.findText('1'), isNotNull);
+    expect(tester.findText('2'), isNotNull);
+    expect(tester.findText('3'), isNotNull);
+    expect(tester.findText('4'), isNull);
+    expect(tester.findText('5'), isNull);
+  });
+
+  test('Drag to the left using item 3', () {
+    // the center of item 3 is visible, so this works;
+    // if item 3 was a bit wider, such that it's center was past the 800px mark, this would fail,
+    // because it wouldn't be hit tested when scrolling from its center, as scroll() does.
+    tester.pumpFrameWithoutChange(t += 1000.0);
+    tester.scroll(tester.findText('3'), const Offset(-290.0, 0.0));
+    tester.pumpFrameWithoutChange(t += 1000.0);
+    // screen is 800px wide, and has the following items:
+    //   -10..280 = 2
+    //   280..570 = 3
+    //   570..860 = 4
+    expect(tester.findText('0'), isNull);
+    expect(tester.findText('1'), isNull);
+    expect(tester.findText('2'), isNotNull);
+    expect(tester.findText('3'), isNotNull);
+    expect(tester.findText('4'), isNotNull);
+    expect(tester.findText('5'), isNull);
+  });
+
+  test('Drag up using item 3', () {
+    tester.pumpFrameWithoutChange(t += 1000.0);
+    tester.scroll(tester.findText('3'), const Offset(0.0, -290.0));
+    tester.pumpFrameWithoutChange(t += 1000.0);
+    // unchanged
+    expect(tester.findText('0'), isNull);
+    expect(tester.findText('1'), isNull);
+    expect(tester.findText('2'), isNotNull);
+    expect(tester.findText('3'), isNotNull);
+    expect(tester.findText('4'), isNotNull);
+    expect(tester.findText('5'), isNull);
+  });
+
+  test('Drag to the left using item 3 again', () {
+    tester.pumpFrameWithoutChange(t += 1000.0);
+    tester.scroll(tester.findText('3'), const Offset(-290.0, 0.0));
+    tester.pumpFrameWithoutChange(t += 1000.0);
+    // screen is 800px wide, and has the following items:
+    //   -10..280 = 3
+    //   280..570 = 4
+    //   570..860 = 5
+    expect(tester.findText('0'), isNull);
+    expect(tester.findText('1'), isNull);
+    expect(tester.findText('2'), isNull);
+    expect(tester.findText('3'), isNotNull);
+    expect(tester.findText('4'), isNotNull);
+    expect(tester.findText('5'), isNotNull);
+  });
+
+  test('Drag to the left using item 3 again again (past the end of the list)', () {
+    tester.pumpFrameWithoutChange(t += 1000.0);
+    // at this point we can drag 60 pixels further before we hit the friction zone
+    // then, every pixel we drag is equivalent to half a pixel of movement
+    // to move item 3 entirely off screen therefore takes:
+    //  60 + (290-60)*2 = 520 pixels
+    // plus a couple more to be sure
+    tester.scroll(tester.findText('3'), const Offset(-522.0, 0.0));
+    tester.pumpFrameWithoutChange(t += 0.0); // just after release
+    // screen is 800px wide, and has the following items:
+    //   -11..279 = 4
+    //   279..569 = 5
+    expect(tester.findText('0'), isNull);
+    expect(tester.findText('1'), isNull);
+    expect(tester.findText('2'), isNull);
+    expect(tester.findText('3'), isNull);
+    expect(tester.findText('4'), isNotNull);
+    expect(tester.findText('5'), isNotNull);
+    tester.pumpFrameWithoutChange(t += 1000.0); // a second after release
+    // screen is 800px wide, and has the following items:
+    //   -70..220 = 3
+    //   220..510 = 4
+    //   510..800 = 5
+    expect(tester.findText('0'), isNull);
+    expect(tester.findText('1'), isNull);
+    expect(tester.findText('2'), isNull);
+    expect(tester.findText('3'), isNotNull);
+    expect(tester.findText('4'), isNotNull);
+    expect(tester.findText('5'), isNotNull);
+  });
+
+  test('Drag to the left using item 2 when the scroll offset is big', () {
+    tester.reset();
+    tester.pumpFrame(buildFrame(), t += 1000.0);
+    tester.scroll(tester.findText('2'), const Offset(-280.0, 0.0));
+    tester.pumpFrameWithoutChange(t += 1000.0);
+    // screen is 800px wide, and has the following items:
+    //  -280..10  = 0
+    //    10..300 = 1
+    //   300..590 = 2
+    //   590..880 = 3
+    expect(tester.findText('0'), isNotNull);
+    expect(tester.findText('1'), isNotNull);
+    expect(tester.findText('2'), isNotNull);
+    expect(tester.findText('3'), isNotNull);
+    expect(tester.findText('4'), isNull);
+    expect(tester.findText('5'), isNull);
+    tester.pumpFrameWithoutChange(t += 1000.0);
+    tester.scroll(tester.findText('2'), const Offset(-290.0, 0.0));
+    tester.pumpFrameWithoutChange(t += 1000.0);
+    // screen is 800px wide, and has the following items:
+    //  -280..10  = 1
+    //    10..300 = 2
+    //   300..590 = 3
+    //   590..880 = 4
+    expect(tester.findText('0'), isNull);
+    expect(tester.findText('1'), isNotNull);
+    expect(tester.findText('2'), isNotNull);
+    expect(tester.findText('3'), isNotNull);
+    expect(tester.findText('4'), isNotNull);
+    expect(tester.findText('5'), isNull);
+  });
+
+}

packages/unit/test/widget/scrollable_list_vertical_test.dart

+import 'package:quiver/testing/async.dart';
+import 'package:sky/src/fn3.dart';
+import 'package:test/test.dart';
+
+import '../fn3/widget_tester.dart';
+
+const List<int> items = const <int>[0, 1, 2, 3, 4, 5];
+
+Widget buildFrame() {
+  return new ScrollableList<int>(
+    items: items,
+    itemBuilder: (BuildContext context, int item) {
+      return new Container(
+        key: new ValueKey<int>(item),
+        child: new Text('$item')
+      );
+    },
+    itemExtent: 290.0,
+    scrollDirection: ScrollDirection.vertical
+  );
+}
+
+void main() {
+  WidgetTester tester = new WidgetTester();
+  tester.pumpFrame(buildFrame());
+
+  test('Drag up using item 1', () {
+    tester.pumpFrameWithoutChange();
+    tester.scroll(tester.findText('1'), const Offset(0.0, -300.0));
+    tester.pumpFrameWithoutChange();
+    // screen is 600px high, and has the following items:
+    //   -10..280 = 1
+    //   280..570 = 2
+    //   570..860 = 3
+    expect(tester.findText('0'), isNull);
+    expect(tester.findText('1'), isNotNull);
+    expect(tester.findText('2'), isNotNull);
+    expect(tester.findText('3'), isNotNull);
+    expect(tester.findText('4'), isNull);
+    expect(tester.findText('5'), isNull);
+  });
+
+  test('Drag up using item 2', () {
+    tester.pumpFrameWithoutChange();
+    tester.scroll(tester.findText('2'), const Offset(0.0, -290.0));
+    tester.pumpFrameWithoutChange();
+    // screen is 600px high, and has the following items:
+    //   -10..280 = 2
+    //   280..570 = 3
+    //   570..860 = 4
+    expect(tester.findText('0'), isNull);
+    expect(tester.findText('1'), isNull);
+    expect(tester.findText('2'), isNotNull);
+    expect(tester.findText('3'), isNotNull);
+    expect(tester.findText('4'), isNotNull);
+    expect(tester.findText('5'), isNull);
+  });
+
+  test('Drag to the left using item 3', () {
+    tester.pumpFrameWithoutChange();
+    tester.scroll(tester.findText('3'), const Offset(-300.0, 0.0));
+    tester.pumpFrameWithoutChange();
+    // nothing should have changed
+    expect(tester.findText('0'), isNull);
+    expect(tester.findText('1'), isNull);
+    expect(tester.findText('2'), isNotNull);
+    expect(tester.findText('3'), isNotNull);
+    expect(tester.findText('4'), isNotNull);
+    expect(tester.findText('5'), isNull);
+  });
+
+}