Skip to content

Commit

Permalink
Fix DraggableScrollableSheet leaks Ticker (#102916)
Browse files Browse the repository at this point in the history
  • Loading branch information
bleroux authored May 5, 2022
1 parent 077e7e1 commit 750ad32
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 17 deletions.
37 changes: 21 additions & 16 deletions packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart
Original file line number Diff line number Diff line change
Expand Up @@ -795,8 +795,7 @@ class _DraggableScrollableSheetScrollController extends ScrollController {
/// See also:
///
/// * [_DraggableScrollableSheetScrollController], which uses this as its [ScrollPosition].
class _DraggableScrollableSheetScrollPosition
extends ScrollPositionWithSingleContext {
class _DraggableScrollableSheetScrollPosition extends ScrollPositionWithSingleContext {
_DraggableScrollableSheetScrollPosition({
required super.physics,
required super.context,
Expand All @@ -805,16 +804,18 @@ class _DraggableScrollableSheetScrollPosition
});

VoidCallback? _dragCancelCallback;
VoidCallback? _ballisticCancelCallback;
final _DraggableSheetExtent Function() getExtent;
final Set<AnimationController> _ballisticControllers = <AnimationController>{};
bool get listShouldScroll => pixels > 0.0;

_DraggableSheetExtent get extent => getExtent();

@override
void beginActivity(ScrollActivity? newActivity) {
// Cancel the running ballistic simulation, if there is one.
_ballisticCancelCallback?.call();
// Cancel the running ballistic simulations
for (final AnimationController ballisticController in _ballisticControllers) {
ballisticController.stop();
}
super.beginActivity(newActivity);
}

Expand Down Expand Up @@ -852,8 +853,10 @@ class _DraggableScrollableSheetScrollPosition

@override
void dispose() {
// Stop the animation before dispose.
_ballisticCancelCallback?.call();
for (final AnimationController ballisticController in _ballisticControllers) {
ballisticController.dispose();
}
_ballisticControllers.clear();
super.dispose();
}

Expand All @@ -873,10 +876,11 @@ class _DraggableScrollableSheetScrollPosition
if (extent.snap) {
// Snap is enabled, simulate snapping instead of clamping scroll.
simulation = _SnappingSimulation(
position: extent.currentPixels,
initialVelocity: velocity,
pixelSnapSize: extent.pixelSnapSizes,
tolerance: physics.tolerance);
position: extent.currentPixels,
initialVelocity: velocity,
pixelSnapSize: extent.pixelSnapSizes,
tolerance: physics.tolerance,
);
} else {
// The iOS bouncing simulation just isn't right here - once we delegate
// the ballistic back to the ScrollView, it will use the right simulation.
Expand All @@ -892,9 +896,8 @@ class _DraggableScrollableSheetScrollPosition
debugLabel: objectRuntimeType(this, '_DraggableScrollableSheetPosition'),
vsync: context.vsync,
);
// Stop the ballistic animation if a new activity starts.
// See: [beginActivity].
_ballisticCancelCallback = ballisticController.stop;
_ballisticControllers.add(ballisticController);

double lastPosition = extent.currentPixels;
void tick() {
final double delta = ballisticController.value - lastPosition;
Expand All @@ -916,8 +919,10 @@ class _DraggableScrollableSheetScrollPosition
..addListener(tick)
..animateWith(simulation).whenCompleteOrCancel(
() {
_ballisticCancelCallback = null;
ballisticController.dispose();
if (_ballisticControllers.contains(ballisticController)) {
_ballisticControllers.remove(ballisticController);
ballisticController.dispose();
}
},
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,59 @@ void main() {
expect(find.text('Item 70'), findsNothing);
}, variant: TargetPlatformVariant.all());

debugDefaultTargetPlatformOverride = null;
testWidgets('Ballistic animation on fling should not leak Ticker', (WidgetTester tester) async {
// Regression test for https://github.com/flutter/flutter/issues/101061
await tester.pumpWidget(
Directionality(
textDirection: TextDirection.ltr,
child: MediaQuery(
data: const MediaQueryData(),
child: Align(
alignment: Alignment.bottomCenter,
child: DraggableScrollableSheet(
initialChildSize: 0.8,
minChildSize: 0.2,
maxChildSize: 0.9,
expand: false,
builder: (_, ScrollController scrollController) {
return ListView.separated(
physics: const BouncingScrollPhysics(),
controller: scrollController,
separatorBuilder: (_, __) => const Divider(),
itemCount: 100,
itemBuilder: (_, int index) => SizedBox(
height: 100,
child: ColoredBox(
color: Colors.primaries[index % Colors.primaries.length],
child: Text('Item $index'),
),
),
);
},
),
),
),
),
);

await tester.flingFrom(
tester.getCenter(find.text('Item 1')),
const Offset(0, 50),
10000,
);

// Pumps several times to let the DraggableScrollableSheet react to scroll position changes.
const int numberOfPumpsBeforeError = 22;
for (int i = 0; i < numberOfPumpsBeforeError; i++) {
await tester.pump(const Duration(milliseconds: 10));
}

// Dispose the DraggableScrollableSheet
await tester.pumpWidget(const SizedBox.shrink());

// When a Ticker leaks an exception is thrown
expect(tester.takeException(), isNull);
});
});

testWidgets('Does not snap away from initial child on build', (WidgetTester tester) async {
Expand Down

0 comments on commit 750ad32

Please sign in to comment.