-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Conversation
Base commit: a70354d |
|
Base commit: a70354d |
Button with importantForAccessibility=no
2022-07-22.18-47-03.mp4 |
https://app.circleci.com/pipelines/github/facebook/react-native/14331/workflows/8382dcf1-9a0a-44d1-bcbd-2619d133e16a/jobs/267919 https://app.circleci.com/pipelines/github/facebook/react-native/14331/workflows/5cc16b5c-f4df-4cb4-a70d-fed9ab7a57e8/jobs/267921 https://app.circleci.com/pipelines/github/facebook/react-native/14331/workflows/5cc16b5c-f4df-4cb4-a70d-fed9ab7a57e8/jobs/267914 https://app.circleci.com/pipelines/github/facebook/react-native/14331/workflows/5cc16b5c-f4df-4cb4-a70d-fed9ab7a57e8/jobs/267918
@dmitryrykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@dmitryrykun Thanks for importing the pull request. Could you share the warning message that makes the PR fail internally? 🙏 |
Ah I think it's the incorrect license for ImageBackground-test.js -- we can fix that internally |
This pull request was successfully merged by @fabriziobertoglio1987 in 62021eb. When will my fix make it into a release? | Upcoming Releases |
…acebook#34245) Summary: Previously published by [grgr-dkrk][2] as [https://github.com/facebook/react-native/issues/31296][3], 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
…acebook#34245) Summary: Previously published by [grgr-dkrk][2] as [https://github.com/facebook/react-native/issues/31296][3], 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
Summary
Previously published by grgr-dkrk as #31296, fixes the following issues:
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
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