Tab bar improvements #63683

emem365 · 2020-08-13T17:23:26Z

Description

This PR adds the ability to add padding to user defined indicators in TabBar. Earlier, the TabBar used to ignore indicatorPadding for user-defined indicators.

To be specific, what I have done is, take this indicatorPadding property and pass it down to the _IndicatorPainter, this way, I apply the padding to the Rect inside which the CustomPainter is going to paint the indicator.

Also, UnderlineTabIndicator, takes in a insets parameter, which is set to indicatorPadding, and this is really where the indicatorPadding was originally being used. This parameter used to ignore EdgeInsets.top and EdgeInsets.bottom from the indicatorPadding. Since I added indicatorPadding to the _IndicatorPainter class, it basically meant that the padding would be applied twice if it is still passed to UnderlineTabIndicator. Therefore, for the time-being, I have commented the line that passes the parameter, while creating the widget(Line-830)(If this feels like a good solution, we can just remove the line). I would really like a maintainers outlook on whether or not I should do this, and figuring out what else we could do if not this.

Related Issues

TabBar shadows are clipped and indicator height cannot be reduced #59728

Tests

Added tests for indicatorPadding being checked for both LTR and RTL text directions. Also added both the test using directional padding, ie, using STEB instead of LTRB.

In these tests, I give the indicator property of TabBar, which takes a Decoration object, a box decoration with a color, as this would cover the entire space available for the indicator. I set some values for the indicatorPadding and then expected to find a a rect being painted with the given padding applied.

Screenshot

To reproduce the behaviour, copy the contents of this gist inside main.dart of a new flutter project.

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 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.
Yes, this is a breaking change.

flutter-dashboard · 2020-08-13T17:23:29Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

chunhtai

Although it is nice to be able to apply padding to the custom indicator, but i think the correct solution is not to clip even without the padding. Does it make sense to somehow allow over painting the shadow outside of the tab bar? cc @HansMuller or @shihaohong ?

chunhtai · 2020-08-20T22:52:11Z

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

This need to handle where insets > original rect, it may become native rect

This need to handle where insets > original rect, it may become native rect

A few assert statements should fix this, i guess. I'll get to it.

Edit: Or should throw some kind of Exception.
I do know all I need to check is if the rect.size is smaller than insets.collapsedSize
I just need to know if I should just assert this or throw an exception. Exception seems a better way to me.

throw a flutter error makes more sense because this is a user define value.

emem365 · 2020-08-21T06:12:01Z

@chunhtai

i think the correct solution is not to clip even without the padding.

I was looking for a way to be able to do that, but I think the way this works is that TabBar draws a Canvas according to the space available to it, and then the indicator is being painted on that.(This is what my understanding of the code says, I might be wrong here.) Painting outside the canvas is obviously a no-no. Could there be a way to be able to increase the size of canvas? That seems like an odd way to solve this because, I think its hard to ever know how big it should be. What do you think about this? Please let me know.

shihaohong · 2020-09-01T13:16:41Z

@chunhtai I suggested increasing the size of the padding to prevent clipping of the shadows in the original issue itself -- Would applying a shadow outside of the boundaries be a valid approach? I was under the assumption that the clipping is expected because the shadow itself "did not fit" into the provided rect, and painting outside of the boundaries of the rect goes against Flutter's layout system since we should not expect any painting to happen outside the rect itself.

chunhtai · 2020-09-02T19:04:59Z

@chunhtai I suggested increasing the size of the padding to prevent clipping of the shadows in the original issue itself -- Would applying a shadow outside of the boundaries be a valid approach? I was under the assumption that the clipping is expected because the shadow itself "did not fit" into the provided rect, and painting outside of the boundaries of the rect goes against Flutter's layout system since we should not expect any painting to happen outside the rect itself.

The other way will be making the shadow part of size calculation, but that maybe a big breaking change, and may be unwanted in different use cases.

That being said, I think this pr is good just by the fact that we can now pad custom indicator.

chunhtai · 2020-09-21T21:23:10Z

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

throw a flutter error makes more sense because this is a user define value.

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

emem365 · 2020-09-22T11:09:38Z

@chunhtai Throwing a FlutterError if (rect.size >= insets.collapsedSize) isnt true. The way size objects implement >=, this condition holds true only if both height and width of rect is bigger than insets.collapsedSize, which is the ideal condition for us. So throwing an error if it doesn't satisfy that condition, i.e., if either of the properties is smaller, for rect made sense.
Also, do you think this PR needs some kind of tests, because I haven't been able to understand what I could test with this PR.

