Skip to content

Commit 442e13d

Browse files
authored
fix(iOS, Modal): Filter-out obsolete state updates from JS (#3760)
## Description This PR introduces logic to collect only active (non-dismissed) modal controllers coming from JS state. When a Modal is dismissed on the native side, its state is sent asynchronously to JavaScript. This can result in some race conditions and stale updates being sent from JS to native. These outdated updates may reference modals that were detached from the native view hierarchy. Since view recycling is disabled, we don't expect dismissed modals to be reattached; a new instance should be created when we push an updated state from JS. Then the new controller instance will be initialized, so these associated with detached screens won't be reused again. Therefore, we can safely filter them out from the JS state on the native side. On the JS side, the state will stabilize at some point and align with native, once we receive all asynchronous events. This fix is brought with an experimental feature flag `iosPreventReattachmentOfDismissedModals` - similar to what we did for `iosPreventReattachmentOfDismissedScreens`. You can opt out in case of any potential regressions. I'm not using the already existing flag deliberately. Extending its responsibility seems to be too much and may unnecessarily force disabling other fixes, when the regression will be touching only the modal hierarchy. Closes: software-mansion/react-native-screens-labs#1017 ## Changes - Added logic to mark detached modals. - Added logic for filtering out screens dismissed natively from JS state. ## Before & after - visual documentation | Before | After | | --- | --- | | <video src="https://github.com/user-attachments/assets/e8156173-c6d7-4d82-ab78-882e7dba77f8" /> | <video src="https://github.com/user-attachments/assets/c2d22d7e-0ecb-4bdf-ae59-6d7fa2370a54" /> | ## Test plan Added Test3760, verified the Modals test. ## Checklist - [x] Included code example that can be used to test this change. - [ ] Updated / created local changelog entries in relevant test files. - [x] For visual changes, included screenshots / GIFs / recordings documenting the change. - [x] For API changes, updated relevant public types. - [ ] Ensured that CI passes
1 parent 156d957 commit 442e13d

11 files changed

Lines changed: 151 additions & 1 deletion

File tree

FabricExample/App.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ featureFlags.experiment.synchronousHeaderSubviewUpdatesEnabled = false;
77
featureFlags.experiment.androidResetScreenShadowStateOnOrientationChangeEnabled =
88
true;
99
featureFlags.experiment.iosPreventReattachmentOfDismissedScreens = true;
10+
featureFlags.experiment.iosPreventReattachmentOfDismissedModals = true;
1011
featureFlags.experiment.ios26AllowInteractionsDuringTransition = true;
1112
featureFlags.stable.debugLogging = true;
1213

android/src/main/java/com/swmansion/rnscreens/ScreenStackViewManager.kt

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,12 +74,19 @@ class ScreenStackViewManager :
7474
)
7575

7676
// iosPreventReattachmentOfDismissedScreens is not available on Android,
77-
// however we must override their setters
77+
// however we must override the setter
7878
override fun setIosPreventReattachmentOfDismissedScreens(
7979
view: ScreenStack?,
8080
value: Boolean,
8181
) = Unit
8282

