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

UIViewManager: Add accessibilityElementsHidden #10918

Closed
wants to merge 3 commits into from
Closed

UIViewManager: Add accessibilityElementsHidden #10918

wants to merge 3 commits into from

Conversation

omeid
Copy link
Contributor

@omeid omeid commented Nov 14, 2016

UIKit accessibilityElementsHidden property is similar to
Android's importantForAccessibility and very important for
creating a good accessible experience by hidding off the screen
and invisible elements/views from VoiceOver and other assistive
technologies.

This commit also improves accessibility of Navigation Experimental
CardStack by marking inactive (off screen) scenes invisible for iOS
VoiceOver and Android's Talkback.

Closes #9725

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @JoelMarcey and @javache to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Nov 14, 2016
@ericvicenti
Copy link
Contributor

Can we avoid adding a new prop, and instead add iOS support for importantForAccessibility="no-hide-descendants"

@omeid
Copy link
Contributor Author

omeid commented Nov 15, 2016

Sure, although if we are going to provide a unifrom API, I suggest it is better to add accessibilityElementsHidden support to Android; as it goes nicer along other accessibility* props.

It is also going to be consistent across platforms as accessibilityElementsHidden can be implement with Android's importantForAccessibility but not the other way around.

Thoughts?

@ericvicenti
Copy link
Contributor

Ok, makes sense to me. We just need to make the documentation and examples very clear. We should highlight breaking changes for the community and put warnings for deprecated usage without breaking anything immediately.

What steps do you think we should take to safely get to a sane state on these accessibility props?

@omeid
Copy link
Contributor Author

omeid commented Nov 15, 2016

I think it is good to implement accessibilityElementsHidden with Android support and deprecate importantForAccessibility with an appropriate warning asking users to switch to accessibilityElementsHidden.

Next up, if someone complains with good reasons to keep importantForAccessibility we can consider what is the appropriate way to tackle their requirements, otherwise, just drop importantForAccessibility as per standard deprecation policy.

@ericvicenti
Copy link
Contributor

Sounds great! Can you add android support for accessibilityElementsHidden and add the deprecation notice for importantForAccessibility?

@omeid
Copy link
Contributor Author

omeid commented Nov 16, 2016

@ericvicenti Done. Please have a look and let me know if anything needs to be improved.
Abd thanks for the feedback that made this a much better fix. :)

*
* @platform android
* Controls whether the accessibility elements contained within this view are hidden.
* Use this property to indicate whatever the should be reported to accessibility services that query the screen.
Copy link
Collaborator

@brentvatne brentvatne Nov 16, 2016

Choose a reason for hiding this comment

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

"whatever the should be reported"

typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, a typo. Just pushing a fix.

@brentvatne
Copy link
Collaborator

I don't know a lot about accessibility but I read over the conversation and this code and it seems perfectly reasonable. Would just fix up the comment describing the prop to fix typo.

@omeid
Copy link
Contributor Author

omeid commented Nov 17, 2016

Typo fixed. :)

@omeid omeid changed the title ios:UIViewManager: Export accessibilityElementsHidden UIViewManager: Add accessibilityElementsHidden Nov 22, 2016
@@ -97,6 +100,20 @@ public void setAccessibilityComponentType(T view, String accessibilityComponentT
AccessibilityHelper.updateAccessibilityComponentType(view, accessibilityComponentType);
}

@ReactProp(name = PROP_ACCESSIBILITY_ELEMENTS_HIDDEN)
public void setImportantForAccessibility(T view, @Nullable Boolean accessibilityElementsHidden) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not name this method setAccessibilityElementsHidden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason, I have no idea how I forgot to change that after copying the initial setImportantForAccessibility method. Pushing a fix now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@mkonicek
Copy link
Contributor

Thanks! I extended the comment a bit.

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Nov 23, 2016
@facebook-github-bot
Copy link
Contributor

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

@mkonicek
Copy link
Contributor

@facebook-github-bot shipit

@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 Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Nov 24, 2016
@omeid
Copy link
Contributor Author

omeid commented Nov 29, 2016

Should I rebase this or something else has failed during import? @mkonicek

@omeid
Copy link
Contributor Author

omeid commented Dec 8, 2016

ping @ericvicenti @javache @joelcloralt

omeid and others added 3 commits December 29, 2016 12:13
UIKit accessibilityElementsHidden property is similar to
Android's importantForAccessibility and very important for
creating a good accessible experience by hidden off the screen
and invisible elements/vies from VoiceOver and other assestive
technologies.

This commit also improves accessiblity of Navigation Experimental
CardStack by marking inactive (off screen) scenes invsible for ios
VoiceOver and Android's Talkback.

Closes #9725
This commit adds Android support for View's `accessibilityElementsHidden` prop
and marks Android only's `importantForAccessibility` as deprecated.

This commit also includes the relevant improvements for
NavigationExperimental NavigationCard.
@omeid
Copy link
Contributor Author

omeid commented Dec 29, 2016

@mkonicek Hey, so I keep rebasing this, any chance this could be shipped? this is really critcal for building accessible apps.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants