Sync RCTAlertManager from upstream RN fork #661
Conversation
|
|
There was a problem hiding this comment.
Thanks! So to recap, the idea here is that in order to add a unified solution that include macOS support, you figured it would be better to do so within the context of the new upstream version of this code straightaway, right?
Verified there are no regressions in RNTester.
Did you verify this with the iOS build target [as well]?
Yup, I thought it will be easier to process
Nope! Good call, will do |
thymikee
force-pushed
the
macos/sync-f319ff321c4b7c0929b99e3ebe7e1ce1fa50b34ca
branch
from
7ee0e00 to
9444854
Compare
|
|
||
| #import <React/RCTUIKit.h> // TODO(macOS ISS#2323203) | ||
|
|
||
| #if TARGET_OS_OSX // [TODO(macOS ISS#2323203) |
There was a problem hiding this comment.
Thank you for your contributions here! This is great stuff.
What we've done in the past is define a cross-plat sounding macro to be the platform specific version. You can see some examples in RCTUIKit.h (e.g. RCTView is either NSView or UIView). Maybe we can do the same for this?
It creates more diffs with upstream but is also a lot less confusing for any engineer parsing it trying to understand why UIKit is compiling for macOS :)
There was a problem hiding this comment.
So something like I did initially here: #661 (comment) but name it e.g. RCTPlatformAlertController? Should I also put it into RCTUIKit.h?
There was a problem hiding this comment.
Seeing as this header might taint other compilation units, it might be more appropriate to not define this alias for any referrer, but instead:
#if TARGET_OS_OSX // [TODO(macOS ISS#2323203)
@interface RCTAlertController : NSViewController
#else
@interface RCTAlertController : UIAlertController
#endif // ]TODO(macOS ISS#2323203)After that this change is good to go from my end. @harrieshin has a good point, but that’s a discussion/change that should be made upstream first.
thymikee
force-pushed
the
macos/sync-f319ff321c4b7c0929b99e3ebe7e1ce1fa50b34ca
branch
from
7498b21 to
492bd42
Compare
…9295) Summary: This should fix facebook#29082 and facebook#10471 Currently when an alert is being shown while a modal is being dismissed, it causes the alert not to show and In some cases it causes the UI to become unresponsive. I think this was caused by using RCTPresentedViewController to try and display the Alert the currently presented view. The View the Alert was going to be shown on is dismissed and the modal doesn't show. I implemented a new RCTAlertController to show the alert on top of the view, the modal being dismissed should now not interfere with the alert being shown. [iOS] [Fixed] - Fixed showing Alert while closing a Modal Pull Request resolved: facebook#29295 Test Plan: To recreate the bug: 1. npx react-native init Test --version 0.63.0-rc.1 2. Paste the following code into App.js ```javascript /** * Sample React Native App * https://github.com/facebook/react-native * * format * flow strict-local */ import React from 'react'; import { SafeAreaView, StyleSheet, View, Text, StatusBar, Modal, Alert } from 'react-native'; const App: () => React$Node = () => { const [visible, setVisible] = React.useState(false) const onShowModal = () => { setVisible(true) } onCloseBroken = () => { setVisible(false) Alert.alert('Alert', 'Alert won\'t show') } onCloseWorking = () => { setVisible(false) setTimeout(() => Alert.alert('Alert', 'Works fine'), 10) } return ( <> <StatusBar barStyle="dark-content" /> <SafeAreaView style={styles.container}> <Text onPress={onShowModal}>Show modal</Text> </SafeAreaView> <Modal animationType="fade" visible={visible} onRequestClose={onCloseWorking} > <View style={styles.container}> <Text onPress={onCloseBroken}>Close modal immediately</Text> <Text onPress={onCloseWorking}>Close modal with delay</Text> </View> </Modal> </> ) } const styles = StyleSheet.create({ container: { flex: 1, alignItems: 'center', justifyContent: 'space-around', }, }) export default App ``` 3. cd Test && npx react-native run-ios 4. Show the modal and click the `Close modal immediately` button The first button doesn't show the alert, the second does because it gets rendered after the modal view is dismissed. After this commit, the alert always shows on top of every view properly. You can test by pointing the react native package to my branch by modifying the package json file like this ``` "react-native": "https://github.com/devon94/react-native.git#fix-ios-modal" ``` I was unable to reproduce the case where it causes the UI to be responsive in the test app but was able to reproduce it in our react native app at work. I can provide a video later if needed but the code is too complex to simplify into a test case. Reviewed By: sammy-SC Differential Revision: D22783371 Pulled By: PeteTheHeat fbshipit-source-id: 3e359645c610074ea855ee5686c59bdb9d6b696b
thymikee
force-pushed
the
macos/sync-f319ff321c4b7c0929b99e3ebe7e1ce1fa50b34ca
branch
from
492bd42 to
43eacfc
Compare
thymikee
deleted the
macos/sync-f319ff321c4b7c0929b99e3ebe7e1ce1fa50b34ca
branch
Sync RCTAlertManager from facebook@f319ff3. There's also facebook@03bd7d7 that affects Alert, but it depends on other turbomodule changes and is pretty small, so shouldn't be problematic with 0.64 sync anyway.
Changed code affects iOS branches only, so I added respective
#if TARGET_OS_OSXdirectives.Please select one of the following
Summary
Slowly working towards #354 and thought it would be helpful to have some Alert commits synchronized with upstream fork first. Another purpose of this diff is to make the v0.64 sync easier to manage.
Changelog
[Internal] [Changed] - Sync RCTAlertManager from upstream RN fork
Test Plan
Verified there are no regressions in RNTester.
Microsoft Reviewers: Open in CodeFlow