Gold has detected about 1 untriaged digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/83047

Gold has detected about 2 untriaged digest(s) on patchset 3.
View them at https://flutter-gold.skia.org/cl/github/83047

@johnsonmh Where can I find Material You guidelines as a non-googler?

@johnsonmh Quick question: where are the Material references for this and do they also feature the double ink? (one for the highlight and one for the tap?) Seems out of place but might be me.

@johnsonmh Thanks for the comments on Twitter (https://twitter.com/MHJohnson8/status/1395530093729140736?s=20), I really appreciated them. 👍🏻

While the NavigationBarDestinationBuilder may make it possible to set something up that replicates the old interface of Widgets as icons that e.g. current BottomNavigationBarItem and NavigationRailDestination` uses, it gets very complex and verbose to actually do so.

I tried, but failed miserably. All I want and need out of it is to be able to provide Widgets for the selected and unselected icons, as in the old/current interfaces for the destinations. Rest of it should behave as the default NavigationBar does.

Please consider adding an example that demonstrates how to do this, or some helper interfaces to easier accomplish it for better/easier backwards compatible API interfaces.

Maybe I just don't fully get all the possibilities yet, but it seemed to be easier to just copy/fork the entire widget and make my own slightly modified version to do it. Mostly due to the need for the SelectableAnimatedBuilder providing animation to NavigationBarDestinationInfo that must be parent to NavigationBarDestinationBuilder.

Use case for Widgets as Icons - Legacy lowest common interface

I have a setup (custom widget/package) where it is possible in the API (and end user UI) to select what type of bottom navigation bar (eg Cupertino, Material, Adaptive) you want to use, and it would be nice to add MaterialYuo (NavigationBar) to it, without stepping down the API interface for its destination wrapper to IconData. This does at the moment seem to be the easy way out, but then it would all loose the Widget flexibility. Which would otherwise be the lowest common interface. This new design still feels like it will for most use cases force it down to IconData, despite the nice builder feature NavigationBar has, but as said , maybe some concrete usage example is enough.

Observation - API Naming Consistency

The API naming for selected and unSelected icons seem to be a bit inconsistent between navigation control Widgets. In particular NavigationBar switches the usage of icon for selectedIcon, which typically has been used for the unselected icon or one that is just the same for both states. The table below compares some API naming differences between navigation control widgets.

Edit 22.5.2021: Added API for selecting an item or destination to the comparison, since it was mentioned in later discussion.

@johnsonmh I noticed you made a commit to change the NavigationBarDestination icon and unselectedIcon to Widgets instead of IconData, I think that is good move. 👍🏻

I did not try the revised version yet, but I think it is a nice choice for consistency and simplicity, especially if all you want to do is to modify the icons with a small custom widget, but otherwise keep the default behavior or want to make a shared wrapper API with current navigation control widgets.

Tooltip Observation

Concerning the tooltips, there is via NavigationBarDestination currently no simple way to control the tooltips.
This makes it complex to change them from the icon label and to even adjust their usage at all.

Currently tooltips also show up on Desktop/Web also when the label is also shown in the NavigationBar, thus duplicating the already shown label as a tooltip, with the same text. This does not add any new information to the UI and is actually a bit annoying to users on desktop/web.

The BottomNavigationBar used to be like this too, but it was later fixed so it is now possible to remove the tooltips, or customize the tooltips via BottomNavigationBarItem to something else than just the icon label.

Basically if you in BottomNavigationBarItem don't define a tooltip, you get the icon label, but you can remove it by specifying an empty string, or customize it to something that adds some other info than just the icon label that is already shown, if you really also want a tooltip when you are showing the labels.

It makes it possible to do something like this:

        child: BottomNavigationBar(
          items: <BottomNavigationBarItem>[
            for (MyDestination item in destinations)
              BottomNavigationBarItem(
                icon: item.icon,
                activeIcon: item.selectedIcon,
                label: item.label,
                tooltip: useTooltips &&
                        item.tooltip != item.label &&
                        item.tooltip != null
                    ? item.tooltip
                    : '',
              ),
          ],
         ...

Which would only show tooltips if an "app" level flag to actually show tooltips is true, and if the tooltip is different from the label. Obviously in a NavigationBar equivalent destination you would also have to combine the logic with the used labelBehavior.
Users may then choose in settings (affecting useTooltips if tooltips are used or not in the app. Many users perceive that tooltips get in the way after you are familiar with the app.

Please consider some similar feature for NavigationBar to make it easier to build support for the above behavior.

This GIF show the tooltips showing up all the time now on Desktop/Web and there is no simple way to get rid of them.

Accessibility/Legibility Design Issue

This is a Material design matter. However, the default NavigationBar label size using the Flutter overline text style is barely legible on Web/Desktop that use device pixel ratio 1.0.

This seems to be something that Material Design team may have overlooked in the design spec.

Majority (+80% globally) of Desktop Windows and Linux PCs use a display setting that have a device pixel ratio of 1.0. Fonts with very small dp become illegible on them. The small 10dp works OK on devices with a higher device pixel ratio, like all modern phones, tablets and also most Mac desktop/laptop computers. Some Windows/Linux laptops also use device pixel ratio of 1.5 or higher, on them the default overline size of the NavigationBar works OK, but generally desktops use 1.0.

I tried by moving the running version of the above test app over to a my laptop screen with pixel density 1.5, on that it looks OK, but not on a normal desktop screen with device pixel ratio 1.0.

The above GIF recording also show this. The above gif recording is from a screen with device pixel ratio 1.0. click on the GIF to open it from GitHub in native resolution and not its inline zoomed state.

@johnsonmh Another wish, I hope NavigationBar also gets its own subtheme where you can address some of the basic customization needs.

Now if you want to customize its look just a little bit, like making it use colorScheme.primary for the selected icon, OK if the icons are now Widgets this part became a easier, you can tweak it there, but normally that part has been preferred to be in the Widgets sub-theme so you can just set it once for your app. Plus you may want it to use colorScheme.primary with same opacity as it has for the current default colorScheme.secondary now for the highlight around the selected icon. But you can't re-use that easily, since it is hard coded. The roundness is also hard coded to border radius 20dp, maybe consider using a shape with a default stadium border if that is the design intent, and make the highlight shape customizable via a theme. The Flutter shapes have the added built-in advantage that they also animates between shape changes if it is changed, just a cool bonus.

When using NavigationBar you may also want to tune the unselected icon color and labels to match that of the NavigationRail's unselected default theme icon and label color (which imo are nicer defaults than this one has now). Regardless of opinions on what is nicer looking, in a scenario where you use both NavigationRail and NavigationBar in a responsive design, you may often want their styles to match exactly. The defaults diverge quite a bit now, so you need to customize one of them quite a bit to accomplish that.

You may also need to customize is the label font/size, if you feel it is required for more legible labels. Having access to them via a theme would be nice.

As it looks now, you kind of have to build the NavigationBar up from all of its atomic builders to do most of above customizations. Doable, sure perhaps so, but imo not very fun or as easy to modify as previous approach. Not for the most common customization scenarios anyway, for totally changing the NavigationBar into something entirely customized, the builders are a great addition of course.

@YazeedAlKhalaf I don't have guidance to link to at the moment - more will be shared later this Fall.

@creativecreatorormaybenot Thanks for spotting this, the intention is not to have a highlight color and only have the ripple. I will update the code to remove the highlight color.

@rydmike Thank you for the detailed feedback, this is all very helpful. I will incorporate the feedback to hopefully 1) Make the API more consistent with existing bottom navigation and navigation rail API and 2) make the customization easier (and with more examples given).

@johnsonmh I noticed another difference and minor inconsistency compared to default background with BottomNavigationBar.

The BottomNavigationBar's default background is when using newer color scheme based Themes ThemeData.from basing its background color on colorScheme.background via the fact that ThemeData.from assigns colorScheme.background to ThemeData.canvasColor that the default Material canvas type uses as background color.

This new NavigationBar on the other hand, bases its default background color on colorScheme.surface via

Material(
      elevation: elevation,
      color: backgroundColor ??
          ElevationOverlay.colorWithOverlay(
              colorScheme.surface, colorScheme.onSurface, 3.0),

Normally when dealing with plain vanilla surface and background colors, these are of course identical. However, when building more nuanced user interfaces for a larger canvas, it is common to use a slight hint of primary color (alphaBlend) mixed into to the colorScheme.surface and/or colorScheme.background. When you go really fancy, you can use a slightly different blend strength of primary into the surface and background colors, for even more nuanced effects.

When you do so, the end result is that the BottomNavigationBar and NavigationBar have backgrounds that then looks different in such designs, since they use different theme based default background colors. Perhaps this is intentional, but it might be worth considering if they should not default to the same base background color definition, it would seem more logical and again have less an impact on expected default background if you switch between them, or eg when you migrate from BottomNavigationBar to the new NavigationBar.

Here is an example showing the difference when using such primary blended surface and background color theming in dark mode. I used a bit exaggerated difference in primary blend strength on surface and background color, than one might normally use, just to show the difference more clearly, the used example theme colors are shown too:

Here it is shown as a hires GIF animation too. I also removed the double Ink ripple for this demo that @creativecreatorormaybenot mentioned:

This was just a quick experimental fix using:

return NavigationBarDestinationSemantics(
      child: NavigationBarDestinationTooltip(
        message: label,
        child: InkWell(
          highlightColor: Colors.transparent,  // Hide ink effect
          splashColor: Colors.transparent,     // Hide ink effect
          onTap: info.onTap,
          child: Row(
            children: <Widget>[
              Expanded(
                child: NavigationBarDestinationLayout(
                  icon: buildIcon(context),
                  label: buildLabel(context),
                ),
              ),
            ],
          ),
        ),
      ),
    );

It still has hover effect on desktop as before though, shown below. The yellow dot ink effect in the GIF are just from the screen recorders mouse click effect, that it uses to show when and where mouse was clicked.

Nice work! I have a few questions:

1. Could you please add onLongPress? It might not be frequent usage, but I've seen apps delegating the long press as reload mechanism or quick settings.
2. Shouldn't you use onPressed instead of onTap? ElevatedButton, OutlinedButton and TextButton uses onPressed.
3. Why is the tooltip vertical offset set to 42? Can't the user change it? For mobile it might not make a lot of sense, but who knows what people do on the web and other platforms. What if someone uses that on the top? Then there would be no tooltip.

Good comments @bernaferrari, here are my additions on the same topic.

1. I agree, onLongPress and with that rationale also, onSecondaryPress, for opening, typically a right mouse click based context menu on desktop apps, could certainly both be useful interactions to support out of the box for other devices than phones and tablets, but perhaps it is not in the Material spec for this widget to support that, pasts ones don't do it either. Hard to say, I don't think none Googlers have the new spec for this new MaterialYou widget yet.

2. The BottomNavigationBar, that this Widget primarily replaces also uses onTap (so does the CupertinoTabBar), so from that viewpoint the current onTap is the correct choice. I added API naming comparison to my earlier table. The onPressed callback used by buttons are as far as I recall always a void callback, whereas as the onTap callbacks are ValueChanged<int>? so there is that as well. But generally sometimes it seems these call back API names are a bit varied, the NavigationRails onDestinationSelected is a good example, what was wrong with onTap for that one for consistency? These things happens in big SDKs.

3. About tooltips, I totally agree the NavigationBar API should expose more config options without requiring constructing the thing from atoms using its builders. Tooltip labels should be possible to easily customize to something else than the navbar's text label, or completely removed. And like you say, if a default vertical offset is need (probably is due to height of this widget) that is different from Tooltip default theme vertical office, it must be available as parameter via a property, if it is hard coded like now it can't be modified even by wrapping the entire widget in its own ThemeData and custom TooltipThemeData.

Some additional observations, these are just my viewpoints from playing with and using the new widget a bit. I know the design is probably written in stone by the Material Design Theme, but I'm going to throw my views out there anyway.

Height when labels are shown.

In my opinion the default height design is a bit excessive when it comes to height when labels are shown (74dp), there is room to remove a few pixels and still keeping the look balanced and nice, the size when labels are not shown, could also be tighter (it is 56dp). Old bottom nav bar is 56dp and Cupertino one is 50dp btw. However, since the API includes a height adjustment it already allows for very easy minor opinionated tweaks to this, so it honestly does not matter so much, since you can tweak it as you want it easily.

However, @johnsonmh I noticed a bug, the height property currently has no impact on the height of the widget! The line:

   final double effectiveHeight = 
      labelBehavior == NavigationBarDestinationLabelBehavior.alwaysHide
            ? 56
            : 74;

should probably be:

   final double effectiveHeight = height ??
        (labelBehavior == NavigationBarDestinationLabelBehavior.alwaysHide
            ? 56
            : 74);

Otherwise the height property has has no impact on the height of the NavigationBar.

Default label font

The current default font style is an odd one, the overline style has letter spacing that just looks a bit strange. Flutter 2.2 still defaults to old typography (2014), my demos use the newer "correct" one (2018). However, the overline style is the same with both, it is just a peculiar style choice imo, with its 1.5 letter spacing. Plus as mentioned before, due to its default small size (10dp), it is barely legible on monitors with device pixel ratio 1.0. The old BottomNavigationBar defaults are clearer, and strangely even Cupertino looks better than this, although it is also too small for device pixel ratio 1.0, but it still looks much nicer.

Like I mentioned before, an API, perhaps via own sub-theme for the NavigationBar is needed to adjust the label text styles (maybe both selected/unselected?) easily, the sub-theme could then also include more/additional style, layout and theming properties, like e.g. custom heights for the different label style behaviors.

Below is an additional example showing NavigationBar, BottomNavigationBar and CupertinoTabBar on same screen.

I applied the height fix so I could change the height of the NavigationBar, just to try how it looks at some other sizes. I tried 50dp, same as Cupertino, when not showing labels, and quite a bit tighter, 62dp when showing labels, alongside with is current default values 56 and 74. The tighter values work well enough too. But like mentioned, if it has a working version of the height property, the default can be whatever, easy enough to tweak. However, the height values should also be themeable, both values, so it is easy to theme it differently globally for an app, and not apply it to every widget, or be forced to make a custom wrapper widget for it and use that instead, just to change the height. Great that you are supposed to be able to easily adjust the height too though, this is a welcome new feature.

The first screen GIF recording below is at device pixel ratio 1.5. On a high res laptop screen you might have that or higher, also on devices the device pixel ratio is normally 1.5 or even much higher, at this device pixel ratio, and higher, the small fonts on NavigationBar and CupertinoTabBar are still legible and fine, although the overline font style looks odd when used here, due to its 1.5 letter spacing.

Here is the same screen, but now on a normal (none Mac) desktop screen, that use the most common desktop device pixel ratio 1.0. Now the small default font size is barely legible and have poor readability and accessibility due to that.

(In both cases, click on the GIFs to open them to see them at actual pixel accurate sizes).

You could consider the caption style instead as the default label style, if it has to have style and not just default text style with a size of 12dp. Caption uses 12dp and very similar style as default unselected label on the BottomNavigationBar, which is smaller than the default selected label on it (14dp). It would look like this, on device pixel ratio 1.0 screen, when using the newer typography 2018.

It still fits nicely, even in my smaller height examples, and this is legible on screens with device pixel ratio 1.0 as well, and the letter spacing does not look odd, like it does with the overline text style.

With typography 2014 there would be a bit more space between the letters, caption 2018 uses letter spacing 0.4 and 2014 is 1.0, both are 12dp.

As a rule of thumb, most fonts become close to illegible at 10dp if the device pixel ratio is 1.0, while 12dp is OK with most fonts, 11dp is OKish. If the device pixel ratio is 1.5 or higher, then using font size 10dp is perfectly fine, the problem is +80% of desktops in the world use monitors that are setup to use device pixel ratio 1.0. Since Flutter also support desktop usage, I think default font choices for widgets should also consider that they also need to work well enough and be legible on screens using device pixel ratio 1.0.

I also drive my 2x32" 4k home office screens at native resolution, thus at screen pixel ratio 1.0. However my 13" touch laptop screen is 3000x2000 pixels, this one is too small to drive at such high native resolution, so here I zoom 150%, meaning the device pixel ratio is 1.5.

The discussion of too small font size (10dp) at device pixel ratio 1.0, was also earlier discussed when it comes to the default Flutter tooltip theme style, that now also uses 10dp, a size that is just too small to be legible at device pixel ratio 1.0. This was discussed and and demonstrated here #71429.

The Material Design team rep even seemed to agree after the demonstration that 10dp is not a very legible choice. Tooltips were 14dp before in Flutter, before it changed to 10dp. I recommended 12dp instead as default as good compromise between compact but still legible. It has however not been changed yet, so the issue is still open. It is however easy to theme the tooltip globally and correct the unfortunately too small default font size.

By the way, here is also the issue where tooltip config (other label than icon label and no tooltip at all shown) was added to the BottomNavigationBar, plus its rationale and discussion too: #71049

The NavigationBar is now in its current form recreating the issue that was fixed for the BottomNavigationBar, can we please not do that? 😃

@johnsonmh Sorry about dropping even more comments. I was however wondering before when I tried the widget why it was getting overlay color also in light theme mode and not changing with elevation changes, not even in dark theme mode.

At least in the past, background overlay color has in Flutter only been applied to Material backgrounds in dark theme mode and when ambient ThemeData applyElevationOverlayColor is set to true. In this Widget you are applying overlay to both dark and light theme mode and it is not controlled by ambient theme setting for this behavior at all, as before in Flutter.

Also the elevation used for the elevation overlay color is hard coded to 3, and not using the actual elevation of the NavigationBar widget, so the overlay color is always constant, it does not change with elevation as it normally would in Flutter, and it is also applied to light theme mode, which is currently not the case in Flutter when using overlay color for elevation, you have this now:

You are using colorWithOverlay

    return Material(
      elevation: elevation,
      color: backgroundColor ??
          ElevationOverlay.colorWithOverlay(colorScheme.surface, colorScheme.onSurface, 3.0),
      child: NavigationBarBottomPadding(
   ...

Perhaps this is a new standard? But more in line with current expected behavior in Flutter would be using applyOverlay:

    return Material(
      elevation: elevation,
      color: backgroundColor ??
          ElevationOverlay.applyOverlay(context, colorScheme.onSurface, elevation),
      child: NavigationBarBottomPadding(
   ...

This would use the widget elevation for applied overlay color, but only in dark theme mode, and only when the theme flag that toggles its usage is true.

This still gives the widget a default colorScheme.surface colored background and not the colorsScheme.Background color, like widgets of this type currently use as their background color, that they actually get from the Material's default background color.

To make it use the same default background and the Material dark overlay color behavior, then simply just pass the backgroundColor, it will get the Material default color if it is null or used supplied backgroundColor, and get overlay color applied based on the elevation of the Material and only in dark theme mode, and only when the flag to use it is set in ambient theme data, simply because this is already baked in behavior in Material.

    return Material(
      elevation: elevation,
      color: backgroundColor,
      child: NavigationBarBottomPadding(
   ...

Still of course, it all depends on what the new Material You design specs are, but it seems a bit off that it would deviate this much from past designs, when it comes to elevation, if that is the case pretty much all past widgets in Flutter will need to be rewritten or replaced with new ones to accommodate MaterialYou color scheme behavior.

Anyway, I just though I would mention this since it deviated quite radically from past Flutter SDK background color and elevation behavior.

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.

Wow, this looks amazing! Any chance of this coming soon?

Any updates?

It's been 4 months, and Android 12 with Material You is now in open-beta; is it time to look at this again?

@JaffaKetchup and @evrifaessa if I would have to guess, I would say that this MaterialYou widget is perhaps being worked on and tested in a none public clone, away from public eyes. Probably together with a number of other new MaterialYou related updates that Flutter will need in order to support MaterialYou that is introduced with Android12.

If not, there will indeed be a big rush to release Anroid12 related MaterialYou widgets and features in Flutter when Android12 is released. So I'm pretty sure Flutter - Android - Material teams are collaborating on including support for MaterialYou in a way that will try to match the release of Android12, if possible. This way Flutter applications can also migrate and use new MaterialYou design features after Abdroid12 is released. Sure it would of course be nice to have them before Android12 is released so that we could also prepare Flutter apps that use the new Material design features in advance too. However, I have not noticed them in master channel yet, although I must admit I have not spent so much time on master channel recently.

I know that the above feedback has been received and reviewed by the Material design team, but I guess we will have to wait and see what the result is until it is released.

cc: @willlarche

Hey folks, we are definitely hard at work in bringing Material You to Flutter soon! Please be patient as we continue to bring all these important updates to Flutter in the coming months :)

Thanks for all the great feedback on this PR folks, MH and I have tried to incorporate much of it, and I believe we can add additional functionality at a later time if needed.

Thanks @HansMuller, what an excellent review!

I was actually looking at it a bit too and I think you captured most of the things I noticed too and then some. I'm just out of curiosity following this review a bit, since I tried the super early version. 😃 Which was when I noticed some issues (on earlier version), like the elevation overlay in dark mode was not used, if theme used it and height parameter had no effect and minor other things, like more control over tooltips. The visual density and text scale factor questions where next on my list too ask about too. Oh, I like the idea with both text labels and optional widget, used it for some custom widgets for similar as mentioned reasons too.

Even if this is a new Material v3 widget, I think it is important that it fits and uses conventions that matches how Flutter widgets currently behave, whenever that is appropriate and may be reasonably expected. Glad to see you are taking time to get it as right as possible. Great job @johnsonmh and I'm looking forward to using this widget for real soon enough 🙏🏻 😃

Design question

The usage of overline style ad default label choice and its default size is still poor imo. It is not very legible on most common desktops that actually use pixel ratio 1.0, as demonstrated earlier. Curios as to what drives design choices like it, as default values anyway. Sure on phone devices with much higher device pixel ratio it works, but not on a 1.0 desktop, but I guess it is what it is 😃 💙

hey @rydmike, thanks for all your useful feedback! Regarding the overline usage. That is going to be what the spec calls for (and keep an eye out for updates to the text theme default values coming soon, which will update overline to be size 12 by default I believe)

The height param is now used and themable. The widget does not adjust its size due to visualDensity or textScaleFactor, by design. The tooltips do adjust to textScaleFactor however.

Thanks for the thorough review so far @HansMuller !

In you screenshots above I noticed that the on click effect (ripple) is a rectangle, but the new clocks app from Google that already uses the new component has a circle which also overlaps with the item next to it.

Also the circle is not a gray shade, but some very transparent shade of the accent color or secondary color or something like that.

Hey @LasseRosenow thanks for the feedback!

I've spoken with our Android counterparts and I think the overlapping/unbounded ripples are actually a bug and should be fixed shortly. The spec (once published) should indicate that the ripple expands out to the bounds of the destination (so a rectangle)

As for the color of the ripple, that can be customized with the ThemeData.splashColor param and we'll make sure to set the correct default for Material 3/You once we have a full Material 3 theme ready.

For some reason Github wouldn't let me directly reply to a few of the comments, so I'll address them here @HansMuller

Not respecting textScaleFactor doesn't seem like a good idea wrt a11y, but OK.

This was an a11y decision made by Material last year since growing the height of the bar wouldn't add value as the labels would just become truncated and you'd lose screen real estate. So the a11y text scaling decision was to just scale the tooltips up instead (this also aligns with Apple's bottom navigation behavior)

If a NavigationDestination needs a builder to create its icon or label, why not just use Builder?

I made the builder class private for now, we can explore making it a building block after

@rami-a off-topic, but you can only reply in the original thread (when responding go a previous review in a review) on GitHub.