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

adding importantForAccessibility for Text, Button, ImageBackground #34245

Closed
wants to merge 15 commits into from

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented Jul 22, 2022

Summary

Previously published by grgr-dkrk as #31296, fixes the following issues:

  1. ImportantForAccessibility="no" does not work on Text, Button
  2. ImportantForAccessibility="no-hide-descendants" does not work on Text, Button, or ImageBackground.

Note: On ImageBackground, focus is prevented on the imageBackground node itself, but focus is not prevented on its descendants.

Note: Button component expected behavior for prop importantForAccessibility

Some components like Button seem like atomic units, but they end up rendering a hierarchy of components for example a wrapper View with a Text inside them. Allowing those descendants to become focusable, breaks the model of these being a single component to consider and forcing no-hide-descendants makes sense here.

The other option is always to render any descendants of these elements with importantForAccessibility="no", so they can never be focusable on their own. This would have the same result, BUT may potentially cause issues when the descendant content is supposed to automatically get moved up to the focusable ancestor to act as a label (which is what Talkback does with unfocusable text elements by default).

Note: importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images

fixes #30850 related #33690

Changelog

[Android] [Fixed] - adding importantForAccessibility for Text, Button, ImageBackground

Test Plan

  1. testing ImageBackground importantForAccessiblity (link to test)
  2. importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images (link to test)
  3. testing ImageBackground importantForAccessiblity (link to test)
  4. Button with importantForAccessibility=no (link to test)

@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 Jul 22, 2022
@fabOnReact fabOnReact changed the title 🚧 importantForAccessibility 🚧 🚧 adding importantForAccessibility for Text, Button, ImageBackground 🚧 Jul 22, 2022
@react-native-bot react-native-bot added Bug Platform: Android Android applications. labels Jul 22, 2022
@analysis-bot
Copy link

analysis-bot commented Jul 22, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,825,913 +87
android hermes armeabi-v7a 7,217,137 +13
android hermes x86 8,138,866 +707
android hermes x86_64 8,116,418 +128
android jsc arm64-v8a 9,703,726 +159
android jsc armeabi-v7a 8,458,179 +94
android jsc x86 9,654,861 +767
android jsc x86_64 10,252,269 +198

Base commit: a70354d
Branch: main

@fabOnReact
Copy link
Contributor Author

fabOnReact commented Jul 22, 2022

@analysis-bot
Copy link

analysis-bot commented Jul 22, 2022

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

Base commit: a70354d
Branch: main

@fabOnReact
Copy link
Contributor Author

Button with importantForAccessibility=no

2022-07-22.18-47-03.mp4

@fabOnReact
Copy link
Contributor Author

testing ImageBackground importantForAccessiblity

365b3d3

2022-07-22.16-49-37.mp4

@fabOnReact fabOnReact changed the title 🚧 adding importantForAccessibility for Text, Button, ImageBackground 🚧 adding importantForAccessibility for Text, Button, ImageBackground Jul 25, 2022
@fabOnReact fabOnReact marked this pull request as ready for review July 25, 2022 11:47
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jul 25, 2022
@facebook-github-bot
Copy link
Contributor

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

@fabOnReact
Copy link
Contributor Author

@dmitryrykun Thanks for importing the pull request. Could you share the warning message that makes the PR fail internally? 🙏

@lunaleaps
Copy link
Contributor

Ah I think it's the incorrect license for ImageBackground-test.js -- we can fix that internally

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @fabriziobertoglio1987 in 62021eb.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Aug 10, 2022
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
…acebook#34245)

Summary:
Previously published by [grgr-dkrk][2] as [facebook#31296], fixes the following issues:

1) ImportantForAccessibility="no" does not work on Text, Button
2) ImportantForAccessibility="no-hide-descendants" does not work on Text, Button, or ImageBackground.

Note: On ImageBackground, focus is prevented on the imageBackground node itself, but focus is not prevented on its descendants.

Note: [Button component expected behavior for prop importantForAccessibility][4]
>Some components like Button seem like atomic units, but they end up rendering a hierarchy of components for example a wrapper View with a Text inside them. Allowing those descendants to become focusable, breaks the model of these being a single component to consider and forcing no-hide-descendants makes sense here.

>The other option is always to render any descendants of these elements with importantForAccessibility="no", so they can never be focusable on their own. This would have the same result, **BUT may potentially cause issues when the descendant content is supposed to automatically get moved up to the focusable ancestor to act as a label** (which is what Talkback does with unfocusable text elements by default).

Note: [importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images][5]

fixes facebook#30850 related facebook#33690

## Changelog

[Android] [Fixed] - adding importantForAccessibility for Text, Button, ImageBackground

Pull Request resolved: facebook#34245

Test Plan:
1) testing ImageBackground importantForAccessiblity ([link to test][1])
2) importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images ([link to test][5])
3) testing ImageBackground importantForAccessiblity ([link to test][6])
4) Button with importantForAccessibility=no ([link to test][7])

[1]: facebook#31296 (comment) ""
[2]: https://github.com/grgr-dkrk "grgr-dkrk"
[3]: facebook#31296 "facebook#31296"
[4]: facebook#31296 (comment) "expected behaviour with prop importantForAccessibility in Button component"
[5]: facebook#30850 (comment) "importantForAccessibility=no does not allow screenreader focus on nested Text Components with accessibilityRole=link or inline Images"
[6]: facebook#34245 (comment) "testing ImageBackground importantForAccessiblity"
[7]: facebook#34245 (comment) "Button with importantForAccessibility=no"

Reviewed By: cipolleschi

Differential Revision: D38121992

Pulled By: dmitryrykun

fbshipit-source-id: 368b4dcb47d7940274820aa2e39ed5e2ca068821
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
…acebook#34245)

Summary:
Previously published by [grgr-dkrk][2] as [facebook#31296], fixes the following issues:

1) ImportantForAccessibility="no" does not work on Text, Button
2) ImportantForAccessibility="no-hide-descendants" does not work on Text, Button, or ImageBackground.

Note: On ImageBackground, focus is prevented on the imageBackground node itself, but focus is not prevented on its descendants.

Note: [Button component expected behavior for prop importantForAccessibility][4]
>Some components like Button seem like atomic units, but they end up rendering a hierarchy of components for example a wrapper View with a Text inside them. Allowing those descendants to become focusable, breaks the model of these being a single component to consider and forcing no-hide-descendants makes sense here.

>The other option is always to render any descendants of these elements with importantForAccessibility="no", so they can never be focusable on their own. This would have the same result, **BUT may potentially cause issues when the descendant content is supposed to automatically get moved up to the focusable ancestor to act as a label** (which is what Talkback does with unfocusable text elements by default).

Note: [importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images][5]

fixes facebook#30850 related facebook#33690

## Changelog

[Android] [Fixed] - adding importantForAccessibility for Text, Button, ImageBackground

Pull Request resolved: facebook#34245

Test Plan:
1) testing ImageBackground importantForAccessiblity ([link to test][1])
2) importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images ([link to test][5])
3) testing ImageBackground importantForAccessiblity ([link to test][6])
4) Button with importantForAccessibility=no ([link to test][7])

[1]: facebook#31296 (comment) ""
[2]: https://github.com/grgr-dkrk "grgr-dkrk"
[3]: facebook#31296 "facebook#31296"
[4]: facebook#31296 (comment) "expected behaviour with prop importantForAccessibility in Button component"
[5]: facebook#30850 (comment) "importantForAccessibility=no does not allow screenreader focus on nested Text Components with accessibilityRole=link or inline Images"
[6]: facebook#34245 (comment) "testing ImageBackground importantForAccessiblity"
[7]: facebook#34245 (comment) "Button with importantForAccessibility=no"

Reviewed By: cipolleschi

Differential Revision: D38121992

Pulled By: dmitryrykun

fbshipit-source-id: 368b4dcb47d7940274820aa2e39ed5e2ca068821
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility Bug 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. Platform: Android Android applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android: Blocking elements from being focused
6 participants