diff --git a/packages/flutter/lib/src/rendering/sliver.dart b/packages/flutter/lib/src/rendering/sliver.dart index ebf6391304..17fb94c3d6 100644 --- a/packages/flutter/lib/src/rendering/sliver.dart +++ b/packages/flutter/lib/src/rendering/sliver.dart @@ -928,13 +928,16 @@ class SliverLogicalParentData extends ParentData { /// /// The number of pixels from from the zero scroll offset of the parent sliver /// (the line at which its [SliverConstraints.scrollOffset] is zero) to the - /// side of the child closest to that offset. + /// side of the child closest to that offset. A [layoutOffset] can be null + /// when it cannot be determined. The value will be set after layout. /// /// In a typical list, this does not change as the parent is scrolled. - double layoutOffset = 0.0; + /// + /// Defaults to null. + double layoutOffset; @override - String toString() => 'layoutOffset=${layoutOffset.toStringAsFixed(1)}'; + String toString() => 'layoutOffset=${layoutOffset == null ? 'None': layoutOffset.toStringAsFixed(1)}'; } /// Parent data for slivers that have multiple children and that position their diff --git a/packages/flutter/lib/src/rendering/sliver_list.dart b/packages/flutter/lib/src/rendering/sliver_list.dart index 4635d84202..cdc9c1a821 100644 --- a/packages/flutter/lib/src/rendering/sliver_list.dart +++ b/packages/flutter/lib/src/rendering/sliver_list.dart @@ -91,8 +91,28 @@ class RenderSliverList extends RenderSliverMultiBoxAdaptor { // it's possible for a child to get removed without notice. RenderBox leadingChildWithLayout, trailingChildWithLayout; - // Find the last child that is at or before the scrollOffset. RenderBox earliestUsefulChild = firstChild; + + // A firstChild with null layout offset is likely a result of children + // reordering. + // + // We rely on firstChild to have accurate layout offset. In the case of null + // layout offset, we have to find the first child that has valid layout + // offset. + if (childScrollOffset(firstChild) == null) { + int leadingChildrenWithoutLayoutOffset = 0; + while (childScrollOffset(earliestUsefulChild) == null) { + earliestUsefulChild = childAfter(firstChild); + leadingChildrenWithoutLayoutOffset += 1; + } + // We should be able to destroy children with null layout offset safely, + // because they are likely outside of viewport + collectGarbage(leadingChildrenWithoutLayoutOffset, 0); + assert(firstChild != null); + } + + // Find the last child that is at or before the scrollOffset. + earliestUsefulChild = firstChild; for (double earliestScrollOffset = childScrollOffset(earliestUsefulChild); earliestScrollOffset > scrollOffset; earliestScrollOffset = childScrollOffset(earliestUsefulChild)) { @@ -140,12 +160,15 @@ class RenderSliverList extends RenderSliverMultiBoxAdaptor { correction += paintExtentOf(firstChild); earliestUsefulChild = insertAndLayoutLeadingChild(childConstraints, parentUsesSize: true); } - geometry = SliverGeometry( - scrollOffsetCorrection: correction - earliestScrollOffset, - ); - final SliverMultiBoxAdaptorParentData childParentData = firstChild.parentData as SliverMultiBoxAdaptorParentData; - childParentData.layoutOffset = 0.0; - return; + earliestUsefulChild = firstChild; + if ((correction - earliestScrollOffset).abs() > precisionErrorTolerance) { + geometry = SliverGeometry( + scrollOffsetCorrection: correction - earliestScrollOffset, + ); + final SliverMultiBoxAdaptorParentData childParentData = firstChild.parentData as SliverMultiBoxAdaptorParentData; + childParentData.layoutOffset = 0.0; + return; + } } final SliverMultiBoxAdaptorParentData childParentData = earliestUsefulChild.parentData as SliverMultiBoxAdaptorParentData; diff --git a/packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart b/packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart index 037b342dcf..95fbc41047 100644 --- a/packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart +++ b/packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart @@ -578,7 +578,6 @@ abstract class RenderSliverMultiBoxAdaptor extends RenderSliver assert(child != null); assert(child.parent == this); final SliverMultiBoxAdaptorParentData childParentData = child.parentData as SliverMultiBoxAdaptorParentData; - assert(childParentData.layoutOffset != null); return childParentData.layoutOffset; } diff --git a/packages/flutter/lib/src/widgets/sliver.dart b/packages/flutter/lib/src/widgets/sliver.dart index 5a27f84bd6..3e264ffc3f 100644 --- a/packages/flutter/lib/src/widgets/sliver.dart +++ b/packages/flutter/lib/src/widgets/sliver.dart @@ -1069,6 +1069,7 @@ class SliverMultiBoxAdaptorElement extends RenderObjectElement implements Render assert(_currentlyUpdatingChildIndex == null); try { final SplayTreeMap newChildren = SplayTreeMap(); + final Map indexToLayoutOffset = HashMap(); void processElement(int index) { _currentlyUpdatingChildIndex = index; @@ -1080,17 +1081,31 @@ class SliverMultiBoxAdaptorElement extends RenderObjectElement implements Render if (newChild != null) { _childElements[index] = newChild; final SliverMultiBoxAdaptorParentData parentData = newChild.renderObject.parentData as SliverMultiBoxAdaptorParentData; + if (index == 0) { + parentData.layoutOffset = 0.0; + } else if (indexToLayoutOffset.containsKey(index)) { + parentData.layoutOffset = indexToLayoutOffset[index]; + } if (!parentData.keptAlive) _currentBeforeChild = newChild.renderObject as RenderBox; } else { _childElements.remove(index); } } - for (final int index in _childElements.keys.toList()) { final Key key = _childElements[index].widget.key; final int newIndex = key == null ? null : widget.delegate.findIndexByKey(key); + final SliverMultiBoxAdaptorParentData childParentData = + _childElements[index].renderObject?.parentData as SliverMultiBoxAdaptorParentData; + + if (childParentData != null && childParentData.layoutOffset != null) + indexToLayoutOffset[index] = childParentData.layoutOffset; + if (newIndex != null && newIndex != index) { + // The layout offset of the child being moved is no longer accurate. + if (childParentData != null) + childParentData.layoutOffset = null; + newChildren[newIndex] = _childElements[index]; // We need to make sure the original index gets processed. newChildren.putIfAbsent(index, () => null); @@ -1309,7 +1324,8 @@ class SliverMultiBoxAdaptorElement extends RenderObjectElement implements Render break; } - return parentData.layoutOffset < renderObject.constraints.scrollOffset + renderObject.constraints.remainingPaintExtent && + return parentData.layoutOffset != null && + parentData.layoutOffset < renderObject.constraints.scrollOffset + renderObject.constraints.remainingPaintExtent && parentData.layoutOffset + itemExtent > renderObject.constraints.scrollOffset; }).forEach(visitor); } diff --git a/packages/flutter/test/rendering/slivers_block_test.dart b/packages/flutter/test/rendering/slivers_block_test.dart index 7bfb6881e0..a7a3cc5850 100644 --- a/packages/flutter/test/rendering/slivers_block_test.dart +++ b/packages/flutter/test/rendering/slivers_block_test.dart @@ -340,18 +340,20 @@ void main() { final SliverMultiBoxAdaptorParentData candidate = SliverMultiBoxAdaptorParentData(); expect(candidate.keepAlive, isFalse); expect(candidate.index, isNull); - expect(candidate.toString(), 'index=null; layoutOffset=0.0'); + expect(candidate.toString(), 'index=null; layoutOffset=None'); candidate.keepAlive = null; - expect(candidate.toString(), 'index=null; layoutOffset=0.0'); + expect(candidate.toString(), 'index=null; layoutOffset=None'); candidate.keepAlive = true; - expect(candidate.toString(), 'index=null; keepAlive; layoutOffset=0.0'); + expect(candidate.toString(), 'index=null; keepAlive; layoutOffset=None'); candidate.keepAlive = false; - expect(candidate.toString(), 'index=null; layoutOffset=0.0'); + expect(candidate.toString(), 'index=null; layoutOffset=None'); candidate.index = 0; - expect(candidate.toString(), 'index=0; layoutOffset=0.0'); + expect(candidate.toString(), 'index=0; layoutOffset=None'); candidate.index = 1; - expect(candidate.toString(), 'index=1; layoutOffset=0.0'); + expect(candidate.toString(), 'index=1; layoutOffset=None'); candidate.index = -1; - expect(candidate.toString(), 'index=-1; layoutOffset=0.0'); + expect(candidate.toString(), 'index=-1; layoutOffset=None'); + candidate.layoutOffset = 100.0; + expect(candidate.toString(), 'index=-1; layoutOffset=100.0'); }); } diff --git a/packages/flutter/test/widgets/sliver_list_test.dart b/packages/flutter/test/widgets/sliver_list_test.dart index 3c1e0720ed..65cb8e9bc4 100644 --- a/packages/flutter/test/widgets/sliver_list_test.dart +++ b/packages/flutter/test/widgets/sliver_list_test.dart @@ -163,6 +163,118 @@ void main() { expect(find.text('Tile 1'), findsOneWidget); expect(find.text('Tile 2'), findsOneWidget); }); + + testWidgets('SliverList should recalculate inaccurate layout offset case 1', (WidgetTester tester) async { + // Regression test for https://github.com/flutter/flutter/issues/42142. + final List items = List.generate(20, (int i) => i); + final ScrollController controller = ScrollController(); + await tester.pumpWidget( + _buildSliverList( + items: List.from(items), + controller: controller, + itemHeight: 50, + viewportHeight: 200, + ) + ); + await tester.pumpAndSettle(); + + await tester.drag(find.text('Tile 2'), const Offset(0.0, -1000.0)); + await tester.pumpAndSettle(); + + // Viewport should be scrolled to the end of list. + expect(controller.offset, 800.0); + expect(find.text('Tile 15'), findsNothing); + expect(find.text('Tile 16'), findsOneWidget); + expect(find.text('Tile 17'), findsOneWidget); + expect(find.text('Tile 18'), findsOneWidget); + expect(find.text('Tile 19'), findsOneWidget); + + // Prepends item to the list. + items.insert(0, -1); + await tester.pumpWidget( + _buildSliverList( + items: List.from(items), + controller: controller, + itemHeight: 50, + viewportHeight: 200, + ) + ); + await tester.pump(); + // We need second pump to ensure the scheduled animation gets run. + await tester.pumpAndSettle(); + // Scroll offset should stay the same, and the items in viewport should be + // shifted by one. + expect(controller.offset, 800.0); + expect(find.text('Tile 14'), findsNothing); + expect(find.text('Tile 15'), findsOneWidget); + expect(find.text('Tile 16'), findsOneWidget); + expect(find.text('Tile 17'), findsOneWidget); + expect(find.text('Tile 18'), findsOneWidget); + expect(find.text('Tile 19'), findsNothing); + + // Drags back to beginning and newly added item is visible. + await tester.drag(find.text('Tile 16'), const Offset(0.0, 1000.0)); + await tester.pumpAndSettle(); + expect(controller.offset, 0.0); + expect(find.text('Tile -1'), findsOneWidget); + expect(find.text('Tile 0'), findsOneWidget); + expect(find.text('Tile 1'), findsOneWidget); + expect(find.text('Tile 2'), findsOneWidget); + expect(find.text('Tile 3'), findsNothing); + + }); + + testWidgets('SliverList should recalculate inaccurate layout offset case 2', (WidgetTester tester) async { + // Regression test for https://github.com/flutter/flutter/issues/42142. + final List items = List.generate(20, (int i) => i); + final ScrollController controller = ScrollController(); + await tester.pumpWidget( + _buildSliverList( + items: List.from(items), + controller: controller, + itemHeight: 50, + viewportHeight: 200, + ) + ); + await tester.pumpAndSettle(); + + await tester.drag(find.text('Tile 2'), const Offset(0.0, -1000.0)); + await tester.pumpAndSettle(); + + // Viewport should be scrolled to the end of list. + expect(controller.offset, 800.0); + expect(find.text('Tile 15'), findsNothing); + expect(find.text('Tile 16'), findsOneWidget); + expect(find.text('Tile 17'), findsOneWidget); + expect(find.text('Tile 18'), findsOneWidget); + expect(find.text('Tile 19'), findsOneWidget); + + // Reorders item to the front. This should make item 19 to be first child + // with layout offset = null. + final int swap = items[19]; + items[19] = items[3]; + items[3] = swap; + + await tester.pumpWidget( + _buildSliverList( + items: List.from(items), + controller: controller, + itemHeight: 50, + viewportHeight: 200, + ) + ); + await tester.pump(); + // We need second pump to ensure the scheduled animation gets run. + await tester.pumpAndSettle(); + // Scroll offset should stay the same + expect(controller.offset, 800.0); + expect(find.text('Tile 14'), findsNothing); + expect(find.text('Tile 15'), findsNothing); + expect(find.text('Tile 16'), findsOneWidget); + expect(find.text('Tile 17'), findsOneWidget); + expect(find.text('Tile 18'), findsOneWidget); + expect(find.text('Tile 3'), findsOneWidget); + }); } Widget _buildSliverListRenderWidgetChild(List items) { @@ -216,6 +328,11 @@ Widget _buildSliverList({ child: Text('Tile ${items[i]}'), ); }, + findChildIndexCallback: (Key key) { + final ValueKey valueKey = key as ValueKey; + final int index = items.indexOf(valueKey.value); + return index == -1 ? null : index; + }, childCount: items.length, ), ), diff --git a/packages/flutter/test/widgets/slivers_test.dart b/packages/flutter/test/widgets/slivers_test.dart index cb69542b1c..d440a36bac 100644 --- a/packages/flutter/test/widgets/slivers_test.dart +++ b/packages/flutter/test/widgets/slivers_test.dart @@ -50,7 +50,8 @@ Future testSliverFixedExtentList(WidgetTester tester, List items) findChildIndexCallback: (Key key) { final ValueKey valueKey = key as ValueKey; final String data = valueKey.value; - return items.indexOf(data); + final int index = items.indexOf(data); + return index == -1 ? null : index; }, ), ),