Fix a11y in embedded Android views post O #9321
Conversation
|
cc @qasidsadiq |
|
We really need a test for this, we may be able to do something on top of the android_views test. |
shell/platform/android/io/flutter/view/AccessibilityViewEmbedder.java
Outdated
Show resolved
Hide resolved
| Log.w(TAG, e); | ||
| } catch (InvocationTargetException e) { | ||
| Log.w(TAG, e); | ||
|
|
There was a problem hiding this comment.
Should we be explicit about an upper Android version bound here as we can't guarantee that the parcel structure won't change?
If R will include a public API that helps us it we should definitely do it, if not I'm not sure if we want to require manual intervention here for any new Android version, or only require it if it breaks(which can potentially lead to more difficult debugging).
Not sure what's better, what are your thoughts?
There was a problem hiding this comment.
Yeah I was going back and forth on this too. Reason I just lower bounded it is that I'm not sure that we would catch the version code bump in time, since this isn't being tested. (I am digging into how to integration test this in flutter/flutter now though, which would probably fix that.) But as it is now if this auto failed on higher Android versions it wouldn't come through in an obvious way back to us, other than us really knowing we needed to check it out. So it would be a guaranteed loss without a good way to prevent it. Maybe a TODO here to bound this once we have CI pointed at this?
shell/platform/android/io/flutter/view/AccessibilityViewEmbedder.java
Outdated
Show resolved
Hide resolved
amirh
left a comment
amirh
left a comment
There was a problem hiding this comment.
LGTM modulo moving a comment
shell/platform/android/io/flutter/view/AccessibilityViewEmbedder.java
Outdated
Show resolved
Hide resolved
flutter/engine@2589785...b0757e6 git log 2589785..b0757e6 --no-merges --oneline b0757e6 Roll buildroot to d1bbc14 to pick up fixes for armv7 iOS targets. (flutter/engine#9336) 70ebfc3 Fix the name of the channel parameter in PlatformMessage constructors (flutter/engine#9334) 7283ae3 Handle one-way platform messages in the embedder library (flutter/engine#9331) e8f8a92 Roll buildroot to 75660ad and complete the C++ 17 transition. (flutter/engine#9319) 6d5aaa0 Fix a11y in embedded Android views post O (flutter/engine#9321) f54a8f1 Roll src/third_party/skia c3252a04b377..3fc5df443e24 (1 commits) (flutter/engine#9328) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.
3 participants
This patch works around Android's limitations on reflection. With it embedded platform views that use virtual node hierarchy trees should be accessible on all Android versions to date.
The workarounds in this PR are brittle. Ideally the methods would be made public in Android instead so we wouldn't need to employ tactics like these to work around the missing methods.
AccessibilityNodeInfo#getChildIdis blocked from any type of reflection access, but the underlying private member that the getter accesses,mChildNodeIds, can still be reflected on. On Android versions where we can't access the getter, this patch falls back on reflectively accessing the field instead. Unfortunately this field is aLongArray, a utility data class private to Android. So in this case we're reflecting to both access the member and actually read data from it, since we need to use reflection to callLongArray.get(index).AccessibilityNodeInfo#getParent()doesn't have any lucky available underlying members. However,AccessibilityNodeInfoitself isParcelable, andmParentNodeIdis one of the pieces of data that's written to a parcel viaAccessibilityNodeInfo#writeToParcel. So the fallback for that is to write the node to a parcel and then read the parcel for the ID in question. This will break if the implementation details ofAccessibilityNodeInfo#writeToParcelever change. The details have already changed enough in the past to require two sets of logic for reading from the parcel.Note: the above parcelable hack will "work" for any of the data we need from the
AccessibilityNodeInfo, and could function as a blanket alternative to reflection if absolutely needed. However it's genuinely much less safe than reflection and shouldn't be used unless absolutely required.flutter/flutter#19418