Skip to content

Conversation

@QuncCccccc
Copy link
Contributor

@QuncCccccc QuncCccccc commented Jun 5, 2024

ThemeData.ScaffoldBackgroundColor originally defaults to ColorScheme.background. As background in ColorScheme is deprecated, customizing background is not able to change scaffold background color. This PR is to make scaffoldBackgroundColor defaults to ColorScheme.surface which is the replacement of ColorScheme.background.

Fixes #149158

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].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jun 5, 2024
@QuncCccccc QuncCccccc marked this pull request as ready for review June 5, 2024 22:52
@QuncCccccc QuncCccccc requested review from Piinks and TahaTesser June 5, 2024 22:52
@QuncCccccc QuncCccccc changed the title ScaffoldBackgroundColor should default to colorScheme.surface ScaffoldBackgroundColor should default to ColorScheme.surface Jun 5, 2024
Copy link
Member

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

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

Love this change.

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Was there a migration guide for ColorScheme.background when it was deprecated? If not, we should probably make one, as this change might break folks and they won't know how to readily fix it.

@QuncCccccc
Copy link
Contributor Author

Was there a migration guide for ColorScheme.background when it was deprecated? If not, we should probably make one, as this change might break folks and they won't know how to readily fix it.

Yes, the migration guide is included in the breaking change page: https://docs.flutter.dev/release/breaking-changes/new-color-scheme-roles :)

@QuncCccccc QuncCccccc requested a review from TahaTesser June 8, 2024 00:34
Copy link
Member

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

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

LGTM!

@YazeedAlKhalaf
Copy link
Contributor

NOTE: Not sure were to put this, but I came from this issue: #149158

I think a lot of apps using material have a background color with a different surface color above it. Take for example an app with a scaffold and then on top of it a card, that would break badly.

I found out the description of the surface color is the following:

/// The background color for widgets like [Card].
final Color surface;

I think there is one missing piece, before we had surface and onSurface. But now there is no replacement, I believe that apps using it are now stuck, unless they create a color extension to get that functionality back.

I think background -> surface doesn't make sense because they are two different colors.
The same thing goes on onBackground -> onSurface

I attached the docs above that are still saying, I copied them from 3.22.0, surface is the background color for cards.

@QuncCccccc
Copy link
Contributor Author

NOTE: Not sure were to put this, but I came from this issue: #149158

I think a lot of apps using material have a background color with a different surface color above it. Take for example an app with a scaffold and then on top of it a card, that would break badly.

I think there is one missing piece, before we had surface and onSurface. But now there is no replacement, I believe that apps using it are now stuck, unless they create a color extension to get that functionality back.

I think background -> surface doesn't make sense because they are two different colors. The same thing goes on onBackground -> onSurface

I attached the docs above that are still saying, I copied them from 3.22.0, surface is the background color for cards.

Thanks a lot for your suggestions! We are replacing surface and onSurface with background and onBackground because the latest Material Design 3 has deprecated these colors. And currently there should be no M3 widgets use background and onBackground as the defaults. The surface and onSurface should be able to replace them well if we use the M3 dynamic colors(ColorScheme.fromSeed or ColorScheme.fromImageProvider) because these two are basically duplicated.

I found out the description of the surface color is the following:

/// The background color for widgets like [Card].
final Color surface;

Thanks for pointing this out! This doc should be updated based on the new ColorScheme change because after we added more tone-based surface color roles(like surfaceContainer/-Low/-Lowest/-High/-Highest), surfaceContainerLow becomes the default for Card and we can see some contrast from the scaffold background. Sorry for the confusion!

@QuncCccccc QuncCccccc force-pushed the scaffold_background_color branch from 99852eb to f0a3dd5 Compare June 13, 2024 17:36
@QuncCccccc QuncCccccc added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 13, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 13, 2024
@auto-submit
Copy link
Contributor

auto-submit bot commented Jun 13, 2024

auto label is removed for flutter/flutter/149772, due to - The status or check suite Windows build_tests_4_7 has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit bot pushed a commit that referenced this pull request Jun 13, 2024
This PR just has a tiny doc change to correct the doc for `ColorScheme.surface`. Previously, Card background color defaults to `ColorScheme.surface` but we updated the default since we've introduced the new tone-based surface colors, like `surfaceContainer`, `surfaceContainerLow` and etc. The `surface` in ColorScheme should now replace deprecated `background` which is the default background color of Scaffold.

