feat: refactor `RCTKeyWindow` to be more resilient and work in multi-window apps #42036

okwasniewski · 2023-12-21T14:14:05Z

Summary:

This PRs refactors RCTKeyWindow() to be more resilient and work in multi-window apps. After my recent PR got merged (#41935) it significantly reduced the number of calls to RCTKeyWindow() and now it's called only when necessary. So this PR makes this function a bit more resource intensive but it guarantees that we will find current scene's key window.

This would also fix some brownfield scenarios where React Native is working in multi-window mode and in the future allow us to more easily adopt UIWindowSceneDelegate

Changelog:

[IOS] [CHANGED] - refactor RCTKeyWindow to be more resilient and work in multi-window apps

Test Plan:

Checkout RNTester example for Alerts and LoadingView.

rct-key-window-refactorConverted.mp4

cipolleschi

Looks good to me, but from a small details.
Can you check that?

cipolleschi · 2023-12-27T16:18:38Z

packages/react-native/React/CoreModules/RCTAlertController.mm

-        NSLog(@"Unable to create alert window: keyWindow is nil");
-      }
-    }
+    _alertWindow = [[UIWindow alloc] initWithWindowScene:RCTKeyWindow().windowScene];


The previous check was also checking that the scene activation state was active.
can the windowScene retrieved by RCTKeyWindow be inactive (e.g.: there is already another alert presented)?

I've updated the function to take the activation state into account. As, we should always get the currently focused window. With the previous approach it was possible to retrieve some inactive keyWindow of additional scene (for example created by the OS). Can you check if the internal tests are passing now?

facebook-github-bot · 2023-12-27T16:35:40Z

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

cipolleschi · 2023-12-28T11:12:47Z

