-
-
Notifications
You must be signed in to change notification settings - Fork 539
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: issues with presenting owned modals from foreign ones #2113
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
import React from 'react'; | ||
import { View, Modal, Button, TouchableWithoutFeedback } from 'react-native'; | ||
import { useState } from 'react'; | ||
|
||
import { NavigationContainer, useNavigation } from '@react-navigation/native'; | ||
import { createNativeStackNavigator } from '@react-navigation/native-stack'; | ||
|
||
type AppStackPages = { | ||
Home: undefined; | ||
Modal: undefined; | ||
}; | ||
|
||
function HomeScreen() { | ||
const navigation = useNavigation(); | ||
const [visible, setVisible] = useState(false); | ||
|
||
return ( | ||
<View style={{ flex: 1, justifyContent: 'center', alignItems: 'center' }}> | ||
<Button | ||
title="Toggle bottom modal" | ||
onPress={() => setVisible(prev => !prev)} | ||
/> | ||
<Modal animationType="slide" visible={visible} transparent> | ||
<TouchableWithoutFeedback onPress={() => setVisible(false)}> | ||
<View style={{ flex: 1 }} /> | ||
</TouchableWithoutFeedback> | ||
<View | ||
style={{ | ||
borderTopLeftRadius: 10, | ||
borderTopRightRadius: 10, | ||
borderWidth: 2, | ||
borderColor: 'red', | ||
padding: 10, | ||
minHeight: '40%', | ||
alignItems: 'center', | ||
justifyContent: 'center', | ||
}}> | ||
<Button | ||
title="Open navigation modal" | ||
onPress={() => { | ||
// Issue: autohiding the Modal that serves as a bottom sheet unmounts | ||
// the anchor component for the screen that is in { presentation: "modal" } mode | ||
// Previously the anchoring component for a { presentation: "modal" }-based screen was different and it worked | ||
// The culprit is: https://github.com/software-mansion/react-native-screens/pull/1912 released in https://github.com/software-mansion/react-native-screens/releases/tag/3.29.0 | ||
// adding setTimeout does not bring any good, because | ||
// - we either don't see navigation action | ||
// - we unmount both the bottom sheet modal and the screen itself | ||
|
||
setVisible(false); | ||
|
||
navigation.navigate('Modal'); | ||
}} | ||
/> | ||
</View> | ||
</Modal> | ||
</View> | ||
); | ||
} | ||
|
||
function ModalScreen() { | ||
return <View style={{ flex: 1, backgroundColor: 'rgb(50,150,50)' }} />; | ||
} | ||
|
||
const AppStack = createNativeStackNavigator<AppStackPages>(); | ||
|
||
function Navigation() { | ||
return ( | ||
<AppStack.Navigator> | ||
<AppStack.Screen name="Home" component={HomeScreen} /> | ||
<AppStack.Screen | ||
name="Modal" | ||
component={ModalScreen} | ||
options={{ presentation: 'modal' }} | ||
/> | ||
</AppStack.Navigator> | ||
); | ||
} | ||
|
||
export default function App() { | ||
return ( | ||
<NavigationContainer> | ||
<Navigation /> | ||
</NavigationContainer> | ||
); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
import React from 'react'; | ||
import { View, Modal, Button, TouchableWithoutFeedback } from 'react-native'; | ||
import { useState } from 'react'; | ||
|
||
import { NavigationContainer, useNavigation } from '@react-navigation/native'; | ||
import { createNativeStackNavigator } from '@react-navigation/native-stack'; | ||
|
||
type AppStackPages = { | ||
Home: undefined; | ||
Modal: undefined; | ||
}; | ||
|
||
function HomeScreen() { | ||
const navigation = useNavigation(); | ||
const [visible, setVisible] = useState(false); | ||
|
||
return ( | ||
<View style={{ flex: 1, justifyContent: 'center', alignItems: 'center' }}> | ||
<Button | ||
title="Toggle bottom modal" | ||
onPress={() => setVisible(prev => !prev)} | ||
/> | ||
<Modal animationType="slide" visible={visible} transparent> | ||
<TouchableWithoutFeedback onPress={() => setVisible(false)}> | ||
<View style={{ flex: 1 }} /> | ||
</TouchableWithoutFeedback> | ||
<View | ||
style={{ | ||
borderTopLeftRadius: 10, | ||
borderTopRightRadius: 10, | ||
borderWidth: 2, | ||
borderColor: 'red', | ||
padding: 10, | ||
minHeight: '40%', | ||
alignItems: 'center', | ||
justifyContent: 'center', | ||
}}> | ||
<Button | ||
title="Open navigation modal" | ||
onPress={() => { | ||
// Issue: autohiding the Modal that serves as a bottom sheet unmounts | ||
// the anchor component for the screen that is in { presentation: "modal" } mode | ||
// Previously the anchoring component for a { presentation: "modal" }-based screen was different and it worked | ||
// The culprit is: https://github.com/software-mansion/react-native-screens/pull/1912 released in https://github.com/software-mansion/react-native-screens/releases/tag/3.29.0 | ||
// adding setTimeout does not bring any good, because | ||
// - we either don't see navigation action | ||
// - we unmount both the bottom sheet modal and the screen itself | ||
|
||
setVisible(false); | ||
|
||
navigation.navigate('Modal'); | ||
}} | ||
/> | ||
</View> | ||
</Modal> | ||
</View> | ||
); | ||
} | ||
|
||
function ModalScreen() { | ||
return <View style={{ flex: 1, backgroundColor: 'rgb(50,150,50)' }} />; | ||
} | ||
|
||
const AppStack = createNativeStackNavigator<AppStackPages>(); | ||
|
||
function Navigation() { | ||
return ( | ||
<AppStack.Navigator> | ||
<AppStack.Screen name="Home" component={HomeScreen} /> | ||
<AppStack.Screen | ||
name="Modal" | ||
component={ModalScreen} | ||
options={{ presentation: 'modal' }} | ||
/> | ||
</AppStack.Navigator> | ||
); | ||
} | ||
|
||
export default function App() { | ||
return ( | ||
<NavigationContainer> | ||
<Navigation /> | ||
</NavigationContainer> | ||
); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -382,9 +382,12 @@ - (void)setModalViewControllers:(NSArray<UIViewController *> *)controllers | |
[newControllers removeObjectsInArray:_presentedModals]; | ||
|
||
// We need to find bottom-most view controller that should stay on the stack | ||
// for the duration of transition. There are couple of scenarios: | ||
// (1) No modals are presented or all modals were presented by this RNSNavigationController, | ||
// (2) There are modals presented by other RNSNavigationControllers (nested/outer) | ||
// for the duration of transition. | ||
|
||
// There are couple of scenarios: | ||
// (1) no modals are presented or all modals were presented by this RNSNavigationController, | ||
// (2) there are modals presented by other RNSNavigationControllers (nested/outer), | ||
// (3) there are modals presented by other controllers (e.g. React Native's Modal view). | ||
|
||
// Last controller that is common for both _presentedModals & controllers | ||
__block UIViewController *changeRootController = _controller; | ||
|
@@ -479,16 +482,35 @@ - (void)setModalViewControllers:(NSArray<UIViewController *> *)controllers | |
} | ||
}; | ||
|
||
// changeRootController is the last controller that *is owned by this stack*, and should stay unchanged after this | ||
// batch of transitions. Therefore changeRootController.presentedViewController is the first constroller to be | ||
// dismissed (implying also all controllers above). Notice here, that firstModalToBeDismissed could have been | ||
// RNSScreen modal presented from *this* stack, another stack, or any other view controller with modal presentation | ||
// provided by third-party libraries (e.g. React Native's Modal view). In case of presence of other (not managed by | ||
// us) modal controllers, weird interactions might arise. The code below, besides handling our presentation / | ||
// dismissal logic also attempts to handle possible wide gamut of cases of interactions with third-party modal | ||
// controllers, however it's not perfect. | ||
// TODO: Find general way to manage owned and foreign modal view controllers and refactor this code. Consider building | ||
// model first (data structue, attempting to be aware of all modals in presentation and some text-like algorithm for | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// computing required operations). | ||
kkafar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
UIViewController *firstModalToBeDismissed = changeRootController.presentedViewController; | ||
|
||
if (firstModalToBeDismissed != nil) { | ||
BOOL shouldAnimate = changeRootIndex == controllers.count && | ||
[firstModalToBeDismissed isKindOfClass:[RNSScreen class]] && | ||
((RNSScreen *)firstModalToBeDismissed).screenView.stackAnimation != RNSScreenStackAnimationNone; | ||
|
||
if ([_presentedModals containsObject:firstModalToBeDismissed]) { | ||
if ([_presentedModals containsObject:firstModalToBeDismissed] || | ||
![firstModalToBeDismissed isKindOfClass:RNSScreen.class]) { | ||
kkafar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// We dismiss every VC that was presented by changeRootController VC or its descendant. | ||
// After the series of dismissals is completed we run completion block in which | ||
// we present modals on top of changeRootController (which may be the this stack VC) | ||
// | ||
// There also might the second case, where the firstModalToBeDismissed is foreign. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// See: https://github.com/software-mansion/react-native-screens/issues/2048 | ||
// For now, to mitigate the issue, we also decide to trigger its dismissal before | ||
// starting the presentation chain down below in finish() callback. | ||
[changeRootController dismissViewControllerAnimated:shouldAnimate completion:finish]; | ||
return; | ||
} | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
controller