-
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
Added support for importantForAccessibility for some components #31296
Added support for importantForAccessibility for some components #31296
Conversation
Base commit: 36f3bf2 |
@@ -110,6 +111,12 @@ function PickerAndroid(props: Props): React.Node { | |||
], | |||
); | |||
|
|||
// If `no` is specified for `importantForAccessibility`, it will be changed to `no-hide-descendants` because the Picker.Item should not be focused. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! I just wanted to ask why this is needed? What happens if someone specifies "no" without this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @grgr-dkrk !
@kacieb , this is due to the fact that while some components like <Button>
seem like atomic units, they actually end up rendering a hierarchy of components for example a wrapper <View>
with a <Text>
inside them.
By allowing those descendants to become focusable, it sort of breaks the model of these being a single component to consider, so I think forcing no-hide-descendants makes sense here.
The other option is to always render any descendants of these elements with importantForAccessibility="no", so that they can never be focusable on their own. This would have the same end 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).
Thanks for working on this! cc @blavalla to check the Android behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this approach looks good to me. I am not sure if it makes sense on ImageBackground though. Waiting for @kacieb to clarify the common usage of that component and whether this pattern fits with it or not.
Libraries/Image/ImageBackground.js
Outdated
@@ -57,9 +57,16 @@ class ImageBackground extends React.Component<$FlowFixMeProps> { | |||
render(): React.Node { | |||
const {children, style, imageStyle, imageRef, ...props} = this.props; | |||
|
|||
// If `no` is specified for `importantForAccessibility`, it will be changed to `no-hide-descendants` because the text inside should not be focused. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kacieb , do you think this same logic makes sense for "ImageBackground"? I am not sure where/how this component is commonly used, but if people tend to consider it a wrapper element that does not impact it's children, I don't think this makes sense necessarily.
Basically, this would cause all children passed into this background to not be focusable even if you only wanted the wrapper itself to not be focusable. Since you are passing these children in yourself (unlike button or picker), this doesn't seem to make as much sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on this RNTester example I found, one possible use case of ImageBackground
is to nest content:
So it seems to me like we want to allow children to be able to be focusable, still, or at least allow the React Native user to choose which they'd prefer.
cc @lunaleaps in case you have any additional context on Image that I'm missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry had missed this comment. Agreed, we should offer the flexibility for focussed children elements nested in an ImageBackground. All my context comes from is the docs: https://reactnative.dev/docs/next/imagebackground
Libraries/Image/ImageBackground.js
Outdated
@@ -57,16 +57,9 @@ class ImageBackground extends React.Component<$FlowFixMeProps> { | |||
render(): React.Node { | |||
const {children, style, imageStyle, imageRef, ...props} = this.props; | |||
|
|||
// If `no` is specified for `importantForAccessibility`, it will be changed to `no-hide-descendants` because the text inside should not be focused. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't importantForAccessibility no longer work at all on ImageBackgrounds now?
I think we should still have this property, it just should work like normal where no = no and no_hide_descendants = no_hide_descendants.
If this is magically being inherited from somewhere else and it does work still, feel free to ignore my comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I seem to have misunderstood.
ImageBackgrounds' importantForAccessibility is given to the Image inside, not the Wrapper itself, and it doesn't seem to work because of that.
I will fix this. Thanks!
@grgr-dkrk Is this ready for review again? If not, can you request for review when it is? Or let me know if you have already (sorry a little disoriented in github!) -- just want it to be clear when you're waiting on review! |
@lunaleaps It slipped my mind. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comment about the Picker component snapshot tests, but otherwise looks good to me!
@@ -1,5 +1,201 @@ | |||
// Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|||
exports[`<Picker /> should be set importantForAccessibility={no-hide-descendants} when importantForAccessibility={no}: should deep render when mocked (please verify output manually) 1`] = ` | |||
<View> | |||
<RCTPicker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see anything about the importantForAccessibility
prop here in the snapshot tests, which is unexpected. Does this snapshot need to be updated? Or perhaps we should allow this to be mocked to ensure importantForAccessibility is being set correctly? It seems like right now this test will not fail if the internal behavior is changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kacieb
I was taking a snapshot of iOS, but since this feature is for Android only, I decided to add the test only to the Android components. Thanks!
Base commit: 36f3bf2 |
I will invest time reviewing this pull request and I will add my comments in this pull request. Thanks a lot. |
…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 #30850 related #33690 ## Changelog [Android] [Fixed] - adding importantForAccessibility for Text, Button, ImageBackground Pull Request resolved: #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]: #31296 (comment) "" [2]: https://github.com/grgr-dkrk "grgr-dkrk" [3]: #31296 "#31296" [4]: #31296 (comment) "expected behaviour with prop importantForAccessibility in Button component" [5]: #30850 (comment) "importantForAccessibility=no does not allow screenreader focus on nested Text Components with accessibilityRole=link or inline Images" [6]: #34245 (comment) "testing ImageBackground importantForAccessiblity" [7]: #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
…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
These changes landed in the main branch with #34245. For this reason I'm moving the PR to done. Thanks a lot. |
|
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
This PR was closed because it has been stalled for 7 days with no activity. |
Summary
fix #30850
This PR supports
importantForAccessibility
on some components (Picker, Button, ImageBackground).These components convert
importantForAccessibility="no"
toimportantForAccessibility="no-hide-descendants"
.This is to prevent focusing on the offspring elements of the component.
Picker is a deprecated component, so I will also work with @react-native-picker/picker if this PR is accepted.
Changelog
[Android] [Fixed] - Added support for importantForAccessibility for some components(Button, Picker, and ImageBackground)
Test Plan