fix(iOS): header snapshots not working (#2393)

The `updateViewControllerIfNeeded` call introduced by #2230 forces the view controller to rebuild the subviews with the recent config. When the screen is being unmounted it replaces the subviews with nil values as the `reactSubviews` are removed from the config one by one. Snapshots made earlier get discarded in the process. This PR adds a condition that prevents updating the view controller when the screen is being changed + stops unnecessary snapshots when the screen is not changed. - updated `Test556.tsx` repro - added `isGoingToBeRemoved` property to `RNSScreenView` - making snapshots / updating the view controller conditionally  - Use `Test556.tsx` repro - [x] Included code example that can be used to test this change - [x] Ensured that CI passes --------- Co-authored-by: Kacper Kafara <[email protected]> Co-authored-by: Kacper Kafara <[email protected]> (cherry picked from commit e4333a1)
software-mansion · Oct 25, 2024 · bb86f5b · bb86f5b
1 parent 5a9afbb
commit bb86f5b
Show file tree

Hide file tree

Showing 5 changed files with 99 additions and 34 deletions.
diff --git a/apps/src/tests/Test556.tsx b/apps/src/tests/Test556.tsx
@@ -1,13 +1,17 @@
 import * as React from 'react';
 import { Button, StyleSheet, Text, View } from 'react-native';
 import { NavigationContainer, ParamListBase } from '@react-navigation/native';
-import { NativeStackNavigationProp, createNativeStackNavigator } from '@react-navigation/native-stack';
+import {
+  NativeStackNavigationProp,
+  createNativeStackNavigator,
+} from '@react-navigation/native-stack';
+import { Square } from '../shared';
 
 const Stack = createNativeStackNavigator();
 
 type ScreenBaseProps = {
   navigation: NativeStackNavigationProp<ParamListBase>;
-}
+};
 
 export default function App() {
   return (
@@ -16,30 +20,48 @@ export default function App() {
         screenOptions={{
           animation: 'fade',
         }}>
-        <Stack.Screen name="First" component={First} options={{
-          headerTitle: () => (
-            <View style={[styles.container, { backgroundColor: 'goldenrod' }]}>
-              <Text>Hello there!</Text>
-            </View>
-          ),
-          headerRight: () => (
-            <View style={[styles.container, { backgroundColor: 'lightblue' }]}>
-              <Text>Right-1</Text>
-            </View>
-          ),
-        }} />
-        <Stack.Screen name="Second" component={Second} options={{
-          headerTitle: () => (
-            <View style={[styles.container, { backgroundColor: 'mediumseagreen' }]}>
-              <Text>General Kenobi</Text>
-            </View>
-          ),
-          headerRight: () => (
-            <View style={[styles.container, { backgroundColor: 'mediumvioletred' }]}>
-              <Text>Right-2</Text>
-            </View>
-          ),
-        }} />
+        <Stack.Screen
+          name="First"
+          component={First}
+          options={{
+            headerTitle: () => (
+              <View
+                style={[styles.container, { backgroundColor: 'goldenrod' }]}>
+                <Text>Hello there!</Text>
+              </View>
+            ),
+            headerRight: () => (
+              <View
+                style={[styles.container, { backgroundColor: 'lightblue' }]}>
+                <Text>Right-1</Text>
+              </View>
+            ),
+          }}
+        />
+        <Stack.Screen
+          name="Second"
+          component={Second}
+          options={{
+            headerTitle: () => (
+              <View
+                style={[
+                  styles.container,
+                  { backgroundColor: 'mediumseagreen' },
+                ]}>
+                <Text>General Kenobi</Text>
+              </View>
+            ),
+            headerRight: () => (
+              <View
+                style={[
+                  styles.container,
+                  { backgroundColor: 'mediumvioletred' },
+                ]}>
+                <Text>Right-2</Text>
+              </View>
+            ),
+          }}
+        />
       </Stack.Navigator>
     </NavigationContainer>
   );
@@ -55,11 +77,34 @@ function First({ navigation }: ScreenBaseProps) {
 }
 
 function Second({ navigation }: ScreenBaseProps) {
+  const [backButtonVisible, setBackButtonVisible] = React.useState(true);
+
   return (
-    <Button
-      title="Tap me for first screen"
-      onPress={() => navigation.popTo('First')}
-    />
+    <>
+      <Button
+        title="Toggle left subview"
+        onPress={() => {
+          setBackButtonVisible(prev => !prev);
+          navigation.setOptions({
+            headerLeft: backButtonVisible
+              ? () => (
+                  <View
+                    style={[
+                      styles.container,
+                      { backgroundColor: 'mediumblue' },
+                    ]}>
+                    <Text>Left</Text>
+                  </View>
+                )
+              : undefined,
+          });
+        }}
+      />
+      <Button
+        title="Tap me for first screen"
+        onPress={() => navigation.popTo('First')}
+      />
+    </>
   );
 }
 

diff --git a/ios/RNSScreen.h b/ios/RNSScreen.h
@@ -99,6 +99,8 @@ namespace react = facebook::react;
 @property (nonatomic) react::LayoutMetrics newLayoutMetrics;
 @property (weak, nonatomic) RNSScreenStackHeaderConfig *config;
 @property (nonatomic, readonly) BOOL hasHeaderConfig;
+@property (nonatomic, readonly, getter=isMarkedForUnmountInCurrentTransaction)
+    BOOL markedForUnmountInCurrentTransaction;
 #else
 @property (nonatomic, copy) RCTDirectEventBlock onAppear;
 @property (nonatomic, copy) RCTDirectEventBlock onDisappear;
@@ -122,6 +124,10 @@ namespace react = facebook::react;
 - (void)updateBounds;
 - (void)notifyDismissedWithCount:(int)dismissCount;
 - (instancetype)initWithFrame:(CGRect)frame;
+/// Tell `Screen` that it will be unmounted in next transaction.
+/// The component needs this so that we can later decide whether to
+/// replace it with snapshot or not.
+- (void)willBeUnmountedInUpcomingTransaction;
 #else
 - (instancetype)initWithBridge:(RCTBridge *)bridge;
 #endif // RCT_NEW_ARCH_ENABLED

diff --git a/ios/RNSScreen.mm b/ios/RNSScreen.mm
@@ -98,6 +98,9 @@ - (void)initCommonProps
 #if !TARGET_OS_TV
   _sheetExpandsWhenScrolledToEdge = YES;
 #endif // !TARGET_OS_TV
+#ifdef RCT_NEW_ARCH_ENABLED
+  _markedForUnmountInCurrentTransaction = NO;
+#endif // RCT_NEW_ARCH_ENABLED
 }
 
 - (UIViewController *)reactViewController
@@ -690,6 +693,11 @@ - (BOOL)hasHeaderConfig
   return _config != nil;
 }
 
