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

accessibilityLabelledBy use DynamicFromObject to parse String to Dynamic #34371

Closed
wants to merge 3 commits into from

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented Aug 9, 2022

Summary

accessibilityLabelledBy accepts String or Array type.

  • The JavaScript Array type corresponds to java ReadableArray (HybridData)
  • The JavaScript String type corresponds to the java String

Use DynamicFromObject to parse String to Dynamic.

if (nativeId.getType() == ReadableType.String) {
view.setTag(R.id.labelled_by, nativeId.asString());
} else if (nativeId.getType() == ReadableType.Array) {
// On Android, this takes a single View as labeledBy. If an array is specified, set the first
// element in the tag.
view.setTag(R.id.labelled_by, nativeId.asArray().getString(0));
}

All credits to grgr-dkrk (PR #32470). fixes #30846

Changelog

[Android] [Fixed] - accessibilityLabelledBy use DynamicFromObject to parse String to Dynamic

Test Plan

testing accessibilityLabelledBy with TextInput

2022-08-09.15-40-35.mp4

testing accessibilityLabelledBy with Switch

Screen Shot 2022-08-09 at 15 53 37

testing paper renderer after the fix

2022-08-09.16-42-58.mp4

<details><summary>testing accessibilityLabelledBy with TextInput</summary>
<p>

https://user-images.githubusercontent.com/24992535/183593138-7ced1974-6a06-4f0f-822a-1ade1edc7212.mp4

</p>
</details>

<details><summary>testing accessibilityLabelledBy with Switch</summary>
<p>

![Screen Shot 2022-08-09 at 15 53 37](https://user-images.githubusercontent.com/24992535/183596336-4b73186b-6d27-485e-a6ea-29a0f0b9ef50.png)

</p>
</details>
@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 Aug 9, 2022
@react-native-bot react-native-bot added the Platform: Android Android applications. label Aug 9, 2022
@analysis-bot
Copy link

analysis-bot commented Aug 9, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,613,502 +3,257
android hermes armeabi-v7a 7,028,485 +3,496
android hermes x86 7,913,278 +3,135
android hermes x86_64 7,886,904 +3,130
android jsc arm64-v8a 9,489,965 +935
android jsc armeabi-v7a 8,267,840 +1,169
android jsc x86 9,427,334 +825
android jsc x86_64 10,020,086 +826

Base commit: ee33385
Branch: main

@analysis-bot
Copy link

analysis-bot commented Aug 9, 2022

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

Base commit: 333ba59
Branch: main

@fabOnReact fabOnReact marked this pull request as ready for review August 10, 2022 11:16
@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 Aug 10, 2022
@@ -211,6 +212,14 @@ class AccessibilityExample extends React.Component<{}> {
value="Foo"
/>
</View>
<View>
<Text nativeID="formLabel4">Enable Fielld</Text>
Copy link
Member

Choose a reason for hiding this comment

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

Typo: Fielld -> Field

Perhaps we want to put this in its own <RNTesterBlock>, since it is a switch and doesn't act on the preceding fields (naming may be confusing). We could label this instead as a concrete example such as "Enable notifications", or more generically: "Switch example".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@huntie Thanks a lot. I included the changes in commit e181990

Perhaps we want to put this in its own <RNTesterBlock>, since it is a switch and doesn't act on the preceding fields (naming may be confusing). We could label this instead as a concrete example such as "Enable notifications", or more generically: "Switch example".
https://github.com/facebook/react-native/pull/34371/files#r942608407
@facebook-github-bot
Copy link
Contributor

@huntie 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 @fabriziobertoglio1987 in 9f43581.

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 Aug 16, 2022
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
…mic (facebook#34371)

Summary:
`accessibilityLabelledBy` accepts String or Array type.
- The JavaScript Array type corresponds to java [ReadableArray][3] ([HybridData][4])
- The JavaScript String type corresponds to the java String

Use [DynamicFromObject][5] to parse String to Dynamic.

https://github.com/facebook/react-native/blob/e509f96baf5e523301a5c9567c416508ff20d175/ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java#L222-L228

All credits to [grgr-dkrk][1] (PR facebook#32470). fixes [https://github.com/facebook/react-native/issues/30846][2]

## Changelog

[Android] [Fixed] - accessibilityLabelledBy use DynamicFromObject to parse String to Dynamic

Pull Request resolved: facebook#34371

Test Plan:
<details><summary>testing accessibilityLabelledBy with TextInput</summary>
<p>

https://user-images.githubusercontent.com/24992535/183593138-7ced1974-6a06-4f0f-822a-1ade1edc7212.mp4

</p>
</details>

<details><summary>testing accessibilityLabelledBy with Switch</summary>
<p>

![Screen Shot 2022-08-09 at 15 53 37](https://user-images.githubusercontent.com/24992535/183596336-4b73186b-6d27-485e-a6ea-29a0f0b9ef50.png)

</p>
</details>

<details><summary>testing paper renderer after the fix</summary>
<p>

https://user-images.githubusercontent.com/24992535/183605619-01f1be64-788a-43bd-88b0-a7b2cad75148.mp4

</p>
</details>

[1]: https://github.com/grgr-dkrk
[2]: facebook#30846
[3]: https://github.com/facebook/react-native/blob/e509f96baf5e523301a5c9567c416508ff20d175/ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableArray.java#L1
[4]: https://github.com/facebookincubator/fbjni/blob/main/java/com/facebook/jni/HybridData.java
[5]: https://github.com/facebook/react-native/blob/e509f96baf5e523301a5c9567c416508ff20d175/ReactAndroid/src/main/java/com/facebook/react/bridge/DynamicFromObject.java#L74

Reviewed By: lunaleaps

Differential Revision: D38706360

Pulled By: huntie

fbshipit-source-id: e4771552d3fddfad50f4d4cbbf971fe4a718e134
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
…mic (facebook#34371)

Summary:
`accessibilityLabelledBy` accepts String or Array type.
- The JavaScript Array type corresponds to java [ReadableArray][3] ([HybridData][4])
- The JavaScript String type corresponds to the java String

Use [DynamicFromObject][5] to parse String to Dynamic.

https://github.com/facebook/react-native/blob/e509f96baf5e523301a5c9567c416508ff20d175/ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java#L222-L228

All credits to [grgr-dkrk][1] (PR facebook#32470). fixes [https://github.com/facebook/react-native/issues/30846][2]

## Changelog

[Android] [Fixed] - accessibilityLabelledBy use DynamicFromObject to parse String to Dynamic

Pull Request resolved: facebook#34371

Test Plan:
<details><summary>testing accessibilityLabelledBy with TextInput</summary>
<p>

https://user-images.githubusercontent.com/24992535/183593138-7ced1974-6a06-4f0f-822a-1ade1edc7212.mp4

</p>
</details>

<details><summary>testing accessibilityLabelledBy with Switch</summary>
<p>

![Screen Shot 2022-08-09 at 15 53 37](https://user-images.githubusercontent.com/24992535/183596336-4b73186b-6d27-485e-a6ea-29a0f0b9ef50.png)

</p>
</details>

<details><summary>testing paper renderer after the fix</summary>
<p>

https://user-images.githubusercontent.com/24992535/183605619-01f1be64-788a-43bd-88b0-a7b2cad75148.mp4

</p>
</details>

[1]: https://github.com/grgr-dkrk
[2]: facebook#30846
[3]: https://github.com/facebook/react-native/blob/e509f96baf5e523301a5c9567c416508ff20d175/ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableArray.java#L1
[4]: https://github.com/facebookincubator/fbjni/blob/main/java/com/facebook/jni/HybridData.java
[5]: https://github.com/facebook/react-native/blob/e509f96baf5e523301a5c9567c416508ff20d175/ReactAndroid/src/main/java/com/facebook/react/bridge/DynamicFromObject.java#L74

Reviewed By: lunaleaps

Differential Revision: D38706360

Pulled By: huntie

fbshipit-source-id: e4771552d3fddfad50f4d4cbbf971fe4a718e134
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility Bug 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. Platform: Android Android applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android: One element labelling another
5 participants