[Tabs] Fix tab indicator flies off issue #65463

nero-angela · 2020-09-09T10:14:47Z

Description

I think the code below is not only wrong, but unnecessary.

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

Lines 400 to 404 in 54ade88

    
           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)); 
        
           } else {

The reason why I think it's unnecessary is the below code already covers the case when the animation is running.

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

Lines 406 to 421 in 54ade88

    
           final int currentIndex = controller.index; 
        
           final Rect previous = currentIndex > 0 ? indicatorRect(size, currentIndex - 1) : null; 
        
           final Rect middle = indicatorRect(size, currentIndex); 
        
           final Rect next = currentIndex < maxTabIndex ? indicatorRect(size, currentIndex + 1) : null; 
        
           final double index = controller.index.toDouble(); 
        
           final double value = controller.animation.value; 
        
           if (value == index - 1.0) 
        
             _currentRect = previous ?? middle; 
        
           else if (value == index + 1.0) 
        
             _currentRect = next ?? middle; 
        
           else if (value == index) 
        
             _currentRect = middle; 
        
           else if (value < index) 
        
             _currentRect = previous == null ? middle : Rect.lerp(middle, previous, index - value); 
        
           else 
        
             _currentRect = next == null ? middle : Rect.lerp(middle, next, value - index);

And as you can see the below code, it is implemented by putting _currentRect in Rect.lerp() and putting the result back in _currentRect. Implemented in this way, the indicator will performs an acceleration motion. I prepared a dart pad to help you understand.

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

Line 403 in e682ec7

    
           _currentRect = Rect.lerp(targetRect, _currentRect ?? targetRect, _indexChangeProgress(controller));

In the test, I think below comment is mentioned because the indicator performs acceleration motion.

flutter/packages/flutter/test/material/tabs_test.dart

Line 1656 in e682ec7

// The x coordinates of p1 and p2 were derived empirically, not analytically.

I have also identified the cause of the disappearance of the indicator, but I will skip this because the problematic code is replaced with the existing code.

before	after

Related Issues

Fixes #48000

Tests

I added the following test:

Tab indicator animation test

I fixed the following test:

flutter/packages/flutter/test/material/tabs_test.dart

Lines 1656 to 1661 in e682ec7

    
           // The x coordinates of p1 and p2 were derived empirically, not analytically. 
        
           expect(tabBarBox, paints..line( 
        
             strokeWidth: indicatorWeight, 
        
             p1: const Offset(2476.0, indicatorY), 
        
             p2: const Offset(2574.0, indicatorY), 
        
           ));

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.

chunhtai

I think we still need the first if case to deal with user tap a tab that are more than one tab away?

chunhtai · 2020-09-21T21:20:21Z

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

+    else if (value == index)
+      _currentRect = middle;
+    else if (value < index)
+      _currentRect = previous == null ? middle : Rect.lerp(middle, previous, index - value);


isn't this assume index - value is always <1.0 ? in case of user tapp on a tab, this value may be >1.0

Yes you are right. index - value can be greater than 1.0. I thought this was the solution for the case of selecting a tab during animation.

I added a gif that the case of user tap a tab that are more than one tab away. It seems no problem, but I'm not sure if this is the case you were talking about or not. If this is not the case of you intended, could you describe more detail situation for reproduce?

Ah I see, we are doing a negative lerp and using the wrong previous/next rect. They just so happen to cancel each other out.

for example we move from 0 to 2
it will lerp from -1.0 to 1.0, and from rect =1, to dest =2
so this just cancel each other out.

I think this is a dangerous because the lerp value should be from 0.0 to 1.0. We do not guarantee the behavior outside the range. it just so that our current implementation of Rect.lerp somehow works with this corner case. However, it is not guaranteed that it will still work the same in the future.

The correct logic should be as following, let's take the 0 to 2 example

it should lerp from 0.0 to 1.0 from rect 0 to dest 2

@chunhtai When there are three tabs A, B, C, the image below is a scenario in which the user taps C and then taps B after the animation has progressed 25%.

Here you can see a case which the lerp value is greater than 1, and I think it can be relatively greater or less than 1 depending on which point the lerp is based on.

As this document also says that a value greater than 1 or less than 0 is not an invalid value, I think it is just a difference in mathematical expressions and there is no problem.

Nevertheless, if the lerp value should be between 0 and 1, I think that the function should be newly implemented regardless of the tapping problem during animation.

This seems to not work correctly if the sizes of the tab are not the same.

import 'package:flutter/material.dart'; import 'package:flutter/scheduler.dart'; void main() { timeDilation = 10.0; runApp(Test()); } class Test extends StatelessWidget { @override Widget build(BuildContext context) { return MaterialApp( title: '', home: Material( color: Colors.black, child: DefaultTabController( length: 3, child: Column( mainAxisAlignment: MainAxisAlignment.center, children: <Widget>[ Text('Tap 1 then tap 2.', style: TextStyle(color: Colors.white)), TabBar( isScrollable: true, indicatorSize: TabBarIndicatorSize.tab, tabs: <Widget>[ Text('-', style: TextStyle(fontSize: 48.0)), Text('1ljkjlkjlk', style: TextStyle(fontSize: 48.0)), Text('2', style: TextStyle(fontSize: 48.0)), ], ), ], ), ), ), ); } }

You are right. It doesn't work properly. It seems my current approach is wrong.
Sorry for wasting your time. 🙏

chunhtai · 2020-10-30T19:02:43Z

Hi @nero-angela are you still working on this PR?

nero-angela · 2020-10-31T02:05:49Z

Hi @nero-angela are you still working on this PR?

Hi @chunhtai, I'm waitting for your answer about this question.

Could you please check my answer?

nero-angela · 2020-11-13T10:41:57Z

@chunhtai
Going back to the beginning, I tried different approaches based on the current master branch. The code for the solution can be found here.

At the moment of tabbing, the current indicator's position was recorded in the TabController as the _indexPositionWhenTapping property, and animation was created naturally by using it. I don't like the location of _indexPositionWhenTapping, but I couldn't find a better way to do this.

In this regard, is it better to close the current PR and create a new PR to proceed?

chunhtai · 2020-11-13T17:44:29Z

I wonder whether we can do it in the following way:

we have tabcontroller.value and tabcontroller.index. with these information we know

The direction of the animation
The current tab index

We can try to grab the current tab index and the index that is one tab ahead. We can just lerp between this two tabs.

For example: value = 1.4, index =5

we know the animation is moving to the right,
we can find the current tab index = math.floor(value) = 1
and the index one tab ahead = math.floor(value) + 1 = 2
we do lerp(tab1, tab2, value - math.floor(value))

for example value = 2.3, index = 1

we know the animation is moving to the left,
we can find the current tab index = math.ceil(value) = 3
and the index one tab ahead = math.ceil(value) -1 = 2
we do lerp(tab3, tab2, math.ceil(value) - value)

For abs(value - index) < 1.0 , we just keep the logic as is.

…n changed

nero-angela · 2020-11-14T06:15:40Z

I wonder whether we can do it in the following way:

we have tabcontroller.value and tabcontroller.index. with these information we know

The direction of the animation

The current tab index

We can try to grab the current tab index and the index that is one tab ahead. We can just lerp between this two tabs.

For example: value = 1.4, index =5

we know the animation is moving to the right,
we can find the current tab index = math.floor(value) = 1
and the index one tab ahead = math.floor(value) + 1 = 2
we do lerp(tab1, tab2, value - math.floor(value))

for example value = 2.3, index = 1

we know the animation is moving to the left,
we can find the current tab index = math.ceil(value) = 3
and the index one tab ahead = math.ceil(value) -1 = 2
we do lerp(tab3, tab2, math.ceil(value) - value)

For abs(value - index) < 1.0 , we just keep the logic as is.

Wow, you are really smart 👍👍. I implemented the solution in the way you gave me. All of the cases found so far seem to work fine, but please check the code and let me know if it's not what you intended.

One of the previous tests failed because the indicator's speed was changed normally, and I fixed it correctly. Check out the test-related code here.

chunhtai · 2020-11-16T17:42:32Z

@nero-angela
The code looks good. Now I think of it, we can probably apply the same logic when abs(value - index) < 1.0

nero-angela · 2020-11-17T08:32:13Z

@chunhtai
Isn't the current code okay even when abs(value - index) < 1.0?
I thought it would work well for all cases with the current code, so I deleted the unnecessary code.
This code passed all the test code and it seems to work fine. What do u think about it?

chunhtai · 2020-11-18T00:26:12Z

@nero-angela yes, I think that looks good, can you revise this PR with the change?

…indicator

nero-angela · 2020-11-18T15:15:46Z

@chunhtai
I have finished revising the PR with the changes. Could you please check?

chunhtai

LGTM

[Tabs] Fix tab indicator flies off issue

141e086

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

googlebot added the cla: yes label Sep 9, 2020

[Tabs] Add indicator animation test

92aaf32

chunhtai self-requested a review September 10, 2020 23:43

chunhtai reviewed Sep 21, 2020

View reviewed changes

nero-angela added 2 commits November 14, 2020 14:58

[Tabs] Fix tab indicator flies off issue

4082e19

[Tabs] Fix the tabs test because the indicator's moving speed has bee…

a134485

…n changed

[Tabs] Delete unnecessary code

42854b3

nero-angela added 2 commits November 18, 2020 15:28

Merge branch 'tabs_indicator' into tab_indicator

6bdd871

Merge branch 'master' of https://github.com/flutter/flutter into tab_…

ab93477

…indicator

chunhtai approved these changes Nov 18, 2020

View reviewed changes

chunhtai added the waiting for tree to go green label Nov 18, 2020

fluttergithubbot merged commit af616a0 into flutter:master Nov 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tabs] Fix tab indicator flies off issue #65463

[Tabs] Fix tab indicator flies off issue #65463

nero-angela commented Sep 9, 2020 •

edited

chunhtai left a comment

chunhtai Sep 21, 2020

nero-angela Sep 22, 2020 •

edited

chunhtai Nov 2, 2020

nero-angela Nov 12, 2020 •

edited

chunhtai Nov 12, 2020 •

edited

nero-angela Nov 13, 2020

chunhtai commented Oct 30, 2020

nero-angela commented Oct 31, 2020 •

edited

nero-angela commented Nov 13, 2020

chunhtai commented Nov 13, 2020

nero-angela commented Nov 14, 2020

chunhtai commented Nov 16, 2020

nero-angela commented Nov 17, 2020 •

edited

chunhtai commented Nov 18, 2020

nero-angela commented Nov 18, 2020

chunhtai left a comment

	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));
	} else {

	final int currentIndex = controller.index;
	final Rect previous = currentIndex > 0 ? indicatorRect(size, currentIndex - 1) : null;
	final Rect middle = indicatorRect(size, currentIndex);
	final Rect next = currentIndex < maxTabIndex ? indicatorRect(size, currentIndex + 1) : null;
	final double index = controller.index.toDouble();
	final double value = controller.animation.value;
	if (value == index - 1.0)
	_currentRect = previous ?? middle;
	else if (value == index + 1.0)
	_currentRect = next ?? middle;
	else if (value == index)
	_currentRect = middle;
	else if (value < index)
	_currentRect = previous == null ? middle : Rect.lerp(middle, previous, index - value);
	else
	_currentRect = next == null ? middle : Rect.lerp(middle, next, value - index);

	// The x coordinates of p1 and p2 were derived empirically, not analytically.
	expect(tabBarBox, paints..line(
	strokeWidth: indicatorWeight,
	p1: const Offset(2476.0, indicatorY),
	p2: const Offset(2574.0, indicatorY),
	));

[Tabs] Fix tab indicator flies off issue #65463

[Tabs] Fix tab indicator flies off issue #65463

Conversation

nero-angela commented Sep 9, 2020 • edited

Description

Related Issues

Tests

Checklist

Breaking Change

chunhtai left a comment

Choose a reason for hiding this comment

chunhtai Sep 21, 2020

Choose a reason for hiding this comment

nero-angela Sep 22, 2020 • edited

Choose a reason for hiding this comment

chunhtai Nov 2, 2020

Choose a reason for hiding this comment

nero-angela Nov 12, 2020 • edited

Choose a reason for hiding this comment

chunhtai Nov 12, 2020 • edited

Choose a reason for hiding this comment

nero-angela Nov 13, 2020

Choose a reason for hiding this comment

chunhtai commented Oct 30, 2020

nero-angela commented Oct 31, 2020 • edited

nero-angela commented Nov 13, 2020

chunhtai commented Nov 13, 2020

nero-angela commented Nov 14, 2020

chunhtai commented Nov 16, 2020

nero-angela commented Nov 17, 2020 • edited

chunhtai commented Nov 18, 2020

nero-angela commented Nov 18, 2020

chunhtai left a comment

Choose a reason for hiding this comment

nero-angela commented Sep 9, 2020 •

edited

nero-angela Sep 22, 2020 •

edited

nero-angela Nov 12, 2020 •

edited

chunhtai Nov 12, 2020 •

edited

nero-angela commented Oct 31, 2020 •

edited

nero-angela commented Nov 17, 2020 •

edited