Fixes #173514 : Add highContrastThemeMode parameter to MaterialApp #173520
Conversation
This commit adds a new highContrastThemeMode parameter to MaterialApp that allows developers to manually control high contrast theme selection, addressing the need for apps to provide manual high contrast toggles when platform support is not available. Changes: - Added HighContrastThemeMode enum with system, standard, and highContrast options - Added highContrastThemeMode parameter to MaterialApp constructor - Modified _themeBuilder to respect the new parameter - Added comprehensive tests for all modes and edge cases The new parameter defaults to HighContrastThemeMode.system to maintain backward compatibility while enabling manual control when needed. Fixes: flutter#168288
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces a highContrastThemeMode to MaterialApp, allowing developers to override the system's high contrast setting. The implementation is solid, with good documentation and comprehensive tests. My main feedback is to improve the clarity of the new HighContrastThemeMode enum by using more explicit names for its values, which would enhance the readability of the new API.
| enum HighContrastThemeMode { | ||
| /// Use either the standard or high contrast theme based on what the user has selected in | ||
| /// the system settings. | ||
| system, | ||
|
|
||
| /// Always use the standard theme regardless of system preference. | ||
| standard, | ||
|
|
||
| /// Always use the high contrast theme regardless of system preference. | ||
| highContrast, | ||
| } |
There was a problem hiding this comment.
The enum values standard and highContrast could be clearer. The term standard is a bit ambiguous, as it could be interpreted as 'the standard high contrast theme'.1
Your PR description mentions enabled and disabled values, which are more explicit about the state of the high contrast feature. Consider renaming standard to disabled and highContrast to enabled for better readability and to align with the terminology in the PR description.
If you apply this change, remember to update the usages throughout the file (constructors, logic, and documentation for highContrastThemeMode) and in the tests.
enum HighContrastThemeMode {
/// Use either the standard or high contrast theme based on what the user has selected in
/// the system settings.
system,
/// Always use the standard theme (high contrast disabled) regardless of system preference.
disabled,
/// Always use the high contrast theme (high contrast enabled) regardless of system preference.
enabled,
}
Style Guide References
Footnotes
- Rename 'standard' to 'disabled' for better clarity - Rename 'highContrast' to 'enabled' for consistency - Update all documentation and test references - Addresses code review feedback for improved readability
24a87d8 to
c9cff0e
Compare
|
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
Fixes #173514
Description
Added support for manual high contrast theme mode control in MaterialApp. This allows developers to explicitly control the high contrast theme mode independently of the system settings, similar to how
themeModeworks for light/dark themes.Changes include:
HighContrastThemeModeenum withsystem,enabled, anddisabledvalueshighContrastThemeModeproperty to MaterialAppTest Plan
Added new test cases in
packages/flutter/test/material/app_test.dartcovering:All tests are passing.
Breaking Change
No breaking changes. This is a new opt-in feature that maintains backward compatibility with existing apps.