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

fix(iOS): remove alert's window when call to hide. #32833

Closed

Conversation

asafkorem
Copy link
Contributor

@asafkorem asafkorem commented Jan 5, 2022

Summary

Resolves this issue: #32304.
NOTE: This PR is based on a prior PR for this fix: #32305, I've co-authorized its creator for this change (@paddlefish).

Without this change, calling to hide an alert, leaves a UIWindow that blocks user interactions with the screen.

The correct way to remove a UIWindow in iOS is to set its hidden property to YES. Also, it is required to remove all references to the window (the associated windowScene for example) and ARC will automatically free this UIWindow.

The line after this change, set the _alertWindow reference to nil, but the window is already associated with a scene (see the screenshots from this PR). So we also need to remove the windowScene from that window, as recommended by Apple: https://developer.apple.com/documentation/uikit/uiwindowscene/3198091-windows.

To remove the window from the current scene, or move it to a different scene, change the value of the window's windowScene property.

Changelog

[iOS] [Fixed] - remove alert's window when call to hide.

Test Plan

See #32305

@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 5, 2022
@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 Jan 5, 2022
@asafkorem asafkorem closed this Jan 5, 2022
@asafkorem asafkorem reopened this Jan 5, 2022
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Jan 5, 2022
@asafkorem asafkorem force-pushed the feature/remove-windowscene branch from 2084c55 to 85fa844 Compare January 5, 2022 15:56
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,290,027 +0
android hermes armeabi-v7a 7,625,041 +0
android hermes x86 8,762,437 +0
android hermes x86_64 8,699,432 +0
android jsc arm64-v8a 9,678,332 +0
android jsc armeabi-v7a 8,670,039 +0
android jsc x86 9,636,232 +0
android jsc x86_64 10,230,830 +0

Base commit: 681ed40
Branch: main

Resolves this issue: facebook#32304.

Without this change, calling to hide an alert, leaves a `UIWindow`
that blocks user interactions with the screen.

The correct way to remove a `UIWindow` in iOS is to set its
hidden property to `YES`.

Also, it is required to remove all references to the window (the
associated `windowScene` for example) and ARC will automatically
free this `UIWindow`.

Co-authored-by: paddlefish <paddlefish@users.noreply.github.com>
@asafkorem asafkorem force-pushed the feature/remove-windowscene branch from 85fa844 to 3767095 Compare January 5, 2022 17:09
@analysis-bot
Copy link

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

Base commit: 681ed40
Branch: main

@paddlefish
Copy link
Contributor

Thanks, that looks like a good addition!

@asafkorem
Copy link
Contributor Author

asafkorem commented Jan 6, 2022

Thanks @paddlefish!

@lunaleaps, can you please take a look at this change? We will also appreciate if it can be pushed as a patch-release for RN v.66. 🙏🏼
This is highly important because this bug breaks UI tests (with Detox).

@lunaleaps
Copy link
Contributor

lunaleaps commented Jan 6, 2022

Does this mean we're dropping #32305 for this change?

@asafkorem
Copy link
Contributor Author

Yes @lunaleaps

@facebook-github-bot
Copy link
Contributor

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

@lunaleaps lunaleaps requested a review from philIip January 7, 2022 19:25
@lunaleaps
Copy link
Contributor

Just FYI for me and anyone watching this -- some ongoing discussion still happening on old PR: #32305 (comment)

@asafkorem
Copy link
Contributor Author

asafkorem commented Jan 11, 2022

@lunaleaps @philIip
Hi guys, let's reach an agreement / compromise regarding the ongoing discussion #32305 (comment) please 🙏🏼

I would greatly appreciate if we could move faster with this bug fix.

I believe that adding the removeFromSuperview should not block us here. I can add this in that PR, or not, and we can continue the discussion over these changes.

@asafkorem
Copy link
Contributor Author

Hey @philIip @lunaleaps, is there anything that blocks us from merging this bug fix?

@lunaleaps
Copy link
Contributor

I got internal approval, will be landing this later today or tomorrow.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @asafkorem in a46a99e.

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 Jan 19, 2022
@asafkorem
Copy link
Contributor Author

Thanks @lunaleaps! 🙂

@asafkorem
Copy link
Contributor Author

@lunaleaps Are you planning on releasing it in a patch-release soon? (we would greatly appreciate it!)

lunaleaps pushed a commit that referenced this pull request Jan 20, 2022
Summary:
Resolves this issue: #32304.
**NOTE:** This PR is based on a prior PR for this fix: #32305, I've co-authorized its creator for this change (paddlefish).

Without this change, calling to hide an alert, leaves a `UIWindow` that blocks user interactions with the screen.

The correct way to remove a `UIWindow` in iOS is to set its hidden property to `YES`. Also, it is required to remove all references to the window (the associated `windowScene` for example) and ARC will automatically free this `UIWindow`.

The line after this change, set the `_alertWindow` reference to `nil`, but the window is already associated with a scene (see the screenshots from [this PR](#32305 (comment))). So we also need to remove the `windowScene` from that window, as recommended by Apple: https://developer.apple.com/documentation/uikit/uiwindowscene/3198091-windows.
>To remove the window from the current scene, or move it to a different scene, change the value of the window's windowScene property.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - remove alert's window when call to `hide`.

Pull Request resolved: #32833

Test Plan: See #32305

Reviewed By: hramos

Differential Revision: D33460430

Pulled By: lunaleaps

fbshipit-source-id: b13c2c7ee6404f1e1c787265bc4af8a31005bcf1
@asafkorem asafkorem deleted the feature/remove-windowscene branch January 25, 2022 09:33
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. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants