Skip to content

feat: Add support for "Prefer Cross-Fade Transitions" into AccessibilityInfo - #34406

Closed
gabrieldonadel wants to merge 4 commits into
react:mainfrom
gabrieldonadel:feat/add-prefer-crossfade-transitions
Closed

feat: Add support for "Prefer Cross-Fade Transitions" into AccessibilityInfo#34406
gabrieldonadel wants to merge 4 commits into
react:mainfrom
gabrieldonadel:feat/add-prefer-crossfade-transitions

Conversation

@gabrieldonadel

@gabrieldonadelgabrieldonadel commented Aug 13, 2022

Copy link
Copy Markdown
Collaborator

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

Closesreact-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-botfacebook-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-botreact-native-bot added Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Aug 13, 2022
@analysis-bot

analysis-bot commented Aug 13, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a7,615,477+241
androidhermesarmeabi-v7a7,030,367+148
androidhermesx867,915,854+141
androidhermesx86_647,889,742+54
androidjscarm64-v8a9,494,477+223
androidjscarmeabi-v7a8,272,246+132
androidjscx869,432,442+125
androidjscx86_6410,025,444+50

Base commit: e0a71fc
Branch: main

@analysis-bot

analysis-bot commented Aug 13, 2022

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

Base commit: e0a71fc
Branch: main

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@gabrieldonadel

Copy link
Copy Markdown
CollaboratorAuthor

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

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

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

makovkastar commented Aug 19, 2022

Copy link
Copy Markdown
Contributor

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

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

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

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

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
gabrieldonadelforce-pushed the feat/add-prefer-crossfade-transitions branch from 4ee496c to ff3802dCompareAugust 20, 2022 19:07
@gabrieldonadel
gabrieldonadelforce-pushed the feat/add-prefer-crossfade-transitions branch from ff3802d to 5c81481CompareAugust 20, 2022 19:14
@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@gabrieldonadel

Copy link
Copy Markdown
CollaboratorAuthor

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

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

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 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
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 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.Platform: iOSiOS applications.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.

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

6 participants

@gabrieldonadel@analysis-bot@facebook-github-bot@cortinico@makovkastar@react-native-bot