[Text Scaling][Material] Update BottomNavigationBar to show tooltips on long press. by johnsonmh · Pull Request #59127 · flutter/flutter

johnsonmh · 2020-06-10T03:31:54Z

Description

This PR deprecates BottomNavigationBarItem.title, which is a Widget parameter, in favor of BottomNavigationBarItem.label, which is a String.

See full proposal: https://flutter.dev/go/bottom-navigation-bar-title-deprecation

Reasoning

Bottom Navigation Bars have limited space, so when the text scale factor increases, the text gets truncated.

Instead, the new Material guidance is to prevent the text in the bottom navigation bar from scaling, but then show tooltips on each item with the proper text scaling.

Before	After

So, when BottomNavigationItem.label is used instead of BottomNavigationBar.title, the textScaleFactor of the bottom navigation component will be capped at 1.0, and a tooltip will be present on long press of the BottomNavigationBar.items.

Additionally, the BottomNavigationBar build method now asserts the presence of an Overlay ancestor, as this is required to show the tooltips.

Tests

I added the following tests:

Added a test to verify that tooltips are shown (with proper text scaling) on bottom nav items when labels are provided.
Added text scaling test to confirm that the bottom nav does not grow with text scale factor when labels are provided.
Updated the bottom nav semantics tests to reflect new semantics tree (with tooltip addition).
Added a test to the Cupertino BottomAppBar verifying that text is created when label param is used.

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.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: flutter.dev/go/bottom-navigation-bar-title-deprecation
- I got input from the developer relations team, specifically from: Andrew Brogdon
- I wrote a migration guide: Add bottom navigation title deprecation migration docs website#4412

rami-a

LGTM but had one question and a few nits

rami-a · 2020-06-10T17:38:17Z

Nit: Since this is only used once, maybe just inline it?

Changed to:

