-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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
base: master
Are you sure you want to change the base?
Conversation
static ui.PlatformDispatcher get _platformDispatcher => | ||
WidgetsBinding.instance.platformDispatcher; | ||
|
||
factory ColorScheme.systemColorsLight({ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
-
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. -
Users of
ColorScheme.highContrastLight
(andColorScheme.highContrastDark
), I agree here that we should make this automatically default to system forced colors. -
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 defaultdynamicSchemeVariant
to make it produce more "contrasty" colors? But again, that would be unrelated to system forced colors.
There was a problem hiding this comment.
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?
|
||
ThemeData? theme = switch ((useDarkTheme, highContrast)) { | ||
((false, false)) => widget.theme, | ||
((false, true)) => widget.highContrastTheme ?? widget.theme, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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({ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@QuncCccccc, I just pushed a few more commits.
|
There was a problem hiding this 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, | ||
), | ||
); |
There was a problem hiding this comment.
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.
@@ -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, |
There was a problem hiding this comment.
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
tocolorScheme
.ColorScheme.highContrastLight
is for M2, the default primary colorColor(0xff0000ba)
is not suitable for M3.
Please let me know if there's anything look confusing! We can talk about it offline😀
There was a problem hiding this comment.
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
tocolorScheme
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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this 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(), |
There was a problem hiding this comment.
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 .
/// ```dart | ||
/// MaterialApp( | ||
/// theme: ThemeData.light(), | ||
/// darkTheme: ThemeData.dark(), | ||
/// highContrastTheme: ThemeData.highContrastLight(), | ||
/// highContrastDarkTheme: ThemeData.highContrastDark(), | ||
/// | ||
/// home: Scaffold( | ||
/// appBar: AppBar( | ||
/// title: const Text('Home'), | ||
/// ), | ||
/// ), | ||
/// ) | ||
/// ``` |
There was a problem hiding this 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?
@chunhtai here's an example that uses system colors.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
This PR introduces:
ColorScheme
s:ColorScheme.systemColorsLight
andColorScheme.systemColorsDark
.TextTheme
s:Typography.systemColorsLight
andTypography.systemColorsDark
.ThemeData
s:ThemeData.systemColorsLight
andThemeData.systemColorsLight
.The goal from this PR is to provide users with convenient high contrast themes for their
MaterialApp
: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