83+
// iosPreventReattachmentOfDismissedModals is not available on Android,
84+
// however we must override the setter
85+
override fun setIosPreventReattachmentOfDismissedModals(
86+
view: ScreenStack?,
87+
value: Boolean,
88+
) = Unit
89+
8390
companion object {
8491
const val REACT_CLASS = "RNSScreenStack"
8592
}
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
import React from 'react';
2+
import { View, Text, Button, StyleSheet } from 'react-native';
3+
import {
4+
NavigationContainer,
5+
type ParamListBase,
6+
} from '@react-navigation/native';
7+
import {
8+
createNativeStackNavigator,
9+
type NativeStackNavigationProp,
10+
} from '@react-navigation/native-stack';
11+
import Colors from '../../shared/styling/Colors';
12+
13+
type RouteParamList = {
14+
Main: undefined;
15+
ModalOne: undefined;
16+
ModalTwo: undefined;
17+
};
18+
19+
type NavigationProp<ParamList extends ParamListBase> = {
20+
navigation: NativeStackNavigationProp<ParamList>;
21+
};
22+
23+
type StackNavigationProp = NavigationProp<RouteParamList>;
24+
25+
function MainScreen({ navigation }: StackNavigationProp) {
26+
return (
27+
<View style={styles.container}>
28+
<Text style={styles.title}>Main Screen</Text>
29+
<View style={styles.buttonContainer}>
30+
<Button
31+
title="Open Modal One"
32+
onPress={() => navigation.navigate('ModalOne')}
33+
/>
34+
</View>
35+
<View style={styles.buttonContainer}>
36+
<Button
37+
title="Open Modal Two"
38+
color="green"
39+
onPress={() => navigation.navigate('ModalTwo')}
40+
/>
41+
</View>
42+
</View>
43+
);
44+
}
45+
46+
function ModalOneScreen() {
47+
return (
48+
<View style={[styles.container, { backgroundColor: Colors.BlueLight20 }]}>
49+
<Text style={styles.title}>This is Modal One</Text>
50+
</View>
51+
);
52+
}
53+
54+
function ModalTwoScreen() {
55+
return (
56+
<View style={[styles.container, { backgroundColor: Colors.GreenLight20 }]}>
57+
<Text style={styles.title}>This is Modal Two</Text>
58+
</View>
59+
);
60+
}
61+
62+
const Stack = createNativeStackNavigator<RouteParamList>();
63+
64+
export default function App() {
65+
return (
66+
<NavigationContainer>
67+
<Stack.Navigator>
68+
<Stack.Screen name="Main" component={MainScreen} />
69+
<Stack.Group screenOptions={{ presentation: 'modal' }}>
70+
<Stack.Screen name="ModalOne" component={ModalOneScreen} />
71+
<Stack.Screen name="ModalTwo" component={ModalTwoScreen} />
72+
</Stack.Group>
73+
</Stack.Navigator>
74+
</NavigationContainer>
75+
);
76+
}
77+
78+
const styles = StyleSheet.create({
79+
container: {
80+
flex: 1,
81+
alignItems: 'center',
82+
justifyContent: 'center',
83+
padding: 20,
84+
},
85+
title: {
86+
fontSize: 24,
87+
marginBottom: 16,
88+
},
89+
buttonContainer: {
90+
marginVertical: 10,
91+
},
92+
});

apps/src/tests/issue-tests/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,7 @@ export { default as Test3576 } from './Test3576';
181181
export { default as Test3596 } from './Test3596';
182182
export { default as Test3611 } from './Test3611';
183183
export { default as Test3617 } from './Test3617';
184+
export { default as Test3760 } from './Test3760';
184185
export { default as TestScreenAnimation } from './TestScreenAnimation';
185186
// The following test was meant to demo the "go back" gesture using Reanimated
186187
// but the associated PR in react-navigation is currently put on hold

ios/RNSScreen.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ namespace react = facebook::react;
6060
- (CGFloat)calculateHeaderHeightIsModal:(BOOL)isModal;
6161
#endif
6262
- (BOOL)isRemovedFromParent;
63+
- (void)notifyPresentedControllerDismissed;
6364

6465
@end
6566

ios/RNSScreen.mm

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -873,6 +873,8 @@ - (void)presentationControllerDidDismiss:(UIPresentationController *)presentatio
873873
[RNSScreenView.viewInteractionManagerInstance enableInteractionsForLastSubtree];
874874
}
875875

876+
[_controller notifyPresentedControllerDismissed];
877+
876878
if ([_reactSuperview respondsToSelector:@selector(presentationControllerDidDismiss:)]) {
877879
[_reactSuperview performSelector:@selector(presentationControllerDidDismiss:) withObject:presentationController];
878880
}
@@ -1945,6 +1947,11 @@ - (BOOL)isRemovedFromParent
19451947
return _isRemovedFromParent;
19461948
}
19471949

1950+
- (void)notifyPresentedControllerDismissed
1951+
{
1952+
_isRemovedFromParent = YES;
1953+
}
1954+
19481955
#pragma mark - transition progress related methods
19491956

19501957
- (void)setupProgressNotification

ios/RNSScreenStack.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ NS_ASSUME_NONNULL_BEGIN
4444
@property (nonatomic) BOOL disableSwipeBack;
4545

