diff --git a/packages/flutter/lib/src/cupertino/tab_scaffold.dart b/packages/flutter/lib/src/cupertino/tab_scaffold.dart index d6fa60d0df..59147fe094 100644 --- a/packages/flutter/lib/src/cupertino/tab_scaffold.dart +++ b/packages/flutter/lib/src/cupertino/tab_scaffold.dart @@ -120,6 +120,10 @@ class CupertinoTabController extends ChangeNotifier { /// pages as there are [tabBar.items]. Inactive tabs will be moved [Offstage] /// and their animations disabled. /// +/// Adding/removing tabs, or changing the order of tabs is supported but not +/// recommended. Doing so is against the iOS human interface guidelines, and +/// [CupertinoTabScaffold] may lose some tabs' state in the process. +/// /// Use [CupertinoTabView] as the root widget of each tab to support tabs with /// parallel navigation state and history. Since each [CupertinoTabView] contains /// a [Navigator], rebuilding the [CupertinoTabView] with a different @@ -193,6 +197,7 @@ class CupertinoTabController extends ChangeNotifier { /// * [CupertinoPageRoute], a route hosting modal pages with iOS style transitions. /// * [CupertinoPageScaffold], typical contents of an iOS modal page implementing /// layout with a navigation bar on top. +/// * [iOS human interface guidelines](https://developer.apple.com/design/human-interface-guidelines/ios/bars/tab-bars/). class CupertinoTabScaffold extends StatefulWidget { /// Creates a layout for applications with a tab bar at the bottom. /// @@ -448,17 +453,13 @@ class _TabSwitchingView extends StatefulWidget { } class _TabSwitchingViewState extends State<_TabSwitchingView> { - List tabs; + List shouldBuildTab; List tabFocusNodes; @override void initState() { super.initState(); - tabs = List(widget.tabNumber); - tabFocusNodes = List.generate( - widget.tabNumber, - (int index) => FocusScopeNode(debugLabel: 'Tab Focus Scope $index'), - ); + shouldBuildTab = List.filled(widget.tabNumber, false, growable: true); } @override @@ -470,10 +471,28 @@ class _TabSwitchingViewState extends State<_TabSwitchingView> { @override void didUpdateWidget(_TabSwitchingView oldWidget) { super.didUpdateWidget(oldWidget); + + // Only partially invalidate the tabs cache to avoid breaking the current + // behavior. We assume that the only possible change is either: + // - new tabs are appended to the tab list, or + // - some trailing tabs are removed. + // If the above assumption is not true, some tabs may lose their state. + final int lengthDiff = widget.tabNumber - shouldBuildTab.length; + if (lengthDiff > 0) { + shouldBuildTab.addAll(List.filled(lengthDiff, false)); + } else if (lengthDiff < 0) { + shouldBuildTab.removeRange(widget.tabNumber, shouldBuildTab.length); + } _focusActiveTab(); } void _focusActiveTab() { + if (tabFocusNodes?.length != widget.tabNumber) { + tabFocusNodes = List.generate( + widget.tabNumber, + (int index) => FocusScopeNode(debugLabel: 'Tab Focus Scope $index'), + ); + } FocusScope.of(context).setFirstFocus(tabFocusNodes[widget.currentTabIndex]); } @@ -491,10 +510,7 @@ class _TabSwitchingViewState extends State<_TabSwitchingView> { fit: StackFit.expand, children: List.generate(widget.tabNumber, (int index) { final bool active = index == widget.currentTabIndex; - - if (active || tabs[index] != null) { - tabs[index] = widget.tabBuilder(context, index); - } + shouldBuildTab[index] = active || shouldBuildTab[index]; return Offstage( offstage: !active, @@ -502,7 +518,9 @@ class _TabSwitchingViewState extends State<_TabSwitchingView> { enabled: active, child: FocusScope( node: tabFocusNodes[index], - child: tabs[index] ?? Container(), + child: shouldBuildTab[index] + ? widget.tabBuilder(context, index) + : Container(), ), ), ); diff --git a/packages/flutter/test/cupertino/tab_scaffold_test.dart b/packages/flutter/test/cupertino/tab_scaffold_test.dart index 156dffd625..6b88b4a7d8 100644 --- a/packages/flutter/test/cupertino/tab_scaffold_test.dart +++ b/packages/flutter/test/cupertino/tab_scaffold_test.dart @@ -66,7 +66,7 @@ void main() { ), ); - expect(tabsPainted, [0]); + expect(tabsPainted, const [0]); RichText tab1 = tester.widget(find.descendant( of: find.text('Tab 1'), matching: find.byType(RichText), @@ -81,7 +81,7 @@ void main() { await tester.tap(find.text('Tab 2')); await tester.pump(); - expect(tabsPainted, [0, 1]); + expect(tabsPainted, const [0, 1]); tab1 = tester.widget(find.descendant( of: find.text('Tab 1'), matching: find.byType(RichText), @@ -96,9 +96,9 @@ void main() { await tester.tap(find.text('Tab 1')); await tester.pump(); - expect(tabsPainted, [0, 1, 0]); + expect(tabsPainted, const [0, 1, 0]); // CupertinoTabBar's onTap callbacks are passed on. - expect(selectedTabs, [1, 0]); + expect(selectedTabs, const [1, 0]); }); testWidgets('Tabs are lazy built and moved offstage when inactive', (WidgetTester tester) async { @@ -116,7 +116,7 @@ void main() { ), ); - expect(tabsBuilt, [0]); + expect(tabsBuilt, const [0]); expect(find.text('Page 1'), findsOneWidget); expect(find.text('Page 2'), findsNothing); @@ -124,14 +124,14 @@ void main() { await tester.pump(); // Both tabs are built but only one is onstage. - expect(tabsBuilt, [0, 0, 1]); + expect(tabsBuilt, const [0, 0, 1]); expect(find.text('Page 1', skipOffstage: false), isOffstage); expect(find.text('Page 2'), findsOneWidget); await tester.tap(find.text('Tab 1')); await tester.pump(); - expect(tabsBuilt, [0, 0, 1, 0, 1]); + expect(tabsBuilt, const [0, 0, 1, 0, 1]); expect(find.text('Page 1'), findsOneWidget); expect(find.text('Page 2', skipOffstage: false), isOffstage); }); @@ -256,12 +256,12 @@ void main() { ), ); - expect(tabsPainted, [1]); + expect(tabsPainted, const [1]); controller.index = 0; await tester.pump(); - expect(tabsPainted, [1, 0]); + expect(tabsPainted, const [1, 0]); // onTap is not called when changing tabs programmatically. expect(selectedTabs, isEmpty); @@ -269,8 +269,8 @@ void main() { await tester.tap(find.text('Tab 2')); await tester.pump(); - expect(tabsPainted, [1, 0, 1]); - expect(selectedTabs, [1]); + expect(tabsPainted, const [1, 0, 1]); + expect(selectedTabs, const [1]); }); testWidgets('Programmatic tab switching by passing in a new controller', (WidgetTester tester) async { @@ -292,7 +292,7 @@ void main() { ), ); - expect(tabsPainted, [0]); + expect(tabsPainted, const [0]); await tester.pumpWidget( CupertinoApp( @@ -311,7 +311,7 @@ void main() { ), ); - expect(tabsPainted, [0, 1]); + expect(tabsPainted, const [0, 1]); // onTap is not called when changing tabs programmatically. expect(selectedTabs, isEmpty); @@ -319,8 +319,8 @@ void main() { await tester.tap(find.text('Tab 1')); await tester.pump(); - expect(tabsPainted, [0, 1, 0]); - expect(selectedTabs, [0]); + expect(tabsPainted, const [0, 1, 0]); + expect(selectedTabs, const [0]); }); testWidgets('Tab bar respects themes', (WidgetTester tester) async { @@ -482,18 +482,18 @@ void main() { ), ); - expect(tabsBuilt, [0]); + expect(tabsBuilt, const [0]); // selectedTabs list is appended to on onTap callbacks. We didn't tap // any tabs yet. - expect(selectedTabs, []); + expect(selectedTabs, const []); tabsBuilt.clear(); await tester.tap(find.text('Tab 4')); await tester.pump(); // Tabs 1 and 4 are built but only one is onstage. - expect(tabsBuilt, [0, 3]); - expect(selectedTabs, [3]); + expect(tabsBuilt, const [0, 3]); + expect(selectedTabs, const [3]); expect(find.text('Page 1', skipOffstage: false), isOffstage); expect(find.text('Page 4'), findsOneWidget); tabsBuilt.clear(); @@ -515,10 +515,10 @@ void main() { ) ); - expect(tabsBuilt, [0, 1]); + expect(tabsBuilt, const [0, 1]); // We didn't tap on any additional tabs to invoke the onTap callback. We // just deleted a tab. - expect(selectedTabs, [3]); + expect(selectedTabs, const [3]); // Tab 1 was previously built so it's rebuilt again, albeit offstage. expect(find.text('Different page 1', skipOffstage: false), isOffstage); // Since all the tabs after tab 2 are deleted, tab 2 is now the last tab and @@ -533,6 +533,61 @@ void main() { expect(find.text('Page 4', skipOffstage: false), findsNothing); }); + // Regression test for https://github.com/flutter/flutter/issues/33455 + testWidgets('Adding new tabs does not crash the app', (WidgetTester tester) async { + final List tabsPainted = []; + final CupertinoTabController controller = CupertinoTabController(initialIndex: 0); + + await tester.pumpWidget( + CupertinoApp( + home: CupertinoTabScaffold( + tabBar: CupertinoTabBar( + items: List.generate(10, tabGenerator), + ), + controller: controller, + tabBuilder: (BuildContext context, int index) { + return CustomPaint( + child: Text('Page ${index + 1}'), + painter: TestCallbackPainter( + onPaint: () { tabsPainted.add(index); } + ), + ); + }, + ), + ), + ); + + expect(tabsPainted, const [0]); + + // Increase the num of tabs to 20. + await tester.pumpWidget( + CupertinoApp( + home: CupertinoTabScaffold( + tabBar: CupertinoTabBar( + items: List.generate(20, tabGenerator), + ), + controller: controller, + tabBuilder: (BuildContext context, int index) { + return CustomPaint( + child: Text('Page ${index + 1}'), + painter: TestCallbackPainter( + onPaint: () { tabsPainted.add(index); } + ), + ); + }, + ), + ), + ); + + expect(tabsPainted, const [0, 0]); + + await tester.tap(find.text('Tab 19')); + await tester.pump(); + + // Tapping the tabs should still work. + expect(tabsPainted, const [0, 0, 18]); + }); + testWidgets('If a controller is initially provided then the parent stops doing so for rebuilds, ' 'a new instance of CupertinoTabController should be created and used by the widget, ' "while preserving the previous controller's tab index", @@ -554,12 +609,12 @@ void main() { onPaint: () { tabsPainted.add(index); } ), ); - } + }, ), ) ); - expect(tabsPainted, [0]); + expect(tabsPainted, const [0]); await tester.pumpWidget( CupertinoApp( @@ -576,24 +631,24 @@ void main() { onPaint: () { tabsPainted.add(index); } ), ); - } + }, ), ) ); - expect(tabsPainted, [0, 0]); + expect(tabsPainted, const [0, 0]); await tester.tap(find.text('Tab 2')); await tester.pump(); // Tapping the tabs should still work. - expect(tabsPainted, [0, 0, 1]); + expect(tabsPainted, const [0, 0, 1]); oldController.index = 10; await tester.pump(); // Changing [index] of the oldController should not work. - expect(tabsPainted, [0, 0, 1]); + expect(tabsPainted, const [0, 0, 1]); }); testWidgets('Do not call dispose on a controller that we do not own' @@ -610,7 +665,7 @@ void main() { controller: mockController, tabBuilder: (BuildContext context, int index) => const Placeholder(), ), - ) + ), ); expect(mockController.numOfListeners, 1); @@ -625,7 +680,7 @@ void main() { controller: null, tabBuilder: (BuildContext context, int index) => const Placeholder(), ), - ) + ), ); expect(mockController.numOfListeners, 0); @@ -644,7 +699,7 @@ void main() { controller: controller, tabBuilder: (BuildContext context, int index) => const Placeholder() ), - ) + ), ); expect(find.text('Tab 1'), findsOneWidget); expect(find.text('Tab 2'), findsOneWidget); @@ -661,7 +716,7 @@ void main() { controller: controller, tabBuilder: (BuildContext context, int index) => const Placeholder() ), - ) + ), ); // Should not crash here. @@ -691,9 +746,9 @@ void main() { return CustomPaint( painter: TestCallbackPainter( onPaint: () => tabsPainted0.add(index) - ) + ), ); - } + }, ), CupertinoTabScaffold( tabBar: CupertinoTabBar( @@ -704,14 +759,14 @@ void main() { return CustomPaint( painter: TestCallbackPainter( onPaint: () => tabsPainted1.add(index) - ) + ), ); - } + }, ), - ] - ) - ) - ) + ], + ), + ), + ), ); expect(tabsPainted0, const [2]); expect(tabsPainted1, const [2]); @@ -738,14 +793,14 @@ void main() { return CustomPaint( painter: TestCallbackPainter( onPaint: () => tabsPainted0.add(index) - ) + ), ); - } + }, ), - ] - ) - ) - ) + ], + ), + ), + ), ); expect(tabsPainted0, const [2, 0, 1]);