Changes fragment commit to synchronous

TestsExample/App.js

@@ -51,6 +51,7 @@ import Test887 from './src/Test887';
 import Test898 from './src/Test898';
 import Test913 from './src/Test913';
 import Test999 from './src/Test999';
+import Test1017 from './src/Test1017';
 import Test1031 from './src/Test1031';
 import Test1032 from './src/Test1032';
 import Test1036 from './src/Test1036';

TestsExample/src/Test1017.tsx

@@ -0,0 +1,146 @@
+import {NavigationContainer, RouteProp} from '@react-navigation/native';
+import {
+  createStackNavigator,
+  StackNavigationProp,
+  TransitionPresets,
+} from '@react-navigation/stack';
+import React from 'react';
+import {Alert, LogBox} from 'react-native';
+import {StyleSheet, Text, View, FlatList, Pressable} from 'react-native';
+import {ScrollView} from 'react-native-gesture-handler';
+
+import {
+  Header,
+  Colors,
+  DebugInstructions,
+} from 'react-native/Libraries/NewAppScreen';
+
+type DataItem = {
+  title: string;
+  value: number;
+};
+
+enum Route {
+  FirstScreen = 'FirstScreen',
+  SecondScreen = 'SecondScreen',
+}
+
+type RootStackParamList = {
+  [Route.FirstScreen]: undefined;
+  [Route.SecondScreen]: undefined;
+};
+
+LogBox.ignoreLogs([
+  'VirtualizedLists should never be nested inside plain ScrollViews',
+]);
+
+const Stack = createStackNavigator();
+
+const dataset = Array.from<unknown, DataItem>({length: 100}, (_, i) => ({
+  title: `Title ${i}`,
+  value: i,
+}));
+
+const Screen: React.FC<{
+  route: RouteProp<RootStackParamList, any>;
+  navigation: StackNavigationProp<RootStackParamList, any>;
+}> = ({route, navigation}) => {
+  for (let i = 0; i < 10 ** 3; i++) {}
+  const isSecondScreen = route.name === Route.SecondScreen;
+
+  const handleItemPress = (item: DataItem) => () => {
+    if (isSecondScreen) {
+      Alert.alert(item.title, item.value.toString());
+      return;
+    }
+    navigation.navigate(Route.SecondScreen);
+  };
+
+  const renderItem = (item: DataItem) => (
+    <Pressable
+      android_ripple={{color: Colors.light}}
+      style={styles.listItemContainer}
+      onPress={handleItemPress(item)}>
+      <View style={styles.listItemContentWrapper}>
+        <Text style={styles.listItemTitle}>{item.title}</Text>
+        <Text>{`This is item number ${item.value}`}</Text>
+      </View>
+    </Pressable>
+  );
+
+  return (
+    // So in a much more complex scenario, we might have a ScrollView that contains multiple horizontal list
+    // I know it's bad to have a FlatList nested in ScrollView, ideally I should use something better
+    // However, it seems like having a nested VirtualizeList in ScrollView will cause the transition lag issue
+    <ScrollView>
+      <FlatList
+        data={dataset}
+        keyExtractor={(item) => item.value.toString()}
+        renderItem={({item}) => renderItem(item)}
+        ItemSeparatorComponent={() => <View style={styles.separator} />}
+        ListHeaderComponent={!isSecondScreen && Header}
+        ListHeaderComponentStyle={styles.headerWrapper}
+        ListFooterComponent={!isSecondScreen && DebugInstructions}
+        ListFooterComponentStyle={styles.footerWrapper}
+        scrollEnabled={false}
+      />
+    </ScrollView>
+  );
+};
+
+const App: React.FC = () => {
+  return (
+    <NavigationContainer>
+      <Stack.Navigator screenOptions={{...TransitionPresets.SlideFromRightIOS}}>
+        <Stack.Screen
+          name={Route.FirstScreen}
+          component={Screen}
+          options={{title: 'Sample List'}}
+        />
+        <Stack.Screen
+          name={Route.SecondScreen}
+          component={Screen}
+          options={{title: 'Sample List 2'}}
+        />
+      </Stack.Navigator>
+    </NavigationContainer>
+  );
+};
+
+const styles = StyleSheet.create({
+  headerWrapper: {
+    overflow: 'hidden',
+  },
+  footerWrapper: {
+    paddingVertical: 50 / 3,
+    paddingHorizontal: 40 / 3,
+  },
+  listItemContainer: {
+    flexDirection: 'row',
+    alignItems: 'center',
+    paddingHorizontal: 50 / 3,
+    paddingVertical: 40 / 3,
+  },
+  image: {
+    width: 200 / 3,
+    aspectRatio: 1,
+    borderRadius: 25 / 3,
+    backgroundColor: Colors.primary,
+  },
+  listItemContentWrapper: {
+    flexDirection: 'column',
+    paddingHorizontal: 30 / 3,
+  },
+  listItemTitle: {
+    fontWeight: 'bold',
+    fontSize: 50 / 3,
+  },
+  separator: {
+    height: 1 / 3,
+    flex: 1,
+    marginHorizontal: 50 / 3,
+    backgroundColor: '#000',
+  },
+});
+
+export default App;

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

