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

feat : add aria labelled as alias for accessibilityLabelledBy #34725

Closed

Conversation

dakshbhardwaj
Copy link
Contributor

@dakshbhardwaj dakshbhardwaj commented Sep 19, 2022

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

  • Enable talkback.
  • Open the RNTester app and navigate to the Api's tab
  • Go to the TextInput with aria-labelledby attribute section

Screenshot 2022-09-19 at 7 46 05 PM

@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 Sep 19, 2022
@github-actions
Copy link

github-actions bot commented Sep 19, 2022

Warnings
⚠️ 📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.
⚠️

packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js#L108 - packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js line 108 – Inline style: { color: 'green' } (react-native/no-inline-styles)

⚠️

packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js#L109 - packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js line 109 – Inline style: { color: 'blue' } (react-native/no-inline-styles)

⚠️

packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js#L115 - packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js line 115 – Inline style: { color: 'green' } (react-native/no-inline-styles)

⚠️

packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js#L116 - packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js line 116 – Inline style: { color: 'blue' } (react-native/no-inline-styles)

⚠️

packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js#L124 - packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js line 124 – Inline style: { color: 'green' } (react-native/no-inline-styles)

⚠️

packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js#L125 - packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js line 125 – Inline style: { color: 'blue' } (react-native/no-inline-styles)

⚠️

packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js#L141 - packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js line 141 – Inline style: { color: 'green' } (react-native/no-inline-styles)

⚠️

packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js#L142 - packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js line 142 – Inline style: { color: 'blue' } (react-native/no-inline-styles)

⚠️

packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js#L151 - packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js line 151 – Inline style: { color: 'green' } (react-native/no-inline-styles)

⚠️

packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js#L152 - packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js line 152 – Inline style: { color: 'blue' } (react-native/no-inline-styles)

⚠️

packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js#L391 - packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js line 391 – Inline style: { color: 'white' } (react-native/no-inline-styles)

⚠️

packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js#L517 - packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js line 517 – Inline style: { flex: 1, flexDirection: 'row' } (react-native/no-inline-styles)

⚠️

packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js#L536 - packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js line 536 – Inline style: { flex: 1, flexDirection: 'row' } (react-native/no-inline-styles)

⚠️

packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js#L550 - packages/rn-tester/js/examples/Accessibility/AccessibilityExample.js line 550 – Inline style: { flex: 1, flexDirection: 'row' } (react-native/no-inline-styles)

Generated by 🚫 dangerJS against 2d479e4

@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 Sep 19, 2022
@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Sep 19, 2022
@analysis-bot
Copy link

analysis-bot commented Sep 19, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,768,800 +293
android hermes armeabi-v7a 7,169,747 +284
android hermes x86 8,081,946 +299
android hermes x86_64 8,053,438 +288
android jsc arm64-v8a 9,629,465 +239
android jsc armeabi-v7a 8,393,889 +233
android jsc x86 9,578,879 +237
android jsc x86_64 10,171,989 +238

Base commit: fa22a6e
Branch: main

@analysis-bot
Copy link

analysis-bot commented Sep 19, 2022

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

Base commit: cb3a5cc
Branch: main

Copy link
Contributor

@necolas necolas left a 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.

Libraries/Components/View/View.js Show resolved Hide resolved
@dakshbhardwaj dakshbhardwaj changed the title Daksh/add aria labelled by prop feat : add aria labelled as alias for accessibilityLabelledBy Sep 19, 2022
*
* @platform android
*/
'aria-labelledby'?: ?string | ?Array<string>,
Copy link
Contributor

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

@cipolleschi
Copy link
Contributor

Hi @dakshbhardwaj! Thank you for taking this. Could you please address @necolas feedback? 🙏

@dakshbhardwaj
Copy link
Contributor Author

@necolas @cipolleschi I have pushed down the changes

'aria-valuemin': ariaValueMin,
'aria-valuenow': ariaValueNow,
'aria-valuetext': ariaValueText,
...otherProps
Copy link
Contributor

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'}
Copy link
Contributor

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?

Copy link
Collaborator

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh good to know

Copy link
Collaborator

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

@necolas
Copy link
Contributor

necolas commented Oct 6, 2022

ping @cipolleschi

@facebook-github-bot
Copy link
Contributor

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

@cipolleschi
Copy link
Contributor

/rebase

@facebook-github-bot
Copy link
Contributor

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @dakshbhardwaj in f353119.

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 Oct 11, 2022
facebook-github-bot pushed a commit that referenced this pull request Oct 11, 2022
Summary: Changelog: [Internal] Add `aria-labelledby` to TS events from changes in #34725

Differential Revision: D40239207

fbshipit-source-id: 27832037bf1de74c2b86c04521b9baaa60c99d44
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…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
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary: Changelog: [Internal] Add `aria-labelledby` to TS events from changes in facebook#34725

Differential Revision: D40239207

fbshipit-source-id: 27832037bf1de74c2b86c04521b9baaa60c99d44
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. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants