Skip to content

F
Front-End
Unverified Commit 1eb8c640 authored by LongCatIsLooong's avatar LongCatIsLooong Committed by GitHub
Browse files
Options

Fix CupertinoTabScaffold index out of range (#33624)

parent 89d887f3
Hide whitespace changes
Inline Side-by-side
Showing with 129 additions and 56 deletions
packages/flutter/lib/src/cupertino/tab_scaffold.dart
View file @ 1eb8c640
......@@ -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<Widget> tabs;
List<bool> shouldBuildTab;
List<FocusScopeNode> tabFocusNodes;
@override
void initState() {
super.initState();
tabs = List<Widget>(widget.tabNumber);
tabFocusNodes = List<FocusScopeNode>.generate(
widget.tabNumber,
(int index) => FocusScopeNode(debugLabel: 'Tab Focus Scope $index'),
);
shouldBuildTab = List<bool>.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<bool>.filled(lengthDiff, false));
} else if (lengthDiff < 0) {
shouldBuildTab.removeRange(widget.tabNumber, shouldBuildTab.length);
}
_focusActiveTab();
}
void _focusActiveTab() {
if (tabFocusNodes?.length != widget.tabNumber) {
tabFocusNodes = List<FocusScopeNode>.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<Widget>.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(),
),
),
);
......
packages/flutter/test/cupertino/tab_scaffold_test.dart
View file @ 1eb8c640
......@@ -66,7 +66,7 @@ void main() {
),
);
expect(tabsPainted, <int>[0]);
expect(tabsPainted, const <int>[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, <int>[0, 1]);
expect(tabsPainted, const <int>[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, <int>[0, 1, 0]);
expect(tabsPainted, const <int>[0, 1, 0]);
// CupertinoTabBar's onTap callbacks are passed on.
expect(selectedTabs, <int>[1, 0]);
expect(selectedTabs, const <int>[1, 0]);
});
testWidgets('Tabs are lazy built and moved offstage when inactive', (WidgetTester tester) async {
......@@ -116,7 +116,7 @@ void main() {
),
);
expect(tabsBuilt, <int>[0]);
expect(tabsBuilt, const <int>[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, <int>[0, 0, 1]);
expect(tabsBuilt, const <int>[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, <int>[0, 0, 1, 0, 1]);
expect(tabsBuilt, const <int>[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, <int>[1]);
expect(tabsPainted, const <int>[1]);
controller.index = 0;
await tester.pump();
expect(tabsPainted, <int>[1, 0]);
expect(tabsPainted, const <int>[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, <int>[1, 0, 1]);
expect(selectedTabs, <int>[1]);
expect(tabsPainted, const <int>[1, 0, 1]);
expect(selectedTabs, const <int>[1]);
});
testWidgets('Programmatic tab switching by passing in a new controller', (WidgetTester tester) async {
......@@ -292,7 +292,7 @@ void main() {
),
);
expect(tabsPainted, <int>[0]);
expect(tabsPainted, const <int>[0]);
await tester.pumpWidget(
CupertinoApp(
......@@ -311,7 +311,7 @@ void main() {
),
);
expect(tabsPainted, <int>[0, 1]);
expect(tabsPainted, const <int>[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, <int>[0, 1, 0]);
expect(selectedTabs, <int>[0]);
expect(tabsPainted, const <int>[0, 1, 0]);
expect(selectedTabs, const <int>[0]);
});
testWidgets('Tab bar respects themes', (WidgetTester tester) async {
......@@ -482,18 +482,18 @@ void main() {
),
);
expect(tabsBuilt, <int>[0]);
expect(tabsBuilt, const <int>[0]);
// selectedTabs list is appended to on onTap callbacks. We didn't tap
// any tabs yet.
expect(selectedTabs, <int>[]);
expect(selectedTabs, const <int>[]);
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, <int>[0, 3]);
expect(selectedTabs, <int>[3]);
expect(tabsBuilt, const <int>[0, 3]);
expect(selectedTabs, const <int>[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, <int>[0, 1]);
expect(tabsBuilt, const <int>[0, 1]);
// We didn't tap on any additional tabs to invoke the onTap callback. We
// just deleted a tab.
expect(selectedTabs, <int>[3]);
expect(selectedTabs, const <int>[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<int> tabsPainted = <int>[];
final CupertinoTabController controller = CupertinoTabController(initialIndex: 0);
await tester.pumpWidget(
CupertinoApp(
home: CupertinoTabScaffold(
tabBar: CupertinoTabBar(
items: List<BottomNavigationBarItem>.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 <int> [0]);
// Increase the num of tabs to 20.
await tester.pumpWidget(
CupertinoApp(
home: CupertinoTabScaffold(
tabBar: CupertinoTabBar(
items: List<BottomNavigationBarItem>.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 <int> [0, 0]);
await tester.tap(find.text('Tab 19'));
await tester.pump();
// Tapping the tabs should still work.
expect(tabsPainted, const <int>[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, <int> [0]);
expect(tabsPainted, const <int> [0]);
await tester.pumpWidget(
CupertinoApp(
......@@ -576,24 +631,24 @@ void main() {
onPaint: () { tabsPainted.add(index); }
),
);
}
},
),
)
);
expect(tabsPainted, <int> [0, 0]);
expect(tabsPainted, const <int> [0, 0]);
await tester.tap(find.text('Tab 2'));
await tester.pump();
// Tapping the tabs should still work.
expect(tabsPainted, <int>[0, 0, 1]);
expect(tabsPainted, const <int>[0, 0, 1]);
oldController.index = 10;
await tester.pump();
// Changing [index] of the oldController should not work.
expect(tabsPainted, <int> [0, 0, 1]);
expect(tabsPainted, const <int> [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 <int>[2]);
expect(tabsPainted1, const <int>[2]);
......@@ -738,14 +793,14 @@ void main() {
return CustomPaint(
painter: TestCallbackPainter(
onPaint: () => tabsPainted0.add(index)
)
),
);
}
},
),
]
)
)
)
],
),
),
),
);
expect(tabsPainted0, const <int>[2, 0, 1]);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment

HIAST - GitLab