@@ -14,22 +14,13 @@ import com.facebook.react.ReactRootView
 import com.facebook.react.modules.core.ChoreographerCompat
 import com.facebook.react.modules.core.ReactChoreographer
 import com.swmansion.rnscreens.Screen.ActivityState
-import java.lang.IllegalStateException
 
 open class ScreenContainer<T : ScreenFragment>(context: Context?) : ViewGroup(context) {
     @JvmField
     protected val mScreenFragments = ArrayList<T>()
     @JvmField
     protected var mFragmentManager: FragmentManager? = null
-    private var mCurrentTransaction: FragmentTransaction? = null
-    private var mProcessingTransaction: FragmentTransaction? = null
-    private var mNeedUpdate = false
     private var mIsAttached = false
-    private val mFrameCallback: ChoreographerCompat.FrameCallback = object : ChoreographerCompat.FrameCallback() {
-        override fun doFrame(frameTimeNanos: Long) {
-            updateIfNeeded()
-        }
-    }
     private var mLayoutEnqueued = false
     private val mLayoutCallback: ChoreographerCompat.FrameCallback = object : ChoreographerCompat.FrameCallback() {
         override fun doFrame(frameTimeNanos: Long) {
@@ -90,15 +81,7 @@ open class ScreenContainer<T : ScreenFragment>(context: Context?) : ViewGroup(co
         get() = mParentScreenFragment != null
 
     protected fun markUpdated() {
-        if (!mNeedUpdate) {
-            mNeedUpdate = true
-            // enqueue callback of NATIVE_ANIMATED_MODULE type as all view operations are executed in
-            // DISPATCH_UI type and we want the callback to be called right after in the same frame.
-            ReactChoreographer.getInstance()
-                .postFrameCallback(
-                    ReactChoreographer.CallbackType.NATIVE_ANIMATED_MODULE, mFrameCallback
-                )
-        }
+        updateIfNeeded()
     }
 
     fun notifyChildUpdate() {
@@ -189,47 +172,23 @@ open class ScreenContainer<T : ScreenFragment>(context: Context?) : ViewGroup(co
         setFragmentManager(context.supportFragmentManager)
     }
 
-    protected fun getOrCreateTransaction(): FragmentTransaction {
-        if (mCurrentTransaction == null) {
-            val fragmentManager = requireNotNull(mFragmentManager, { "mFragmentManager is null when creating transaction" })
-            val transaction = fragmentManager.beginTransaction()
-            transaction.setReorderingAllowed(true)
-            mCurrentTransaction = transaction
-        }
-        mCurrentTransaction?.let { return it }
-        throw IllegalStateException("mCurrentTransaction changed to null during creating transaction")
-    }
-
-    protected fun tryCommitTransaction() {
-        val transaction = mCurrentTransaction
-        if (transaction != null) {
-            mProcessingTransaction = transaction
-            mProcessingTransaction?.runOnCommit {
-                if (mProcessingTransaction === transaction) {
-                    // we need to take into account that commit is initiated with some other transaction
-                    // while the previous one is still processing. In this case mProcessingTransaction
-                    // gets overwritten and we don't want to set it to null until the second transaction
-                    // is finished.
-                    mProcessingTransaction = null
-                }
-            }
-            transaction.commitAllowingStateLoss()
-            mCurrentTransaction = null
-        }
+    protected fun createTransaction(): FragmentTransaction {
+        val fragmentManager = requireNotNull(mFragmentManager, { "mFragmentManager is null when creating transaction" })
+        val transaction = fragmentManager.beginTransaction()
+        transaction.setReorderingAllowed(true)
+        return transaction
     }
 
-    private fun attachScreen(screenFragment: T) {
-        getOrCreateTransaction().add(id, screenFragment)
+    private fun attachScreen(screenFragment: ScreenFragment) {
+        createTransaction().add(id, screenFragment).commitNowAllowingStateLoss()
     }
 
     private fun moveToFront(screenFragment: ScreenFragment) {
-        val transaction = getOrCreateTransaction()
-        transaction.remove(screenFragment)
-        transaction.add(id, screenFragment)
+        createTransaction().remove(screenFragment).add(id, screenFragment).commitNowAllowingStateLoss()
     }
 
     private fun detachScreen(screenFragment: ScreenFragment) {
-        getOrCreateTransaction().remove(screenFragment)
+        createTransaction().remove(screenFragment).commitNowAllowingStateLoss()
     }
 
     private fun getActivityState(screenFragment: ScreenFragment): ActivityState? {
@@ -243,7 +202,6 @@ open class ScreenContainer<T : ScreenFragment>(context: Context?) : ViewGroup(co
     override fun onAttachedToWindow() {
         super.onAttachedToWindow()
         mIsAttached = true
-        mNeedUpdate = true
         setupFragmentManager()
     }
 
@@ -312,23 +270,13 @@ open class ScreenContainer<T : ScreenFragment>(context: Context?) : ViewGroup(co
     }
 
     private fun updateIfNeeded() {
-        if (!mNeedUpdate || !mIsAttached || mFragmentManager == null) {
+        if (!mIsAttached || mFragmentManager == null) {
             return
         }
-        mNeedUpdate = false
         onUpdate()
     }
 
     private fun onUpdate() {
-        // We double check if fragment manager have any pending transactions to run.
-        // In performUpdate we often check whether some fragments are added to
-        // manager to avoid adding them for the second time (which result in crash).
-        // By design performUpdate should be called at most once per frame, so this
-        // should never happen, but in case there are some pending transaction we
-        // need to flush them here such that Fragment#isAdded checks reflect the
-        // reality and that we don't have enqueued fragment add commands that will
-        // execute shortly and cause "Fragment already added" crash.
-        mFragmentManager?.executePendingTransactions()
         performUpdate()
         notifyContainerUpdate()
     }
@@ -372,7 +320,6 @@ open class ScreenContainer<T : ScreenFragment>(context: Context?) : ViewGroup(co
             }
             screenFragment.screen.setTransitioning(transitioning)
         }
-        tryCommitTransaction()
     }
 
     protected open fun notifyContainerUpdate() {

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

@@ -185,7 +185,7 @@ class ScreenStack(context: Context?) : ScreenContainer<ScreenStackFragment>(cont
             stackAnimation = mTopScreen?.screen?.stackAnimation
         }
 
-        getOrCreateTransaction().let {
+        createTransaction().let {
             // animation logic start
             if (stackAnimation != null) {
                 if (shouldUseOpenAnimation) {
@@ -278,7 +278,7 @@ class ScreenStack(context: Context?) : ScreenContainer<ScreenStackFragment>(cont
             mTopScreen = newTop
             mStack.clear()
             mStack.addAll(mScreenFragments)
-            tryCommitTransaction()
+            it.commitNowAllowingStateLoss()
             mTopScreen?.let { screen -> setupBackHandlerIfNeeded(screen) }
         }
     }
@@ -338,7 +338,7 @@ class ScreenStack(context: Context?) : ScreenContainer<ScreenStackFragment>(cont
                     .show(topScreen)
                     .addToBackStack(BACK_STACK_TAG)
                     .setPrimaryNavigationFragment(topScreen)
-                    .commitAllowingStateLoss()
+                    .commitNowAllowingStateLoss()
                 it.addOnBackStackChangedListener(mBackStackListener)
             }
         }