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 multiple windows scenario #1526

Closed

Conversation

alexbumbu
Copy link
Contributor

Using RNSFullWindowOverlay in multiple windows scenarios is causing weird presentation issues and it's causing a crash for applications which use UISceneDelegate. This is a fix I propose for these issues.
For fixing this I ended up using keyWindow, even though it's deprecated starting with iOS 13. After doing some research I found that filtering for the foreground active scene, and retrieve the keyWindow this way won't cover all the cases (more details in this post: https://teng.pub/technical/2021/11/9/uiapplication-key-window-replacement).

@WoLewicki
Copy link
Member

Seems that RN also uses this prop in their implementation: https://github.com/facebook/react-native/blob/8bd3edec88148d0ab1f225d2119435681fbbba33/React/CoreModules/RCTDevLoadingView.mm#L117 so I think it is ok to do it like that. Did you test if it does not destroy any of the current behaviors (https://github.com/software-mansion/react-native-screens/blob/cc5de7ba28e90ecce9bbb3464ef0a625eaae032f/TestsExample/src/Test1096.tsx is good for testing it)? If so, I think we can merge it.

@tboba
Copy link
Member

tboba commented Feb 14, 2024

Hi @alexbumbu, thanks for creating this PR!

I think your solution is great, but better solution would be to choose the keyWindow from RCTKeyWindow(). I have created a separate PR for that (just to match the description with our template and not ingerent into your changes). If you still have a reproducer that shows the scenario with multiple windows, could you check if this PR works properly for you?

@tboba tboba closed this in #2031 Apr 11, 2024
tboba added a commit that referenced this pull request Apr 11, 2024
## Description

This PR is a follow up for the PR #1526.

Currently, in React Native there's `RCTKeyWindow()` method which goes
through each window of `RCTSharedApplication()` and selects the window
that is a key window of a given window. This is a better solution, since
we don't rely on a single `keyWindow` from RCTSharedApplication.
I've also added a support for Fabric.

I've checked how does FullWindowOverlay work with the change and it
works as usual.
Closes #1526.

## Changes

- Changed the method of retrieving window from `delegate.window` to
`RCTKeyWindow()`

## Test code and steps to reproduce

You can run `Test1844` and check if FullWindowOverlay works correctly.

## Checklist

- [X] Included code example that can be used to test this change
- [X] Ensured that CI passes

Co-authored-by: alexbumbu <[email protected]>
ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this pull request Oct 9, 2024
…sion#2031)

## Description

This PR is a follow up for the PR software-mansion#1526.

Currently, in React Native there's `RCTKeyWindow()` method which goes
through each window of `RCTSharedApplication()` and selects the window
that is a key window of a given window. This is a better solution, since
we don't rely on a single `keyWindow` from RCTSharedApplication.
I've also added a support for Fabric.

I've checked how does FullWindowOverlay work with the change and it
works as usual.
Closes software-mansion#1526.

## Changes

- Changed the method of retrieving window from `delegate.window` to
`RCTKeyWindow()`

## Test code and steps to reproduce

You can run `Test1844` and check if FullWindowOverlay works correctly.

## Checklist

- [X] Included code example that can be used to test this change
- [X] Ensured that CI passes

Co-authored-by: alexbumbu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants