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

feat: set disabled accessibilityState when TouchableHighlight is disabled #31135

Closed
wants to merge 4 commits into from

Conversation

Naturalclar
Copy link
Contributor

@Naturalclar Naturalclar commented Mar 11, 2021

#30950

Summary

automatically set disabled to accessibilityState when TouchableHighlight is disabled

Changelog

[General] [Changed] - Set disabled accessibilityState when TouchableHighlight is disabled

Test Plan

Tested on physical android device that pressing disabled TouchableHighlight announces "dim" when talkback is on

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 11, 2021
@analysis-bot
Copy link

analysis-bot commented Mar 11, 2021

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

Base commit: dbf5fa2

@Naturalclar Naturalclar marked this pull request as ready for review March 11, 2021 11:14
@analysis-bot
Copy link

analysis-bot commented Mar 11, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,908,986 -23,907
android hermes armeabi-v7a 8,408,025 -15,920
android hermes x86 9,399,614 -23,460
android hermes x86_64 9,343,422 -24,434
android jsc arm64-v8a 10,642,521 -23,266
android jsc armeabi-v7a 10,124,648 -15,286
android jsc x86 10,694,646 -22,819
android jsc x86_64 11,278,720 -23,797

Base commit: dbf5fa2

@kacieb
Copy link
Contributor

kacieb commented Mar 16, 2021

This is great and those test cases are awesome! 😍 Thank you so much for working on this!

@lunaleaps Do you think the component should also be disabled if accessibilityState.disabled=true, if the disabled prop is not set? Like in this PR: #31001

@lunaleaps
Copy link
Contributor

lunaleaps commented Mar 17, 2021

Yea! I think it'd be good to have this consistent across the components. @Naturalclar can you follow the logic in #31001 where accessibilityState.disabled set to true will also disable the component?

@Naturalclar
Copy link
Contributor Author

@lunaleaps Sure thing! made the change in 4dc00ba

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@facebook-github-bot
Copy link
Contributor

@kacieb merged this pull request in f69e096.

@amarlette
Copy link

Thank you @Naturalclar for this contribution! 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? Is your twitter @ the same as your Github?

@Naturalclar
Copy link
Contributor Author

@amarlette thank you!
My Twitter account is natural_clar 😃

@Naturalclar Naturalclar deleted the fix/30950 branch March 23, 2021 08:05
@kacieb
Copy link
Contributor

kacieb commented Mar 23, 2021

Thank you so much for this pull request!!!

I made a few small changes to the test cases before landing, and wanted to let you know in case you write any future tests! Thanks so much for including the tests by the way, that will make sure this functionality doesn't break in the future.

I changed the expectRenderMatchingSnapshot calls to use this structure instead:

expect(
  ReactTestRenderer.create(<TouchableHighlight ... />)
).toMatchSnapshot();

This is because expectRenderMatchingSnapshot is primarily only needed for checking that the display name works correctly, and that mocking and unmocking the component works as expected. This wasn't necessary in this case, since we're only checking prop values.

Thanks again for helping improve React Native's accessibility! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TouchableHighlight Component doesn't disable click functionality when disabled
6 participants