final MediaQueryData mediaQueryData = MediaQuery.of(context); return MediaQuery( // Do not grow text in bottom navigation bar because there is no room, // instead, properly scaled tooltips will show on long press. data: mediaQueryData.copyWith( textScaleFactor: math.min(1.0, mediaQueryData.textScaleFactor), ),

rami-a · 2020-06-10T17:38:59Z

Why is this needed? Wouldn't it be null to start with?

It needs to be explicitly initialized because it's a final field, but I don't want to expose it in this constructor.

rami-a · 2020-06-10T17:39:41Z

Same thing here, wouldn't message already be null?

Same as above, it needs to be explicitly initialized (because it's a final field) and I don't want to expose it in this constructor.

rami-a · 2020-06-10T17:41:13Z

Would we only want the Tooltip if the text scale factor is greater than 1?

Guidance from design was to always include it, because it is useful for more than just text scale, for example when an item has no label.

rami-a · 2020-06-10T17:41:46Z

I dont think you need this new line

jonahwilliams

Does this lose the semantics service announce if a Widget is provided to a tooltip?

flutter-github-sync · 2020-06-24T20:44:47Z

Started Google testing for this PR

flutter-github-sync · 2020-06-24T22:52:25Z

Google testing passed!

goderbauer

Not a full review, just the solution for the issue you mentioned in the chat.

goderbauer · 2020-06-24T23:28:38Z

container: true can be removed here.

goderbauer · 2020-06-24T23:33:58Z

What I noticed when playing around with this: the tooltip now also shows up when longpressing a Button in the Bar even when the textscale factor is "normal" and wouldn't require it. Is that expected?

johnsonmh · 2020-06-24T23:37:14Z

@goderbauer Good question, yes that is intentional. The idea is that Tooltips provide value in more than just the text scaling case (say, when an item has no label at all), and having the behavior be consistent will help users learn that the tooltips will reliably be there on long press.

flutter-github-sync · 2020-06-26T00:58:08Z

Started Google testing for this PR

flutter-github-sync · 2020-06-26T01:12:46Z

Started Google testing for this PR

flutter-github-sync · 2020-06-26T01:32:42Z

Google testing passed!

flutter-github-sync · 2020-06-26T01:41:53Z

Google testing passed!

clocksmith · 2020-06-26T20:12:45Z

I hadn't really thought about this before in other contexts, but is there any advantage in scaling down the text if it already fits?

In this particular case, I believe that there are some users who are using textScaleFactor: 0 as a workaround to hide the labels, I do not want to break this use case. There was a period of time when BottomNavigationBar did not support hiding labels on the items.

clocksmith · 2020-06-26T20:13:37Z

nit: extra line

clocksmith · 2020-06-26T20:14:47Z

why are 5 seconds needed?

They weren't needed, changed to 2 seconds, which is what the tooltip uses (there needs to be some time for the tooltip to appear).

rami-a

LGTM

rami-a · 2020-06-26T20:36:08Z

Maybe I'm just unfamiliar with these APIs but why do you call this in the constructor?

markNeedsSemanticsUpdate is what triggers assembleSemanticsNode to run, and as soon as this node is created, and I wanted to make sure that method gets called. I'm also loosely following the _RenderScrollSemantics example, which adds a listener to call markNeedsSemanticsUpdate in its constructor.

flutter/packages/flutter/lib/src/widgets/scrollable.dart

Line 726 in 025463f

position.addListener(markNeedsSemanticsUpdate);

It's possible that this is unnecessary. @goderbauer Do you think this call is safe to remove?

This seems unnecessary. Newly created RenderObjects will always receive a semantics update when they first render (if semantics are turned on). What breaks if you remove this?

goderbauer · 2020-06-29T16:33:44Z

Maybe add a quick comment here explaining why we have this widget and what it does to the future reader.

goderbauer · 2020-06-29T16:34:56Z

This seems unnecessary. Newly created RenderObjects will always receive a semantics update when they first render (if semantics are turned on). What breaks if you remove this?

goderbauer · 2020-06-29T16:39:31Z

assembleSemanticsNode only runs if semantics are turned on. Semantics are only turned on if an a11y service (e.g. TalkBack or VoiceOver) is running. I thought this label also needs to show if semantics are turned off? Based on that, I would expect that the tooltip is not showing a message when a11y services are turned off?

Side-note: For technical reasons, a11y are always turned on when running in a simulator. Did you try the tooltip thing on a real device without having any a11y services (outside of scaling the text) enabled? You may have to force-quit and restart the app after turning the a11y service off to delete the cached label.

goderbauer · 2020-06-29T16:41:09Z

Maybe we should have a separate constructor for this new feature. It may be difficult to debug if you forget to specify the message and suddenly a random string from the semantics tree appears.

rami-a · 2020-07-06T13:09:29Z

Is this because of the overlay assert you added?

Yes, exactly

goderbauer

Approach looks good.

goderbauer · 2020-07-06T16:15:57Z

This should follow the deprecation style: https://github.com/flutter/flutter/wiki/Tree-hygiene#deprecation

Also: Can you update the doc comment on this to mention that this property is deprecated and link to the one that should be used instead?

Done.

Based on the breaking change guide you sent, I also created this breaking change proposal: flutter.dev/go/bottom-navigation-bar-title-deprecation, it has been open to discussion for a ~week or so now.

goderbauer · 2020-07-08T16:12:20Z

I saw you responded to the comments, but the code is still the old one. Did you by any chance forget to push? :)

goderbauer

LGTM

…le' param in favor of 'String label'

…omNavTextScaling

…on long press. (flutter#59127)

fluttergithubbot added p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels Jun 10, 2020

googlebot added the cla: yes label Jun 10, 2020

rami-a approved these changes Jun 10, 2020

View reviewed changes

jonahwilliams self-requested a review June 10, 2020 21:14

jonahwilliams reviewed Jun 11, 2020

View reviewed changes

goderbauer reviewed Jun 24, 2020

View reviewed changes

goderbauer mentioned this pull request Jun 26, 2020

Do not return partial semantics from tester.getSemantics #60367

Merged

9 of 13 tasks

clocksmith reviewed Jun 26, 2020

View reviewed changes

clocksmith approved these changes Jun 26, 2020

View reviewed changes

rami-a approved these changes Jun 26, 2020

View reviewed changes

goderbauer reviewed Jun 29, 2020

View reviewed changes

fluttergithubbot added the p: cupertino_ui cupertino_ui package in flutter/packages label Jul 1, 2020

perclasson reviewed Jul 2, 2020

View reviewed changes

Comment thread packages/flutter/lib/src/widgets/bottom_navigation_bar_item.dart Outdated

rami-a approved these changes Jul 6, 2020

View reviewed changes

goderbauer reviewed Jul 6, 2020

View reviewed changes

goderbauer approved these changes Jul 16, 2020

View reviewed changes

johnsonmh mentioned this pull request Jul 22, 2020

[animations] Ignore deprecated use of BottomNavigationBarItem.title flutter/packages#180

Merged

fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Jul 22, 2020

johnsonmh added 23 commits August 13, 2020 16:36

add Tooltip.fromWidget test

b8d6971

fix tests

78460e1

fix test

848d6b2

rami comments

94a661d

fix whitespace

57fc81e

close to working nullable message solution

34f6968

attempts

492d435

bottom nav bar updates

7310b72

fixing semantics

951ed8e

switching semantics ordering

28b3a12

fix semantics labels

839e4a3

fixed semantics

97344c9

remove trailing whitespace

62ce11d

fix analyzer error

e5382cb

comments

02aa76a

Instead of reading semantics label for tooltip, deprecate 'Widget tit…

00c7889

…le' param in favor of 'String label'

move comment

52f5817

remove unused function

f014252

goderbauer comments

178c56b

update cupertino test to use new params

d86c0e7

fix test

28f8d43

update docs

d603d4b

empty commit to rerun test

3c2593e

johnsonmh force-pushed the feature-bottomNavTextScaling branch from dd0a2de to 3c2593e Compare August 13, 2020 20:39

Merge branch 'master' of github.com:flutter/flutter into feature-bott…

7d707ca

…omNavTextScaling

johnsonmh merged commit bee9522 into flutter:master Aug 14, 2020

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

[Text Scaling][Material] Update BottomNavigationBar to show tooltips …

0b34833

…on long press. (flutter#59127)

HansMuller mentioned this pull request Nov 16, 2020

Improve BottomNavigationBar.title deprecation assertion message #70605

Closed

Piinks mentioned this pull request Feb 18, 2021

Add fix for BottomNavigationBarItem.title -> label #76339

Merged

8 tasks done

github-actions Bot locked as resolved and limited conversation to collaborators Aug 14, 2021

Conversation

johnsonmh commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reasoning

Tests

Checklist

Breaking Change

Uh oh!

rami-a 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!

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!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-github-sync commented Jun 24, 2020

Uh oh!

flutter-github-sync commented Jun 24, 2020

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Jun 24, 2020

Uh oh!

johnsonmh commented Jun 24, 2020

Uh oh!

flutter-github-sync commented Jun 26, 2020

Uh oh!

flutter-github-sync commented Jun 26, 2020

Uh oh!

flutter-github-sync commented Jun 26, 2020

Uh oh!

flutter-github-sync commented Jun 26, 2020

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!

rami-a 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!

johnsonmh commented Jun 10, 2020 •

edited

Loading

johnsonmh Jul 7, 2020 •

edited

Loading