Default dimensions `scale` value to 1.0 if window is nil #1970

shwanton · 2023-11-03T23:56:56Z

Please select one of the following

I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
I am making a fix / change for the macOS implementation of react-native
I am making a change required for Microsoft usage of react-native

Summary:

In 0.73 on Paper, StyleSheet.hairlineWidth would throw an NaN error when PixelRatio.get() returned 0.

react-native-macos/packages/react-native/Libraries/StyleSheet/StyleSheet.js

Lines 166 to 169 in 43122d4

    
           let hairlineWidth: number = PixelRatio.roundToNearestPixel(0.4); 
        
           if (hairlineWidth === 0) { 
        
             hairlineWidth = 1 / PixelRatio.get(); 
        
           }

This was because we were calculating the scale before the window was created and returning zero.

react-native-macos/packages/react-native/React/UIUtils/RCTUIUtils.m

Line 51 in 43122d4

.scale = [[window screen] backingScaleFactor],

With the New Arch Enabled flag, the RCTDeviceInfo Turbomodule will run after the window is created so it will always have a current window.

Changelog:

[MACOS] [FIXED] - Default dimensions scale value to 1.0 if window is nil

Test Plan:

Paper - Before window is created

Paper - After window is created

Fabric

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

packages/react-native/React/UIUtils/RCTUIUtils.m

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

Saadnajmi

Nit: PR targets main, so shouldn't have [0.73] in the title

packages/react-native/React/Base/RCTUIKit.h

) * [0.73] Default scale to 1.0 on if window is nil * Unify RCTExportedDimensions between iOS & macOS * Use typedef instead of #define for NSWindow

* Remove deprecated uses of UIActivityIndicatorViewStyle (facebook#38208) Summary: Super simple change to replace some deprecated ENUM values with the correct one, now that we're on iOS 13.0+. From `UIActivityIndicator.h`: ``` typedef NS_ENUM(NSInteger, UIActivityIndicatorViewStyle) { UIActivityIndicatorViewStyleMedium API_AVAILABLE(ios(13.0), tvos(13.0)) = 100, UIActivityIndicatorViewStyleLarge API_AVAILABLE(ios(13.0), tvos(13.0)) = 101, UIActivityIndicatorViewStyleWhiteLarge API_DEPRECATED_WITH_REPLACEMENT("UIActivityIndicatorViewStyleLarge", ios(2.0, 13.0), tvos(9.0, 13.0)) = 0, UIActivityIndicatorViewStyleWhite API_DEPRECATED_WITH_REPLACEMENT("UIActivityIndicatorViewStyleMedium", ios(2.0, 13.0), tvos(9.0, 13.0)) = 1, UIActivityIndicatorViewStyleGray API_DEPRECATED_WITH_REPLACEMENT("UIActivityIndicatorViewStyleMedium", ios(2.0, 13.0)) API_UNAVAILABLE(tvos) = 2, }; ``` ## Changelog: [IOS] [CHANGED] - Remove deprecated uses of UIActivityIndicatorViewStyle Pull Request resolved: facebook#38208 Test Plan: CI should pass Reviewed By: cipolleschi Differential Revision: D47254035 Pulled By: rshest fbshipit-source-id: 6d3270899e8d52611d91c777f324c3c6f0c520be * Refactor RCTActivityIndicator * Remove TARGET_OS_UIKITFORMAC macros (facebook#42278) Summary: There seems to be a lot of `TARGET_OS_UIKITFORMAC` macro in React Native that don't need to be there. Let's remove them. First off, what is `TARGET_OS_UIKITFORMAC` targeting? You might think it's [Mac Catalyst](https://developer.apple.com/mac-catalyst/), if you look at the [commit](facebook@3724810) introducing the ifdefs. However.. that doesn't seem right because `TARGET_OS_MACCATALYST` exists, and is used elsewhere in the codebase. In fact, if you look at this handy comment inside `TargetConditionals.h` (the file that defines all these conditionals), `TARGET_OS_UIKITFORMAC` is not even on there! ``` /* * TARGET_OS_* * * These conditionals specify in which Operating System the generated code will * run. Indention is used to show which conditionals are evolutionary subclasses. * * The MAC/WIN32/UNIX conditionals are mutually exclusive. * The IOS/TV/WATCH/VISION conditionals are mutually exclusive. * * TARGET_OS_WIN32 - Generated code will run on WIN32 API * TARGET_OS_WINDOWS - Generated code will run on Windows * TARGET_OS_UNIX - Generated code will run on some Unix (not macOS) * TARGET_OS_LINUX - Generated code will run on Linux * TARGET_OS_MAC - Generated code will run on a variant of macOS * TARGET_OS_OSX - Generated code will run on macOS * TARGET_OS_IPHONE - Generated code will run on a variant of iOS (firmware, devices, simulator) * TARGET_OS_IOS - Generated code will run on iOS * TARGET_OS_MACCATALYST - Generated code will run on macOS * TARGET_OS_TV - Generated code will run on tvOS * TARGET_OS_WATCH - Generated code will run on watchOS * TARGET_OS_VISION - Generated code will run on visionOS * TARGET_OS_BRIDGE - Generated code will run on bridge devices * TARGET_OS_SIMULATOR - Generated code will run on an iOS, tvOS, watchOS, or visionOS simulator * TARGET_OS_DRIVERKIT - Generated code will run on macOS, iOS, tvOS, watchOS, or visionOS * * TARGET_OS_EMBEDDED - DEPRECATED: Use TARGET_OS_IPHONE and/or TARGET_OS_SIMULATOR instead * TARGET_IPHONE_SIMULATOR - DEPRECATED: Same as TARGET_OS_SIMULATOR * TARGET_OS_NANO - DEPRECATED: Same as TARGET_OS_WATCH * * +--------------------------------------------------------------------------------------+ * | TARGET_OS_MAC | * | +-----+ +------------------------------------------------------------+ +-----------+ | * | | | | TARGET_OS_IPHONE | | | | * | | | | +-----------------+ +----+ +-------+ +--------+ +--------+ | | | | * | | | | | IOS | | | | | | | | | | | | | * | | OSX | | | +-------------+ | | TV | | WATCH | | BRIDGE | | VISION | | | DRIVERKIT | | * | | | | | | MACCATALYST | | | | | | | | | | | | | | * | | | | | +-------------+ | | | | | | | | | | | | | * | | | | +-----------------+ +----+ +-------+ +--------+ +--------+ | | | | * | +-----+ +------------------------------------------------------------+ +-----------+ | * +--------------------------------------------------------------------------------------+ */ ``` Going even deeper into `TargetConditionals.h`, you will see `TARGET_OS_UIKITFORMAC` defined... and it's always 1 when `TARGET_OS_MACCATALYST` is 1, making it feel even more redundant. My current conclusion is it's either another variant of Mac Catalyst (the one where they just run unmodified UIKit maybe..), or it's an older macro back from when Catalyst was still experimental. Either way, it's pretty obvious nobody is running or testing this codepath, and it adds bloat, especially to React Native macOS where we have extra ifdef blocks for macOS support (and eventually visionOS support). Let's remove it. Another change I made while we're here: I've seen this lingering TODO to replace setTargetRect:InView: / setMenuVisible:animated: (deprecated as of iOS 13, below our minimum OS requirement) with showMenuFromView (deprecated as of iOS 16, in line with the availability check). Let's just.... do that? [IOS] [REMOVED] - Remove TARGET_OS_UIKITFORMAC macros Pull Request resolved: facebook#42278 Test Plan: RNTester with Mac Catalyst still compiles: ![Screenshot 2024-01-15 at 12 26 03 AM](https://github.com/facebook/react-native/assets/6722175/015bd37d-f536-43c7-9586-96187cdbd013) Reviewed By: cipolleschi Differential Revision: D52780690 Pulled By: sammy-SC fbshipit-source-id: df6a333e8e15f79de0ce6f538ebd73b92698dcb6 * Remove an early return to suppress a deprecated API warning for `UIMenuController` (facebook#42277) Summary: `UIMenuController` is deprecated as of iOS 16. facebook@e08a197 migrated a usage into an `available` check. However, it does not properly fall back to the deprecated API in the "else" block of the availability check, instead it uses an early return. It seems this means Xcode still sees the API as used, and spits out a deprecated warning. Let's just refactor the code so we don't have that anymore. [IOS] [FIXED] - Remove an early return to suppress a deprecated API warning for `UIMenuController` Pull Request resolved: facebook#42277 Test Plan: CI should pass. Reviewed By: cipolleschi Differential Revision: D52785488 Pulled By: sammy-SC fbshipit-source-id: 0b47e8aa8d7c94728e3d68332fbb8f97f8ded34e * DeviceInfo: Simplify RCTExportedDimensions's API Summary: RCTExportedDimensions doesn't need access to the ModuleRegistry, or the bridge. It just uses those two things to get the fontScale. We could make RCTExportedDimensions easier to understand, by making it do fewer things (i.e: computing the fontScale up front, and passing it into RCTExportedDimensions). Let's just do that. Changelog: [Internal] Reviewed By: cipolleschi Differential Revision: D48237715 fbshipit-source-id: b3af648d88276846742d0e1192d33d180ee49dbb * iOS: trigger didUpdateDimensions event when resizing without changing traits (facebook#37649) Summary: - when using the `useWindowDimensions()` hook in a component, it's possible for the hook to report incorrect sizes and not update as frequently as it should - this is most applicable to apps built for iPad and macOS - closes facebook#36118 - either when resizing a React Native app to a different [Size Class](https://developer.apple.com/design/human-interface-guidelines/layout#Device-size-classes) or changing the Appearance, we dispatch an `RCTUserInterfaceStyleDidChangeNotification` notification - these are then handled in the `interfaceFrameDidChange` method of `RCTDeviceInfo` - this results in a `didUpdateDimensions` Device Event, which in turn updates the results of `useWindowDimensions()` - see [RCTDeviceInfo.mm#L217-L232](https://github.com/facebook/react-native/blob/v0.72.0-rc.4/packages/react-native/React/CoreModules/RCTDeviceInfo.mm#L217-L232) - and [Dimensions.js#L119-L124](https://github.com/facebook/react-native/blob/v0.72.0-rc.4/packages/react-native/Libraries/Utilities/Dimensions.js#L119-L124) 🐛 **However** 🐛 - if you are resizing without triggering a `UITraitCollection` change, the Dimensions reported by `useWindowDimensions()` can become stale, until you either:- - hit a certain width that is considered a different Size Class - change the Appearance - background then resume the app - make the app full-screen - added a new `RCTRootViewFrameDidChangeNotification` notification - the thinking here is to avoid additional overhead by re-using the same `RCTUserInterfaceStyleDidChangeNotification` - maybe it's overkill? - the new notifications are sent from an override of `setFrame` on `RCTRootView` - the new notifications call the same `interfaceFrameDidChange` method of `RCTDeviceInfo` - Dimensions are now reported correctly when resizing; even within the same Size Class  [IOS] [FIXED] - Dimensions could be reported incorrectly when resizing iPad or macOS apps Pull Request resolved: facebook#37649 Test Plan: **Reproduction: https://github.com/jpdriver/Dimensions** or to recreate it yourself:- - Generate a new project - Change App.tsx ``` import * as React from 'react'; import {View, Text, useWindowDimensions} from 'react-native'; export default function App() { const {width, height} = useWindowDimensions(); return ( <View style={{flex: 1, justifyContent: 'center', alignItems: 'center'}}> <Text>Width: {width}</Text> <Text>Height: {height}</Text> </View> ); } ``` - Open the iOS project in Xcode and enable iPad support - Enable the iPad app to be used in any orientation - Run the app - Enable Stage Manager - Drag one of the corners to resize the app Reviewed By: javache Differential Revision: D46335537 Pulled By: NickGerleman fbshipit-source-id: 1533f511cf3805fdc9629a2ee115cc00e204d82c * Refactor RCTDeviceInfo * Rename `RCTRootViewFrameDidChangeNotification` as it's not trac… (facebook#39835) Summary: …king root view frame changes Looking through where this was introduced (facebook#37649), it seems the notification went from tracking root view size changes to window size changes. However, it was not renamed. I was using it for root view changes in RN-macOS, which.. I guess I'll refactor. Meanwhile, let's update the name? ## Changelog: [IOS] [CHANGED] - Rename `RCTRootViewFrameDidChangeNotification` as it's not tracking root view frame changes Pull Request resolved: facebook#39835 Test Plan: CI should pass Reviewed By: cipolleschi Differential Revision: D50173742 Pulled By: javache fbshipit-source-id: 4651696174c439800984a5e6cf642200bb9c4f3c * Default dimensions `scale` value to 1.0 if window is nil (#1970) * [0.73] Default scale to 1.0 on if window is nil * Unify RCTExportedDimensions between iOS & macOS * Use typedef instead of #define for NSWindow * Fix issues from cherry-picks * Add visionOS support * Extra changes for visionOS on 0.72-stable * Use Xcode 15.2 --------- Co-authored-by: Ramanpreet Nara <[email protected]> Co-authored-by: JP <[email protected]> Co-authored-by: Shawn Dempsey <[email protected]>

[0.73] Default scale to 1.0 on if window is nil

38cf856

shwanton commented Nov 3, 2023

View reviewed changes

packages/react-native/React/CoreModules/RCTDeviceInfo.mm Show resolved Hide resolved

shwanton commented Nov 3, 2023

View reviewed changes

packages/react-native/React/UIUtils/RCTUIUtils.m Outdated Show resolved Hide resolved

shwanton changed the title ~~[0.73] Default scale to 1.0 on if window is nil~~ [0.73] Default scale value to 1.0 if window is nil Nov 3, 2023

shwanton changed the title ~~[0.73] Default scale value to 1.0 if window is nil~~ [0.73] Default dimensions scale value to 1.0 if window is nil Nov 4, 2023

Saadnajmi reviewed Nov 4, 2023

View reviewed changes

packages/react-native/React/UIUtils/RCTUIUtils.m Outdated Show resolved Hide resolved

Saadnajmi reviewed Nov 4, 2023

View reviewed changes

packages/react-native/React/UIUtils/RCTUIUtils.m Show resolved Hide resolved

Saadnajmi reviewed Nov 4, 2023

View reviewed changes

packages/react-native/React/CoreModules/RCTDeviceInfo.mm Show resolved Hide resolved

Unify RCTExportedDimensions between iOS & macOS

4f74cb4

shwanton marked this pull request as ready for review November 4, 2023 00:48

shwanton requested a review from a team as a code owner November 4, 2023 00:48

Saadnajmi reviewed Nov 4, 2023

View reviewed changes

packages/react-native/React/Base/RCTUIKit.h Outdated Show resolved Hide resolved

shwanton changed the title ~~[0.73] Default dimensions scale value to 1.0 if window is nil~~ Default dimensions scale value to 1.0 if window is nil Nov 4, 2023

Use typedef instead of #define for NSWindow

4f1bce1

shwanton mentioned this pull request Nov 7, 2023

[0.73] Default scale to 1.0 on if window is nil #1975

Merged

Saadnajmi approved these changes Nov 7, 2023

View reviewed changes

Merge branch 'main' into shwanton/fix-pixelratio-for-nil-window

077d01d

Saadnajmi merged commit d1ed53d into microsoft:main Nov 7, 2023
22 of 25 checks passed

shwanton deleted the shwanton/fix-pixelratio-for-nil-window branch April 27, 2024 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default dimensions `scale` value to 1.0 if window is nil #1970

Default dimensions `scale` value to 1.0 if window is nil #1970

shwanton commented Nov 3, 2023 •

edited

Loading

Saadnajmi left a comment

	let hairlineWidth: number = PixelRatio.roundToNearestPixel(0.4);
	if (hairlineWidth === 0) {
	hairlineWidth = 1 / PixelRatio.get();
	}

Default dimensions scale value to 1.0 if window is nil #1970

Default dimensions scale value to 1.0 if window is nil #1970

Conversation

shwanton commented Nov 3, 2023 • edited Loading

Please select one of the following

Summary:

Changelog:

Test Plan:

Saadnajmi left a comment

Choose a reason for hiding this comment

Default dimensions `scale` value to 1.0 if window is nil #1970

Default dimensions `scale` value to 1.0 if window is nil #1970

shwanton commented Nov 3, 2023 •

edited

Loading