[material_ui] Let TabBarThemeData.indicator satisfy the indicatorWeight assert - #12665
[material_ui] Let TabBarThemeData.indicator satisfy the indicatorWeight assert#12665m1roxx wants to merge 1 commit into
Conversation
…ht assert TabBar asserted `indicator != null || indicatorWeight > 0.0` in its constructors, where the resolved TabBarThemeData is not available. An app-wide indicator supplied via TabBarThemeData.indicator therefore could not be combined with `indicatorWeight: 0`, even though indicatorWeight is documented to be ignored whenever an indicator is provided by the widget or the theme. Moves the check to _getIndicator, which runs after both the widget-level and theme-level indicators have been ruled out, so it now fires only when the TabBar actually draws its default underline indicator. Fixes flutter/flutter#188837
There was a problem hiding this comment.
Code Review
This pull request removes the constructor assertion requiring indicatorWeight to be greater than zero when an indicator is provided by the theme, deferring the assertion to the state build phase when the default underline indicator is actually used. The review feedback suggests using DefaultTabController in the newly added tests to manage the controller lifecycle more safely and prevent potential ticker leaks.
| Widget buildTabBar({bool secondaryTabBar = false}) { | ||
| final TabController controller = createTabController( | ||
| vsync: const TestVSync(), | ||
| length: tabs.length, | ||
| ); | ||
| return boilerplate( | ||
| useMaterial3: false, | ||
| tabBarTheme: tabBarTheme, | ||
| child: Container( | ||
| alignment: Alignment.topLeft, | ||
| child: secondaryTabBar | ||
| ? TabBar.secondary(indicatorWeight: 0.0, controller: controller, tabs: tabs) | ||
| : TabBar(indicatorWeight: 0.0, controller: controller, tabs: tabs), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Using DefaultTabController is more idiomatic and safer here, as it automatically manages the lifecycle of the TabController and avoids potential memory or ticker leaks in tests.
Widget buildTabBar({bool secondaryTabBar = false}) {
return boilerplate(
useMaterial3: false,
tabBarTheme: tabBarTheme,
child: DefaultTabController(
length: tabs.length,
child: Container(
alignment: Alignment.topLeft,
child: secondaryTabBar
? TabBar.secondary(indicatorWeight: 0.0, tabs: tabs)
: TabBar(indicatorWeight: 0.0, tabs: tabs),
),
),
);
}| final tabs = List<Widget>.generate(2, (int index) => Tab(text: 'Tab $index')); | ||
| final TabController controller = createTabController( | ||
| vsync: const TestVSync(), | ||
| length: tabs.length, | ||
| ); | ||
|
|
||
| await tester.pumpWidget( | ||
| boilerplate( | ||
| child: TabBar(indicatorWeight: 0.0, controller: controller, tabs: tabs), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Using DefaultTabController here avoids manually creating a TabController and ensures proper disposal, preventing potential ticker leaks in the test.
| final tabs = List<Widget>.generate(2, (int index) => Tab(text: 'Tab $index')); | |
| final TabController controller = createTabController( | |
| vsync: const TestVSync(), | |
| length: tabs.length, | |
| ); | |
| await tester.pumpWidget( | |
| boilerplate( | |
| child: TabBar(indicatorWeight: 0.0, controller: controller, tabs: tabs), | |
| ), | |
| ); | |
| final tabs = List<Widget>.generate(2, (int index) => Tab(text: 'Tab $index')); | |
| await tester.pumpWidget( | |
| boilerplate( | |
| child: DefaultTabController( | |
| length: tabs.length, | |
| child: TabBar(indicatorWeight: 0.0, tabs: tabs), | |
| ), | |
| ), | |
| ); |
TabBarandTabBar.secondaryassertedindicator != null || (indicatorWeight > 0.0)in theirconstructors. A constructor has no
BuildContext, so that check can only see the widget-levelindicatorand is blind toTabBarThemeData.indicator. As a result, an app that supplies itsindicator app-wide through the theme cannot also pass
indicatorWeight: 0to suppress the defaultunderline thickness — the only workarounds are an epsilon weight, or moving the indicator to the
widget, which then overrides the themed one.
This contradicts the documented behavior of
indicatorWeight, which already states:This PR moves the check to
_TabBarState._getIndicator, which runs after both the widget-level andthe theme-level indicator have been ruled out, so it now fires only when the
TabBaractually drawsits default underline indicator. This matches the direction suggested by @dkwingsmt in
flutter/flutter#188837 (comment).
The
indicatorWeightdoc comment is updated to describe the relaxed constraint.Fixes flutter/flutter#188837
Tests
Two tests are added to
packages/material_ui/test/tabs_test.dart; both fail before this change:TabBar.indicatorWeight can be zero when the indicator comes from the theme— a regression testcovering both
TabBarandTabBar.secondary, verifying that the widget builds and paints thethemed indicator.
TabBar asserts when indicatorWeight is zero and no indicator is provided— verifies the check isstill enforced when the
TabBarfalls back to its default underline indicator.material_uiis a batch-release package, so the changelog entry is a new file underpending_changelogs/withversion: patchrather than a directCHANGELOG.md/pubspec.yamledit.Pre-Review Checklist
[shared_preferences]///).Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2