Skip to content

Commit

Permalink
Fix crasher in DragableScrollableSheet when controller is animating a…
Browse files Browse the repository at this point in the history
…nd switching widgets (#125721)

We were failing to dispose of animation controllers created by `animateTo` when `didUpdateWidget` happens and we null out the `_attachedController`. This would cause the listener added here to fail on a null assertion:

https://github.com/flutter/flutter/blob/fef41cfce0e3582907f6846cf2385470bb17ecd1/packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart#L135

Without the code change, the test ends up throwing exceptions related to that line. I don't have a great way to verify what this does visually though, hoping for help on that from internal customer.

b/279930163

See cl/527868355 as well.

Fixes #125709
  • Loading branch information
dnfield committed Apr 29, 2023
1 parent 9417b2a commit e1e8ad8
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ class DraggableScrollableController extends ChangeNotifier {
} else {
_attachedController?.extent._currentSize.removeListener(notifyListeners);
}
_disposeAnimationControllers();
_attachedController = null;
}

Expand Down
80 changes: 67 additions & 13 deletions packages/flutter/test/widgets/draggable_scrollable_sheet_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1404,22 +1404,21 @@ void main() {
await tester.pumpWidget(boilerplate);
expect(controller.isAttached, true);
expect(controller.size, isNotNull);
});
});

testWidgets('DraggableScrollableController.animateTo should not leak Ticker', (WidgetTester tester) async {
// Regression test for https://github.com/flutter/flutter/issues/102483
final DraggableScrollableController controller = DraggableScrollableController();
await tester.pumpWidget(boilerplateWidget(() {}, controller: controller));
testWidgets('DraggableScrollableController.animateTo after detach', (WidgetTester tester) async {
final DraggableScrollableController controller = DraggableScrollableController();
await tester.pumpWidget(boilerplateWidget(() {}, controller: controller));

controller.animateTo(0.0, curve: Curves.linear, duration: const Duration(milliseconds: 200));
await tester.pump();
controller.animateTo(0.0, curve: Curves.linear, duration: const Duration(milliseconds: 200));
await tester.pump();

// Dispose the DraggableScrollableSheet
await tester.pumpWidget(const SizedBox.shrink());
// Controller should be detached and no exception should be thrown
expect(controller.isAttached, false);
expect(tester.takeException(), isNull);
});
// Dispose the DraggableScrollableSheet
await tester.pumpWidget(const SizedBox.shrink());
// Controller should be detached and no exception should be thrown
expect(controller.isAttached, false);
expect(tester.takeException(), isNull);
});

testWidgets('DraggableScrollableSheet should not reset programmatic drag on rebuild', (WidgetTester tester) async {
// Regression test for https://github.com/flutter/flutter/issues/101114
Expand Down Expand Up @@ -1636,4 +1635,59 @@ void main() {
controller2.jumpTo(1.0);
expect(loggedSizes, <double>[1.0].map((double v) => closeTo(v, precisionErrorTolerance)));
});

testWidgets('DraggableScrollableSheet controller can be changed while animating', (WidgetTester tester) async {
final DraggableScrollableController controller1 = DraggableScrollableController();
final DraggableScrollableController controller2 = DraggableScrollableController();

DraggableScrollableController controller = controller1;
await tester.pumpWidget(MaterialApp(
home: StatefulBuilder(
builder: (BuildContext context, StateSetter setState) => Scaffold(
body: DraggableScrollableSheet(
initialChildSize: 0.25,
snap: true,
snapSizes: const <double>[0.25, 0.5, 1.0],
controller: controller,
builder: (BuildContext context, ScrollController scrollController) {
return ListView(
controller: scrollController,
children: <Widget>[
ElevatedButton(
onPressed: () => setState(() {
controller = controller2;
}),
child: const Text('Switch controller'),
),
Container(
height: 10000,
color: Colors.blue,
),
],
);
},
),
),
),
));
expect(controller1.isAttached, true);
expect(controller2.isAttached, false);


controller1.animateTo(0.5, curve: Curves.linear, duration: const Duration(milliseconds: 200));
await tester.pump();

await tester.tap(find.text('Switch controller'));
await tester.pump();

expect(controller1.isAttached, false);
expect(controller2.isAttached, true);

controller2.animateTo(1.0, curve: Curves.linear, duration: const Duration(milliseconds: 200));
await tester.pump();

await tester.pumpWidget(const SizedBox.shrink());
expect(controller1.isAttached, false);
expect(controller2.isAttached, false);
});
}

0 comments on commit e1e8ad8

Please sign in to comment.