We still need the text for the titles, because otherwise we can't provide anything to the accessibility layer.

But maybe we can have a new BottomNavigationBarType that hides the labels from view?

Thank you so much, @dooboolab , for writing this!

Make the following changes when you have a moment:

• Keep the title strings mandatory for the Semantics system to read.
• Make titles something that's shown by default but hideable.
  • When hidden, the layout responds as if they are not there at all (as opposed to now where you pass "" and still have room for titles.)

@willlarche Thank you for the review. I've updated the codes applying to your review. I've created showLabel parameter in BottomNavigationBarItem that judges the rendering for the label. Would this be ok?

@willlarche Hi. Sorry for the late comeback. I was busy preparing for the Y-Combinator interview.
I don't quite understand your review because the purpose of this update is to provide ability to support bottomNavigationBarItem without label.

Below is expected feature.

BottomNavigationBarItem with labels.

BottomNavigationBarItem without labels.

I supported this providing showLabel attribute. Is there something I've not understood?

Yes, sorry. The accessibility experience needs to be the same with and without titles. Accessibility in Flutter is done thru the Semantics APIs. https://www.didierboelens.com/2018/07/semantics/

I've posted an image of what the semantics tree looks like with the titles (like before) and then what the tree looks like without the titles (new version.) We need them to be identical no matter what the front end looks like. So, even without seeing the titles, a blind user would be able to hear them.

@hyochan are you ready for re-review on this?

Did patch my flutter using this commit. Eg. win my case bottom_navigation_bar builds fixed label. I declare:

BottomNavigationBarItem(
icon: Icon(Icons.search),
title: Text("search"),
label: "search",
showLabel: false),

I still have "ghost" space for the text and therefore my icons are not centered on the main axis.
Am I using it wrong or it is the intended behavior?
Thx.

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

CLAs look good, thanks!

@willlarche I've just updated my code. Sorry for the delay, I needed to think the way to support semanticLabel since the title widget could be any widget including, Container. I tried to get text value out of Text widget. I've also done the rebase. Lastly, I've tried to expose heightFactor in last commit to support flexible design (Don't know why the last commit fails the build tough). Please review my update!

@ohenley My code wasn't ready since I was applying to support semanticLabel. Could you try current update?

@willlarche Well, I thought that was expected when bottomNavigationBar is built with

type: BottomNavigationBarType.shifting

which is the default type. If you don't want your icon to rise, I think we should use below.

type: BottomNavigationBarType.fixed

P.S. I've just pushed another commit since I found unnecessary margin in fixedLabel.

@hyochan:
can not try today, will do tomorrow.

@willlarche and @hyochan :
regarding the clock icon rising, just to add to the comment by hyochan, from what I recall of the implementation, type: BottomNavigationBarType.shifting (icon rising behaviour) is selected automatically in code when you have more than 3 navigation_bar_item in the nav_bar.

You're right! Thanks for your patience.

So, this looks almost there. I'll put in a couple notes at the request of @jonahwilliams who will take over review as I'll be unavailable for 3 weeks.

@jonahwilliams I've made the fix. Thanks.

CLAs look good, thanks!

@jonahwilliams Thanks. I was able to do the job with your grateful help 🍻.

P.S I've tested this locally and checked it is passing. I don't get why CI build is failing.

Several files have trailing whitespace:

⏩ RUNNING: cd .; grep --line-number --extended-regexp [[:blank:]]$ packages/flutter/lib/src/material/bottom_navigation_bar.dart packages/flutter/test/material/bottom_navigation_bar_test.dart
packages/flutter/test/material/bottom_navigation_bar_test.dart:882:    
packages/flutter/test/material/bottom_navigation_bar_test.dart:885: 
packages/flutter/test/material/bottom_navigation_bar_test.dart:927:    
🕐 ELAPSED TIME: 0:00:00.022591 for grep --line-number --extended-regexp [[:blank:]]$ packages/flutter/lib/src/material/bottom_navigation_bar.dart packages/flutter/test/material/bottom_navigation_bar_test.dart in .: 
Whitespace detected at the end of source code lines.
Please remove:
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
ERROR: Last command exited with 0 (expected: 1).
Command: grep --line-number --extended-regexp [[:blank:]]$ packages/flutter/lib/src/material/bottom_navigation_bar.dart packages/flutter/test/material/bottom_navigation_bar_test.dart
Relative working directory: .

@jonahwilliams 's approval and passing tests all that's needed now.

Hi Hyo, thanks so much for opening this PR and working on this feature since it is highly requested! I’m making a number of unrelated changes to the BottomNavigationBar component in: #27698. My changes update the motion logic of the BottomNavigationBar and will remove some of the code you've been working in here. Would it be OK if I incorporate some of the work you did in your PR into my current PR? I will close your PR and add you as a co-author on the new PR.

@willlarche @jonahwilliams I think I made it now. Sorry that it took me times to recognize the process of contribution to flutter since this was my first time. I can do better next time. Please review once more.

@johnsonmh Hi. Thank you for the great offer, but I'd like to make this PR as one complete one since I've been taking quite a time working on it. Also, I am thinking of another work based on this which is #22685. I am planning to close that one when I start working on that. I hope you could rebase if possible.

Put a temporary flag on it so I can merge it simultaneously with #27698 next week.

@hyochan , You've worked super hard on this and we want to land it. Since @johnsonmh 's PR would have a slight breaking change to yours, we're going to merge them together next week so there's no Flutter release between their merges. Just means holding for a few days! You're all set.

Thank you so much. I am very honored to contribute to this awesome Google project. I will do more and better next time.

Similar versions of the changes and new tests included in this PR were also included in what is now #28159

I'm going to close this PR in favor of #28159. I have listed you as a co-contributor on #28159, please let me know if that's OK.

@HansMuller I thought I've already answered above that I wish this PR could be merged as one complete one. I wished myself to list up my PR's and being motivated to contribute to flutter further. Why couldn't this be merged and rebase separately? I've also done quite a work rebasing because I had to wait for the reviews even though I was on the recent branch.

@hyochan Right, but unfortunately the work you did here was independently duplicated in another PR that had broader scope (#28159). It wouldn't make sense to land this PR only to effectively remove it with #28159.

I wanted to give you credit for the work you did, which is why I added you as a co-contributor to #28519.