Skip to content

Commit

Permalink
fix(Android): fix draw ordering in transparent modal & stack nested i…
Browse files Browse the repository at this point in the history
…n 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
  • Loading branch information
kkafar authored Jan 24, 2025
1 parent c0b5586 commit ec7afd0
Show file tree
Hide file tree
Showing 3 changed files with 87 additions and 7 deletions.
23 changes: 16 additions & 7 deletions android/src/main/java/com/swmansion/rnscreens/ScreenStack.kt
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ class ScreenStack(

if (shouldUseOpenAnimation &&
newTop != null &&
needsDrawReordering(newTop) &&
needsDrawReordering(newTop, stackAnimation) &&
visibleBottom == null
) {
// When using an open animation in which two screens overlap (eg. fade_from_bottom or
Expand All @@ -258,6 +258,7 @@ class ScreenStack(
// appears on top of the previous one. You can read more about in the comment
// for the code we use to change that behavior:
// https://github.com/airbnb/native-navigation/blob/9cf50bf9b751b40778f473f3b19fcfe2c4d40599/lib/android/src/main/java/com/airbnb/android/react/navigation/ScreenCoordinatorLayout.java#L18
// 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.
isDetachingCurrentScreen = true
}

Expand Down Expand Up @@ -426,14 +427,22 @@ class ScreenStack(
companion object {
const val TAG = "ScreenStack"

private fun needsDrawReordering(fragmentWrapper: ScreenFragmentWrapper): Boolean =
private fun needsDrawReordering(
fragmentWrapper: ScreenFragmentWrapper,
resolvedStackAnimation: StackAnimation?,
): Boolean {
val stackAnimation = if (resolvedStackAnimation != null) resolvedStackAnimation else fragmentWrapper.screen.stackAnimation
// On Android sdk 33 and above the animation is different and requires draw reordering.
// For React Native 0.70 and lower versions, `Build.VERSION_CODES.TIRAMISU` is not defined yet.
// Hence, we're comparing numerical version here.
Build.VERSION.SDK_INT >= 33 ||
fragmentWrapper.screen.stackAnimation === StackAnimation.SLIDE_FROM_BOTTOM ||
fragmentWrapper.screen.stackAnimation === StackAnimation.FADE_FROM_BOTTOM ||
fragmentWrapper.screen.stackAnimation === StackAnimation.IOS_FROM_RIGHT ||
fragmentWrapper.screen.stackAnimation === StackAnimation.IOS_FROM_LEFT
return (
Build.VERSION.SDK_INT >= 33 ||
stackAnimation === StackAnimation.SLIDE_FROM_BOTTOM ||
stackAnimation === StackAnimation.FADE_FROM_BOTTOM ||
stackAnimation === StackAnimation.IOS_FROM_RIGHT ||
stackAnimation === StackAnimation.IOS_FROM_LEFT
) &&
stackAnimation !== StackAnimation.NONE
}
}
}
70 changes: 70 additions & 0 deletions apps/src/tests/Test2167.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import React from 'react';
import { Text, SafeAreaView, Pressable, View } from 'react-native';
import { NavigationContainer, ParamListBase } from '@react-navigation/native';
import { NativeStackNavigationProp, createNativeStackNavigator } from '@react-navigation/native-stack';
import { createBottomTabNavigator } from '@react-navigation/bottom-tabs';

type NavigationProps = {
navigation: NativeStackNavigationProp<ParamListBase>;
}

const NestedStack = createNativeStackNavigator();
const Tabs = createBottomTabNavigator();

function ModalScreen({ navigation }: NavigationProps) {
return <SafeAreaView style={{ backgroundColor: 'green', flex: 1, alignItems: 'center', justifyContent: 'center' }}>
<Text>Hello from modal screen</Text>
<Pressable onPress={navigation.goBack}><Text>Go Back</Text></Pressable>
</SafeAreaView>;
}

function Content({ navigation }: NavigationProps) {
const showTransparentModal = React.useCallback(() => {
console.log('showTransparentModal pressed');
navigation.navigate('ModalScreen');
}, [navigation]);

return (
<View style={{ flex: 1, alignItems: 'center', justifyContent: 'center' }}>
<Text>Home Screen</Text>
<Pressable onPress={showTransparentModal}><Text>Open Modal Screen</Text></Pressable>
</View>
);
}

function HomeScreen() {
return (
<NestedStack.Navigator>
<NestedStack.Screen name="HomeScreen" component={Content} />
<NestedStack.Screen name="ModalScreen" component={ModalScreen}
options={{ headerShown: false, presentation: 'transparentModal' }} />
</NestedStack.Navigator>
);
}

function OtherScreen({ navigation }: NavigationProps) {
return (
<Pressable onPress={navigation.goBack}><Text>
Go back
</Text></Pressable>
);
}


function TabStack() {
return (
<Tabs.Navigator>
<Tabs.Screen name="HomeTab" component={HomeScreen} />
<Tabs.Screen name="Other" component={OtherScreen} />
</Tabs.Navigator>
);
}

export default function App() {
return (
<NavigationContainer>
<TabStack />
</NavigationContainer>
);
}

1 change: 1 addition & 0 deletions apps/src/tests/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ export { default as Test2028 } from './Test2028';
export { default as Test2048 } from './Test2048';
export { default as Test2069 } from './Test2069';
export { default as Test2118 } from './Test2118';
export { default as Test2167 } from './Test2167';
export { default as Test2175 } from './Test2175';
export { default as Test2184 } from './Test2184';
export { default as Test2223 } from './Test2223';
Expand Down

0 comments on commit ec7afd0

Please sign in to comment.