From ad2fa5a6e70e43c4a794e00acc7015b030b24660 Mon Sep 17 00:00:00 2001 From: Adam Barth Date: Fri, 12 Feb 2016 13:38:29 -0800 Subject: [PATCH] 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 | 4 +- .../lib/src/widgets/scrollable_list.dart | 65 ++++++++------- .../lib/src/widgets/virtual_viewport.dart | 9 ++- .../scrollable_list_hit_testing_test.dart | 80 +++++++++++++++++-- 4 files changed, 119 insertions(+), 39 deletions(-) diff --git a/packages/flutter/lib/src/rendering/list.dart b/packages/flutter/lib/src/rendering/list.dart index 0724e2bbe8..48a867e00d 100644 --- a/packages/flutter/lib/src/rendering/list.dart +++ b/packages/flutter/lib/src/rendering/list.dart @@ -142,13 +142,13 @@ class RenderList extends RenderVirtualViewport { 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; } diff --git a/packages/flutter/lib/src/widgets/scrollable_list.dart b/packages/flutter/lib/src/widgets/scrollable_list.dart index 363a03a990..b1e49fe460 100644 --- a/packages/flutter/lib/src/widgets/scrollable_list.dart +++ b/packages/flutter/lib/src/widgets/scrollable_list.dart @@ -79,7 +79,7 @@ class _ScrollableListState extends ScrollableState { 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 { 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 { 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 { 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, diff --git a/packages/flutter/lib/src/widgets/virtual_viewport.dart b/packages/flutter/lib/src/widgets/virtual_viewport.dart index c45ca5dc77..e55cb9eb01 100644 --- a/packages/flutter/lib/src/widgets/virtual_viewport.dart +++ b/packages/flutter/lib/src/widgets/virtual_viewport.dart @@ -102,18 +102,19 @@ abstract class VirtualViewportElement 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; } diff --git a/packages/flutter/test/widget/scrollable_list_hit_testing_test.dart b/packages/flutter/test/widget/scrollable_list_hit_testing_test.dart index e028e00cf4..2ae288bd72 100644 --- a/packages/flutter/test/widget/scrollable_list_hit_testing_test.dart +++ b/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 items = const [0, 1, 2, 3, 4, 5]; -List tapped = []; void main() { test('Tap item after scroll - horizontal', () { testWidgets((WidgetTester tester) { - tester.pumpWidget(new Container()); + List tapped = []; 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 tapped = []; 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 tapped = []; + + 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 tapped = []; + + 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])); }); }); }