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.

Sign up for GitHub

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

Jump to bottom

Added talkback support for button accessibility: disabled prop #31001

Closed

huzaifaaak wants to merge 12 commits into facebook:master from huzaifaaak:accessibility/button

huzaifaaak commented Feb 16, 2021

Summary

Issue # #30934 .When using a screen reader disabled buttons do not announce that they are disabled.

Changelog

[Android] [Changed] - Passing accessibility state in button so it can announce disabled in talkback

Test Plan

I have added Button in Button Example with accessibiltyState prop that will announce button is disabled when testing with talkback.

Ios test

I am unable to run ios project on my machine. RNTesterPods.xcworkspace gives workspace integrity error :/


          Chores: added talkback for button accessibility: disabled prop

599cd81

Contributor

facebook-github-bot commented Feb 16, 2021

Hi @huzaifaaak!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

analysis-bot commented Feb 16, 2021 •

edited

Loading

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 4170726

analysis-bot commented Feb 16, 2021 •

edited

Loading

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,908,772	+366
android	hermes	armeabi-v7a	8,407,852	+375
android	hermes	x86	9,399,329	+376
android	hermes	x86_64	9,343,247	+368
android	jsc	arm64-v8a	10,642,464	+144
android	jsc	armeabi-v7a	10,124,638	+159
android	jsc	x86	10,694,515	+149
android	jsc	x86_64	11,278,700	+142

Base commit: 4170726


          Merge branch 'master' into accessibility/button

Contributor

facebook-github-bot commented Feb 16, 2021

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

facebook-github-bot added the CLA Signed label

Huzaifa added 2 commits

February 17, 2021 01:17


          fix: fixed flow error by adding type and refactoring accessibility st…

eb7bda3

…ate. Also, fixed linting errors


          Merge branch 'accessibility/button' of https://github.com/huzaifaaak/…

eb3c9a0

…react-native into accessibility/button

Contributor

facebook-github-bot commented Feb 16, 2021

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

analysis-bot suggested changes

View reviewed changes

analysis-bot left a comment

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

packages/rn-tester/js/examples/Modal/ModalExample.js Outdated Show resolved Hide resolved

packages/rn-tester/js/examples/Modal/ModalExample.js Outdated Show resolved Hide resolved

Contributor

facebook-github-bot commented Feb 16, 2021

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

4 similar comments

Contributor

facebook-github-bot commented Feb 16, 2021

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Contributor

facebook-github-bot commented Feb 17, 2021

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Contributor

facebook-github-bot commented Feb 17, 2021

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Contributor

facebook-github-bot commented Feb 17, 2021

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Author

huzaifaaak commented Feb 17, 2021 •

edited

Loading

@kacieb Can you please review this and give your comments 🙏

kacieb requested review from lunaleaps, nadiia and kacieb

February 17, 2021 16:33

lunaleaps reviewed

View reviewed changes

packages/rn-tester/js/examples/Button/ButtonExample.js Outdated Show resolved Hide resolved

Libraries/Components/Button.js Outdated

@@ @@ -273,11 +278,14 @@ class Button extends React.Component<ButtonProps> { @@
                       buttonStyles.push({backgroundColor: color});
                     }
                   }
-                  const accessibilityState = {};
+                  const accessibilityState =
+                    disabled != null

Contributor

lunaleaps Feb 17, 2021

I'd suggest const accessibilityState = disabled === true ? ... : ... so it's accessiblityState.disabled | disabled.

Contributor

lunaleaps Mar 10, 2021

Sorry to follow up on this can we avoid creating a new object here?

const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState

packages/rn-tester/js/examples/Modal/ModalExample.js Outdated Show resolved Hide resolved

Contributor

lunaleaps commented Feb 17, 2021

Thanks for adding an example as well!


          Chores: refactored condition and updated string in example

analysis-bot suggested changes

View reviewed changes

analysis-bot left a comment

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

