Fix BottomNavigationBarItem label overflow #120206

sharmashashi · 2023-02-07T16:18:13Z

This PR wraps the label with IntrinsicWidth and then Flexible which allows DefaulTextStyle TextOverflow.ellipsis to work. Wrapping label directly with Flexible brings more space between icon and label. IntrinsicWidth fixes this by giving reasonable width.

Fixes #112163

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

flutter-dashboard · 2023-02-07T16:18:17Z

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 (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

HansMuller

This looks OK, assuming that it doesn't break any existing layouts. Running a large internal test suite now to check.

HansMuller · 2023-03-02T01:01:36Z

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

+          children: <Widget>[
+            icon,
+            const SizedBox(width: 8),
+            Flexible(child: IntrinsicWidth(child: label))


A comment that explains what this unusual combination of widgets is for would be helpful here.

Okay. Defining what Flexible and IntrinsicWidth does for the label would be fine, right?

It's pretty obvious what <Widget>[icon, const SizedBox(width: 8), label] means however it's not obvious what wrapping label in Flexible and IntrinsicWidth widgets is intended to accomplish in the new version. Which is to say, "yes" :-)

HansMuller · 2023-03-02T01:03:11Z

packages/flutter/test/material/bottom_navigation_bar_test.dart

+        home: Builder(
+          builder: (BuildContext context) {
+            return Scaffold(
+                bottomNavigationBar: BottomNavigationBar(


The two-space indentation needs to be fixed up here.

builder: (BuildContext context) { return Scaffold( bottomNavigationBar: BottomNavigationBar( type: BottomNavigationBarType.fixed, landscapeLayout: BottomNavigationBarLandscapeLayout.linear, ...

It looks like line 2500 still needs to be backed up two spaces.

HansMuller · 2023-03-02T01:04:02Z

packages/flutter/test/material/bottom_navigation_bar_test.dart

  });
+
+  testWidgets('BottomNavigationBar linear landscape layout label RenderFlex overflow',(WidgetTester tester) async {
+    tester.binding.window.physicalSizeTestValue = const Size(540, 340);


Please insert a comment here:

Regression test for https://github.com/flutter/flutter/issues/112163

HansMuller · 2023-03-02T01:05:22Z

packages/flutter/test/material/bottom_navigation_bar_test.dart

+        ),
+      ),
+    );
+


Shouldn't this test be verifying that the expected ellipsis suffixes have appeared?

I am not sure because the ellipsis was already applied, should we verify that the overflow property is TextOverflow.ellipsis? Can you please guide me with this?

I think you need to verify that the "..." suffix actually appears as expected.

I don't know how to verify that. I have looked into it and couldn't find any way except golden test. The current test verifies that there will be no render overflow. I am kind of stuck here.

HansMuller · 2023-03-02T03:57:55Z

I've checked this against the internal tests, see (google internal link) cl/513379794. All appears to be well.

HansMuller

Sorry, I lost track of this one. In addition to the feedback, it looks like there are analyzer failures to fix up.

You can check for analyzer errors locally with flutter analyze --flutter-repo

HansMuller · 2023-05-09T23:33:15Z

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

+          children: <Widget>[
+            icon,
+            const SizedBox(width: 8),
+            Flexible(child: IntrinsicWidth(child: label))


It's pretty obvious what <Widget>[icon, const SizedBox(width: 8), label] means however it's not obvious what wrapping label in Flexible and IntrinsicWidth widgets is intended to accomplish in the new version. Which is to say, "yes" :-)

HansMuller · 2023-05-09T23:34:57Z

packages/flutter/test/material/bottom_navigation_bar_test.dart

+        home: Builder(
+          builder: (BuildContext context) {
+            return Scaffold(
+                bottomNavigationBar: BottomNavigationBar(


It looks like line 2500 still needs to be backed up two spaces.

HansMuller · 2023-05-09T23:35:50Z

packages/flutter/test/material/bottom_navigation_bar_test.dart

+        ),
+      ),
+    );
+


I think you need to verify that the "..." suffix actually appears as expected.

…12163

flutter-dashboard · 2023-06-15T08:05:06Z

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.

HansMuller · 2023-09-20T19:56:34Z

I asked around and it turns out it actually is difficult to programmatically verify that the ellipsis has been included at the end of text. So in this case creating a golden image test is warranted. See Writing a golden file test for package:flutter.

Looks like the presubmit bots have some issues as well.

goderbauer · 2023-12-12T23:42:09Z

(triage) @sharmashashi Do you still have plans to come back to this and address the feedback given above?

sharmashashi · 2023-12-13T02:10:55Z

(triage) @sharmashashi Do you still have plans to come back to this and address the feedback given above?

Yes, I do. I will look into this within a week and let know if I can continue with the golden test.

sharmashashi · 2023-12-14T17:41:47Z

@HansMuller I write the golden test. I read the guide on creating the new golden file test but I did not understand how to include golden(s) in PR description as stated The reviewer should also verify your golden image(s), so make sure to include the golden(s) in your PR description.
I was also reading the conversation from an old PR. I was wondering if these steps are still being followed.

flutter-dashboard · 2023-12-14T18:14:25Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

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.

Changes reported for pull request #120206 at sha ff11944

HansMuller

LGTM. Thanks for sorting out how to write a golden file test!

auto-submit · 2023-12-14T21:20:02Z

auto label is removed for flutter/flutter/120206, due to This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

MitchellGoodwin

LGTM!

…20206)

flutter/flutter@a51e33a...2407f69 2023-12-15 [email protected] Move package:web dependency to dev dependency (flutter/flutter#139696) 2023-12-15 [email protected] Roll Flutter Engine from 9524a185b055 to 986a6fe198dc (1 revision) (flutter/flutter#140221) 2023-12-15 [email protected] Roll Packages from 1151191 to 3f2e16b (9 revisions) (flutter/flutter#140218) 2023-12-15 [email protected] Roll Flutter Engine from 7a50221733c2 to 9524a185b055 (1 revision) (flutter/flutter#140217) 2023-12-15 [email protected] Roll Flutter Engine from 767223f7a4f8 to 7a50221733c2 (1 revision) (flutter/flutter#140216) 2023-12-15 [email protected] Roll Flutter Engine from 91f65eea0c11 to 767223f7a4f8 (1 revision) (flutter/flutter#140210) 2023-12-15 [email protected] Roll Flutter Engine from a47da28c9a62 to 91f65eea0c11 (1 revision) (flutter/flutter#140207) 2023-12-15 [email protected] Roll Flutter Engine from cde1a596432d to a47da28c9a62 (1 revision) (flutter/flutter#140204) 2023-12-15 [email protected] Roll Flutter Engine from 46ff5c08a905 to cde1a596432d (1 revision) (flutter/flutter#140200) 2023-12-15 [email protected] Roll Flutter Engine from a17bb0a63b7e to 46ff5c08a905 (1 revision) (flutter/flutter#140198) 2023-12-15 [email protected] Roll Flutter Engine from 4cb3ba7a85f6 to a17bb0a63b7e (1 revision) (flutter/flutter#140196) 2023-12-15 [email protected] Roll Flutter Engine from 0e7248d43251 to 4cb3ba7a85f6 (14 revisions) (flutter/flutter#140195) 2023-12-15 [email protected] Increase versions of leak tracker libraries. (flutter/flutter#140018) 2023-12-15 [email protected] Set compile test iOS app target version to not embed Swift runtime (flutter/flutter#140188) 2023-12-15 [email protected] Cupertino text clear label (flutter/flutter#129727) 2023-12-15 [email protected] [github actions] use token from real user flutter mirror bot (flutter/flutter#140191) 2023-12-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 0e7248d43251 to 0b0fab821536 (4 revisions)" (flutter/flutter#140194) 2023-12-14 [email protected] Roll Flutter Engine from 0e7248d43251 to 0b0fab821536 (4 revisions) (flutter/flutter#140180) 2023-12-14 [email protected] feat: Add onTapAlwaysCalled in TextFormField (flutter/flutter#140089) 2023-12-14 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 3.1.3 to 4.0.0 (flutter/flutter#140177) 2023-12-14 [email protected] Roll Flutter Engine from 2140942444ea to 0e7248d43251 (2 revisions) (flutter/flutter#140176) 2023-12-14 [email protected] fix reorderable_list drop animation (flutter/flutter#139362) 2023-12-14 [email protected] Roll Flutter Engine from 997d3dfa1e74 to 2140942444ea (4 revisions) (flutter/flutter#140171) 2023-12-14 [email protected] Fix BottomNavigationBarItem label overflow (flutter/flutter#120206) 2023-12-14 [email protected] Roll Flutter Engine from a565cea256c7 to 997d3dfa1e74 (2 revisions) (flutter/flutter#140170) 2023-12-14 [email protected] Revert "Dynamic view sizing" (flutter/flutter#140165) 2023-12-14 [email protected] ð��¨: fix cupertionActionSheet design (flutter/flutter#134345) 2023-12-14 [email protected] Make improvements to existing new issue templates (flutter/flutter#140142) 2023-12-14 [email protected] Roll Flutter Engine from caf33276468b to a565cea256c7 (1 revision) (flutter/flutter#140163) 2023-12-14 [email protected] Expand and update a few release.yml categories (flutter/flutter#140120) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…20206)

Fix BottomNavigationBarItem label overflow

5dad5c6

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Feb 7, 2023

sharmashashi added 2 commits February 7, 2023 23:53

Write test

186000e

Remove space characters

11108a9

HansMuller self-requested a review February 17, 2023 23:31

sharmashashi added 3 commits February 22, 2023 17:52

Merge branch 'master' into fix112163

03caa12

Merge branch 'master' into fix112163

ce50064

Merge branch 'master' into fix112163

a813712

HansMuller reviewed Mar 2, 2023

View reviewed changes

Merge branch 'master' into fix112163

d1b7f21

HansMuller reviewed May 9, 2023

View reviewed changes

HansMuller and others added 3 commits May 10, 2023 08:43

Merge branch 'master' into fix112163

37b741c

Add comment for wrapping label with IntrinsicWidth

97972d5

Merge branch 'master' into fix112163

1b53e7b

github-actions bot removed the framework flutter/packages/flutter repository. See also f: labels. label May 24, 2023

sharmashashi and others added 4 commits May 25, 2023 00:20

Merge branch 'master' of github.com:flutter/flutter into fix112163

12d6976

Merge branch 'fix112163' of github.com:sharmashashi/flutter into fix1…

40c4d8c

…12163

Merge branch 'master' into fix112163

06fe9b1

Merge branch 'master' into fix112163

10f024c

Merge branch 'master' into fix112163

04ce0df

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 26, 2023

Merge branch 'master' into fix112163

e1604c0

sharmashashi requested a review from HansMuller August 6, 2023 03:11

Merge branch 'master' into fix112163

93a8f54

add golden test

ff11944

flutter-dashboard bot added the will affect goldens Changes to golden files label Dec 14, 2023

HansMuller approved these changes Dec 14, 2023

View reviewed changes

HansMuller added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 14, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 14, 2023

MitchellGoodwin approved these changes Dec 14, 2023

View reviewed changes

MitchellGoodwin added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 14, 2023

auto-submit bot merged commit cf3ed1e into flutter:master Dec 14, 2023

engine-flutter-autoroll mentioned this pull request Dec 15, 2023

Roll Flutter from a51e33ac76e4 to ba30653f2ec9 (26 revisions) flutter/packages#5686

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 15, 2023

cf3ed1e Fix BottomNavigationBarItem label overflow (flutter/flutter#1…

c3f6793

…20206)

engine-flutter-autoroll mentioned this pull request Dec 15, 2023

Roll Flutter from a51e33ac76e4 to 8889a0981b69 (27 revisions) flutter/packages#5687

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 15, 2023

cf3ed1e Fix BottomNavigationBarItem label overflow (flutter/flutter#1…

7d96c4c

…20206)

engine-flutter-autoroll mentioned this pull request Dec 15, 2023

Roll Flutter from a51e33ac76e4 to f9ee379b35e0 (28 revisions) flutter/packages#5688

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 15, 2023

cf3ed1e Fix BottomNavigationBarItem label overflow (flutter/flutter#1…

174a290

…20206)

engine-flutter-autoroll mentioned this pull request Dec 15, 2023

Roll Flutter from a51e33ac76e4 to 8b8d39593d9b (29 revisions) flutter/packages#5690

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 15, 2023

cf3ed1e Fix BottomNavigationBarItem label overflow (flutter/flutter#1…

5c4ccc4

…20206)

engine-flutter-autoroll mentioned this pull request Dec 15, 2023

Roll Flutter from a51e33ac76e4 to 2407f699e935 (30 revisions) flutter/packages#5691

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 15, 2023

cf3ed1e Fix BottomNavigationBarItem label overflow (flutter/flutter#1…

1402c9f

…20206)

HansMuller mentioned this pull request Dec 15, 2023

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024

cf3ed1e Fix BottomNavigationBarItem label overflow (flutter/flutter#1…

5e94577

…20206)

Fix BottomNavigationBarItem label overflow #120206

Fix BottomNavigationBarItem label overflow #120206

Uh oh!

Conversation

sharmashashi commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Feb 7, 2023

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharmashashi Mar 2, 2023 • 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!

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 commented Mar 2, 2023

Uh oh!

HansMuller left a comment

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!

flutter-dashboard bot commented Jun 15, 2023

Uh oh!

HansMuller commented Sep 20, 2023

Uh oh!

goderbauer commented Dec 12, 2023

Uh oh!

sharmashashi commented Dec 13, 2023

Uh oh!

sharmashashi commented Dec 14, 2023

Uh oh!

flutter-dashboard bot commented Dec 14, 2023

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Dec 14, 2023

Uh oh!

MitchellGoodwin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sharmashashi commented Feb 7, 2023 •

edited

Loading

sharmashashi Mar 2, 2023 •

edited

Loading