From 77b4505c8014330fba2f2706c658a64bc5d1891b Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Thu, 6 Aug 2020 18:51:02 -0700 Subject: [PATCH] Fix a crash when disposing tabs (#63142) --- .../lib/src/material/tab_controller.dart | 6 +- packages/flutter/lib/src/material/tabs.dart | 6 +- .../lib/src/widgets/scroll_physics.dart | 3 +- .../lib/src/widgets/scroll_position.dart | 98 +++++++++---------- packages/flutter/test/material/tabs_test.dart | 37 +++++++ 5 files changed, 94 insertions(+), 56 deletions(-) diff --git a/packages/flutter/lib/src/material/tab_controller.dart b/packages/flutter/lib/src/material/tab_controller.dart index 0d6be0a5db..4c1a6a9948 100644 --- a/packages/flutter/lib/src/material/tab_controller.dart +++ b/packages/flutter/lib/src/material/tab_controller.dart @@ -220,8 +220,10 @@ class TabController extends ChangeNotifier { _animationController .animateTo(_index.toDouble(), duration: duration, curve: curve) .whenCompleteOrCancel(() { - _indexIsChangingCount -= 1; - notifyListeners(); + if (_animationController != null) { // don't notify if we've been disposed + _indexIsChangingCount -= 1; + notifyListeners(); + } }); } else { _indexIsChangingCount += 1; diff --git a/packages/flutter/lib/src/material/tabs.dart b/packages/flutter/lib/src/material/tabs.dart index b132f78974..2b387dbd3c 100644 --- a/packages/flutter/lib/src/material/tabs.dart +++ b/packages/flutter/lib/src/material/tabs.dart @@ -1199,8 +1199,6 @@ class TabBarView extends StatefulWidget { _TabBarViewState createState() => _TabBarViewState(); } -const PageScrollPhysics _kTabBarViewPhysics = PageScrollPhysics(); - class _TabBarViewState extends State { TabController _controller; PageController _pageController; @@ -1370,8 +1368,8 @@ class _TabBarViewState extends State { dragStartBehavior: widget.dragStartBehavior, controller: _pageController, physics: widget.physics == null - ? _kTabBarViewPhysics.applyTo(const ClampingScrollPhysics()) - : _kTabBarViewPhysics.applyTo(widget.physics), + ? const PageScrollPhysics().applyTo(const ClampingScrollPhysics()) + : const PageScrollPhysics().applyTo(widget.physics), children: _childrenWithKey, ), ); diff --git a/packages/flutter/lib/src/widgets/scroll_physics.dart b/packages/flutter/lib/src/widgets/scroll_physics.dart index be4160dd79..1c1c72c590 100644 --- a/packages/flutter/lib/src/widgets/scroll_physics.dart +++ b/packages/flutter/lib/src/widgets/scroll_physics.dart @@ -428,8 +428,9 @@ class RangeMaintainingScrollPhysics extends ScrollPhysics { @required bool isScrolling, @required double velocity, }) { - if (velocity != 0.0 || ((oldPosition.minScrollExtent == newPosition.minScrollExtent) && (oldPosition.maxScrollExtent == newPosition.maxScrollExtent))) + if (velocity != 0.0 || ((oldPosition.minScrollExtent == newPosition.minScrollExtent) && (oldPosition.maxScrollExtent == newPosition.maxScrollExtent))) { return super.adjustPositionForNewDimensions(oldPosition: oldPosition, newPosition: newPosition, isScrolling: isScrolling, velocity: velocity); + } if (oldPosition.pixels < oldPosition.minScrollExtent) { final double oldDelta = oldPosition.minScrollExtent - oldPosition.pixels; return newPosition.minScrollExtent - oldDelta; diff --git a/packages/flutter/lib/src/widgets/scroll_position.dart b/packages/flutter/lib/src/widgets/scroll_position.dart index 3f84d1c149..8799f22edc 100644 --- a/packages/flutter/lib/src/widgets/scroll_position.dart +++ b/packages/flutter/lib/src/widgets/scroll_position.dart @@ -432,55 +432,6 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { return true; } - Set _semanticActions; - - /// Called whenever the scroll position or the dimensions of the scroll view - /// change to schedule an update of the available semantics actions. The - /// actual update will be performed in the next frame. If non is pending - /// a frame will be scheduled. - /// - /// For example: If the scroll view has been scrolled all the way to the top, - /// the action to scroll further up needs to be removed as the scroll view - /// cannot be scrolled in that direction anymore. - /// - /// This method is potentially called twice per frame (if scroll position and - /// scroll view dimensions both change) and therefore shouldn't do anything - /// expensive. - void _updateSemanticActions() { - SemanticsAction forward; - SemanticsAction backward; - switch (axisDirection) { - case AxisDirection.up: - forward = SemanticsAction.scrollDown; - backward = SemanticsAction.scrollUp; - break; - case AxisDirection.right: - forward = SemanticsAction.scrollLeft; - backward = SemanticsAction.scrollRight; - break; - case AxisDirection.down: - forward = SemanticsAction.scrollUp; - backward = SemanticsAction.scrollDown; - break; - case AxisDirection.left: - forward = SemanticsAction.scrollRight; - backward = SemanticsAction.scrollLeft; - break; - } - - final Set actions = {}; - if (pixels > minScrollExtent) - actions.add(backward); - if (pixels < maxScrollExtent) - actions.add(forward); - - if (setEquals(actions, _semanticActions)) - return; - - _semanticActions = actions; - context.setSemanticsActions(_semanticActions); - } - @override bool applyContentDimensions(double minScrollExtent, double maxScrollExtent) { assert(minScrollExtent != null); @@ -560,6 +511,55 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { _updateSemanticActions(); // will potentially request a semantics update. } + Set _semanticActions; + + /// Called whenever the scroll position or the dimensions of the scroll view + /// change to schedule an update of the available semantics actions. The + /// actual update will be performed in the next frame. If non is pending + /// a frame will be scheduled. + /// + /// For example: If the scroll view has been scrolled all the way to the top, + /// the action to scroll further up needs to be removed as the scroll view + /// cannot be scrolled in that direction anymore. + /// + /// This method is potentially called twice per frame (if scroll position and + /// scroll view dimensions both change) and therefore shouldn't do anything + /// expensive. + void _updateSemanticActions() { + SemanticsAction forward; + SemanticsAction backward; + switch (axisDirection) { + case AxisDirection.up: + forward = SemanticsAction.scrollDown; + backward = SemanticsAction.scrollUp; + break; + case AxisDirection.right: + forward = SemanticsAction.scrollLeft; + backward = SemanticsAction.scrollRight; + break; + case AxisDirection.down: + forward = SemanticsAction.scrollUp; + backward = SemanticsAction.scrollDown; + break; + case AxisDirection.left: + forward = SemanticsAction.scrollRight; + backward = SemanticsAction.scrollLeft; + break; + } + + final Set actions = {}; + if (pixels > minScrollExtent) + actions.add(backward); + if (pixels < maxScrollExtent) + actions.add(forward); + + if (setEquals(actions, _semanticActions)) + return; + + _semanticActions = actions; + context.setSemanticsActions(_semanticActions); + } + /// Animates the position such that the given object is as visible as possible /// by just scrolling this position. /// diff --git a/packages/flutter/test/material/tabs_test.dart b/packages/flutter/test/material/tabs_test.dart index 5c1c2884ea..711276cfce 100644 --- a/packages/flutter/test/material/tabs_test.dart +++ b/packages/flutter/test/material/tabs_test.dart @@ -2670,6 +2670,13 @@ void main() { final PageView pageView = tester.widget(find.byType(PageView)); expect(pageView.physics.toString().contains('ClampingScrollPhysics'), isFalse); }); + + testWidgets('Crash on dispose', (WidgetTester tester) async { + await tester.pumpWidget(Padding(padding: const EdgeInsets.only(right: 200.0), child: TabBarDemo())); + await tester.tap(find.byIcon(Icons.directions_bike)); + // There was a time where this would throw an exception + // because we tried to send a notification on dispose. + }); } class KeepAliveInk extends StatefulWidget { @@ -2693,3 +2700,33 @@ class _KeepAliveInkState extends State with AutomaticKeepAliveClie @override bool get wantKeepAlive => true; } + +class TabBarDemo extends StatelessWidget { + @override + Widget build(BuildContext context) { + return MaterialApp( + home: DefaultTabController( + length: 3, + child: Scaffold( + appBar: AppBar( + bottom: const TabBar( + tabs: [ + Tab(icon: Icon(Icons.directions_car)), + Tab(icon: Icon(Icons.directions_transit)), + Tab(icon: Icon(Icons.directions_bike)), + ], + ), + title: const Text('Tabs Demo'), + ), + body: const TabBarView( + children: [ + Icon(Icons.directions_car), + Icon(Icons.directions_transit), + Icon(Icons.directions_bike), + ], + ), + ), + ), + ); + } +}