Extended Accessibility Actions Support #24695
Conversation
There was a problem hiding this comment.
Overall the Android Implementation looks good to me. I had one inline comment about the expected behavior when setting both an "activate" handler and a standard click handler, but other than that this looks great.
There is another outstanding PR that was adding "focus" and "blur" actions, which you might want to take a look at (#24642). I think your approach here is more generalizable so is the preferred approach, and it should be pretty easy to incorporate focus and blur into sActionIdMap on Android (I'm unsure what this would take on the iOS side).
I'll comment on their PR as well to make sure they are aware of your changes here.
| host.getId(), | ||
| "performAction", | ||
| event); | ||
| return true; |
There was a problem hiding this comment.
Since you are returning here, rather than calling super it means that if someone sets onAccessibilityAction "activate", and also sets a standard click event, only the "activate" event would be fired when a user of an accessibility service clicks on the element.
Is this an intentional design choice? It definitely removes the ambiguity of which should fire first, but I am not sure if people will expect setting an onAccessibilityAction handler to break their click handler for some users. If it is by design, we should probably include an example showing this in the AccessibilityExample.js file.
Thoughts?
There was a problem hiding this comment.
@blavalla great point. I'll take a look at the AOSP source code to see what the default behavior for CLICK does these days-- it used to be that double tapping on an object simply synthesized a single tap to it, but things have changed of late.
In any case, it seems like activate might be a special case where we do want to call the super class's action implementation. But I think in most cases, we wouldn't want to call the superclass implementation.
Thoughts?
There was a problem hiding this comment.
@blavalla just to be clear, setting an action handler for activate won't affect the standard click behavior-- It's generally used as an alternative path to that same code. So the normal case is that the user taps an object to activate it for example, and the "activate" action handler generally would just call the same logic as gets called when the user taps on the component.
I honestly can't think of a great use case where you'd want to override the standard behavior, but both iOS and Android support it, so...
There was a problem hiding this comment.
By default as of Android 8.0, double-tap will first attempt to call the ACTION_CLICK AccessibilityAction:
And only if that fails it will then generate a synthetic click on the element.
This was added in Feb 2017 in this commit:
aosp-mirror/platform_frameworks_base@0adfbd3#diff-1da9a3224134e2b3bf4c5d6be9ed0e52
So in 8.0 and above only "activate" would be called if a user had both "activate" and a normal click handler on an element, and they clicked it via the double-tap gesture. But before 8.0, both the standard click handler and the "activate" handler would be called, one after the other.
I am not sure of the best way to handle this discrepancy to be honest. I'm not familiar with how ReactNative handles standard click events, but if we are already manually managing that behavior, we might be able to normalize the differences between newer and older versions of Android.
There was a problem hiding this comment.
We discussed this internally, and all things considered, I think the behavior as defined in this PR is appropriate.
With recent versions of Android, the only reason you would want to override the default view behavior is if it didn't work for some reason, in which case, you probably don't want to call the superclass implementation anyway.
In older versions of Android, which synthesize the single tap by default when the user double taps an item using a screen reader, you might want to override this behavior to allow braille display users to click your component, in which case, you'd want to trigger the same logic as is triggered by double tap, which I think you could do in Javascript code.
So, I think overriding the default behavior is probably expected, although as you point out, might be confusing at first. Perhaps we can clear things up with documentation.
|
In the latest patches, I've added iOS and Android "focus" and "blur" support. Although I couldn't think of an accessible way to build a test into RNTester for them. Any thoughts? |
|
I have disabled focus/blur support for now. I tried them both using accessibility actions, as well as by trapping the appropriate accessibility events as was done in #24642. And in both cases, I couldn't get blur working properly. And I suspect focus without blur support isn't terribly useful. I'd prefer to separate those into a separate PR and sort them out there. Anything still needing addressing with this PR? |
There was a problem hiding this comment.
Let's ship it.
Thanks again @marcmulcahy and also @blavalla for the review.
There was a problem hiding this comment.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@marcmulcahy it seems like one of the Android tests is failing. Do you mind fixing it? |
|
@cpojer Sure, I won't be able to look at it for another couple hours. Can you point me to the failing test? |
|
Check out the link next to the failed |
|
Thank you for fixing the test! |
There was a problem hiding this comment.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
This pull request was successfully merged by @xuelgong in 14b4668. When will my fix make it into a release? | Upcoming Releases |
Summary: D15391408 (#24695) added a new event type with the registration name 'onAccessibilityAction' on Android, using the key 'performAction'. On iOS the same event uses the key 'topAccessibilityAction', which caused a runtime error after I started registering both using the unified JS view config in D15488008. This diff changes Android to use the same name as iOS since the convention is to start with 'top'. Reviewed By: cpojer Differential Revision: D15542623 fbshipit-source-id: c339621d2b4d3e1700feb5419ae3e3af8b185ca8
Summary: This is a reconstitution of facebook#24190. It extends accessibility actions to include both a name and user facing label. These extensions support both standard and custom actions. We've also added actions support on Android, and added examples to RNTester showing how both standard and custom accessibility actions are used. ## Changelog [general] [changed] - Enhanced accessibility actions support Pull Request resolved: facebook#24695 Differential Revision: D15391408 Pulled By: cpojer fbshipit-source-id: 5ed48004d46d9887da53baea7fdcd0e7e15c5739
Summary: D15391408 (facebook#24695) added a new event type with the registration name 'onAccessibilityAction' on Android, using the key 'performAction'. On iOS the same event uses the key 'topAccessibilityAction', which caused a runtime error after I started registering both using the unified JS view config in D15488008. This diff changes Android to use the same name as iOS since the convention is to start with 'top'. Reviewed By: cpojer Differential Revision: D15542623 fbshipit-source-id: c339621d2b4d3e1700feb5419ae3e3af8b185ca8
Summary
This is a reconstitution of #24190. It extends accessibility actions to include both a name and user facing label. These extensions support both standard and custom actions.
We've also added actions support on Android, and added examples to RNTester showing how both standard and custom accessibility actions are used.
Changelog
[general] [changed] - Enhanced accessibility actions support
Test Plan
Added RNTester examples of both standard and custom actions, and verified that all examples work properly on Android with TalkBack and iOS with VoiceOver.