packages/rn-tester/js/examples/Modal/ModalExample.js Outdated

                           this.setState({currentOrientation: evt.nativeEvent.orientation})
                         }
                         onDismiss={() => {
-                          if (this.state.action === 'onDismiss') alert(this.state.action);
+                          if (this.state.action === 'onDismiss') {
+                            alert(this.state.action);

analysis-bot Feb 17, 2021

no-alert: Unexpected alert.

packages/rn-tester/js/examples/Modal/ModalExample.js Outdated

                         }}
                         onShow={() => {
-                          if (this.state.action === 'onShow') alert(this.state.action);
+                          if (this.state.action === 'onShow') {
+                            alert(this.state.action);

analysis-bot Feb 17, 2021

no-alert: Unexpected alert.


          chores: formatting reverted 🍎

db1644f

lunaleaps approved these changes

View reviewed changes

Contributor

lunaleaps left a comment

Looks good to me!

Libraries/Components/Button.js

@@ @@ -18,7 +18,7 @@ const Text = require('../Text/Text'); @@
               const TouchableNativeFeedback = require('./Touchable/TouchableNativeFeedback');
               const TouchableOpacity = require('./Touchable/TouchableOpacity');
               const View = require('./View/View');
+              import type {AccessibilityState} from './View/ViewAccessibility';

Contributor

lunaleaps Feb 17, 2021

Nit: should this also be grouped with line 24? I can also fix this on import

Author

huzaifaaak Feb 17, 2021 •

edited

Loading

Yes, this should be grouped with other type imports 💯. Please fix this during import

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

lunaleaps reviewed

View reviewed changes

Libraries/Components/Button.js

                   if (disabled) {
                     buttonStyles.push(styles.buttonDisabled);
                     textStyles.push(styles.textDisabled);
-                    accessibilityState.disabled = true;

Contributor

lunaleaps Feb 18, 2021

@kacieb brought up a great point -- this PR doesn't seem to be fixing accessibility for talkback but rather adding an ability for accessibilityState -- I tested on an Android device and did notice that talkback does announce "disabled" for the button in your example where you solely use accessibilityState.

That raises the question of what was the original issue reporting? Was it reporting that accessibilityState={{disabled.. wasn't working -- which is indeed what your PR fixes -- but the fact that the property didn't even exist before has me puzzled

Contributor

lunaleaps Feb 18, 2021

cc @blavalla who reported the issue for clarification -- relevant to our discussion on disabled vs. accessibilityState.disabled

Contributor

lunaleaps Feb 18, 2021

I'm going to hold off landing this PR until we have some clarity about what the original issue is.

Author

huzaifaaak Feb 18, 2021

Noted!
Link to the accessibility issue: #30840

Contributor

blavalla Feb 18, 2021

@lunaleaps, the original issue was that setting accessibilityState: disabled alone was not actually disabling the button for accessibility.

This snack (https://snack.expo.io/d1Ikg7nqo) from #30840 shows the issue well.

Contributor

blavalla Feb 18, 2021

To clarify further, the issue was mostly that it is weird that you can actually set conflicting states for accessibility and non-accessibility users. Basically, there is nothing stopping someone from setting disabled="true" accessibilityState={{disabled: false}} or vice-versa.

If we are going to explicitly allow this though, they should at least work as expected, with the accessibilityState: disabled actually disabling the element for accessibility users.

Contributor

lunaleaps Feb 19, 2021 •

edited

Loading

Ah right! ~~accessibilityState is a View level prop so it should already exist on <Button />~~. My mistake! Okay that clarifies things! It sounds like this change is still worth pursuing -- to just sync the two values

Edit: Ah right because the snack does not have Flow enabled, it didn't warn by the fact that accessibiltyState isn't a prop on Button.

Author

huzaifaaak Feb 19, 2021

If we are going to explicitly allow this though, they should at least work as expected, with the accessibilityState: disabled actually disabling the element for accessibility users.

@lunaleaps I explicitly tested disabled="false" and accessibilityState={{disabled: true}} and it announces that button is disabled but it does not disables the onPress. I believe this needs to be done on the View level.

Having two different props is confusing. To disable the onPress we have to explicitly pass the disabled={true} prop.

Contributor

lunaleaps Feb 19, 2021 •

edited

Loading

and it announces that button is disabled but it does not disables the onPress. I believe this needs to be done on the View level.

Yea I think that's covered by #30943

I think we should set this down for a bit and circle back to how we should handle these two props

Huzaifa added 2 commits

February 26, 2021 19:38


          Merge branch 'master' into accessibility/button

d3fcdb6


          Chores: updated disable to have higher precedence over accessibilitys…

23a1f9b

…tate


          chores: added accState.disabled to apply styling too

b499b2f

lunaleaps approved these changes

View reviewed changes

Contributor

lunaleaps left a comment

Otherwise looks good to me

Libraries/Components/Button.js Outdated

@@ @@ -273,11 +278,14 @@ class Button extends React.Component<ButtonProps> { @@
                       buttonStyles.push({backgroundColor: color});
                     }
                   }
-                  const accessibilityState = {};
+                  const accessibilityState =
+                    disabled != null

Contributor

lunaleaps Mar 10, 2021

Sorry to follow up on this can we avoid creating a new object here?

const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState


          Refactor: Avoid creating a new object

7e6dfc8

Author

huzaifaaak commented Mar 10, 2021

@lunaleaps I have updated it to avoid object creation. Do let me know if it can be done in a better way


          Merge branch 'master' into accessibility/button

c25f8e8

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot closed this in

5889cbe

facebook-github-bot added the Merged label

Contributor

facebook-github-bot commented Mar 11, 2021

@lunaleaps merged this pull request in 5889cbe.

Contributor

kacieb commented Mar 12, 2021

This is awesome, thank you so much for working on this and improving React Native's accessibility! I'd love to see a snapshot test for this component at some point, similar to this Pressable one added here, that ensures the accessibilityState values are correctly set as expected.

amarlette mentioned this pull request

Button does not announce "disabled" #30934

Closed

This was referenced Mar 15, 2021

Button Component doesn't disable click functionality when disabled #30943

Closed

Selected State does not annonce when Button Component selected #30954

Closed

amarlette added the Accessibility label

kacieb mentioned this pull request

feat: set disabled accessibilityState when TouchableHighlight is disabled #31135

Closed

amarlette commented Mar 17, 2021

Hey @huzaifaaak I would like to give you a shout-out on Twitter and include you in our end-of-month issues update for your contribution. Do you have a Twitter we can tag and is your @ the same as your Github?

Author

huzaifaaak commented Mar 17, 2021

Hey @amarlette yes my twitter handle is the same huzaifaaak

huzaifaaak mentioned this pull request

Accessibility/button test #31189

Closed

blavalla mentioned this pull request

Image does not announce "disabled" #30935

Closed

amarlette linked an issue

that may be closed by this pull request

Selected State does not annonce when Button Component selected #30954

Closed

amarlette mentioned this pull request

Fix/30842 accessibility selected state button #31116

Closed

facebook-github-bot pushed a commit that referenced this pull request


          Accessibility/button test (#31189)

b5e649f

Summary:
This PR aims to add test's for button.
Snapshot test for PR #31001 . This would make sure `accessibilityState` is properly set.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Added] - Test's for button

Pull Request resolved: #31189

Test Plan:
`npm test` to run the test's.
Since the disabled prop of button has precedence over `accessibilityState.disabled` the test's will make sure it remains this way.

Reviewed By: kacieb

Differential Revision: D27473082

Pulled By: lunaleaps

fbshipit-source-id: 65d82620e8c245c2a8e29c3e9a8252d3a4275b09

anaskhraza mentioned this pull request

Button Component does not announce Checked/Unchecked State #30959

Closed

martinivanalfonso mentioned this pull request

TextInput Component does not announce Checked/Unchecked State #30960

Closed

amarlette linked an issue

that may be closed by this pull request

Button Component does not announce Checked/Unchecked State #30959

Closed

This was referenced Jan 28, 2022

react-native-slider accessibilityState disabled fabOnReact/react-native-notes#2

Closed

Fix: Conflicting states for accessibility and non-accessibility users callstack/react-native-slider#354

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

blavalla blavalla left review comments

facebook-github-bot facebook-github-bot left review comments

analysis-bot analysis-bot requested changes

lunaleaps lunaleaps approved these changes

nadiia Awaiting requested review from nadiia

kacieb Awaiting requested review from kacieb

Labels

Accessibility CLA Signed Merged