Skip to content

Commit ec7afd0

Browse files
authored
fix(Android): fix draw ordering in transparent modal & stack nested in tabs interaction (#2647)
## Description Fixes #2167 The exact error mechanism is **really** convoluted. The gist of it however, and the issue cause lies in the fact that our drawing / container updating logic worked under implicit (and unspoken of) assumption that draw reordering would not be applied for the transaction attaching very first screen in the stack. Everything worked correctly until #2019 caused `needsDrawReordeing` to return `true` **always when `Build.VERSION.SDK_INT > 33`** - and that means pretty much **always** in new apps. Previously it returned `false`, in particular for the very first screen on stack because no one really sets `stackAnimation` for the very first screen, since [it will have no animation anyway](https://github.com/software-mansion/react-native-screens/blob/c0b5586b7e645ed1d22143b5f84e0dd65dcd06be/android/src/main/java/com/swmansion/rnscreens/ScreenStack.kt#L137) (and we might enforce this somewhere in JS code also, I'm not sure now). This PR restores returning `false` there for first screen on the stack & for any screen that uses `animation: 'none'`. ### Summary of the error mechanism Consider following case: ```tsx function App() { return ( <Tabs> <Screen A> <Stack> <Screen SA /> <Screen TM /> </Stack> </Screen A> <Screen B /> </Tabs> ); } ``` Initially `Screen SA` is rendered. Basically when [`isDetachingCurrentScreen`] was set for the very first screen (directly because return value of `needsDrawReordeing`) and then we navigated to other tab `Screen B` - we cause whole stack `Stack` to be dropped & detached from window. Native callback `onDetachedFromWindow` gets called in `ScreenContainer`, we detach every fragment and subview (to prevent whole different class of bugs) causing `removeView` callbacks in `ScreenStack`, leading to `reverseLastTwoChildren` flag being set to `true`. When we then change tab back to `Screen SA` in `Stack` the drawing works as normal, because we have only one child. On navigation to `Screen TM` (transparent modal) value of the `reverseLastTwoChildren` flag causes the views to being drawn in wrong order - transparent modal first and `Screen SA` second. In case of not `transparent` presentation there is no issue, because `Screen SA` would get [detached](https://github.com/software-mansion/react-native-screens/blob/c0b5586b7e645ed1d22143b5f84e0dd65dcd06be/android/src/main/java/com/swmansion/rnscreens/ScreenStack.kt#L113-L115). ## Changes Added param to `needsDrawReordeing` method informing of actual stack animation in use (in case of first screen we always set it to `none`). When there is no animation for the disappearing screen - there is no need to change the draw ordering. Added appropriate code comment for the future. ## Test code and steps to reproduce `Test2167` ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes
1 parent c0b5586 commit ec7afd0

3 files changed

Lines changed: 87 additions & 7 deletions

File tree

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

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ class ScreenStack(
248248

249249
if (shouldUseOpenAnimation &&
250250
newTop != null &&
251-
needsDrawReordering(newTop) &&
251+
needsDrawReordering(newTop, stackAnimation) &&
252252
visibleBottom == null
253253
) {
254254
// When using an open animation in which two screens overlap (eg. fade_from_bottom or
@@ -258,6 +258,7 @@ class ScreenStack(
258258
// appears on top of the previous one. You can read more about in the comment
259259
// for the code we use to change that behavior:
260260
// https://github.com/airbnb/native-navigation/blob/9cf50bf9b751b40778f473f3b19fcfe2c4d40599/lib/android/src/main/java/com/airbnb/android/react/navigation/ScreenCoordinatorLayout.java#L18
261+
// Note: This should not be set in case there is only a single screen in stack or animation `none` is used. Atm needsDrawReordering implementation guards that assuming that first screen on stack uses `NONE` animation.
261262
isDetachingCurrentScreen = true
262263
}
263264

@@ -426,14 +427,22 @@ class ScreenStack(
426427
companion object {
427428
const val TAG = "ScreenStack"
428429

429-
private fun needsDrawReordering(fragmentWrapper: ScreenFragmentWrapper): Boolean =
430+
private fun needsDrawReordering(
431+
fragmentWrapper: ScreenFragmentWrapper,
432+
resolvedStackAnimation: StackAnimation?,
433+
): Boolean {
434+
val stackAnimation = if (resolvedStackAnimation != null) resolvedStackAnimation else fragmentWrapper.screen.stackAnimation
430435
// On Android sdk 33 and above the animation is different and requires draw reordering.
431436
// For React Native 0.70 and lower versions, `Build.VERSION_CODES.TIRAMISU` is not defined yet.
432437
// Hence, we're comparing numerical version here.
433-
Build.VERSION.SDK_INT >= 33 ||
434-
fragmentWrapper.screen.stackAnimation === StackAnimation.SLIDE_FROM_BOTTOM ||
435-
fragmentWrapper.screen.stackAnimation === StackAnimation.FADE_FROM_BOTTOM ||
436-
fragmentWrapper.screen.stackAnimation === StackAnimation.IOS_FROM_RIGHT ||
437-
fragmentWrapper.screen.stackAnimation === StackAnimation.IOS_FROM_LEFT
438+
return (
439+
Build.VERSION.SDK_INT >= 33 ||
440+
stackAnimation === StackAnimation.SLIDE_FROM_BOTTOM ||
441+
stackAnimation === StackAnimation.FADE_FROM_BOTTOM ||
442+
stackAnimation === StackAnimation.IOS_FROM_RIGHT ||
443+
stackAnimation === StackAnimation.IOS_FROM_LEFT
444+
) &&
445+
stackAnimation !== StackAnimation.NONE
446+
}
438447
}
439448
}

apps/src/tests/Test2167.tsx

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
import React from 'react';
2+
import { Text, SafeAreaView, Pressable, View } from 'react-native';
3+
import { NavigationContainer, ParamListBase } from '@react-navigation/native';
4+
import { NativeStackNavigationProp, createNativeStackNavigator } from '@react-navigation/native-stack';
5+
import { createBottomTabNavigator } from '@react-navigation/bottom-tabs';
6+
7+
type NavigationProps = {
8+
navigation: NativeStackNavigationProp<ParamListBase>;
9+
}
10+
11+
const NestedStack = createNativeStackNavigator();
12+
const Tabs = createBottomTabNavigator();
13+
14+
function ModalScreen({ navigation }: NavigationProps) {
15+
return <SafeAreaView style={{ backgroundColor: 'green', flex: 1, alignItems: 'center', justifyContent: 'center' }}>
16+
<Text>Hello from modal screen</Text>
17+
<Pressable onPress={navigation.goBack}><Text>Go Back</Text></Pressable>
18+
</SafeAreaView>;
19+
}
20+
21+
function Content({ navigation }: NavigationProps) {
22+
const showTransparentModal = React.useCallback(() => {
23+
console.log('showTransparentModal pressed');
24+
navigation.navigate('ModalScreen');
25+
}, [navigation]);
26+
27+
return (
28+
<View style={{ flex: 1, alignItems: 'center', justifyContent: 'center' }}>
29+
<Text>Home Screen</Text>
30+
<Pressable onPress={showTransparentModal}><Text>Open Modal Screen</Text></Pressable>
31+
</View>
32+
);
33+
}
34+
35+
function HomeScreen() {
36+
return (
37+
<NestedStack.Navigator>
38+
<NestedStack.Screen name="HomeScreen" component={Content} />
39+
<NestedStack.Screen name="ModalScreen" component={ModalScreen}
40+
options={{ headerShown: false, presentation: 'transparentModal' }} />
41+
</NestedStack.Navigator>
42+
);
43+
}
44+
45+
function OtherScreen({ navigation }: NavigationProps) {
46+
return (
47+
<Pressable onPress={navigation.goBack}><Text>
48+
Go back
49+
</Text></Pressable>
50+
);
51+
}
52+
53+
54+
function TabStack() {
55+
return (
56+
<Tabs.Navigator>
57+
<Tabs.Screen name="HomeTab" component={HomeScreen} />
58+
<Tabs.Screen name="Other" component={OtherScreen} />
59+
</Tabs.Navigator>
60+
);
61+
}
62+
63+
export default function App() {
64+
return (
65+
<NavigationContainer>
66+
<TabStack />
67+
</NavigationContainer>
68+
);
69+
}
70+

apps/src/tests/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ export { default as Test2028 } from './Test2028';
101101
export { default as Test2048 } from './Test2048';
102102
export { default as Test2069 } from './Test2069';
103103
export { default as Test2118 } from './Test2118';
104+
export { default as Test2167 } from './Test2167';
104105
export { default as Test2175 } from './Test2175';
105106
export { default as Test2184 } from './Test2184';
106107
export { default as Test2223 } from './Test2223';

0 commit comments

Comments
 (0)