Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing elevation issues with Material 3 #110624

Merged
merged 6 commits into from Sep 6, 2022

Conversation

darrenaustin
Copy link
Contributor

@darrenaustin darrenaustin commented Aug 30, 2022

This PR attempts to alleviate some of the issues described in: #107190.

Previously we had two colors that controlled the presentation of elevation on a Material widget: shadowColor and the new surfaceTintColor. The defaults for both of these when useMaterial3 is true was null which meant to turn off that presentation (either a drop shadow or surface tint overlay). However, there are a lot of widgets that assume that the default shadow color will be non-null which caused a lot of the issues described in the issue. This especially impacted widgets that depended on this default, but hadn't yet been migrated to M3.

To address these issues, this PR makes the following changes to the defaults for useMaterial3: true:

  • Material.shadowColor will default to ThemeData.colorScheme.shadow if it is null. So by default there will be a drop shadow. You can turn this off by setting it to Colors.transparent.
  • Material.surfaceTintColor will default to null. So by default no surface tint overlay will be applied.
  • The surface tint feature can also be disabled if you set the surfaceTintColor to Colors.transparent.
  • Added surfaceTint and shadowColor properties to Dialog and Drawer widgets.

Here is @rydmike's example showing Material 2 and Material 3 with these changes:

Material 2Material 3

Fixes: #107190.
Fixes: #107423.

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 flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. labels Aug 30, 2022
Copy link
Member

@guidezpl guidezpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, love this! Just some small notes about using Colors.transparent, and documentation improvements and consistency

packages/flutter/lib/src/material/material.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/material.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/navigation_bar.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/elevation_overlay.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/dialog.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/dialog.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/dialog.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/dialog.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/dialog.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/dialog.dart Show resolved Hide resolved
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once Pierre's feedback has been addressed, LGTM. As he noted, it's important to differentiate between recognizing a special value, Colors.transparent, vs any color with alpha 0. Either version works (you could even say "any color whose alpha is 0, like Colors.transparent), just need to be consistent.

This is a nice PR BTW; appreciate all the new const values.

@darrenaustin
Copy link
Contributor Author

Once Pierre's feedback has been addressed, LGTM. As he noted, it's important to differentiate between recognizing a special value, Colors.transparent, vs any color with alpha 0. Either version works (you could even say "any color whose alpha is 0, like Colors.transparent), just need to be consistent.

Yeah, I wasn't sure if we should keep it more general (alpha == 0) or just have it require exactly Colors.transparent. I am open to either, but perhaps going with Colors.transparent is simpler to describe and understand.

This is a nice PR BTW; appreciate all the new const values.

Thx!

Copy link
Member

@guidezpl guidezpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  _    ___ _____ __  __ 
 | |  / __|_   _|  \/  |
 | |_| (_ | | | | |\/| |
 |____\___| |_| |_|  |_|
                        

@flutter-dashboard flutter-dashboard bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation labels Sep 2, 2022
@rydmike
Copy link
Contributor

rydmike commented Sep 3, 2022

This is nice @darrenaustin, much appreciated. It will also help with migration to M3, widgets composed with Material will get shadow as before when opting in on M3. If you want tint, you can add it yourself, logical.

Would be nice if it could be snuck into a hotfix in 3.3 at some point, it makes opting on on Material3 in its current incomplete state a much more viable option.

Did you try it with PopupMenuButton? It is also quite a problematic case. Sure like all others it will get fixed when it gets its M3 implementation. Plus at least with this is gets elevation shadow in light and dark mode in M3 as it should.

Light mode is fine without tint, even in M3 in a clinch, but dark mode is a bit more challenging, since it wont get the M2 dark mode overlay color, in M3 mode when it is elevated, and it has no tint color we could set for M3 tint, so it wont be very distinguishable on dark backgrounds.

Problematic since popup overlays has no scrim like dialogs typically do, so it melts into the background in dark mode where shadow is it hard to see. Well yes, it has background color, even in theme, and I could certainly theme it based on themed elevation to give its surface color a tint in M3 mode, using the M3 tint algo on its background color theme tint color. I might just do that until support for it arrives in stable, at least if this PR and/or popup's tint will take a while to land in stable.

Any thoughts on when you plan to land this PR in stable? Next stable release in 3...4 months, or in a hotfix to 3.3, like 3.3.1 in near future?

@darrenaustin darrenaustin merged commit 33ed6a3 into flutter:master Sep 6, 2022
@darrenaustin darrenaustin deleted the drop_shadow_surface_tint branch September 6, 2022 23:16
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: text input Entering text in a text field or keyboard related problems d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels.
Projects
None yet
4 participants