Make TabBar indicator color automatic adjustment optional #68171

xu-baolin · 2020-10-15T04:13:19Z

Description

The automatic adjustment behavior of the tab bar indicator color is not documented in the API doc, I think we should document that.

flutter/packages/flutter/lib/src/material/tabs.dart

Lines 814 to 825 in ed2a480

    
           // ThemeData tries to avoid this by having indicatorColor avoid being the 
        
           // primaryColor. However, it's possible that the tab bar is on a 
        
           // Material that isn't the primaryColor. In that case, if the indicator 
        
           // color ends up matching the material's color, then this overrides it. 
        
           // When that happens, automatic transitions of the theme will likely look 
        
           // ugly as the indicator color suddenly snaps to white at one end, but it's 
        
           // not clear how to avoid that any further. 
        
           // 
        
           // The material's color might be null (if it's a transparency). In that case 
        
           // there's no good way for us to find out what the color is so we don't. 
        
           if (color.value == Material.of(context)?.color?.value) 
        
             color = Colors.white;

At the same time, there are several reasons that let developers have the opportunity to not apply this behavior:

1、This automatic adjustment solution is not perfect as the discussion at #14741 (comment)

2、Sometimes developer wants the same color such as #68077

3、Even if they are the same color, it does not necessarily cause the indicator to be invisible such as #68077

In summary, considering backward compatibility, I set the automaticIndicatorColorAdjustment default value to true. Of course, I would love to hear that you have a better solution.

flutter/packages/flutter/lib/src/material/theme_data.dart

Line 348 in ed2a480

indicatorColor ??= accentColor == primaryColor ? Colors.white : accentColor;

I believe this should be a typo instead of accentColor.value == primaryColor.value.
Most of the time, primaryColor is MaterialColor type and accentColor is Color type. They cannot be compared directly with ==, otherwise it will not be the expected result.
Considering backward compatibility and do not want to break something, I make this change to avoid future undesired behaviors.

Related Issues

Fixes #68077

Tests

I added the following tests:

See files.

shihaohong

I think adding the parameter is fine for a workaround, although a longer term solution is probably warranted. I think it's weird that we try to "be too helpful" here and default to Colors.white when the colors are the same.

But, as you said, it's a breaking change and maybe the right thing to do if it's not too breaking is to just remove the Colors.white code and the automaticIndicatorColorAdjustment parameter, but we'll need to look into how prevalent this actually is.

shihaohong · 2020-10-27T14:17:03Z

packages/flutter/lib/src/material/tabs.dart

+  ///
+  /// If [automaticIndicatorColorAdjustment] is true,
+  /// then the [indicatorColor] will be automatically adjusted to [Colors.white]
+  /// when the [indicatorColor] is same as [Material.color].


It might be worth being specific about which Material.color you're referencing here:

Suggested change

/// when the [indicatorColor] is same as [Material.color].

/// when the [indicatorColor] is same as [Material.color] of the [Material] parent widget.

shihaohong · 2020-10-27T14:26:59Z

packages/flutter/lib/src/material/theme_data.dart

@@ -345,7 +345,7 @@ class ThemeData with Diagnosticable {
    textSelectionHandleColor ??= isDark ? Colors.tealAccent[400]! : primarySwatch[300]!;
    backgroundColor ??= isDark ? Colors.grey[700]! : primarySwatch[200]!;
    dialogBackgroundColor ??= isDark ? Colors.grey[800]! : Colors.white;
-    indicatorColor ??= accentColor == primaryColor ? Colors.white : accentColor;
+    indicatorColor ??= accentColor;


I saw that you explained it in the PR description, but I don't quite understand what you mean since it's still possible for both accentColor and pimaryColor to be a Color, which would cause the theme default indicator color to be Colors.white. Any apps expecting this behavior would break.

I understand what you mean.
Imagine such a situation, if the pimaryColor is MaterialColor(primary value: Color(0xff2196f3)) or Color(0xff2196f3)) and the accentColor is Color(0xff2196f3)), Visually they are the same color but have different indicator colors. This behavior is very strange and incomprehensible. What I think is that if we haven't broken something now, we can fix it to avoid this weird behavior in the future.

Of course, this issue has nothing to do with this PR, I will open another topic to discuss this, thank you for your suggestions.

Track at #69162

shihaohong · 2020-10-27T14:29:21Z

packages/flutter/test/material/tabs_test.dart

