Add support for high contrast and color inversion on Android#182263
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for high contrast and color inversion accessibility features on Android. The implementation in AccessibilityBridge.java is well-structured, introducing a new observer pattern for handling accessibility feature changes, which significantly improves code organization and maintainability. The changes are well-tested, covering different API levels and scenarios. I've added a few suggestions to make the tests even more precise. Overall, this is a high-quality contribution.
chunhtai
left a comment
There was a problem hiding this comment.
reusing AccessibilityFeature is fine for now, but I think we do need a property for this setting sooner or later
| } | ||
|
|
||
| float uiContrast = uiModeManager.getContrast(); | ||
| // 0.0 (standard), 0.5 (medium), 1.0 (high) |
There was a problem hiding this comment.
is the value a slider in android setting?
looks like we need a new API in framework to accomodate this, something like PlatformDispatcher.colorContrastScale.
I assume the 1.0 is the same as highcontrast in iOS?
There was a problem hiding this comment.
I agree that in the future we should move the contrast value into a separate API. I’ll leave a TODO.
The contrast value can range from -1.0 to 1.0 link I don’t fully understand how to obtain a value below zero. Possibly this will be added in future versions of Android.
In stock Android, the contrast selection is represented by three values. However, in different Android skins, the way the value is selected may be different
On iOS, contrast is represented as a boolean value
There was a problem hiding this comment.
And we already have highContrastTheme. In the case of a floating contrast value, we will need to modify this slightly.
Maybe we could do this:
highContrastTheme: (double contrast) => ThemeData()
Or merge theme and highContrastTheme into a single parameter:
theme: (double contrast) => ThemeData()
Or come up with a more elegant solution that doesn't break the existing API and takes negative contrast values into account
There was a problem hiding this comment.
You could probably set the contrast to -1 via adb, but I think that would correlate to 'low contrast' - which android doesn't seem to do right now. OEMs could probably set it differently.
For now, since we only have the on/off switch for high contrast, this seems like the way to go. Please create an issue to track the todo (if you haven't already and put that in the comment instead of your username).
There was a problem hiding this comment.
Thanks for the advice. I created an issue and updated the TODO: #182863
4016cc1 to
4b73804
Compare
4b73804 to
a802439
Compare
ba39851 to
4bf8689
Compare
|
autosubmit label was removed for flutter/flutter/182263, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
|
autosubmit label was removed for flutter/flutter/182263, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
|
Yes we need to respect the code freeze so we do not end up in a state where the new material and cupertino packages are missing changes. |
|
I can understand. So unless the doc change part can be reasonably excluded (which I'll leave it to the reviewers @chunhtai and @mboetger ), we'll have to hold landing this PR until after the decoupling. For now I'll assume that it's required and go ahead to convert this PR to a draft and apply a label so that we can quickly find it when the freeze is lifted. In case that we've decided it's ok to exclude the doc changes in Material, we can always convert it back and resume landing. Anyway, @xxxOVALxxx thank you for your contribution and thanks for understanding while we prepare for the decoupling! |
|
I'd be ok removing the material comment change. |
|
Great! @xxxOVALxxx If you're ok as well, can you remove the material widget change? |
There was a problem hiding this comment.
Code Review
This pull request implements support for high contrast and color inversion accessibility features on Android (API 34+). It refactors the AccessibilityBridge to use a modular observer pattern for monitoring system settings and updates documentation across the engine and framework. The review feedback suggests simplifying setting retrieval using API overloads, using epsilon for floating-point comparisons, and optimizing IPC calls by checking for flag changes before notifying the framework.
| protected boolean isFeatureEnabled() { | ||
| try { | ||
| return Settings.Secure.getInt(contentResolver, settingKey) == 1; | ||
| } catch (Settings.SettingNotFoundException e) { | ||
| Log.d(TAG, "Setting not found: " + settingKey + ", using default: " + defaultValue); | ||
| return defaultValue == 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic can be simplified by using the Settings.Secure.getInt overload that accepts a default value. This makes the code more concise and avoids the overhead of an explicit try-catch block for SettingNotFoundException.
@Override
protected boolean isFeatureEnabled() {
return Settings.Secure.getInt(contentResolver, settingKey, defaultValue) == 1;
}| protected boolean isFeatureEnabled() { | ||
| float value = Settings.Global.getFloat(contentResolver, settingKey, defaultValue); | ||
| return value == enabledValue; | ||
| } |
There was a problem hiding this comment.
Comparing floating-point numbers directly with == can be unreliable due to precision issues. It is generally safer to check if the difference between the values is within a very small threshold (epsilon).
@Override
protected boolean isFeatureEnabled() {
float value = Settings.Global.getFloat(contentResolver, settingKey, defaultValue);
return Math.abs(value - enabledValue) < 1e-6;
}| private void updateAccessibilityFeature(AccessibilityFeature feature, boolean enabled) { | ||
| if (enabled) { | ||
| accessibilityFeatureFlags |= feature.value; | ||
| } else { | ||
| accessibilityFeatureFlags &= ~AccessibilityFeature.BOLD_TEXT.value; | ||
| accessibilityFeatureFlags &= ~feature.value; | ||
| } | ||
| sendLatestAccessibilityFlagsToFlutter(); | ||
| } |
There was a problem hiding this comment.
The updateAccessibilityFeature method currently sends the latest accessibility flags to Flutter every time it is called. During initialization, this results in multiple redundant IPC calls as each feature (animations, invert colors, high contrast, bold text) is initialized. Consider checking if the flag value has actually changed before triggering the update to Flutter.
private void updateAccessibilityFeature(AccessibilityFeature feature, boolean enabled) {
int oldFlags = accessibilityFeatureFlags;
if (enabled) {
accessibilityFeatureFlags |= feature.value;
} else {
accessibilityFeatureFlags &= ~feature.value;
}
if (oldFlags != accessibilityFeatureFlags) {
sendLatestAccessibilityFlagsToFlutter();
}
}|
Done |
|
autosubmit label was removed for flutter/flutter/182263, because - The status or check suite Mac mac_ios_engine_ddm has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
Looks like there are Google test failures here. |
|
I assume go/lssc is the path forward here, I just don't have the bandwidth currently. |
|
There should be a fusion view that let you triage all scubas diff at once. also scuba doesn't need g3 fix, they will auto generate upon g3 roll |
|
also looking at the code change, I won't expect scuba change, do you have link the the failures? the frob entry seems to be dead https://frob.corp.google.com/#/pr/182263 |
This PR implements Android support for two previously unsupported accessibility features in
AccessibilityBridgePart of #168288
On Android, unlike iOS, color contrast can range from -1.0 to 1.0. The current implementation of
AccessibilityBridgeonly allows communication using bitmasks. In the future, this implementation will need to be changed to allow other types of data to be sent (And probably modifications to the corresponding ColorScheme).In the video, you can also see that color inversion is enabled with a delay (about 10 seconds). I tested it on all my devices and created an empty project on Jetpack Compose for testing, and it turned on with a delay everywhere. The only thing I found was that older versions of the Android API do not have this problem. I have not determined exactly which version this problem starts with, but API 28 and 24 do not have this problem.
output.webm
Code Example
Pre-launch Checklist
///).