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

Add accessibilityElementsHidden prop #17627

Closed
wants to merge 5 commits into from
Closed

Add accessibilityElementsHidden prop #17627

wants to merge 5 commits into from

Conversation

aputinski
Copy link
Contributor

@aputinski aputinski commented Jan 16, 2018

Motivation

Allow iOS to have similar accessibility functionality to Android. This PR exposes the accessibilityElementsHidden property on iOS which is similar to Android's importantForAccessibility="no-hide-descendants"

Test Plan

I didn't see any existing examples for testing native props being passed through, but I did add an example to the RNTester app. I've attached some screenshots using the Accessibility Inspector to verify the property was correctly passed through.

a
b

Related PRs

I've updated the website with appropriate documentation.

facebook/react-native-website#141

screen shot 2018-01-16 at 10 23 50
screen shot 2018-01-16 at 10 23 59

Release Notes

[IOS] [FEATURE] [View] - Added accessibilityElementsHidden property

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. cla signed labels Jan 16, 2018
@pull-bot
Copy link

pull-bot commented Jan 16, 2018

Attention: @shergin

Generated by 🚫 dangerJS

@@ -173,6 +174,16 @@ module.exports = {
* See http://facebook.github.io/react-native/docs/view.html#accessibilityviewismodal
*/
accessibilityViewIsModal: PropTypes.bool,

Choose a reason for hiding this comment

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

no-trailing-spaces: Trailing spaces not allowed.

@aputinski
Copy link
Contributor Author

I'm not super familiar with Android, but it doesn't appear that my changes caused the test failure?

@shergin
Copy link
Contributor

shergin commented Jan 17, 2018

Well, this is really great.
Thank you for raising this topic! However, I think we have to land #10918 instead... finally.

@aputinski What do you think?

@aputinski
Copy link
Contributor Author

Looks like that was closed and we're now discussing #11788. I don't have a strong preference, but it seems like there is quite a bit of confusion about trying to unify this behavior. Would it be possible to merge this (since it's non-breaking) and then continue discussing unifying this property down the road?

@shergin @omeid

.gitignore Outdated
@@ -48,6 +48,7 @@ node_modules
*.log
.nvm
/danger/node_modules/
scripts/.packager.env
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I was running the build locally, that file was generated and I didn't want to commit any build artifacts. It's unrelated to my change and I can remove it if necessary.

@shergin
Copy link
Contributor

shergin commented Jan 25, 2018

@aputinski Yeah, that's reasonable. Even if I am going to land that PR as well, it's "safer" to land yours first.

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jan 25, 2018
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.

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

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.

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

@omeid
Copy link
Contributor

omeid commented Jan 25, 2018

I think landing this is a mistake without understanding the android part. The reason I abandoned my initial PR was because no one from facebook actually showed interested and were willing to consider a practical solution.

I am still interested to see this feature landed so I don't have to maintain a fork that has some decent a18y.

@facebook-github-bot facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Jan 25, 2018
@facebook-github-bot
Copy link
Contributor

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jan 26, 2018
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.

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

Plo4ox pushed a commit to Plo4ox/react-native that referenced this pull request Feb 17, 2018
Summary:
Allow iOS to have similar accessibility functionality to Android. This PR exposes the `accessibilityElementsHidden` property on iOS which is similar to Android's `importantForAccessibility="no-hide-descendants"`

I didn't see any existing examples for testing native props being passed through, but I did add an example to the RNTester app. I've attached some screenshots using the Accessibility Inspector to verify the property was correctly passed through.

![a](https://user-images.githubusercontent.com/603528/34998153-50e66776-faac-11e7-826d-1445a6813929.png)
![b](https://user-images.githubusercontent.com/603528/34998158-535a7420-faac-11e7-80d4-992fb7cd82dd.png)

I've updated the website with appropriate documentation.

facebook/react-native-website#141

![screen shot 2018-01-16 at 10 23 50](https://user-images.githubusercontent.com/603528/34998202-6f2f39a6-faac-11e7-8651-0cfe8e037a30.png)
![screen shot 2018-01-16 at 10 23 59](https://user-images.githubusercontent.com/603528/34998205-711d6f94-faac-11e7-974d-54340c72fce4.png)

[IOS] [FEATURE] [View] - Added accessibilityElementsHidden property
Closes facebook#17627

Differential Revision: D6806444

Pulled By: hramos

fbshipit-source-id: 50d31fdb92f4c59ae9355b019c422418b2e6cc24
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Mar 1, 2018
Summary:
Allow iOS to have similar accessibility functionality to Android. This PR exposes the `accessibilityElementsHidden` property on iOS which is similar to Android's `importantForAccessibility="no-hide-descendants"`

I didn't see any existing examples for testing native props being passed through, but I did add an example to the RNTester app. I've attached some screenshots using the Accessibility Inspector to verify the property was correctly passed through.

![a](https://user-images.githubusercontent.com/603528/34998153-50e66776-faac-11e7-826d-1445a6813929.png)
![b](https://user-images.githubusercontent.com/603528/34998158-535a7420-faac-11e7-80d4-992fb7cd82dd.png)

I've updated the website with appropriate documentation.

facebook/react-native-website#141

![screen shot 2018-01-16 at 10 23 50](https://user-images.githubusercontent.com/603528/34998202-6f2f39a6-faac-11e7-8651-0cfe8e037a30.png)
![screen shot 2018-01-16 at 10 23 59](https://user-images.githubusercontent.com/603528/34998205-711d6f94-faac-11e7-974d-54340c72fce4.png)

[IOS] [FEATURE] [View] - Added accessibilityElementsHidden property
Closes facebook/react-native#17627

Differential Revision: D6806444

Pulled By: hramos

fbshipit-source-id: 50d31fdb92f4c59ae9355b019c422418b2e6cc24
@hramos hramos added the Merged This PR has been merged. label Mar 8, 2019
@react-native-bot react-native-bot removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Mar 8, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

8 participants