+  testWidgets('Tab indicator color should not be adjusted when disable [automaticIndicatorColorAdjustment]', (WidgetTester tester) async {
+     // Regression test for https://github.com/flutter/flutter/issues/68077
+     final List<String> tabs = <String>['LEFT', 'RIGHT'];
+     await tester.pumpWidget(buildLeftRightApp(tabs: tabs, value: 'LEFT', automaticIndicatorColorAdjustment: false));


You would also need to apply the indicatorColor and Material.color to be the same value in the test, since it's expected that otherwise, the color of the indicator is always the expected value

This case is the situation that indicatorColor equal Material.color because automaticIndicatorColorAdjustment is false and the color didn't adjust to white.

This is an opposite test of the above test Default tab indicator color is white

testWidgets('Default tab indicator color is white', (WidgetTester tester) async { // Regression test for https://github.com/flutter/flutter/issues/15958 final List<String> tabs = <String>['LEFT', 'RIGHT']; await tester.pumpWidget(buildLeftRightApp(tabs: tabs, value: 'LEFT')); final RenderBox tabBarBox = tester.firstRenderObject<RenderBox>(find.byType(TabBar)); expect(tabBarBox, paints..line( color: Colors.white, )); });

xu-baolin · 2020-10-28T02:53:08Z

packages/flutter/lib/src/material/tabs.dart

@@ -821,7 +829,9 @@ class _TabBarState extends State<TabBar> {
    //
    // The material's color might be null (if it's a transparency). In that case
    // there's no good way for us to find out what the color is so we don't.
-    if (color.value == Material.of(context)?.color?.value)
+    // TODO(xu-baolin): Remove this code without breaking something.


I add a TODO here for the possible long-term solutions in the future

I think adding a TODO here is good, let's just make it a little more descriptive since we might never get back to it, but a future Flutter developer might. I'm thinking:

// TODO(xu-baolin): Remove automatic adjustment to white color indicator with a better long-term solution.

shihaohong

LGTM. Just a note on improving clarification of the TODO

shihaohong · 2020-11-02T10:06:46Z

Might need a rebase -- unrelated customer_testing checks are failing.

xu-baolin · 2020-11-03T12:16:37Z

I really don't know how to solve this problem, can I just turn off this PR and reopen one? +_+

shihaohong · 2020-11-04T10:17:34Z

According to the flutter discord channel, this should work:

git pull --rebase upstream master followed by git push --force origin <branch>

However, if that doesn't work, feel free and just create a new branch and close this one out. This problem is a weird one, thanks for putting up with it!

xu-baolin · 2020-11-04T11:55:51Z

It actually works! Thanks very much !

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Oct 15, 2020

google-cla bot added the cla: yes label Oct 15, 2020

HansMuller requested a review from shihaohong October 15, 2020 23:52

shihaohong reviewed Oct 27, 2020

View reviewed changes

xu-baolin mentioned this pull request Oct 28, 2020

MaterialColor canot be compared directly with Color type #69162

Closed

xu-baolin commented Oct 28, 2020

View reviewed changes

xu-baolin requested a review from shihaohong October 30, 2020 10:30

shihaohong approved these changes Oct 31, 2020

View reviewed changes

shihaohong changed the title ~~Improve the tab bar indicator color behaviors~~ Make TabBar indicator color automatic adjustment optional Nov 1, 2020

xu-baolin added 3 commits November 4, 2020 18:20

improve the tab bar indicator color behaviors

d056315

For code review suggestions

c80590a

improving clarification of the TODO

4d29a2e

xu-baolin force-pushed the tabbar1015 branch from ad76453 to 4d29a2e Compare November 4, 2020 10:21

shihaohong added the waiting for tree to go green label Nov 4, 2020

fluttergithubbot merged commit 9b564e9 into flutter:master Nov 4, 2020

shihaohong mentioned this pull request Nov 5, 2020

AdoptAWidget: Stepper #69525

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make TabBar indicator color automatic adjustment optional #68171

Make TabBar indicator color automatic adjustment optional #68171

Uh oh!

xu-baolin commented Oct 15, 2020

Uh oh!

shihaohong left a comment

Uh oh!

shihaohong Oct 27, 2020

Uh oh!

xu-baolin Oct 28, 2020

Uh oh!

shihaohong Oct 27, 2020

Uh oh!

xu-baolin Oct 28, 2020

Uh oh!

xu-baolin Oct 28, 2020

Uh oh!

shihaohong Oct 27, 2020

Uh oh!

xu-baolin Oct 28, 2020

Uh oh!

xu-baolin Oct 28, 2020

Uh oh!

shihaohong Oct 31, 2020

Uh oh!

shihaohong left a comment

Uh oh!

shihaohong commented Nov 2, 2020

Uh oh!

xu-baolin commented Nov 3, 2020

Uh oh!

shihaohong commented Nov 4, 2020 •

edited

Loading

Uh oh!

xu-baolin commented Nov 4, 2020

Uh oh!

Uh oh!

	// ThemeData tries to avoid this by having indicatorColor avoid being the
	// primaryColor. However, it's possible that the tab bar is on a
	// Material that isn't the primaryColor. In that case, if the indicator
	// color ends up matching the material's color, then this overrides it.
	// When that happens, automatic transitions of the theme will likely look
	// ugly as the indicator color suddenly snaps to white at one end, but it's
	// not clear how to avoid that any further.
	//
	// The material's color might be null (if it's a transparency). In that case
	// there's no good way for us to find out what the color is so we don't.
	if (color.value == Material.of(context)?.color?.value)
	color = Colors.white;

	/// when the [indicatorColor] is same as [Material.color].
	/// when the [indicatorColor] is same as [Material.color] of the [Material] parent widget.

Make TabBar indicator color automatic adjustment optional #68171

Make TabBar indicator color automatic adjustment optional #68171

Uh oh!

Conversation

xu-baolin commented Oct 15, 2020

Description

Related Issues

Tests

Uh oh!

shihaohong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shihaohong left a comment

Choose a reason for hiding this comment

Uh oh!

shihaohong commented Nov 2, 2020

Uh oh!

xu-baolin commented Nov 3, 2020

Uh oh!

shihaohong commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xu-baolin commented Nov 4, 2020

Uh oh!

Uh oh!

shihaohong commented Nov 4, 2020 •

edited

Loading