Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extended Accessibility Actions Support #24695

Closed
wants to merge 6 commits into from

Conversation

@marcmulcahy
Copy link

commented May 2, 2019

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.

@blavalla
Copy link

left a 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;

This comment has been minimized.

Copy link
@blavalla

blavalla May 9, 2019

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?

This comment has been minimized.

Copy link
@marcmulcahy

marcmulcahy May 14, 2019

Author

@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?

This comment has been minimized.

Copy link
@marcmulcahy

marcmulcahy May 14, 2019

Author

@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...

This comment has been minimized.

Copy link
@blavalla

blavalla May 14, 2019

By default as of Android 8.0, double-tap will first attempt to call the ACTION_CLICK AccessibilityAction:

https://github.com/aosp-mirror/platform_frameworks_base/blob/master/services/accessibility/java/com/android/server/accessibility/TouchExplorer.java#L393

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.

This comment has been minimized.

Copy link
@marcmulcahy

marcmulcahy May 16, 2019

Author

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.

@marcmulcahy

This comment has been minimized.

Copy link
Author

commented May 14, 2019

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?

@marcmulcahy marcmulcahy force-pushed the marcmulcahy:a11y-actions branch from 203ceeb to 2947396 May 16, 2019

@marcmulcahy

This comment has been minimized.

Copy link
Author

commented May 16, 2019

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?

@blavalla

This comment has been minimized.

Copy link

commented May 16, 2019

I don't have any other feedback on the Android side.

@shergin, @ikenwoo, do you guys have any more feedback on the iOS or Javascript sides?

@cpojer

cpojer approved these changes May 17, 2019

Copy link
Contributor

left a comment

Let's ship it.

Thanks again @marcmulcahy and also @blavalla for the review.

@facebook-github-bot
Copy link

left a comment

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@cpojer

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@marcmulcahy it seems like one of the Android tests is failing. Do you mind fixing it?

@marcmulcahy

This comment has been minimized.

Copy link
Author

commented May 17, 2019

@cpojer Sure, I won't be able to look at it for another couple hours. Can you point me to the failing test?

@cpojer

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Marc Mulcahy
@cpojer

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Thank you for fixing the test!

@facebook-github-bot
Copy link

left a comment

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot

This comment has been minimized.

Copy link
Collaborator

commented May 20, 2019

This pull request was successfully merged by @xuelgong in 14b4668.

When will my fix make it into a release? | Upcoming Releases

@marcmulcahy marcmulcahy deleted the marcmulcahy:a11y-actions branch May 20, 2019

facebook-github-bot added a commit that referenced this pull request May 30, 2019

Rename onAccessibilityAction event on Android
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.