Related to the comment here: #149772 (comment)
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jun 14, 2024
This PR just has a tiny doc change to correct the doc for `ColorScheme.surface`. Previously, Card background color defaults to `ColorScheme.surface` but we updated the default since we've introduced the new tone-based surface colors, like `surfaceContainer`, `surfaceContainerLow` and etc. The `surface` in ColorScheme should now replace deprecated `background` which is the default background color of Scaffold.

Related to the comment here: flutter#149772 (comment)
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jun 14, 2024
This PR just has a tiny doc change to correct the doc for `ColorScheme.surface`. Previously, Card background color defaults to `ColorScheme.surface` but we updated the default since we've introduced the new tone-based surface colors, like `surfaceContainer`, `surfaceContainerLow` and etc. The `surface` in ColorScheme should now replace deprecated `background` which is the default background color of Scaffold.

Related to the comment here: flutter#149772 (comment)
@QuncCccccc QuncCccccc added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 17, 2024
@auto-submit auto-submit bot merged commit 17eda50 into flutter:master Jun 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 19, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 19, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 20, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 20, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 20, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 20, 2024
Manual roll requested by [email protected]

flutter/flutter@ccf3abe...6c06abb

2024-06-18 [email protected] Add test for engine artifact framework permissions (flutter/flutter#148786)
2024-06-18 [email protected] Add test for icon_button.3.dart (flutter/flutter#149988)
2024-06-18 [email protected] Roll Flutter Engine from 78fdd06af541 to 74f42ca3544c (6 revisions) (flutter/flutter#150421)
2024-06-18 [email protected] Fix transparent `dividerColor` breaks `TabBar.tabAlignment` (flutter/flutter#150350)
2024-06-18 [email protected] Fix scrollable `TabBar` jittering (flutter/flutter#150041)
2024-06-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland 3: [CupertinoActionSheet] Match colors to native (#150386)" (flutter/flutter#150413)
2024-06-18 [email protected] Extend the Windows web_tool_tests_1_2 shard timeout to 45 minutes (flutter/flutter#150393)
2024-06-18 [email protected] Roll Flutter Engine from 1c4e5e230ecb to 78fdd06af541 (3 revisions) (flutter/flutter#150403)
2024-06-18 [email protected] Roll Flutter Engine from a4f266f7eb1a to 1c4e5e230ecb (8 revisions) (flutter/flutter#150399)
2024-06-18 [email protected] Rename doc file to use standard hyphens (flutter/flutter#150314)
2024-06-17 [email protected] Fix typo in `SliverLayoutDimensions.hashCode` where not all properties are used in the hash code. (flutter/flutter#150306)
2024-06-17 [email protected] Fix doc comment references to 'this' (flutter/flutter#150379)
2024-06-17 [email protected] Add 'fail-fast' argument to flutter test (flutter/flutter#149587)
2024-06-17 [email protected] Update matchesGoldenFile documentation reference to goldenFileComparator (flutter/flutter#150343)
2024-06-17 [email protected] Reland 3: [CupertinoActionSheet] Match colors to native (flutter/flutter#150386)
2024-06-17 [email protected] [a11y] Add semantics: button to bottom navigation bar items and dropdown menu items (flutter/flutter#149375)
2024-06-17 [email protected] Reland "sliverGridDelegate mainAxisExtent add assert (#148470)"  (flutter/flutter#149720)
2024-06-17 [email protected] `ScaffoldBackgroundColor` should default to `ColorScheme.surface` (flutter/flutter#149772)
2024-06-17 [email protected] Reland TreeSliver  (flutter/flutter#149839)
2024-06-17 [email protected] Reland: [CupertinoActionSheet] Add sliding tap gesture (flutter/flutter#150219)
2024-06-17 [email protected] Roll Flutter Engine from 5989f0215fed to a4f266f7eb1a (1 revision) (flutter/flutter#150377)

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] 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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
@ishaanbahal
Copy link

ishaanbahal commented Aug 7, 2024

Got a surprise on 3.24 like @YazeedAlKhalaf mentioned, we are not using M3 design guidelines strictly, rather using a different background and surface. Anyway, for anyone coming here because of the surprise after update, please set your scaffoldBackgroundColor in main ThemeData to revert to your original look:

final _m3Theme = ThemeData(
    useMaterial3: true,
    scaffoldBackgroundColor: Color(0xFFFAFAFA), // Or a color from scheme you don't generally use, as that helps you keep dart and light modes in check
  );

Flutter upgrades generally bring these surprises which require some work to fix, and have to generally go through the changelog every time to see what broke and why, even the migration pages only work if you know how to find them. The doc mentioned above is not part of the breaking-changes landing page here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Changing app background color requires deprecated attribute background

5 participants