Generalized TabBar selected tab indicator #14741

HansMuller · 2018-02-16T00:49:29Z

A TabBar's selected tab indicator can now be defined with a Decoration, using indicator. The new UnderlineTabIndicator (a Decoration) defines the default underline-style selected tab indicator.

The selected tab indicator's width can be defined by the overall width of the tab, with indicatorSize: TabBarIndicatorSize.tab, or just the width of the tab's label with indicatorSize: TabBarIndicatorSize.label.

Here are some examples that use indicator and indicatorSize.

indicator: new ShapeDecoration(
  shape: new RoundedRectangleBorder(
    borderRadius: new BorderRadius.circular(8.0),
    side: new BorderSide(
      color: Colors.white,
      width: 2.0,
    ),
  ) + // Inset the white border by 6.0
  new RoundedRectangleBorder(
    side: new BorderSide(
      color: Colors.transparent,
      width: 6.0,
    ),
  ),
),

This example sets indicatorSize and configures UnderlineIndicatorShape's insets:

indicatorSize: TabBarIndicatorSize.label,
indicator: const UnderlineTabIndicator(
  insets: const EdgeInsets.only(bottom: 12.0),
),

This PR includes an incompatible change: the Tab Widget's width is no longer padded horizontally. It's height is still fixed depending on what it contains (text or icon or both) and it's still centered vertically. TabBar still horizontally centers tab widgets within the space they're allocated.

Tests that assume that Tab's widgets will expand to fill the available width, for example tests that check TabBar Tab bounds, will have to be updated.

This change was made to enable computing a tab's intrinsic width for TabIndicatorSize.label. It should only affect apps that used Tab widgets outside of the TabBar or tests that check Tab bounds.

Hixie · 2018-02-16T01:07:18Z

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

