BottomAppBar: Fix doubled layers of color and shadow #123294

chrisbobbe · 2023-03-22T22:34:48Z

Fixes #123283.
Fixes #122667.
Fixes #123064.

(EDITED for the current revision, following #123283 (comment).)

The BottomAppBar implementation uses both a Material and a PhysicalShape because it needs them both in order to support the "notch" for a FAB in Material 2.

In the current version of the Material 3 support, it ends up putting a color and shadow on both of these, so that there are two color layers and two shadows. This gives wrong results when the color is translucent: #122667, #123064.

Also in Material 3, it doesn't work right if you ask for a notch, because the Material paints through the notch: #123283. (The Material 3 spec doesn't allow for a notch in the bottom app bar, but we plan to support one anyway: #123283 (comment).)

So, fix these bugs by making the Material transparent (and without overlay or shadow, using MaterialType.transparency), like we do in Material 2.

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

It's now a bit easier to read complete pictures of the Material 3 behavior and of the Material 2 behavior. Soon we'll remove the PhysicalShape widget from the Material 3 case entirely -- its job isn't needed in Material 3 because the bottom app bar's "notched" shape isn't part of the spec anymore: https://m3.material.io/components/bottom-app-bar/specs

Fixes flutter#123283. Fixes flutter#122667. Fixes flutter#123064. The Material 3 spec removes the "notched" shape for bottom app bars that was present Material 2: https://m3.material.io/components/bottom-app-bar/specs So, remove the `PhysicalShape` widget that was providing that notched shape, and throw if callers in Material 3 ask for a notch. Callers expecting a notch with Material 3 will have been disappointed since da7b832, when the `Material` widget (which doesn't support a notch) started painting its background color through any notch provided by the `PhysicalShape` widget. This is issue flutter#123283. As a bonus, this fixes some bugs observed in Material 3 that were invited by BottomAppBar's two-layer implementation (the `PhysicalShape` and the `Material`): - flutter#122667, "BottomAppBar: Material 3 makes the color too opaque" - flutter#123064, "BottomAppBar: Elevation effect (shadow) doubled in Material 3"

Done by removing the line in bottom_app_bar_template.dart then: dart dev/tools/gen_defaults/bin/gen_defaults.dart and undoing some unrelated-looking changes that made in these files: packages/flutter/lib/src/material/bottom_sheet.dart packages/flutter/lib/src/material/search_anchor.dart

`Material.surfaceTintColor` is ignored in Material 2. From the doc: https://github.com/flutter/flutter/blob/2ad6cd72c0/packages/flutter/lib/src/material/material.dart#L283 > If [ThemeData.useMaterial3] is false, then this property is not used. It was added for Material 3 support in 6a66aa2.

…terial We already pass these to the `PhysicalShape`; the transparent Material doesn't need them too. As noted in flutter#123064 (comment) , I don't think this redundancy in the Material 2 case has actually caused a bug where an elevation effect gets doubled, because: - A `Material` with `type: MaterialType.transparency` doesn't display a shadow: https://github.com/flutter/flutter/blob/f4c3facfd/packages/flutter/lib/src/material/material.dart#L522-L529 - `BottomAppBar` tries to apply an elevation overlay effect using `color` (with `ElevationOverlay.applyOverlay`), but `color` gets ignored by `Material` if `type` is `MaterialType.transparency`: https://github.com/flutter/flutter/blob/f4c3facfd/packages/flutter/lib/src/material/material.dart#L522-L529 Still, that's not trivial to verify, and it's more confusing to keep passing these arguments. (Note that this addresses the Material 2 case. In a recent commit, we removed the `PhysicalShape` in the Material 3 case, fixing some live bugs where some effects had in fact been duplicated by the presence of the `PhysicalShape` together with the `Material`.)

chrisbobbe · 2023-03-22T22:42:25Z

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

+      child = PhysicalShape(
+        clipper: clipper,
+        elevation: elevation,
+        shadowColor: shadowColor,
+        color: ElevationOverlay.applyOverlay(context, color, elevation),
+        clipBehavior: widget.clipBehavior,
+        child: Material(
+          key: materialKey,
+          type: MaterialType.transparency,
+          child: SafeArea(child: child),


This, for the Material 2 case, is exactly like how this code was before Material 3 support was first added in #106525, except for the customizations added for padding in #115175 and for shadow color in #121406.

gnprice

Thanks for the fix! Generally this looks good.

packages/flutter/test/material/bottom_app_bar_test.dart

gnprice · 2023-03-22T23:07:38Z

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

@@ -174,6 +189,8 @@ class _BottomAppBarState extends State<BottomAppBar> {
  Widget build(BuildContext context) {
    final ThemeData theme = Theme.of(context);
    final bool isMaterial3 = theme.useMaterial3;
+    assert(!isMaterial3 || widget.shape == null); // `shape` not accepted in Material 3


The information in the comment should go in an assertion message instead, so the developer sees it when they hit the assertion. For example:

Suggested change

assert(!isMaterial3 || widget.shape == null); // `shape` not accepted in Material 3

assert(!isMaterial3 || widget.shape == null,

'The [BottomAppBar.shape] field is not permitted in Material 3.');

(Could also throw a FlutterError for a fancier error message, but this might be enough.)

If throwing a FlutterError, would we want to do that in production too? AIUI asserts only throw in debug mode.

No, the check and the throw would both go inside an assert. For example, here's [State.context]:

BuildContext get context { assert(() { if (_element == null) { throw FlutterError( 'This widget has been unmounted, so the State no longer has a context (and should be considered defunct). \n' 'Consider canceling any active work during "dispose" or using the "mounted" getter to determine if the State is still active.', ); } return true; }()); return _element!; }

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

…fety

chrisbobbe · 2023-03-22T23:32:40Z

Thanks for the review, @gnprice! Changes pushed; PTAL. 🙂

gnprice

LGTM! One nit.

packages/flutter/test/material/bottom_app_bar_test.dart

chrisbobbe · 2023-03-22T23:46:24Z

Thanks for the review! Fix pushed.

chrisbobbe · 2023-03-23T18:03:55Z

I'm reworking this following @HansMuller's comment at #123283 (comment):

[T]he notch should still work with useMaterial3: true.

flutter-dashboard · 2023-03-23T18:05:28Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

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.

justinmc · 2023-03-23T20:34:40Z

Sounds good, I'm happy to give a secondary review when this is back out of draft mode.

Edit: By the way, the Google testing failures look like there's a small pixel difference at the right edge of the BottomAppBar in some image diff tests. Likely it's intended, but I'll take another look after the PR is updated.

chrisbobbe · 2023-03-24T04:39:23Z

Thanks for the review!

(I think a merge would probably also do it.)

Done.

gnprice · 2023-03-24T21:11:38Z

Google testing failed. This is likely the same failures @justinmc mentioned above:

By the way, the Google testing failures look like there's a small pixel difference at the right edge of the BottomAppBar in some image diff tests. Likely it's intended, but I'll take another look after the PR is updated.

Customer testing also failed, in a way unrelated to this PR: #123385

justinmc · 2023-03-31T22:19:00Z

@chrisbobbe Can you merge master into this PR? That will fix the customer tests and update the Google tests so I can take a look.

chrisbobbe · 2023-03-31T22:28:57Z

Sure! Done.

justinmc · 2023-03-31T23:40:05Z

Thanks, looks like the customer tests are fixed. I'll check on the Google tests next week.

justinmc · 2023-04-03T18:38:33Z

The Google test failure looks unrelated... Can you try pushing another merge commit? Sorry for all the headache with this.

chrisbobbe · 2023-04-03T18:51:46Z

Sure, no problem! Done.

chrisbobbe · 2023-04-04T03:44:39Z

Hmm, Google testing is still showing as "pending" after 7+ hours. Could be #123289?

This time I'm going to try and see if I can update the branch without leaving GitHub's UI. There's a helpful-looking "Update branch" button that I'm going to click now. (If that does something wrong, I can always reset the branch back to b084982, my merge commit from earlier today.)

HansMuller · 2023-04-04T15:48:52Z

@XilaiZhang - apparently this PR has failed "Google Testng" however the details link doesn't reveal anything: "There is no unmerged PR.". I tried the "Test a PR" button to start another test and that failed with:

How can we move the process along?

XilaiZhang · 2023-04-04T17:22:33Z

temp workaround: the test failures are at test/521647510 and the cl is cl/521647510

Thanks for reporting the issue! will keep track of it in b/260764527

HansMuller · 2023-04-05T16:34:14Z

The Google testing failures don't appear to have anything to do with this PR. Will try updating the branch...

justinmc

LGTM 👍

I think that Google testing is broken right now, so we'll have to try pushing another merge commit in a day or so when it's fixed.

justinmc · 2023-03-23T20:31:29Z

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

@@ -33,7 +33,8 @@ import 'theme.dart';
 /// {@end-tool}
 ///
 /// {@tool dartpad}
-/// This example shows the [BottomAppBar], which can be configured to have a notch using the
+/// This example shows the [BottomAppBar], which (in Material 2)


As mentioned in #123283 (comment) we ideally want to keep the ability to have a notch with useMaterial3: true. I'm guessing but I imagine there will be a significant amount of people that want a Material 3-looking app with a notch.

Ah, this is already addressed in this revision (I suspect your review-draft crossed with my sending that new revision). I've updated the PR description to match this revision. 🙂

Thanks, yeah this review was messed up somehow...

justinmc · 2023-04-05T22:58:01Z

packages/flutter/test/material/bottom_app_bar_test.dart

@@ -218,19 +284,18 @@ void main() {
                ),
                bottomNavigationBar: BottomAppBar(
                  color: Color(0xff0000ff),
+                  surfaceTintColor: Colors.transparent,


Could you file a new issue about the docs being wrong?

chrisbobbe · 2023-04-10T21:54:51Z

Thanks for the review!

I think that Google testing is broken right now, so we'll have to try pushing another merge commit in a day or so when it's fixed.

Sure, I'll try that now.

justinmc · 2023-04-11T18:05:51Z

Thanks for pushing the merge commit. It looks like there are real Google test failures, but they are just nearly invisible image diffs due to the changes here. I've accepted the changes and should be able to merge this soon.

gnprice · 2023-04-11T20:24:34Z

Thanks @justinmc and everyone for your help in taking care of Google testing!

chrisbobbe · 2023-04-11T20:36:24Z

Yeah, thanks all! 🎉

…/flutter#123294)

Fixes BottomAppBar with translucent colors.

chrisbobbe added 9 commits March 22, 2023 14:34

BottomAppBar: Refactor nested-child pattern to use fewer variable names

aadfb97

BottomAppBar: Remove some redundant arg values that match defaults

eb031cf

BottomAppBar: Refactor to tighten up code

8b6ee32

BottomAppBar: Inline effectiveColor in its two use sites

7085235

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Mar 22, 2023

chrisbobbe commented Mar 22, 2023

View reviewed changes

BottomAppBar tests: Fill in missing issue number flutter#123283

2385db0

gnprice reviewed Mar 22, 2023

View reviewed changes

chrisbobbe added 4 commits March 22, 2023 16:24

BottomAppBar tests: Reorder some "regression test" comments

23b697f

BottomAppBar: Add developer-facing error message in no-notch assert

d030f4b

BottomAppBar: Adjust docs from gnprice's review

b723bf8

BottomAppBar: Cut legacy "must not be null" comments from pre-null-sa…

30900d9

…fety

gnprice approved these changes Mar 22, 2023

View reviewed changes

packages/flutter/test/material/bottom_app_bar_test.dart Outdated Show resolved Hide resolved

BottomAppBar tests: Fix indentation nits

0f943b9

HansMuller requested a review from justinmc March 23, 2023 16:26

chrisbobbe mentioned this pull request Mar 23, 2023

BottomAppBar: "Notch" shape silently broken in Material 3 #123283

Closed

chrisbobbe marked this pull request as draft March 23, 2023 18:02

BottomAppBar tests: Make some element-tree expectations more explicit

5b8ea33

BottomAppBar tests: Fix an incomplete comment

0dc7a45

Merge remote-tracking branch 'upstream/main'

e463155

Merge remote-tracking branch 'upstream/main'

b084982

Merge branch 'master' into pr-bottom-app-bar-notch-material-3

2342144

gnprice mentioned this pull request Apr 4, 2023

Google testing fails to report status to GitHub #123289

Closed

HansMuller mentioned this pull request Apr 4, 2023

FIX: SegmentedButton segments background cannot be made transparent #123293

Merged

8 tasks

Merge branch 'master' into pr-bottom-app-bar-notch-material-3

64978c8

justinmc approved these changes Apr 5, 2023

View reviewed changes

Merge branch 'master' into pr-bottom-app-bar-notch-material-3

25d28e9

justinmc merged commit 952ace6 into flutter:master Apr 11, 2023

chrisbobbe deleted the pr-bottom-app-bar-notch-material-3 branch April 11, 2023 20:36

engine-flutter-autoroll mentioned this pull request Apr 12, 2023

Roll Flutter from 535b4d5ec3f1 to 3b4b14919173 (44 revisions) flutter/packages#3694

Merged

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

952ace6 BottomAppBar: Fix doubled layers of color and shadow (flutter…

7c02b5b

…/flutter#123294)

exaby73 pushed a commit to exaby73/flutter_nevercode that referenced this pull request Apr 17, 2023

BottomAppBar: Fix doubled layers of color and shadow (flutter#123294)

eaf73a8

Fixes BottomAppBar with translucent colors.

HansMuller mentioned this pull request Apr 20, 2023

Flutter Framework Team: Notable Commits #121415

Closed

	assert(!isMaterial3 \|\| widget.shape == null); // `shape` not accepted in Material 3
	assert(!isMaterial3 \|\| widget.shape == null,
	'The [BottomAppBar.shape] field is not permitted in Material 3.');

BottomAppBar: Fix doubled layers of color and shadow #123294

BottomAppBar: Fix doubled layers of color and shadow #123294

Uh oh!

Conversation

chrisbobbe commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

chrisbobbe Mar 22, 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!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gnprice Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Mar 22, 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!

gnprice Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chrisbobbe commented Mar 22, 2023

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrisbobbe commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Mar 23, 2023

Uh oh!

flutter-dashboard bot commented Mar 23, 2023

Uh oh!

justinmc commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Mar 24, 2023

Uh oh!

gnprice commented Mar 24, 2023

Uh oh!

justinmc commented Mar 31, 2023

Uh oh!

chrisbobbe commented Mar 31, 2023

Uh oh!

justinmc commented Mar 31, 2023

Uh oh!

justinmc commented Apr 3, 2023

Uh oh!

chrisbobbe commented Apr 3, 2023

Uh oh!

chrisbobbe commented Apr 4, 2023

Uh oh!

HansMuller commented Apr 4, 2023

Uh oh!

XilaiZhang commented Apr 4, 2023

Uh oh!

HansMuller commented Apr 5, 2023

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

justinmc Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

justinmc Apr 5, 2023

Choose a reason for hiding this comment

chrisbobbe commented Mar 22, 2023 •

edited

Loading

chrisbobbe Mar 22, 2023 •

edited

Loading

chrisbobbe Mar 22, 2023 •

edited

Loading

chrisbobbe commented Mar 22, 2023 •

edited

Loading

justinmc commented Mar 23, 2023 •

edited

Loading