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

High contrast color scheme based on system forced colors #165068

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Mar 12, 2025

This PR introduces:

  • 2 new ColorSchemes: ColorScheme.systemColorsLight and ColorScheme.systemColorsDark.
  • 2 new TextThemes: Typography.systemColorsLight and Typography.systemColorsDark.
  • 2 new ThemeDatas: ThemeData.systemColorsLight and ThemeData.systemColorsLight.

The goal from this PR is to provide users with convenient high contrast themes for their MaterialApp:

MaterialApp(
  theme: ThemeData.light(),
  darkTheme: ThemeData.dark(),
  highContrastTheme: ThemeData.systemColorsLight(),
  highContrastDarkTheme: ThemeData.systemColorsDark(),
  // ...
)

The MaterialApp widget will automatically pick the correct one of the 4 themes based on system settings (light/dark mode, high contrast enabled/disabled, system colors).

Depends on #164933
Closes #118853

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Mar 12, 2025
static ui.PlatformDispatcher get _platformDispatcher =>
WidgetsBinding.instance.platformDispatcher;

factory ColorScheme.systemColorsLight({
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there use cases where one would prefer to not use systemcolor when creating the color scheme? If not, we should consider override this color at a more base level (hardcode in this class or fromSeed?) so that exsiting customer will automatically gets opted in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you are saying, and I like the idea of doing the right thing by default. But I think forcing all colors at a low level is a bit extreme.

How about this: we can modify _MaterialAppState._themeBuilder so that it automatically defaults to the new ThemeData.systemColorsLight or ThemeData.systemColorsDark when high contrast is enabled in user's system?

Copy link
Contributor

Choose a reason for hiding this comment

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

_MaterialAppState._themeBuilder

That won't help user who uses ColorScheme.highContrastLight or fromSeed right?

My main concern is how would a developer know to use systemColorsLight unless they know about forced color, which i doubt there are many developers know about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's split this into parts so that we know where we agree and where we still need to discuss:

  1. Users of MaterialApp can either pass their own high contrast theme or they will automatically get one based on system forced colors. I think we are in agreement here.

  2. Users of ColorScheme.highContrastLight (and ColorScheme.highContrastDark), I agree here that we should make this automatically default to system forced colors.

  3. Users of ColorScheme.fromSeed, I still think this is too low level for us to change its behavior. I actually don't even know how we would change it when the user is providing an explicit seed color. Maybe what we can do is change the default dynamicSchemeVariant to make it produce more "contrasty" colors? But again, that would be unrelated to system forced colors.

Copy link
Contributor

Choose a reason for hiding this comment

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

ColorScheme.fromSeed
I actually don't even know how we would change it when the user is providing an explicit seed color

yeah, this is a good point. Can we do something in the MaterialApp to override the forced colors in the highcontrast theme if one is provided?

@mdebbar mdebbar requested review from QuncCccccc and chunhtai March 17, 2025 17:12

ThemeData? theme = switch ((useDarkTheme, highContrast)) {
((false, false)) => widget.theme,
((false, true)) => widget.highContrastTheme ?? widget.theme,
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to fallback here if we doing the fallback in line 1042?

To make the code a bit more consistent
Either doing all fallback here and remove fallback in 1042, or remove all fallback here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that too, and I decided to keep it for symmetry with line 1033 which has a different fallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed how I'm doing fallbacks. I think it looks cleaner now, but please take a look and let me know what you think.

@@ -749,18 +749,18 @@ class ColorScheme with Diagnosticable {
/// ),
/// ```
/// {@end-tool}
const ColorScheme.highContrastLight({
ColorScheme.highContrastLight({
Copy link
Contributor

Choose a reason for hiding this comment

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

Some where in the document should mention this will prioritize system colors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, once we all agree on the code changes, I'll go through docs and update. I didn't want to change the docs too early in the process and keep going back to change them multiple times.

@mdebbar
Copy link
Contributor Author

mdebbar commented Mar 18, 2025

@QuncCccccc, I just pushed a few more commits.

  1. Does this match what you had in mind? Want to make sure I understood and applied your comments correctly.
  2. Is there a different default seed color for dark material 3? I think Color(0xFF6750A4) is for the light theme?
  3. I made useMaterial3 true by default because I saw other APIs doing that. Is that good or should I make it false by default?

Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

Does this match what you had in mind? Want to make sure I understood and applied your comments correctly.

Yes! Looks good to me! Thanks! The only thing I'm not sure is whether we should map canvas and canvasText to primary and onPrimary. But we can test it later!

Is there a different default seed color for dark material 3? I think Color(0xFF6750A4) is for the light theme?

Yes, Color(0xFF6750A4) is the primary color of the default light color scheme(here). For dark, maybe we can use
Color(0xFFD0BCFF)

I made useMaterial3 true by default because I saw other APIs doing that. Is that good or should I make it false by default?

Defaulting to true sounds good to me! The flag default value was updated to true since late 2023:)

error: highContrast.error,
onError: highContrast.onError,
),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I also saw some forced colors are related to buttons, so I'm thinking if we can also provide button-related themes here to map buttonBorder, buttonFace and buttonText. For example, maybe we can do:

ThemeData.from(
...
).copyWith(
  elevatedButtonTheme: ElevatedButtonTheme(
    data: ElevatedButtonThemeData(
      style: ElevatedButton.styleFrom(
        foregroundColor: highContrast.buttonText,
        backgroundColor: highContrast.buttonFace,
        side: BorderSide(color: buttonBorder),
      )
    )
  ),
  textButtonTheme:... // Similar to above, each type of buttons have their xxxButton.styleFrom().
  outlinedButtonTheme:...
  filledButtonTheme:...
)

As for field in forced colors, seems we can map it to InputDecorationTheme.fillColor
For fieldText, InputDecorationTheme.helperStyle, InputDecorationTheme.hintStyle, and textTheme might be related. But I'm not sure how textTheme should be updated here because we only want the text color in text field to be changed.

@mdebbar
Copy link
Contributor Author

mdebbar commented Mar 19, 2025

Ok, I added the button themes to the high contrast theme. Tested on the Material 3 Demo:

Light

image

Dark

image

@mdebbar mdebbar requested a review from QuncCccccc March 19, 2025 19:20
@@ -840,6 +844,91 @@ class ThemeData with Diagnosticable {
factory ThemeData.dark({bool? useMaterial3}) =>
ThemeData(brightness: Brightness.dark, useMaterial3: useMaterial3);

factory ThemeData._highContrast({
required bool useMaterial3,
required ColorScheme colorScheme,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need this parameter here?

  • If use M3, then we generate the complete color scheme by ColorScheme.fromSeed(), and for the primary, secondary, surface, error and their on- colors, we probably should use systemColors to override them if the systemColors are not empty.
  • If not M3, we can directly assign ColorScheme.highContrastLight to colorScheme. ColorScheme.highContrastLight is for M2, the default primary color Color(0xff0000ba) is not suitable for M3.

Please let me know if there's anything look confusing! We can talk about it offline😀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • If not M3, we can directly assign ColorScheme.highContrastLight to colorScheme

The reason I did it this way is I wanted to avoid this:

colorScheme: brightness == Brightness.light ? ColorScheme.highContrastLight() : ColorScheme.highContrastDark(),

So I made each factory pass its own color scheme (see ThemeData.highContrastLight and ThemeData.highContrastDark).

seedColor: const Color(0xFF6750A4),
brightness: colorScheme.brightness,
contrastLevel: 1.0,
primary: colorScheme.primary,
Copy link
Contributor

Choose a reason for hiding this comment

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

So following the comment above, instead of using colorScheme.primary here, I think we should use systemColors?.canvas.value. Does this make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually thinking we should let the primary color come from the M3 seed color. System colors will be used for surface/onSurface and all the button themes and text theme. WDYT?

@mdebbar mdebbar requested a review from QuncCccccc March 21, 2025 19:15
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

Can you include a example or just a snippet of code on how user will initialize their own theme that respect the system color?

return ThemeData._highContrast(
useMaterial3: useMaterial3,
seedColor: const Color(0xFF6750A4),
colorScheme: ColorScheme.highContrastLight(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought ColorScheme.highContrastLight is not m3 according to @QuncCccccc .

Comment on lines +942 to +955
/// ```dart
/// MaterialApp(
/// theme: ThemeData.light(),
/// darkTheme: ThemeData.dark(),
/// highContrastTheme: ThemeData.highContrastLight(),
/// highContrastDarkTheme: ThemeData.highContrastDark(),
///
/// home: Scaffold(
/// appBar: AppBar(
/// title: const Text('Home'),
/// ),
/// ),
/// )
/// ```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you include a example or just a snippet of code on how user will initialize their own theme that respect the system color?

@chunhtai here's an example that uses system colors.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh i meant for someone that has their own theme (component theme and colorScheme), what should they do to integrate the system color?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They should look at the implementation of ColorScheme.highContrastLight() 🙂

I'll try to come up with an example. Where would you recommend putting it?

Copy link
Contributor

@chunhtai chunhtai Mar 21, 2025

Choose a reason for hiding this comment

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

I was hoping there will be an easier migration for them without copy paste the entire code. It kind of defeat the purpose to attempt integrate systmecolor to material theme. For example, something like this

MaterialApp(
  highContrasttheme: Theme(
    textTheme: MyTextTheme(),
    colorTheme: ColorTheme.fromSeed(seed: Colors.purple),
    useSystemColor: true // this will override all color
  ),
  ...
)
MaterialApp(
  theme: myTheme,
  highContrasttheme: myHighContrastTheme,
  useSystemColor: true, // Where _themeBuilder will override systmecolor
  ...
)

I am also curious to hear from @QuncCccccc that how much of support we should provide in regard of the systemcolor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

[web] Support forced-colors feature
3 participants