+- (void)willBeUnmountedInUpcomingTransaction
+{
+  _markedForUnmountInCurrentTransaction = YES;
+}
+
 + (react::ComponentDescriptorProvider)componentDescriptorProvider
 {
   return react::concreteComponentDescriptorProvider<react::RNSScreenComponentDescriptor>();

diff --git a/ios/RNSScreenStack.mm b/ios/RNSScreenStack.mm
@@ -1175,6 +1175,7 @@ - (void)mountingTransactionWillMount:(const facebook::react::MountingTransaction
     if (mutation.type == react::ShadowViewMutation::Delete) {
       RNSScreenView *_Nullable toBeRemovedChild = [self childScreenForTag:mutation.oldChildShadowView.tag];
       if (toBeRemovedChild != nil) {
+        [toBeRemovedChild willBeUnmountedInUpcomingTransaction];
         _toBeDeletedScreens.push_back(toBeRemovedChild);
       }
     }

diff --git a/ios/RNSScreenStackHeaderConfig.mm b/ios/RNSScreenStackHeaderConfig.mm
@@ -768,12 +768,17 @@ - (void)mountChildComponentView:(UIView<RCTComponentViewProtocol> *)childCompone
 
 - (void)unmountChildComponentView:(UIView<RCTComponentViewProtocol> *)childComponentView index:(NSInteger)index
 {
-  // For explanation of why we can make a snapshot here despite the fact that our children are already
-  // unmounted see https://github.com/software-mansion/react-native-screens/pull/2261
-  [self replaceNavigationBarViewsWithSnapshotOfSubview:(RNSScreenStackHeaderSubview *)childComponentView];
+  BOOL isGoingToBeRemoved = _screenView.isMarkedForUnmountInCurrentTransaction;
+  if (isGoingToBeRemoved) {
+    // For explanation of why we can make a snapshot here despite the fact that our children are already
+    // unmounted see https://github.com/software-mansion/react-native-screens/pull/2261
+    [self replaceNavigationBarViewsWithSnapshotOfSubview:(RNSScreenStackHeaderSubview *)childComponentView];
+  }
   [_reactSubviews removeObject:(RNSScreenStackHeaderSubview *)childComponentView];
   [childComponentView removeFromSuperview];
-  [self updateViewControllerIfNeeded];
+  if (!isGoingToBeRemoved) {
+    [self updateViewControllerIfNeeded];
+  }
 }
 
 - (void)replaceNavigationBarViewsWithSnapshotOfSubview:(RNSScreenStackHeaderSubview *)childComponentView