Have TabBar indicator properly follow tabController animation value by giantsol · Pull Request #57799 · flutter/flutter

giantsol · 2020-05-22T10:00:03Z

Description

There are three parts that move in TabBar and TabBarView when we call tabController.animateTo():

TabBar's indicator
TabBar's scroll position (if it's scrollable)
TabBarView's scroll position

Currently, none of them properly follow the duration and curve given to tabController.animateTo(). Whether we should change 2 and 3's behavior needs a consensus, I believe, but 1 was definitely a bug. There was a code intended to make the indicator follow tabController.animation.value but it wasn't exactly working as intended. So that's what I've tried to fix here.

Reproducible code

class Main extends StatefulWidget {
  Main({Key key}) : super(key: key);

  @override
  _MainState createState() => _MainState();
}

class _MainState extends State<Main> with SingleTickerProviderStateMixin {
  final List<Tab> _tabs = <Tab>[
    Tab(text: 'Tab 1'),
    Tab(text: 'Tab 2'),
    Tab(text: 'Tab 3'),
  ];

  TabController _tabController;

  @override
  void initState() {
    super.initState();
    _tabController = TabController(vsync: this, length: _tabs.length);
  }

  @override
  void dispose() {
    _tabController.dispose();
    super.dispose();
  }

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        appBar: AppBar(
          bottom: TabBar(
            controller: _tabController,
            tabs: _tabs,
            isScrollable: true,
            labelPadding: const EdgeInsets.symmetric(horizontal: 56),
          ),
        ),
        body: TabBarView(
          controller: _tabController,
          children: _tabs.map((Tab tab) {
            return Center(
              child: Column(
                mainAxisSize: MainAxisSize.min,
                children: [
                  Text(tab.text),
                  RaisedButton(
                    child: Text('animate index to 0'),
                    onPressed: () {
                      _tabController.animateTo(0, duration: const Duration(seconds: 3));
                    },
                  ),
                ],
              ),
            );
          }).toList(),
        ),
      ),
    );
  }
}

Before

Just looking at the indicator, I've given 3 seconds duration to the animation, but indicator moves much faster than that. Although it looks like the animation has finished early, when we drag the TabBarView, indicator doesn't respond because it's actually still in animation.

After

Now the indicator properly follows the animation value.

Actually, if you look at the second gif, there's one more behavior I've changed here: e7c9efc. Previously, while tabController is in animation, dragging the TabBarView doesn't interfere, i.e. tabController keeps animating. But I personally thought this could look more like a bug. Rather, if user starts dragging, tabController's animation should stop and follow that value. If this change looks inappropriate, I'm happy to revert it.

Related Issues

#55625
#19143

Tests

I've modified one existing test and also added a new one, but it's not actually a breaking change.. The one I've modified was actually giving a false positive, i.e. giving ok sign to improperly acting logic. You can check it here: 304b1cf

So I'll first say no to is it breaking change? checkbox below. Please tell me if there's an issue :)

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

…ng TabBarView

giantsol · 2020-05-22T10:01:07Z

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

    if (controller.indexIsChanging) {
      // The user tapped on a tab, the tab controller's animation is running.
      final Rect targetRect = indicatorRect(size, controller.index);
-      _currentRect = Rect.lerp(targetRect, _currentRect ?? targetRect, _indexChangeProgress(controller));


it was lerping between targetRect and _currentRect, in which _currentRect was getting updated every frame. Thus it moved faster than intended.

chunhtai · 2020-05-30T00:01:34Z

I think we should definitely do something about 2 and 3. Do you have plan to fix those two? I have not look into it deeper. but if they are different cause, i am ok with separating out to different pr.

We need more discussion on whether the gesture can interrupt the animation. From the look of the tab bar code, it seems like we are disallowing that.

giantsol · 2020-05-30T00:56:03Z

@chunhtai

Fixing 2 and 3 needs more look into it. I've been looking at it but paused because I thought I needed your opinions first. But surely, they have different causes and it'd be nice if I can shoot out a separate PR for fixing 2 and 3 together :)

If your intentions were not to make gestures interrupt the animation, I can just revert my second commit. Should I do so?

chunhtai · 2020-06-01T16:35:03Z

@chunhtai

Fixing 2 and 3 needs more look into it. I've been looking at it but paused because I thought I needed your opinions first. But surely, they have different causes and it'd be nice if I can shoot out a separate PR for fixing 2 and 3 together :)

If your intentions were not to make gestures interrupt the animation, I can just revert my second commit. Should I do so?

We can have more discussion on whether the gesture should interrupt the animation. If there is no use case that we should change the behavior, we should leave it as is.

giantsol · 2020-06-01T16:41:58Z

@chunhtai
Fixing 2 and 3 needs more look into it. I've been looking at it but paused because I thought I needed your opinions first. But surely, they have different causes and it'd be nice if I can shoot out a separate PR for fixing 2 and 3 together :)
If your intentions were not to make gestures interrupt the animation, I can just revert my second commit. Should I do so?

We can have more discussion on whether the gesture should interrupt the animation. If there is no use case that we should change the behavior, we should leave it as is.

Sure, the reasoning behind me changing that behavior was, after I fixed indicator to properly follow the given duration, it took so long for the indicator to start responding to gesture. If I called animateTo with say duration of 10 seconds, indicator won't respond to gesture for that long time. That looked unnecessary for me, so I thought 'why not let the gesture interrupt ongoing animation?'

chunhtai · 2020-06-01T17:26:15Z

It looks fine because the page view warping is fast and usually finish before the gesture happens. However, that is something we should fix at some point. Once we fix that, user will more likely to interrupt the page warping. That will become a problem because the page will reorder its children during warping. We need to figure out what should we do when user interrupts it.
Do we directly reorder the children back? (we will see pageview skew for one or two frame)
Do we keep the children in the current order? When do we reorder back without seeing visual skew?

giantsol · 2020-06-02T02:48:14Z

I see the point. The problem you mentioned will be apparent when we fix page warping to follow the given duration.

I guess the first thing I'm gonna try is the first path you've mentioned i.e. directly reorder the children back. We will see the skew, but this skew looks inevitable whatever paths we take. It will have to appear at one point of time. I have to try it out and see how unnatural it looks. If it looks too unnatural, I think keeping the original behavior (i.e. gesture not interrupting animation) is also fine, if it looks better than sudden skewing.

I think it will become clearer after I test it out. If I could do that as a separate PR, I think it'd be better I revert the second commit for now and only merge indicator fix. What do you think?

giantsol · 2020-06-02T14:02:16Z

Hold on. Without any code change, page warping was already being interrupted by user gesture. Because when user starts dragging the TabBarView, previous _pageController.animateToPage(...) gets cancelled and children are reordered instantly. I tested it with just changing the duration parameter in _warpToCurrentIndex()'s _pageController.animateToPage() method.

So there was already a skew in TabBarView, and the behavior I've changed only made TabBar's scroll position and indicator to move in accordance with TabBarView's scroll position. Previously, only TarBarView's animation would get interrupted. Now, TabBarView and TabBar's animation all together will get interrupted, which looks quite reasonable to me now.

chunhtai

Interesting, I didnt realize the we only block gesture on tabbar but not on pageview. I really do no have a good answer whether or not we should allow it on both or disable it on both. I am a bit concern about the skew, and we should find a good way to smooth it if we want to allow it, otherwise we should disable.

chunhtai · 2020-06-02T17:16:20Z

packages/flutter/test/material/tabs_test.dart

      strokeWidth: indicatorWeight,
-      p1: const Offset(2476.0, indicatorY),
-      p2: const Offset(2574.0, indicatorY),
+      p1: const Offset(midIndicatorLeft, indicatorY),


This might be a breaking change, you will need to follow the breaking change protocol. https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes

Since it is likely fixing 2 and 3 might be a breaking change as well due to failing test, It may be good to try to fix them first and see if they are indeed breaking change and group them together if they are

giantsol · 2020-06-02T17:33:25Z

Understood! I guess then I'd better close this PR and write a breaking change proposal first. After accepted, I'll send a PR grouping them all together.
Thanks :)

giantsol added 2 commits May 22, 2020 18:14

Have TabBar indicator properly follow tabController animation value

304b1cf

Make TabController animation stop immediately when user starts draggi…

e7c9efc

…ng TabBarView

fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 22, 2020

googlebot added the cla: yes label May 22, 2020

giantsol commented May 22, 2020

View reviewed changes

HansMuller requested a review from chunhtai May 29, 2020 22:54

chunhtai reviewed Jun 2, 2020

View reviewed changes

giantsol closed this Jun 2, 2020

giantsol mentioned this pull request Jun 4, 2020

Flutter issues with TabController flutter-moum/flutter-moum#41

Closed

giantsol mentioned this pull request Jun 23, 2020

Make TabController animateTo() method work as expected #60080

Closed

10 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have TabBar indicator properly follow tabController animation value#57799

Have TabBar indicator properly follow tabController animation value#57799
giantsol wants to merge 2 commits intoflutter:masterfrom
giantsol:tabcontroller_animation

giantsol commented May 22, 2020 •

edited

Loading

Uh oh!

giantsol May 22, 2020

Uh oh!

chunhtai commented May 30, 2020

Uh oh!

giantsol commented May 30, 2020 •

edited

Loading

Uh oh!

chunhtai commented Jun 1, 2020

Uh oh!

giantsol commented Jun 1, 2020

Uh oh!

chunhtai commented Jun 1, 2020

Uh oh!

giantsol commented Jun 2, 2020

Uh oh!

giantsol commented Jun 2, 2020

Uh oh!

chunhtai left a comment

Uh oh!

chunhtai Jun 2, 2020

Uh oh!

chunhtai Jun 2, 2020 •

edited

Loading

Uh oh!

giantsol commented Jun 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

giantsol commented May 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

giantsol May 22, 2020

Choose a reason for hiding this comment

Uh oh!

chunhtai commented May 30, 2020

Uh oh!

giantsol commented May 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chunhtai commented Jun 1, 2020

Uh oh!

giantsol commented Jun 1, 2020

Uh oh!

chunhtai commented Jun 1, 2020

Uh oh!

giantsol commented Jun 2, 2020

Uh oh!

giantsol commented Jun 2, 2020

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

chunhtai Jun 2, 2020 • 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!

giantsol commented Jun 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

giantsol commented May 22, 2020 •

edited

Loading

giantsol commented May 30, 2020 •

edited

Loading

chunhtai Jun 2, 2020 •

edited

Loading