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

useWindowDimensions() and Dimensions.get("window") both return { width: 0, height: 0 } when window does not have keyboard focus [cause and solution provided] #2296

Open
shirakaba opened this issue Nov 27, 2024 · 2 comments
Labels
bug Something isn't working Needs: Triage 🔍

Comments

@shirakaba
Copy link

shirakaba commented Nov 27, 2024

Environment

react-native -v: 0.73.9
npm ls react-native-macos: 0.73.34 (but affects latest `main` as well)
node -v: v22.11.0
npm -v: v10.9.0
yarn --version: 1.22.19
xcodebuild -version: Xcode 16.1, Build version 16B40

Steps to reproduce the bug

  1. Add a call to useWindowDimensions() in your JS code.
  2. In Xcode, set a breakpoint inside applicationDidFinishLaunching. This is a simple way to make the app launch without keyboard focus.
  3. Place a breakpoint on the call to [NSApp keyWindow] inside the RCTKeyWindow method in node_modules/react-native-macos/React/Base/RCTUtils.m.
  4. See how it returns nil for [NSApp keyWindow] when no window has keyboard focus (keyWindow is nil when there is no window receiving keyboard events):
    Image
  5. This leads to downstream functions falling back to dimensions of CGSizeZero:
    Image
  6. The JS side gets the wrong dimensions, so is unable to lay out correctly.

Although here we reproduce the problem by using breakpoints, in practice this can happen if the user manages to launch the app without keyboard focus (e.g. they begin opening it, then quickly switch to another app before the first window has opened). This is a potentially severe problem as, due to the small dimensions, it forced our app to display the mobile layout, which was not appropriate for the spacious window size.

Expected Behavior

The dimensions of the "main window" should be reliably read, even when the window does not have keyboard focus.

Actual Behavior

Dimensions are read as { width: 0, height: 0 } despite the main window's size being non-zero.

Reproducible Demo

No response

Additional context

I've fixed it in my apps with the following pnpm patch:

diff --git a/React/Base/RCTUtils.m b/React/Base/RCTUtils.m
index 36916fc85429f40771aec650f8b1fdf422708aca..aab38f7b9d8f74cccd3094f393643c65b86bbdf0 100644
--- a/React/Base/RCTUtils.m
+++ b/React/Base/RCTUtils.m
@@ -620,7 +620,30 @@ BOOL RCTRunningInAppExtension(void)
 
   return nil;
 #else // [macOS
-  return [NSApp keyWindow];
+  // Formerly, we were returning `[NSApp keyWindow]` from here. However, it is
+  // not completely dependable:
+  //
+  // 1) keyWindow is `nil` "when there is no window receiving keyboard events":
+  //    https://developer.apple.com/documentation/appkit/nsapplication/keywindow?language=objc
+  // 2) Upon a full reload (e.g. pressing the 'R' button in the CLI), there are
+  //    briefly two windows (as it cycles between the two), and the keyWindow
+  //    value actually briefly points to the older of the two.
+  //
+  // So in this patch, we instead just pick the first window in the array.
+  // - In single-window apps, when there is just one window in the array, that
+  //   will be the "main" window.
+  // - During hot reloads, when single-window apps can briefly have two windows
+  //   in the array, the first window will be the "old" main window while the
+  //   second window will be the "new" main window. We take dimensions from the
+  //   old main window rather than the new one because the old one has already
+  //   had a sensible size set, while the new one is still at zero size.
+  //
+  //
+  // Yes, this logic will fall apart if you have a genuine multi-window app, but
+  // react-native-macos doesn't have first-class support for multi-window apps
+  // in the first place. And for now in our app, this patch solves more problems
+  // than it creates.
+  return [NSApplication.sharedApplication.windows firstObject];
 #endif // macOS]
 }

Evidence of fix:

  1. When [NSApp keyWindow] is nil, we pick from the list of available windows instead. In the screenshot, we pick the lastObject (the new window during a hot reload), but later I changed the patch to take firstObject because the old window had the desired dimensions while the new one genuinely was CGSizeZero, meaning that full reloads always got a size zero and thus an incorrect layout if reading the size from the new window (there was no followup resize event to correct this after the window was updated to a non-zero size).
    Image
  2. Downstream functions get a proper window size:
    Image

Related issues:

People are talking about several different things in #915, but I think some of them are touching upon this issue.

@shirakaba shirakaba added the bug Something isn't working label Nov 27, 2024
@shirakaba shirakaba changed the title useWindowDimensions() and Dimensions.get("window") both return { width: 0, height: 0 } when window does not have keyboard focus useWindowDimensions() and Dimensions.get("window") both return { width: 0, height: 0 } when window does not have keyboard focus [cause and solution provided] Nov 27, 2024
@Saadnajmi
Copy link
Collaborator

Thanks for the detailed response! In our apps at Microsoft (mostly the office apps), we are using genuine multi window experiences, and this API does its best to handle that. However, it is flawed, we've often thought there should be a variant that is aware which window your RN instances root view is in, etc.

For this, maybe we can add that patch as a fallback? I'll have to think about it.

@shirakaba
Copy link
Author

shirakaba commented Nov 27, 2024

That's the main reason I didn't open a PR, yeah, as it doesn't handle multi-window experiences.

I think basically the original react-native JS APIs, assuming a single-window world, are too limited and we do need a macOS-specific one where you can specify which root view you're talking about (maybe you can set some kind of key on the root view from the native side). It would be just as necessary for multi-window Windows apps.

I think if we did add my patch as a fallback, it would only trade some problems for others. I guess the ideal approach would be:

  1. The starter template for new projects explicitly sets a default key like "main" on the root view.
  2. Instead of referring to [NSApp keyWindow], we cycle over all windows in the application, filtering by the ones that have an RCTRootView with that default key (⚠ this may require a little more thought for cases where users have multiple roots in the same window, but maybe it's still fine?).
  3. If there are multiple windows that match, assume it's a full-app hot reload and choose the firstObject in the array (as the lastObject in the array, although being the newer window, will not have had its size set yet – or alternatively ensure that we initialize its size to the size of the old window). Another way to be more sure of the age of the window is to sort by the timestamp of something like the _lastWindowMovedEvent:
    Image
  4. Additionally provide more desktop-appropriate APIs like useNamedWindowDimensions("main") and Dimensions.getNamedWindow("main").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Needs: Triage 🔍
Projects
None yet
Development

No branches or pull requests

2 participants