+/// tab's boundary in terms of its (centered) widget, [TabIndicatorSize.label],
+/// or the the entire tab, [TabIndicatorSize.tab].
+class DefaultTabIndicator extends ShapeBorder {
+  /// Create a underline style tab indicator.


Hixie · 2018-02-16T01:07:47Z

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

+/// The [TabBar.indicatorSize] property can be used to define the
+/// tab's boundary in terms of its (centered) widget, [TabIndicatorSize.label],
+/// or the the entire tab, [TabIndicatorSize.tab].
+class DefaultTabIndicator extends ShapeBorder {


i would call this an UnderlineTabIndicator, so that if we ever want to change the default we're not stuck with a weird class name

Good idea, done.

Hixie · 2018-02-16T01:08:14Z

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

+  ///
+  /// The [TabBar.indicatorSize] property can be used to define the
+  /// tab's boundary in terms of its (centered) widget, [TabIndicatorSize.label],
+  /// or the the entire tab,  [TabIndicatorSize.tab].


extraneous double space

Hixie · 2018-02-16T01:09:05Z

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

+
+  @override
+  EdgeInsetsGeometry get dimensions {
+    return padding.subtract(new EdgeInsets.only(bottom: borderSide.width));


won't this normally mean it returns negative values?

I'm not sure what this is trying to do.

Hixie · 2018-02-16T01:09:27Z

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

+    assert(textDirection != null);
+    final Rect indicator = padding.resolve(textDirection).deflateRect(rect);
+    return new Rect.fromLTWH(
+        indicator.left,


nit: indent by 2, not 4

Hixie · 2018-02-16T01:10:41Z

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

+
+  @override
+  bool operator ==(dynamic other) {
+    if (identical(this, other))


you can drop the identical check, the rest of the method is cheap enough

Hixie · 2018-02-16T01:11:21Z

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

+    this.padding: EdgeInsets.zero,
+  }) : assert(borderSide != null), assert(padding != null);
+
+  /// The color and weight of the line drawn below the selected tab.


here and elsewhere in this class: consider reframing the documentation to describe this just as a shape, rather than as an indicator-specific feature.

Hixie · 2018-02-16T01:16:43Z

I know I told you to go down the ShapeBorder route, but in looking at this I think actually the Decoration route would be better.

Don't worry, ShapeBorder and Decoration are basically the same thing. But Decoration fits your use case better (e.g. it doesn't have opinions about inner and outer paths). ShapeBorder would make more sense if this was going to actually clip something. Decoration is more about just drawing a box or whatever, which is what you're doing here.

General approach looks good.

HansMuller · 2018-02-20T18:23:49Z

TabBar's indicator (was indicatorShape) is now a Decoration.

amirh · 2018-02-20T20:20:02Z

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

+import 'colors.dart';
+
+/// Used with [TabBar.indicator] to draw a horizontal line below the
+/// selected tab


nit: period

amirh · 2018-02-20T20:44:48Z

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

+/// defines the line's color and weight.
+///
+/// The [TabBar.indicatorSize] property can be used to define the
+/// tab's boundary in terms of its (centered) widget, [TabIndicatorSize.label],


Should we say "the indicator's bounding rectangle" instead of "the tab's boundary"?

amirh · 2018-02-20T20:45:46Z

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

+///
+/// The [TabBar.indicatorSize] property can be used to define the
+/// tab's boundary in terms of its (centered) widget, [TabIndicatorSize.label],
+/// or the the entire tab, [TabIndicatorSize.tab].


nit: extra the

amirh · 2018-02-20T20:54:05Z

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

+  /// The color and weight of the line drawn below the selected tab.
+  final BorderSide borderSide;
+
+  /// Locates the [borderSide] style selected tab underline relative to the


I'm a bit confused by this sentence, maybe (as you suggested) omit the "[borderSide] style selected tab" part?

amirh · 2018-02-20T20:58:47Z

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

+  /// the tab widget itself.
+  final Decoration indicator;
+
+  /// The location of the selected tab indicator is defined relative to the


nit: might make it easier to read if we start with a summary sentence. (maybe something like "Defines the sizing behavior of the tab indicator"?)

amirh · 2018-02-20T21:48:51Z

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

+  /// tab.
+  tab,
+
+  /// The tab's bounds are only as wide as (centered) tab widget itself.


nit: ...the (centered) tab widget

amirh · 2018-02-20T22:04:06Z

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

@@ -488,14 +529,16 @@ class TabBar extends StatefulWidget implements PreferredSizeWidget {
    this.indicatorColor,
    this.indicatorWeight: 2.0,
    this.indicatorPadding: EdgeInsets.zero,
+    this.indicator, // TBD indicator


Should we say something about the nullability of this in the constructor's doc?

Yes, I added a sentence about indicatorColor et al being ignored if indicator is specified.

amirh · 2018-02-20T22:15:40Z

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

+    // 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.
+    if (color == Material.of(context).color)


Maybe just not do anything? I would think it's the developer's responsibility to not use same background/foreground colors. I don't think we can just decide he is making a mistake (maybe he wants the same color), especially as we don't have a good resolution here. (Also, what happens if the material color is white?)

If we leave this here, we should probably document this behavior in the relevant places (TabBar.indicatorColor and Theme.indicatorColor)

This is old code that I left alone for the sake of backwards compatibility. I'd be happy to document or revise it in a separate PR

amirh · 2018-02-20T22:21:40Z

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

@@ -636,12 +742,24 @@ class _TabBarState extends State<TabBar> {
  @override
  void didUpdateWidget(TabBar oldWidget) {
    super.didUpdateWidget(oldWidget);
-    if (widget.controller != oldWidget.controller)
+    if (widget.controller != oldWidget.controller ||
+        widget.indicatorColor != oldWidget.indicatorColor ||


It looks like _updateTabController() short circuits if newController == _controller, so the new or conditions here doesn't do anything no?

Should we have a test that changes one of these parameters (e.g indicatorColor) and verifies that the change worked?

amirh · 2018-02-20T22:26:21Z

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

@@ -591,6 +665,37 @@ class _TabBarState extends State<TabBar> {
  _IndicatorPainter _indicatorPainter;
  int _currentIndex;
  double _tabStripWidth;
+  List<GlobalKey> _tabKeys;


nit: can you add a comment saying what is this used for?

Yes (added where it's first used)

amirh · 2018-02-20T22:35:00Z

I did not review the tests

amirh

LGTM

tvolkert · 2018-02-21T23:16:19Z

This change made the selected tab indicator a little bit wider than it was previously, which turned up in our internal pixel diffing tests. Was this intentional?

/cc @jason-simmons

HansMuller · 2018-02-21T23:17:31Z

Yes, and I should have mentioned that here. The original tab indicator painter had a 1/2 pixel error.

tvolkert · 2018-02-22T00:17:58Z

Actually it looks like this caused a regression in certain cases where the selected tab changes, and the font of the tab is updated to reflect the selection change, but the selected tab indicator doesn't move.

googlebot added the cla: yes label Feb 16, 2018

Hixie reviewed Feb 16, 2018

View reviewed changes

HansMuller force-pushed the tab_indicator_widget branch from d5ea0c3 to d5d7c9a Compare February 20, 2018 18:22

HansMuller changed the title ~~Generalized TabBar selected tab indicator [WIP]~~ Generalized TabBar selected tab indicator Feb 20, 2018

amirh reviewed Feb 20, 2018

View reviewed changes

HansMuller force-pushed the tab_indicator_widget branch from d5d7c9a to 55d06f2 Compare February 21, 2018 01:18

HansMuller added 5 commits February 21, 2018 07:15

Generalized TabBar selected tab indicator

9d26666

tests pass

2f7489e

DefaultTabIndicator => UnderlineTabIndicator

38af44a

ShapeDecoration instead of ShapeBorder

50a8faa

Updated per review feedback

bdcb5d1

HansMuller force-pushed the tab_indicator_widget branch from 55d06f2 to bdcb5d1 Compare February 21, 2018 15:15

HansMuller added 2 commits February 21, 2018 07:48

restored tab_indicator.dart changes

0a8a617

the the

e4ebd49

amirh approved these changes Feb 21, 2018

View reviewed changes

HansMuller merged commit 24efb55 into flutter:master Feb 21, 2018

tvolkert mentioned this pull request Feb 22, 2018

Selected tab indicator sometimes isn't updated #14812

Closed

HansMuller mentioned this pull request Feb 28, 2018

Slider Visual Update #14901

Merged

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018

Generalized TabBar selected tab indicator (flutter#14741)

5343adf

xu-baolin mentioned this pull request Oct 15, 2020

Make TabBar indicator color automatic adjustment optional #68171

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2021

Generalized TabBar selected tab indicator #14741

Generalized TabBar selected tab indicator #14741

Uh oh!

Conversation

HansMuller commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Hixie commented Feb 16, 2018

Uh oh!

HansMuller commented Feb 20, 2018

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!

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!

HansMuller Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirh commented Feb 20, 2018

Uh oh!

amirh left a comment

Choose a reason for hiding this comment

Uh oh!

tvolkert commented Feb 21, 2018

Uh oh!

HansMuller commented Feb 21, 2018

Uh oh!

tvolkert commented Feb 22, 2018

Uh oh!

Uh oh!

HansMuller commented Feb 16, 2018 •

edited

Loading

HansMuller Feb 21, 2018 •

edited

Loading