Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix TabBarView desynchronized after animation interruption #132748

Merged
merged 1 commit into from
Sep 12, 2023
Merged

Fix TabBarView desynchronized after animation interruption #132748

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Fix TabBarView desynchronized after animation interruption
  • Loading branch information
@bleroux
bleroux committed Sep 12, 2023
commit ff5ca3464c4d71379f10bb8834ebb1c092391648
22 changes: 17 additions & 5 deletions packages/flutter/lib/src/widgets/scroll_activity.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 63,8 @@ abstract class ScrollActivity {
ScrollActivityDelegate get delegate => _delegate;
ScrollActivityDelegate _delegate;

bool _isDisposed = false;

/// Updates the activity's link to the [ScrollActivityDelegate].
///
/// This should only be called when an activity is being moved from a defunct
Expand Down Expand Up @@ -134,7 136,9 @@ abstract class ScrollActivity {

/// Called when the scroll view stops performing this activity.
@mustCallSuper
void dispose() { }
void dispose() {
_isDisposed = true;
}

@override
String toString() => describeIdentity(this);
Expand Down Expand Up @@ -535,7 539,7 @@ class BallisticScrollActivity extends ScrollActivity {
)
..addListener(_tick)
..animateWith(simulation)
.whenComplete(_end); // won't trigger if we dispose _controller first
.whenComplete(_end); // won't trigger if we dispose _controller before it completes.
}

late AnimationController _controller;
Expand Down Expand Up @@ -569,7 573,11 @@ class BallisticScrollActivity extends ScrollActivity {
}

void _end() {
delegate.goBallistic(0.0);
// Check if the activity was disposed before going ballistic because _end might be called
// if _controller is disposed just after completion.
if (!_isDisposed) {
delegate.goBallistic(0.0);
}
}

@override
Expand Down Expand Up @@ -628,7 636,7 @@ class DrivenScrollActivity extends ScrollActivity {
)
..addListener(_tick)
..animateTo(to, duration: duration, curve: curve)
.whenComplete(_end); // won't trigger if we dispose _controller first
.whenComplete(_end); // won't trigger if we dispose _controller before it completes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we have the same condition in BallisticScrollActivity.

I think having the _isDisposed flag makes sense, it's not dissimilar from the mounted check that is available elsewhere.

Can you move the isDisposed flag to the super class - ScrollActivity - so we can use it the same way in _end for BallisticScrollActivity and DrivenScrollActivity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!
I pushed an update where _isDisposed is moved to ScrollActivity.

}

late final Completer<void> _completer;
Expand All @@ -648,7 656,11 @@ class DrivenScrollActivity extends ScrollActivity {
}

void _end() {
delegate.goBallistic(velocity);
// Check if the activity was disposed before going ballistic because _end might be called
// if _controller is disposed just after completion.
if (!_isDisposed) {
delegate.goBallistic(velocity);
}
}

@override
Expand Down
51 changes: 51 additions & 0 deletions packages/flutter/test/material/tabs_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2224,6 2224,57 @@ void main() {
expect(tabController.index, 0);
});

testWidgets('On going TabBarView animation can be interrupted by a new animation', (WidgetTester tester) async {
// This is a regression test for https://github.com/flutter/flutter/issues/132293.

final List<String> tabs = <String>['A', 'B', 'C'];
final TabController tabController = TabController(
vsync: const TestVSync(),
length: tabs.length,
);
await tester.pumpWidget(boilerplate(
child: Column(
children: <Widget>[
TabBar(
tabs: tabs.map<Widget>((String tab) => Tab(text: tab)).toList(),
controller: tabController,
),
SizedBox(
width: 400.0,
height: 400.0,
child: TabBarView(
controller: tabController,
children: const <Widget>[
Center(child: Text('0')),
Center(child: Text('1')),
Center(child: Text('2')),
],
),
),
],
),
));

// First page is visible.
expect(tabController.index, 0);
expect(find.text('0'), findsOneWidget);
expect(find.text('1'), findsNothing);

// Animate to the second page.
tabController.animateTo(1);
await tester.pump();
await tester.pump(const Duration(milliseconds: 300));

// Animate back to the first page before the previous animation ends.
tabController.animateTo(0);
await tester.pumpAndSettle();

// First page should be visible.
expect(tabController.index, 0);
expect(find.text('0'), findsOneWidget);
expect(find.text('1'), findsNothing);
});

testWidgets('Can switch to non-neighboring tab in nested TabBarView without crashing', (WidgetTester tester) async {
// This is a regression test for https://github.com/flutter/flutter/issues/18756
final TabController mainTabController = _tabController(length: 4, vsync: const TestVSync());
Expand Down