[material_ui] Add contrastLevel for M3 ColorScheme - #11801
Conversation
129b42b to
c410028
Compare
de5b083 to
6d663fa
Compare
4513c4f to
e293b35
Compare
| stdout.writeln('Generating contents...'); | ||
| } | ||
| final String contents = generateContents(_className); | ||
| final String contents = generateContents(requiresGeneratedClass ? _className : ''); |
There was a problem hiding this comment.
We don't need a class for ColorScheme, so I added a requiresGeneratedClass field.
There was a problem hiding this comment.
Code Review
This pull request introduces support for Material 3 contrast levels in ThemeData by adding a contrastLevel parameter to the constructor. This allows standard, medium, high, or custom contrast levels to be applied when generating or selecting a ColorScheme. The generation of default color schemes is migrated to a new template (ColorSchemeTemplateM3), and corresponding unit tests are added. Feedback on the changes points out a broken documentation link referencing a non-existent public ThemeData.contrastLevel property, suggesting it be updated to refer to the constructor parameter instead.
| /// A custom `double` between -1.0 and 1.0 can also be passed to | ||
| /// [ThemeData.contrastLevel] when [ThemeData.colorSchemeSeed] is provided, in | ||
| /// which case the value is passed to [ColorScheme.fromSeed]. |
There was a problem hiding this comment.
The documentation references [ThemeData.contrastLevel], but ThemeData does not actually have a public contrastLevel property or getter (similar to colorSchemeSeed). This results in a broken documentation link. We should update the documentation to refer to the constructor parameter instead.
/// A custom double between -1.0 and 1.0 can also be passed as the
/// contrastLevel parameter of the [ThemeData] constructor when
/// [ThemeData.colorSchemeSeed] is provided, in which case the value is passed
/// to [ColorScheme.fromSeed].There was a problem hiding this comment.
Updated documentation.
| ColorScheme? colorScheme, | ||
| Brightness? brightness, | ||
| Color? colorSchemeSeed, | ||
| double? contrastLevel, |
There was a problem hiding this comment.
how does this work with MaterialApp. highContrastTheme and MaterialApp . highContrastDarkTheme? it feels like we are inventing another system.
Android also has contrast setting that can set contrast from -1 and 1 which felt like where the material design is based on. is there a good way a developer can set this value based on Android settings?
There was a problem hiding this comment.
There was a problem hiding this comment.
This PR is to add medium and high contrast ColorScheme baselines. See the M3 update section: https://m3.material.io/styles/color/system/overview#07f3a1d9-6c54-49f6-ae79-e52a6ee3e192
how does this work with MaterialApp. highContrastTheme and MaterialApp . highContrastDarkTheme?
MaterialApp(
highContrastTheme: ThemeData(contrastLevel: ContrastLevel.high)
highContrastTheme: ThemeData(brightness: Brightness.dart, contrastLevel: ContrastLevel.high)
)
will provide default M3 colorSchemes with high contrast.
Android also has contrast setting that can set contrast from -1 and 1 which felt like where the material design is based on. is there a good way a developer can set this value based on Android settings?
We do have an api ColorScheme.contrastLevel to set -1.0 - 1.0 and build a color scheme. But I don't think it is wired up to Android contrast setting. Are there an Android API available so we can set it up? If so, maybe we can so it in a separate PR.
There was a problem hiding this comment.
Are there an Android API available so we can set it up?
we don't have yet, but i think sooner or later we need to do so. Just want to make sure the api you propose here can be wired up in the future.
Is the intention to deprecate the highContrastTheme and other variant and solely relies on developer to create their own theme class by reading highcontrast double and brightness from mediaquery?
There was a problem hiding this comment.
No, the intention is not to deprecate highContrastTheme or highContrastDarkTheme in this PR.
This PR is just to let ThemeData generate/select the right M3 ColorScheme for standard, medium, and high contrast. MaterialApp.highContrastTheme still has a broader role because it lets developers provide a complete high-contrast ThemeData, not only a different color scheme. Does this make sense?
| /// brightness, and contrast level. | ||
| /// * <https://m3.material.io/styles/color/system/overview>, the Material 3 | ||
| /// color system overview. | ||
| abstract final class ContrastLevel { |
There was a problem hiding this comment.
Disregard if this is not a normal Flutter pattern, but I wonder if we could make this an enhanced enum instead?
ThemeData could then take the enum as a parameter, whereas ColorScheme.fromSeed would take a double (and we could document that there are preset values in the ContrastLevel enum.)
ThemeData(contrastLevel: ContrastLevel.medium)
vs
ColorScheme.fromSeed(contrastLevel: ContrastLevel.medium.value)
This would disallow any non-supported levels in ThemeData, so we wouldn't need the warning comment about them being dropped.
There was a problem hiding this comment.
Thanks for your review! I considered this, but I think keeping ThemeData.contrastLevel as a double better fits here.
ThemeData uses contrastLevel in two paths:
- when
colorSchemeSeedis provided, it forwards the value directly toColorScheme.fromSeed, which supports the full-1.0..1.0range; - when
colorSchemeSeedis null, it selects among the token-backed baseline schemes, where onlystandard,medium, andhighhave generated tokens.
Using an enum would make the token-backed path stricter, but it would prevent ThemeData(colorSchemeSeed: Colors.red contrastLevel: 0.6), even though that is valid for ColorScheme.fromSeed. It would also make future system contrast integration
harder, since platform APIs may expose a continuous contrast value rather than only the three named presets.
So I’d prefer to keep the parameter as double?, with ContrastLevel.standard/medium/high as named constants for the token-backed baselines, and document that custom values only apply when a seed is provided. WDYT😄?
0e4fd51 to
af390b8
Compare
af390b8 to
fd2108b
Compare
| } | ||
|
|
||
| /// Whether the generated contents must declare the generated defaults class. | ||
| bool get requiresGeneratedClass => true; |
There was a problem hiding this comment.
Instead of adding requiresGeneratedClass, what if we simply override the className (like how outputFileName is being overridden?)
Fixes flutter/flutter#184951
This PR is to add medium- and high-contrast M3 ColorScheme baselines. I also added a
double? contrastLeveltoThemeData.By default,
ThemeData()shows standard M3 ColorScheme. To choose medium/high contrast, useThis change will not cause breaking changes because the standard-contrast m3 color scheme baseline stays the same.
TODO:
Add an example or update an existing example to reflect the new feature.
Pre-Review Checklist
[shared_preferences]///).