4646
@property (nonatomic, readwrite) BOOL iosPreventReattachmentOfDismissedScreens;
47+
@property (nonatomic, readwrite) BOOL iosPreventReattachmentOfDismissedModals;
4748

4849
#ifdef RCT_NEW_ARCH_ENABLED
4950
#else

ios/RNSScreenStack.mm

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,7 @@ - (void)initCommonProps
253253
_controller.delegate = self;
254254
_sinkEventsPanGestureRecognizer = [[UIPanGestureRecognizer alloc] init];
255255
_iosPreventReattachmentOfDismissedScreens = YES;
256+
_iosPreventReattachmentOfDismissedModals = YES;
256257
#if !TARGET_OS_TV && !TARGET_OS_VISION
257258
[self setupGestureHandlers];
258259
#endif
@@ -780,6 +781,17 @@ - (void)updateContainer
780781
}
781782
[pushControllers addObject:screen.controller];
782783
} else {
784+
/// When a Modal is dismissed natively the event is sent to JS **asynchronously**.
785+
/// If multiple modals are pushed/dismissed quickly, JS might send back
786+
/// a delayed update containing modals that were already dismissed. Attempting to render this
787+
/// stale state could force UIKit to illegally reshuffle presented controllers.
788+
/// We're preventing this, identifying reshuffling as an invalid state.
789+
/// Since view recycling is disabled, once we detect that a modal has been removed from the view
790+
/// hierarchy, it won't be reused. This allows us to safely filter out dismissed modal from modals coming
791+
/// from JS state via `controllers`.
792+
if (_iosPreventReattachmentOfDismissedModals && screen.controller.isRemovedFromParent) {
793+
continue;
794+
}
783795
[modalControllers addObject:screen.controller];
784796
}
785797
}
@@ -1378,6 +1390,11 @@ - (void)updateProps:(const facebook::react::Props::Shared &)props
13781390
[self setIosPreventReattachmentOfDismissedScreens:newScreenProps.iosPreventReattachmentOfDismissedScreens];
13791391
}
13801392

1393+
if (newScreenProps.iosPreventReattachmentOfDismissedModals !=
1394+
oldScreenProps.iosPreventReattachmentOfDismissedModals) {
1395+
[self setIosPreventReattachmentOfDismissedModals:newScreenProps.iosPreventReattachmentOfDismissedModals];
1396+
}
1397+
13811398
[super updateProps:props oldProps:oldProps];
13821399
}
13831400

@@ -1535,6 +1552,7 @@ @implementation RNSScreenStackManager {
15351552

15361553
RCT_EXPORT_VIEW_PROPERTY(onFinishTransitioning, RCTDirectEventBlock);
15371554
RCT_EXPORT_VIEW_PROPERTY(iosPreventReattachmentOfDismissedScreens, BOOL);
1555+
RCT_EXPORT_VIEW_PROPERTY(iosPreventReattachmentOfDismissedModals, BOOL);
15381556

15391557
#ifdef RCT_NEW_ARCH_ENABLED
15401558
#else

src/components/ScreenStack.tsx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,9 @@ function ScreenStack(props: ScreenStackProps) {
102102
iosPreventReattachmentOfDismissedScreens={
103103
featureFlags.experiment.iosPreventReattachmentOfDismissedScreens
104104
}
105+
iosPreventReattachmentOfDismissedModals={
106+
featureFlags.experiment.iosPreventReattachmentOfDismissedModals
107+
}
105108
/**
106109
* This messy override is to conform NativeProps used by codegen and
107110
* our Public API. To see reasoning go to this PR:

src/fabric/ScreenStackNativeComponent.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ type FinishTransitioningEvent = Readonly<{}>;
88

99
export interface NativeProps extends ViewProps {
1010
iosPreventReattachmentOfDismissedScreens?: CT.WithDefault<boolean, true>;
11+
iosPreventReattachmentOfDismissedModals?: CT.WithDefault<boolean, true>;
1112

1213
onFinishTransitioning?: CT.DirectEventHandler<FinishTransitioningEvent>;
1314
}

0 commit comments

Comments
 (0)