-
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
feat : add aria labelled as alias for accessibilityLabelledBy #34725
feat : add aria labelled as alias for accessibilityLabelledBy #34725
Conversation
|
Base commit: fa22a6e |
Base commit: cb3a5cc |
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.
Let's add unit test snapshots because a number of these new props look like they are being overridden by the "restProps".
This prop should also be added to TextInput, Image, and Text. If there's no support for the underlying prop in RN, just adding the types will be enough.
* | ||
* @platform android | ||
*/ | ||
'aria-labelledby'?: ?string | ?Array<string>, |
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.
Array of string isn't the web type. It would be comma separated IDs in a single string
Hi @dakshbhardwaj! Thank you for taking this. Could you please address @necolas feedback? 🙏 |
@necolas @cipolleschi I have pushed down the changes |
Libraries/Components/View/View.js
Outdated
'aria-valuemin': ariaValueMin, | ||
'aria-valuenow': ariaValueNow, | ||
'aria-valuetext': ariaValueText, | ||
...otherProps |
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.
Please can we alpha-sort props so it's easier to see the aria props grouped together etc. Same for the props applied to the native component. I'm surprised this isn't a lint rule.
<View> | ||
<Text nativeID="testAriaLabelledBy">Phone Number</Text> | ||
<TextInput | ||
aria-labelledby={'testAriaLabelledBy'} |
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.
Where is the support for this implemented in TextInput, Text, Image components?
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.
@necolas I was checking this to update the docs and it seems that Text
doesn't ever support accessibilityLabelledBy
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.
Oh good to know
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've just opened a PR removing it, #35327
ping @cipolleschi |
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
/rebase |
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This pull request was successfully merged by @dakshbhardwaj in f353119. When will my fix make it into a release? | Upcoming Releases |
Summary: Changelog: [Internal] Add `aria-labelledby` to TS events from changes in #34725 Differential Revision: D40239207 fbshipit-source-id: 27832037bf1de74c2b86c04521b9baaa60c99d44
…ok#34725) Summary: This adds the [aria-labelledby](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-labelledby) prop to the components where it's used as requested on facebook#34424, equivalent [accessibilityLabelledBy](https://reactnative.dev/docs/accessibility#accessibilitylabelledby-android) ## Changelog [General] [Added] - Add aria-modal prop to basic component ## TestPlan - Enable talkback. - Open the RNTester app and navigate to the Api's tab - Go to the TextInput with aria-labelledby attribute section <img width="495" alt="Screenshot 2022-09-19 at 7 46 05 PM" src="https://user-images.githubusercontent.com/22423684/191038924-017dba93-ea7d-494d-ba6f-161e986f9b54.png"> Pull Request resolved: facebook#34725 Reviewed By: lunaleaps Differential Revision: D40176143 Pulled By: lunaleaps fbshipit-source-id: 003d1ab27bfd01b5c6d4c58a4de501ec7966359d
Summary: Changelog: [Internal] Add `aria-labelledby` to TS events from changes in facebook#34725 Differential Revision: D40239207 fbshipit-source-id: 27832037bf1de74c2b86c04521b9baaa60c99d44
Summary
This adds the aria-labelledby prop to the components where it's used as requested on #34424, equivalent accessibilityLabelledBy
Changelog
[General] [Added] - Add aria-modal prop to basic component
TestPlan