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

[iOS] - Display Alert correctly if app is using iOS13 Scenes API. #30740

Closed
wants to merge 1 commit into from

Conversation

hellostu
Copy link

@hellostu hellostu commented Jan 14, 2021

Summary

UIApplication.keyWindow is deprecated. This is only a problem if an app is using the Scenes API. An app includes Scenes to display multiple windows and/or to enable the latest CarPlay. Using this API breaks Alerts. RCTAlertController uses UIApplication.keyWindow to display an alert. It does not display if using the Scenes API. This PR fixes this issue by doing a check for iOS13 and use of the Scenes API. It then grabs the foreground window. It falls back to the previous implementation in the else case.

Changelog

[iOS] [Fixed] - Display Alert correctly if app is using iOS13 Scenes API.

Test Plan

See React Native test app to demonstrate the fix below:
https://github.com/mixcloud/SceneApiAlertTest

@facebook-github-bot
Copy link
Contributor

Hi @hellostu!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@hellostu hellostu changed the title [iOS] - Support iOS13 App using Scenes Api [iOS] - Display Alert correctly if app is using iOS13 Scenes API. Jan 14, 2021
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Jan 14, 2021
@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 Jan 14, 2021
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@analysis-bot
Copy link

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

Base commit: fc0b3fa

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,896,629 -23
android hermes armeabi-v7a 8,394,358 -29
android hermes x86 9,384,925 -21
android hermes x86_64 9,330,063 -23
android jsc arm64-v8a 10,348,457 +0
android jsc armeabi-v7a 9,829,278 +0
android jsc x86 10,398,251 +0
android jsc x86_64 10,983,646 +0

Base commit: fc0b3fa

@sammy-SC
Copy link
Contributor

sammy-SC commented Feb 6, 2021

Hello @hellostu

could you please add photos that demonstrate the fix? Something that shows broken AlertController and then fixed.

Thank you.

@gaodeng
Copy link
Contributor

gaodeng commented Jun 17, 2021

anynews ?

@hellostu
Copy link
Author

hellostu commented Jun 17, 2021

Hey @sammy-SC. Thanks for getting back to me and sorry for the late reply. If you are using the Scenes framework, then keyWindow doesn't work, which is react's mechanism for displaying an Alert. So this project demonstrates the fix:
https://github.com/mixcloud/SceneApiAlertTest

If you enable the Scenes framework on an existing project (as in the link above) then alerts simply don't display. This change would fix that by detecting the use of that framework and pointing to the correct UIWindow.

I'm not sure how best to demo it in an image?

@sota000 sota000 added this to Needs Triage in React Native Pull Requests via automation Jul 27, 2021
@sammy-SC
Copy link
Contributor

@hellostu I think a video would demo it best.

Something like:
Trying to present alert controller with Scenes framework enabled

  • Include video before
  • Include video after

Including code used to record the video is also helpful.

@github-actions
Copy link

This PR is waiting for author's feedback since 24 days. Please provide the requested feedback or this will be closed in 7 days

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Mar 14, 2023
@github-actions
Copy link

This PR was closed because the author hasn't provided the requested feedback after 7 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Needs: Author Feedback Platform: iOS iOS applications. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants