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 support for "Prefer Cross-Fade Transitions" into AccessibilityInfo #34406

Conversation

gabrieldonadel
Copy link
Collaborator

@gabrieldonadel gabrieldonadel commented Aug 13, 2022

Summary

This PR adds prefersCrossFadeTransitions() to AccessibilityInfo in order to add support for "Prefer Cross-Fade Transitions", exposing the iOS settings option as proposed here react-native-community/discussions-and-proposals#452.
I believe this would be especially helpful for solving #31484

Closes react-native-community/discussions-and-proposals#452

TODO

Changelog

[iOS] [Added] - Add support for "Prefer Cross-Fade Transitions" into AccessibilityInfo

Test Plan

On iOS 14+

  1. Access Settings > "General" > "Accessibility" > "Reduce Motion", enable "Reduce Motion" then enable "Prefer Cross-Fade Transitions".
  2. Open the RNTester app and navigate to the Accessibility page
Screen.Recording.2022-02-17.at.20.23.42.mov

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Aug 13, 2022
@react-native-bot react-native-bot added Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Aug 13, 2022
@analysis-bot
Copy link

analysis-bot commented Aug 13, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,615,477 +241
android hermes armeabi-v7a 7,030,367 +148
android hermes x86 7,915,854 +141
android hermes x86_64 7,889,742 +54
android jsc arm64-v8a 9,494,477 +223
android jsc armeabi-v7a 8,272,246 +132
android jsc x86 9,432,442 +125
android jsc x86_64 10,025,444 +50

Base commit: e0a71fc
Branch: main

@analysis-bot
Copy link

analysis-bot commented Aug 13, 2022

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

Base commit: e0a71fc
Branch: main

@facebook-github-bot
Copy link
Contributor

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

@gabrieldonadel
Copy link
Collaborator Author

Hi @cortinico I see that the Facebook Internal - Linter action raised 1 warning, do you mind checking if there is something I need to update/change?

@cortinico
Copy link
Contributor

Hi @cortinico I see that the Facebook Internal - Linter action raised 1 warning, do you mind checking if there is something I need to update/change?

@makovkastar is taking care of this PR and will get back to you in the near future. We might need to some adjustments to make this check green.

<DisplayOptionStatusExample
optionName={'Prefer Cross-Fade Transitions'}
optionChecker={AccessibilityInfo.prefersCrossFadeTransitions}
notification={'prefersCrossFadeTransitionsChanged'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add prefersCrossFadeTransitionsChanged to AccessibilityEventDefinitionsIOS inside AccessibilityInfo.js?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@makovkastar I'm not sure, UIKit has this bug that causes UIAccessibilityPrefersCrossFadeTransitionsStatusDidChangeNotification to not get triggered when Prefers Cross-Fade Transitions changes, I reported this bug on Apple Developer Forums a couple months ago but they haven't fixed this yet, that's also the reason why I've removed support for this here -> 71632b4.

AccessibilityInfo.prefersCrossFadeTransitions should be enough to fix #31484 though, at least while apple doesn't fix this bug on UIKit

@makovkastar
Copy link
Contributor

makovkastar commented Aug 19, 2022

Hi @gabrieldonadel, thanks for your patience here. In order to make the internal Linter happy, we need to make the getCurrentPrefersCrossFadeTransitionsState() method optional in NativeAccessibilityManager.js. Then add a null check in AccessibilityInfo.js to make sure that the method exists before calling it.

@gabrieldonadel
Copy link
Collaborator Author

Hi @gabrieldonadel, thanks for your patience here. In order to make the internal Linter happy, we need to make the getCurrentPrefersCrossFadeTransitionsState() method optional in NativeAccessibilityManager.js. Then add a null check in AccessibilityInfo.js to make sure that the method exists before calling it.

Got it, thanks for reviewing this @makovkastar, I've just pushed a commit making getCurrentPrefersCrossFadeTransitionsState optional

@facebook-github-bot
Copy link
Contributor

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

*
* See https://reactnative.dev/docs/accessibilityinfo#prefersCrossFadeTransitions
*/
prefersCrossFadeTransitions(): Promise<boolean> {
Copy link
Contributor

Choose a reason for hiding this comment

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

AccessibilityInfo is typed with AccessibilityInfoType that is defined here: https://github.com/facebook/react-native/blob/main/Libraries/Components/AccessibilityInfo/AccessibilityInfo.flow.js.

@gabrieldonadel you need to add this method to the interface, otherwise Flow is complaining about the missing property.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ohh I see, seems that this file was recently added and my branch was out of date, I've just added this AccessibilityInfo.flow.js.

@gabrieldonadel gabrieldonadel force-pushed the feat/add-prefer-crossfade-transitions branch from 4ee496c to ff3802d Compare August 20, 2022 19:07
@facebook-github-bot
Copy link
Contributor

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

@gabrieldonadel
Copy link
Collaborator Author

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

@makovkastar seems that the linter check failed again, do you mind checking what is causing it?

@makovkastar
Copy link
Contributor

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

@makovkastar seems that the linter check failed again, do you mind checking what is causing it?

There was an unrelated internal breakage, I'm rebasing this PR past the fix.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @gabrieldonadel in be7c50f.

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 25, 2022
facebook-github-bot pushed a commit that referenced this pull request Aug 26, 2022
Summary:
While working on a fix for #29974 (I have a draft for that already (gabrieldonadel#16), just waiting for #34406 to get merged) I noticed that the `KeyboardAvoidingView` tests on RNTester on iOS were not working with Fabric enabled because the `ModalHostView` component was still not implemented. Upon some more investigation, I found this code suggestion from Milker90 (#33652 (comment)) that enables the Modal component on iOS when Fabric is enabled.

Closes #33652

## Changelog

[iOS] [Added] - Add support for Modal on iOS when Fabric is enabled

Pull Request resolved: #34487

Test Plan:
1. With Fabric enabled open the RNTester app and navigate to the Modal page
2. Test the `Modal` component through the sections changing props

https://user-images.githubusercontent.com/11707729/186289099-5223907d-b300-46bf-bfde-73259c29d544.mov

Reviewed By: christophpurrer

Differential Revision: D38981895

Pulled By: cipolleschi

fbshipit-source-id: cd493a8d2035900da2576323bc112e2565df4834
raykle pushed a commit to raykle/react-native that referenced this pull request Aug 27, 2022
…4487)

Summary:
While working on a fix for facebook#29974 (I have a draft for that already (gabrieldonadel#16), just waiting for facebook#34406 to get merged) I noticed that the `KeyboardAvoidingView` tests on RNTester on iOS were not working with Fabric enabled because the `ModalHostView` component was still not implemented. Upon some more investigation, I found this code suggestion from Milker90 (facebook#33652 (comment)) that enables the Modal component on iOS when Fabric is enabled.

Closes facebook#33652

## Changelog

[iOS] [Added] - Add support for Modal on iOS when Fabric is enabled

Pull Request resolved: facebook#34487

Test Plan:
1. With Fabric enabled open the RNTester app and navigate to the Modal page
2. Test the `Modal` component through the sections changing props

https://user-images.githubusercontent.com/11707729/186289099-5223907d-b300-46bf-bfde-73259c29d544.mov

Reviewed By: christophpurrer

Differential Revision: D38981895

Pulled By: cipolleschi

fbshipit-source-id: cd493a8d2035900da2576323bc112e2565df4834
kelset pushed a commit that referenced this pull request Oct 3, 2022
…ityInfo (#34406)

Summary:
This PR adds `prefersCrossFadeTransitions()` to AccessibilityInfo in order to add support for "Prefer Cross-Fade Transitions", exposing the iOS settings option as proposed here react-native-community/discussions-and-proposals#452.
I believe this would be especially helpful for solving #31484

#### TODO
- [ ]  Submit react-native-web PR updating AccessibilityInfo documentation.

## Changelog

[iOS] [Added] - Add support for "Prefer Cross-Fade Transitions" into AccessibilityInfo

Pull Request resolved: #34406

Test Plan:
**On iOS 14+**

1.  Access Settings > "General" > "Accessibility" > "Reduce Motion", enable "Reduce Motion" then enable "Prefer Cross-Fade Transitions".
2. Open the RNTester app and navigate to the Accessibility page

https://user-images.githubusercontent.com/11707729/154588402-7d050858-3c2d-4d86-9585-928b8c66941b.mov

Reviewed By: cipolleschi

Differential Revision: D38711316

Pulled By: makovkastar

fbshipit-source-id: b9965cd4285f1aa0f1fa927080370a22329c2f62
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. Platform: iOS iOS applications. 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.

Proposal to add support for "Prefer Cross-Fade Transitions" into AccessibilityInfo
6 participants