From ca6c9dc7dd95499f9d0ac57ba6d4e0feaae95eb3 Mon Sep 17 00:00:00 2001 From: Kacper Kafara Date: Wed, 19 Jul 2023 15:08:53 +0200 Subject: [PATCH] fix(iOS): buggy search bar / large title behaviour on Fabric (#1825) ## Description `UIKit` requires `ScrollView` to be at index 0 in given view's subview array to enable it's interaction with navigation bar. On Fabric our `MaybeNestedStack` view got flattened, however it was not removed from hierarchy -- instead the view was attached as first child of the `RNSScreenView` disabling system interaction between navigation bar and a scroll view. ## Changes 1. Added `collapsable={false}` to `MaybeNestedStack` view, preventing it from being flattened by React Native. 2. Moved `HeaderConfig` component "down" -- so it is attached as second child of a `Screen`. It was necessary, because currently on Fabric we are adding `RNSScreenStackHeaderConfig` into subviews of the `Screen` and it also interrupted the interaction. 3. Refactored code that relied on `RNSScreenStackHeaderConfig` being first child of a `Screen` (if present at all). Initially I've played around with fixing (modyfing) the view hierarchy on the native side just by removing / adding some views when scroll view was being attached under the `RNSScreenView`, however such approach led to cascade of bugs (fixing first, caused another one and so on). We can assume that in general discrepancy between native & shadow tree leads to unpleasant bugs. Possible alternative solution would be to not add `RNSScreenStackHeaderConfig` as a subview of `RNSScreenView` on Fabric. I did not research it, but it seems that it would require more changes. **Note** We need to consult with `@react-navigation` whether the `@react-navigation/native-stack` has to be adjusted. ### Before https://github.com/software-mansion/react-native-screens/assets/50801299/3b89dee8-a307-44ba-a9f4-02e4dfdd95ac ### After https://github.com/software-mansion/react-native-screens/assets/50801299/205f24d4-590b-402e-8b0a-848fd4482d59 ## Test code and steps to reproduce `Test1097` in `FabricTestExample` can be used to test these changes. Just use the scroll view and observe that the navigation bar gets stretched / dwindled while scrolling down / up respectively. ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes --- FabricTestExample/src/Test1097.tsx | 1 - ios/RNSScreen.h | 9 ++++++- ios/RNSScreen.mm | 28 +++++++++++++++------- ios/RNSScreenStack.mm | 4 +--- src/native-stack/views/NativeStackView.tsx | 9 ++++++- 5 files changed, 37 insertions(+), 14 deletions(-) diff --git a/FabricTestExample/src/Test1097.tsx b/FabricTestExample/src/Test1097.tsx index 640ccde74d..378a4ecbb7 100644 --- a/FabricTestExample/src/Test1097.tsx +++ b/FabricTestExample/src/Test1097.tsx @@ -1,4 +1,3 @@ -/* eslint-disable @typescript-eslint/ban-ts-comment */ import * as React from 'react'; import { Button, NativeSyntheticEvent, ScrollView } from 'react-native'; import { diff --git a/ios/RNSScreen.h b/ios/RNSScreen.h index df10c44eef..055b0bb59e 100644 --- a/ios/RNSScreen.h +++ b/ios/RNSScreen.h @@ -40,6 +40,8 @@ NS_ASSUME_NONNULL_BEGIN @end +@class RNSScreenStackHeaderConfig; + @interface RNSScreenView : #ifdef RCT_NEW_ARCH_ENABLED RCTViewComponentView @@ -88,7 +90,8 @@ NS_ASSUME_NONNULL_BEGIN // we recreate the behavior of `reactSetFrame` on new architecture @property (nonatomic) facebook::react::LayoutMetrics oldLayoutMetrics; @property (nonatomic) facebook::react::LayoutMetrics newLayoutMetrics; -@property (weak, nonatomic) UIView *config; +@property (weak, nonatomic) RNSScreenStackHeaderConfig *config; +@property (nonatomic, readonly) BOOL hasHeaderConfig; #else @property (nonatomic, copy) RCTDirectEventBlock onAppear; @property (nonatomic, copy) RCTDirectEventBlock onDisappear; @@ -109,12 +112,16 @@ NS_ASSUME_NONNULL_BEGIN - (void)notifyDisappear; - (void)updateBounds; - (void)notifyDismissedWithCount:(int)dismissCount; +- (instancetype)initWithFrame:(CGRect)frame; #endif - (void)notifyTransitionProgress:(double)progress closing:(BOOL)closing goingForward:(BOOL)goingForward; - (void)notifyDismissCancelledWithDismissCount:(int)dismissCount; - (BOOL)isModal; +/// Looks for header configuration in instance's `reactSubviews` and returns it. If not present returns `nil`. +- (RNSScreenStackHeaderConfig *_Nullable)findHeaderConfig; + @end @interface UIView (RNSScreen) diff --git a/ios/RNSScreen.mm b/ios/RNSScreen.mm index 86ad9350fb..5d541507fe 100644 --- a/ios/RNSScreen.mm +++ b/ios/RNSScreen.mm @@ -54,7 +54,6 @@ - (instancetype)initWithFrame:(CGRect)frame _reactSubviews = [NSMutableArray new]; [self initCommonProps]; } - return self; } #endif // RCT_NEW_ARCH_ENABLED @@ -531,6 +530,16 @@ - (BOOL)isModal return self.stackPresentation != RNSScreenStackPresentationPush; } +- (RNSScreenStackHeaderConfig *_Nullable)findHeaderConfig +{ + for (UIView *view in self.reactSubviews) { + if ([view isKindOfClass:RNSScreenStackHeaderConfig.class]) { + return (RNSScreenStackHeaderConfig *)view; + } + } + return nil; +} + #if !TARGET_OS_TV /** * Updates settings for sheet presentation controller. @@ -588,6 +597,11 @@ - (void)updatePresentationStyle #pragma mark - Fabric specific #ifdef RCT_NEW_ARCH_ENABLED +- (BOOL)hasHeaderConfig +{ + return _config != nil; +} + + (facebook::react::ComponentDescriptorProvider)componentDescriptorProvider { return facebook::react::concreteComponentDescriptorProvider(); @@ -595,12 +609,12 @@ - (void)updatePresentationStyle - (void)mountChildComponentView:(UIView *)childComponentView index:(NSInteger)index { - [super mountChildComponentView:childComponentView index:index]; if ([childComponentView isKindOfClass:[RNSScreenStackHeaderConfig class]]) { - _config = childComponentView; - ((RNSScreenStackHeaderConfig *)childComponentView).screenView = self; + _config = (RNSScreenStackHeaderConfig *)childComponentView; + _config.screenView = self; } [_reactSubviews insertObject:childComponentView atIndex:index]; + [super mountChildComponentView:childComponentView index:index]; } - (void)unmountChildComponentView:(UIView *)childComponentView index:(NSInteger)index @@ -1194,12 +1208,10 @@ - (void)hideHeaderIfNecessary // we need to check whether reactSubviews array is empty, because on Fabric child nodes are unmounted first -> // reactSubviews array may be empty - if (currentIndex > 0 && [self.screenView.reactSubviews count] > 0 && - [self.screenView.reactSubviews[0] isKindOfClass:[RNSScreenStackHeaderConfig class]]) { + RNSScreenStackHeaderConfig *config = [self.screenView findHeaderConfig]; + if (currentIndex > 0 && config != nil) { UINavigationItem *prevNavigationItem = [self.navigationController.viewControllers objectAtIndex:currentIndex - 1].navigationItem; - RNSScreenStackHeaderConfig *config = ((RNSScreenStackHeaderConfig *)self.screenView.reactSubviews[0]); - BOOL wasSearchBarActive = prevNavigationItem.searchController.active; #ifdef RCT_NEW_ARCH_ENABLED diff --git a/ios/RNSScreenStack.mm b/ios/RNSScreenStack.mm index e97011e70e..6d42dcf90c 100644 --- a/ios/RNSScreenStack.mm +++ b/ios/RNSScreenStack.mm @@ -1,4 +1,5 @@ #ifdef RCT_NEW_ARCH_ENABLED +#import #import #import #import @@ -6,9 +7,6 @@ #import #import #import - -#import - #else #import #import diff --git a/src/native-stack/views/NativeStackView.tsx b/src/native-stack/views/NativeStackView.tsx index a6dc7670fd..6f389c0aae 100644 --- a/src/native-stack/views/NativeStackView.tsx +++ b/src/native-stack/views/NativeStackView.tsx @@ -100,6 +100,10 @@ const MaybeNestedStack = ({ ]} // @ts-ignore Wrong props passed to View stackPresentation={stackPresentation} + // This view must *not* be flattened. + // See https://github.com/software-mansion/react-native-screens/pull/1825 + // for detailed explanation. + collapsable={false} > {children} @@ -331,7 +335,6 @@ const RouteView = ({ isHeaderInPush !== false ? headerHeight : parentHeaderHeight ?? 0 } > - {renderScene()} + {/* HeaderConfig must not be first child of a Screen. + See https://github.com/software-mansion/react-native-screens/pull/1825 + for detailed explanation */} + );