On top of the linting issue, we have several tests that are failing internally when they try to present a picker. :(
I'll have to invest some more time to understand why they are failing.

okwasniewski · 2023-12-28T13:03:02Z

Thanks for letting me know, I'm on PTO until next week, when I will be back I will check whether we need the active state check and maybe that would resolve the issue with failing internal tests.

Can you provide me a minimal reproduction of the test that is failing? Is it not finding the current window or something else is happening?

cipolleschi · 2023-12-28T15:24:29Z

Can you provide me a minimal reproduction of the test that is failing? Is it not finding the current window or something else is happening?

This is tricky. I don't even know where they are failing as our infra is quite complex. I'll need more time to investigate and understand what happens. 😞

…ndow apps

facebook-github-bot · 2024-01-03T10:00:24Z

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

okwasniewski · 2024-01-03T15:06:36Z

Ugh, the internal linter is failing.. Are those internal tests also failing?

cipolleschi · 2024-01-03T15:43:12Z

Cool, so, the linter is failing, but that's easy and I have to run a simple command to fix that.
This is a well known problem where, internally, we use a linter that is different from the OSS. It is an internally modified version, so it is very hard to have them matching.

The other tests should be good. I'll try to land the diff before EOW.

facebook-github-bot · 2024-01-05T15:14:08Z

@cipolleschi merged this pull request in 782e9ea.

…window apps (facebook#42036) Summary: This PRs refactors `RCTKeyWindow()` to be more resilient and work in multi-window apps. After my recent PR got merged (facebook#41935) it significantly reduced the number of calls to `RCTKeyWindow()` and now it's called only when necessary. So this PR makes this function a bit more resource intensive but it guarantees that we will find current scene's key window. This would also fix some brownfield scenarios where React Native is working in multi-window mode and in the future allow us to more easily adopt `UIWindowSceneDelegate` bypass-github-export-checks ## Changelog: [IOS] [CHANGED] - refactor `RCTKeyWindow` to be more resilient and work in multi-window apps Pull Request resolved: facebook#42036 Test Plan: Checkout RNTester example for Alerts and LoadingView. https://github.com/facebook/react-native/assets/52801365/8cf4d698-db6d-4a12-8d8d-7a5acf34858b Reviewed By: huntie Differential Revision: D52431720 Pulled By: cipolleschi fbshipit-source-id: 0d6ef1d46b2428c30c9f64dae66b95dbc69f0a3b

…window apps (facebook#42036) Summary: This PRs refactors `RCTKeyWindow()` to be more resilient and work in multi-window apps. After my recent PR got merged (facebook#41935) it significantly reduced the number of calls to `RCTKeyWindow()` and now it's called only when necessary. So this PR makes this function a bit more resource intensive but it guarantees that we will find current scene's key window. This would also fix some brownfield scenarios where React Native is working in multi-window mode and in the future allow us to more easily adopt `UIWindowSceneDelegate` bypass-github-export-checks [IOS] [CHANGED] - refactor `RCTKeyWindow` to be more resilient and work in multi-window apps Pull Request resolved: facebook#42036 Test Plan: Checkout RNTester example for Alerts and LoadingView. https://github.com/facebook/react-native/assets/52801365/8cf4d698-db6d-4a12-8d8d-7a5acf34858b Reviewed By: huntie Differential Revision: D52431720 Pulled By: cipolleschi fbshipit-source-id: 0d6ef1d46b2428c30c9f64dae66b95dbc69f0a3b

## Summary This PR adds `visionos` support. I've refactored call sites to `UIApplication.sharedApplication.keyWindow` (it's unavailable on visionOS) for `RCTKeyWindow()` its a React Native core utility for retrieving key window. (In one of my recent PRs it got support for multi-window scenarios: facebook/react-native#42036) https://github.com/software-mansion/react-native-reanimated/assets/52801365/8b535f78-c91c-4a7c-b956-29c6985d157a ## Test plan 1. Init new visionOS app `npx @callstack/react-native-visionos@latest init MyApp` 2. Install Reanimated from GitHub commit or link it locally: `"react-native-reanimated": "git@github.com:okwasniewski/react-native-reanimated.git#feat/visionos",` --------- Co-authored-by: Tomek Zawadzki <tomekzawadzki98@gmail.com> Co-authored-by: Krzysztof Piaskowy <krzysztof.piaskowy@swmansion.com>

…window apps (facebook#42036) Summary: This PRs refactors `RCTKeyWindow()` to be more resilient and work in multi-window apps. After my recent PR got merged (facebook#41935) it significantly reduced the number of calls to `RCTKeyWindow()` and now it's called only when necessary. So this PR makes this function a bit more resource intensive but it guarantees that we will find current scene's key window. This would also fix some brownfield scenarios where React Native is working in multi-window mode and in the future allow us to more easily adopt `UIWindowSceneDelegate` bypass-github-export-checks [IOS] [CHANGED] - refactor `RCTKeyWindow` to be more resilient and work in multi-window apps Pull Request resolved: facebook#42036 Test Plan: Checkout RNTester example for Alerts and LoadingView. https://github.com/facebook/react-native/assets/52801365/8cf4d698-db6d-4a12-8d8d-7a5acf34858b Reviewed By: huntie Differential Revision: D52431720 Pulled By: cipolleschi fbshipit-source-id: 0d6ef1d46b2428c30c9f64dae66b95dbc69f0a3b

okwasniewski force-pushed the okwasniewski/KeyWindow branch 2 times, most recently from 2dc9793 to 177ae7d Compare December 22, 2023 09:03

cipolleschi reviewed Dec 27, 2023

View reviewed changes

okwasniewski force-pushed the okwasniewski/KeyWindow branch 2 times, most recently from d5f98bb to 7371947 Compare January 2, 2024 12:31

feat: refactor RCTKeyWindow to be more resilient and work in multi-wi…

ca16e3b

…ndow apps

okwasniewski force-pushed the okwasniewski/KeyWindow branch from 7371947 to ca16e3b Compare January 2, 2024 12:35

facebook-github-bot closed this in 782e9ea Jan 5, 2024

facebook-github-bot added the Merged This PR has been merged. label Jan 5, 2024

okwasniewski mentioned this pull request Feb 1, 2024

feat: add visionos support software-mansion/react-native-reanimated#5627

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor `RCTKeyWindow` to be more resilient and work in multi-window apps #42036

feat: refactor `RCTKeyWindow` to be more resilient and work in multi-window apps #42036

okwasniewski commented Dec 21, 2023

cipolleschi left a comment

cipolleschi Dec 27, 2023

okwasniewski Jan 2, 2024

facebook-github-bot commented Dec 27, 2023

cipolleschi commented Dec 28, 2023

okwasniewski commented Dec 28, 2023

cipolleschi commented Dec 28, 2023

facebook-github-bot commented Jan 3, 2024

okwasniewski commented Jan 3, 2024

cipolleschi commented Jan 3, 2024

facebook-github-bot commented Jan 5, 2024

feat: refactor RCTKeyWindow to be more resilient and work in multi-window apps #42036

feat: refactor RCTKeyWindow to be more resilient and work in multi-window apps #42036

Conversation

okwasniewski commented Dec 21, 2023

Summary:

Changelog:

Test Plan:

cipolleschi left a comment

Choose a reason for hiding this comment

cipolleschi Dec 27, 2023

Choose a reason for hiding this comment

okwasniewski Jan 2, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Dec 27, 2023

cipolleschi commented Dec 28, 2023

okwasniewski commented Dec 28, 2023

cipolleschi commented Dec 28, 2023

facebook-github-bot commented Jan 3, 2024

okwasniewski commented Jan 3, 2024

cipolleschi commented Jan 3, 2024

facebook-github-bot commented Jan 5, 2024

feat: refactor `RCTKeyWindow` to be more resilient and work in multi-window apps #42036

feat: refactor `RCTKeyWindow` to be more resilient and work in multi-window apps #42036