flutter-dashboard · 2020-10-12T11:12:09Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

chunhtai · 2020-10-12T16:44:45Z

@emem365 Sorry for the delay.
In that case, you may need to check both width and height separately.

Yes this need a test. Can you try to see if you can verify the indicatorRect when the indicatorPadding is applied?

emem365 · 2020-10-12T18:30:12Z

Ok sure. I'll try to work on this asap

emem365 · 2020-10-17T05:33:16Z

@chunhtai
Added tests for indicatorPadding being checked for both LTR and RTL text directions. Also added both the test using directional padding, ie, using STEB instead of LTRB.

In these tests, I give the indicator property of TabBar, which takes a Decoration object, a box decoration with a color, as this would cover the entire space available for the indicator. I set some values for the indicatorPadding and then expected to find a a rect being painted with the given padding applied.

Similar tests already existed for indicatorPadding being tested against UnderlineIndicator, ie, the default indicator.
Would we want to change the description for these tests that already existed to indicate more clearly that it tests for default indicator or is it fine as is?

chunhtai · 2020-10-30T19:06:07Z

@emem365 For some reason I keep thinking I have reviewed this PR, sorry for the delay again.

Instead of changing the existing test description, can you make the new test name more descriptive and indicate that it is testing for custom indicator?

chunhtai · 2020-10-30T19:07:28Z

There seems to be some conflict and you may need to rebase off latest master to fix the test failures. Otherwise, this looks good to me except for the test description.

emem365 · 2020-11-02T07:56:02Z

@emem365 For some reason I keep thinking I have reviewed this PR, sorry for the delay again.

@chunhtai No worries. I understand it must be quite overwhelming with so many issues and PRs

Instead of changing the existing test description, can you make the new test name more descriptive and indicate that it is testing for custom indicator?

Sure, that makes sense. I'll rebase and change the names soon

emem365 · 2020-11-04T18:13:05Z

Hey @chunhtai,
I have rebased the branch and renamed the tests.
Have a look and let me know if anything else needs to be changed.

chunhtai

LGTM

emem365 · 2020-11-04T19:27:36Z

Thanks 🙌

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

googlebot added the cla: yes label Aug 13, 2020

emem365 mentioned this pull request Aug 13, 2020

TabBar shadows are clipped and indicator height cannot be reduced #59728

Open

HansMuller requested a review from chunhtai August 13, 2020 23:45

chunhtai reviewed Aug 20, 2020

View reviewed changes

emem365 requested a review from chunhtai August 27, 2020 15:10

chunhtai reviewed Sep 21, 2020

View reviewed changes

emem365 requested a review from chunhtai September 22, 2020 11:10

emem365 added 6 commits November 4, 2020 21:43

indicatorPadding now does not get ignored

3a1eb34

Removing unnecessary comments

922d177

Throw FlutterError on indicatorPadding

8a90450

Tests for indicatorPadding

af1edee

Migrate test to NNBD

1a1df80

Tests Renamed

92356dc

chunhtai approved these changes Nov 4, 2020

View reviewed changes

chunhtai merged commit 595df47 into flutter:master Nov 4, 2020

This was referenced Feb 27, 2023

tabs.dart documentation needs some cleanup #121523

Closed

Cleanup material/tabs.dart documentation #121524

Merged

Tab bar improvements #63683

Tab bar improvements #63683

Uh oh!

Conversation

emem365 commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Screenshot

Checklist

Breaking Change

Uh oh!

flutter-dashboard bot commented Aug 13, 2020

Uh oh!

chunhtai left a comment • 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!

chunhtai Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

emem365 Aug 21, 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!

chunhtai Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

emem365 commented Aug 21, 2020

Uh oh!

shihaohong commented Sep 1, 2020

Uh oh!

chunhtai commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chunhtai Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

flutter-dashboard bot commented Oct 12, 2020

Uh oh!

chunhtai commented Oct 12, 2020

Uh oh!

emem365 commented Oct 12, 2020

Uh oh!

emem365 commented Oct 17, 2020

Uh oh!

chunhtai commented Oct 30, 2020

Uh oh!

chunhtai commented Oct 30, 2020

Uh oh!

emem365 commented Nov 2, 2020

Uh oh!

emem365 commented Nov 4, 2020

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

emem365 commented Nov 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

emem365 commented Aug 13, 2020 •

edited

Loading

chunhtai left a comment •

edited

Loading

emem365 Aug 21, 2020 •

edited

Loading

chunhtai commented Sep 2, 2020 •

edited

Loading

emem365 commented Sep 22, 2020 •

edited

Loading