Skip to content

feat : add aria labelled as alias for accessibilityLabelledBy - #34725

Closed
dakshbhardwaj wants to merge 11 commits into
react:mainfrom
dakshbhardwaj:daksh/add-aria-labelledBy-prop
Closed

feat : add aria labelled as alias for accessibilityLabelledBy#34725
dakshbhardwaj wants to merge 11 commits into
react:mainfrom
dakshbhardwaj:daksh/add-aria-labelledBy-prop

Conversation

@dakshbhardwaj

@dakshbhardwajdakshbhardwaj commented Sep 19, 2022

Copy link
Copy Markdown
Contributor

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-botfacebook-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

github-actionsBot commented Sep 19, 2022

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

analysis-bot commented Sep 19, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a7,768,800+293
androidhermesarmeabi-v7a7,169,747+284
androidhermesx868,081,946+299
androidhermesx86_648,053,438+288
androidjscarm64-v8a9,629,465+239
androidjscarmeabi-v7a8,393,889+233
androidjscx869,578,879+237
androidjscx86_6410,171,989+238

Base commit: fa22a6e
Branch: main

@analysis-bot

analysis-bot commented Sep 19, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: cb3a5cc
Branch: main

@necolasnecolas left a comment

Copy link
Copy Markdown

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.

Comment threadLibraries/Components/View/View.js
@dakshbhardwajdakshbhardwaj changed the title Daksh/add aria labelled by propfeat : add aria labelled as alias for accessibilityLabelledBySep 19, 2022
*
* @platform android
*/
'aria-labelledby'?: ?string | ?Array<string>,

Copy link
Copy Markdown

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
Copy Markdown
Contributor

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

@dakshbhardwaj

Copy link
Copy Markdown
ContributorAuthor

@necolas@cipolleschi I have pushed down the changes

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

Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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

necolas commented Oct 6, 2022

Copy link
Copy Markdown

ping @cipolleschi

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@cipolleschi

Copy link
Copy Markdown
Contributor

/rebase

@facebook-github-bot

Copy link
Copy Markdown
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
Copy Markdown
Collaborator

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

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

@react-native-botreact-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
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Shared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.Type: EnhancementA new feature or enhancement of an existing feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@dakshbhardwaj@analysis-bot@cipolleschi@necolas@facebook-github-bot@react-native-bot@gabrieldonadel