fix(iOS): buggy search bar / large title behaviour on Fabric #1825

kkafar · 2023-07-11T17:32:25Z

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

Added collapsable={false} to MaybeNestedStack view, preventing it from being flattened by React Native.
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.
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

Simulator.Screen.Recording.-.iPhone.14.-.2023-07-18.at.23.31.05.mp4

After

Simulator.Screen.Recording.-.iPhone.14.-.2023-07-18.at.23.28.42.mp4

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

Included code example that can be used to test this change
Ensured that CI passes

1. Prevent MaybeNestedStack from being flattened! <-- this was the view that was put at bad location in hierarchy by react. 2. Assert that `HeaderConfig` is not first child (so that scroll view is first). **IMPORTANT** I remember that there is some Paper code that relies on the fact that config is mounted as the first child (at index 0) (it uses it for checking) -> it must be refactored -> maybe just create property on scrollview indicating whether the config is present or not.

Just to have general diff on GitHub & I'll revert unnecessary changes 1 by 1 later

tboba

Great job with this one! I just left some comments to consider 🎉

ios/RNSScreen.h

src/native-stack/views/NativeStackView.tsx

…e stack v7 (#11501) ## 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. See * software-mansion/react-native-screens#1825 **Test plan** ### 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

…#1883) ## Description It looks like the change introduced in #1825 has also came with a bug where at least Search Bar was not working on Android. The reason for that has turned out to be the wrong implementation of `headerConfig` property on Android, which was looking for the first child and assuming that it is ScreenStackHeaderConfig - which was wrong, because the change in #1825 has moved the ScreenStackHeaderConfig to the bottom of the hierarchy. Fixes not working Search Bar on Android (and some other unknown things maybe?). ## Changes From now `Screen#headerConfig` will traverse through all children of the View and will check if one of them is `ScreenStackHeaderConfig`. ## Test code and steps to reproduce You can check Test1166 if SearchBar is working right now; from now the Search Bar in Example section should work properly. ## Checklist - [ ] Ensured that CI passes

## Description Since #1825 header config is no longer first child of a screen & `hitTest:withEvent:` method assumed this invariant to be true. Fixed that by using appropriate screen method instead of blind assumption. Fixes #1981 ## Changes * Fixed `hitTest:withEvent:` method by using `findHeaderConfig` `RNSScreenView`'s method * Improved `findHeaderConfig` method itself ## Test code and steps to reproduce `Test1981` ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes

t1gu1 · 2024-06-17T20:45:35Z

On the web, HeaderConfig must be first child of a Screen.
Otherwise A11y on web is kinda broken because the header is the last element of the DOM.

…software-mansion#1883) ## Description It looks like the change introduced in software-mansion#1825 has also came with a bug where at least Search Bar was not working on Android. The reason for that has turned out to be the wrong implementation of `headerConfig` property on Android, which was looking for the first child and assuming that it is ScreenStackHeaderConfig - which was wrong, because the change in software-mansion#1825 has moved the ScreenStackHeaderConfig to the bottom of the hierarchy. Fixes not working Search Bar on Android (and some other unknown things maybe?). ## Changes From now `Screen#headerConfig` will traverse through all children of the View and will check if one of them is `ScreenStackHeaderConfig`. ## Test code and steps to reproduce You can check Test1166 if SearchBar is working right now; from now the Search Bar in Example section should work properly. ## Checklist - [ ] Ensured that CI passes

…nsion#1995) ## Description Since software-mansion#1825 header config is no longer first child of a screen & `hitTest:withEvent:` method assumed this invariant to be true. Fixed that by using appropriate screen method instead of blind assumption. Fixes software-mansion#1981 ## Changes * Fixed `hitTest:withEvent:` method by using `findHeaderConfig` `RNSScreenView`'s method * Improved `findHeaderConfig` method itself ## Test code and steps to reproduce `Test1981` ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes

kkafar added 5 commits June 28, 2023 10:12

Reinstall pods in FTE

f991e54

Stash various changes (POSSIBLY TO REVERT)

a15d63f

Partially working non react-native setup

d8a83a5

Fully working non react-native setup

32a8d9a

Stash experiments

cd6153d

kkafar changed the title ~~fix(iOS): buggy search bar behaviour on Fabric~~ fix(iOS): buggy search bar / large title behaviour on Fabric Jul 11, 2023

kkafar added 21 commits July 14, 2023 11:08

Update example in FTE

44620d2

set headerTranslucent & largeTitle in TE

66005ae

Stash various native changes (to revert)

73dde0e

Just to have general diff on GitHub & I'll revert unnecessary changes 1 by 1 later

Restore unnecessary changes in RNSScreen.mm

612d158

Remove unnecessary comment

8b0ccf6

Restore App.js in TE

e6bbb0c

Restore changes in navigation container

5adccc8

Remove experiments code

dcdf94b

Remove unused imports in ScreenStack

d3dd5f4

Restore more changes in RNSScreenStack

aa6f139

Restore changes in RNSScreenStackHeaderConfig

f829aef

Restore changes in FTE App.js

e3a6db1

Restore some changes in Test1097 in FTE

9d78701

Merge branch 'main' into @kkafar/searchbar-fabric

529d554

Update & merge main

8679e03

Restore more changes in Test1097 FTE

5a7bc7a

Replace logic relying on config position in reactSubviews array

a650c08

Refactor hideHeaderIfNecessary once more

0ddb965

Add some comments

794861f

Fix findHeaderConfig impl on Paper

b7beb2f

kkafar requested a review from tboba July 18, 2023 21:26

tboba requested changes Jul 19, 2023

View reviewed changes

ios/RNSScreen.h Show resolved Hide resolved

ios/RNSScreen.h Show resolved Hide resolved

src/native-stack/views/NativeStackView.tsx Show resolved Hide resolved

Add missing comment on why collapsable={false} is set on Container

037a348

Add comment on HeaderConfig placement

d19fb23

kkafar requested a review from tboba July 19, 2023 10:53

kkafar merged commit ca6c9dc into main Jul 19, 2023

kkafar deleted the @kkafar/searchbar-fabric branch July 19, 2023 13:08

tboba mentioned this pull request Sep 6, 2023

fix(Android): Change implementation of headerConfig prop on Android #1883

Merged

1 task

tboba mentioned this pull request Sep 29, 2023

Header search bar not working on Android #1906

Closed

kkafar mentioned this pull request Dec 27, 2023

fix(iOS): not working hitslop for headerRight/Left views #1995

Merged

2 tasks

t1gu1 mentioned this pull request Jun 17, 2024

NativeStack on WEB - Header is the last element in the DOM and it is bad for screenReader on web react-navigation/react-navigation#12025

Open

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(iOS): buggy search bar / large title behaviour on Fabric #1825

fix(iOS): buggy search bar / large title behaviour on Fabric #1825

kkafar commented Jul 11, 2023 •

edited

Loading

tboba left a comment •

edited

Loading

t1gu1 commented Jun 17, 2024 •

edited

Loading

fix(iOS): buggy search bar / large title behaviour on Fabric #1825

fix(iOS): buggy search bar / large title behaviour on Fabric #1825

Conversation

kkafar commented Jul 11, 2023 • edited Loading

Description

Changes

Before

After

Test code and steps to reproduce

Checklist

tboba left a comment • edited Loading

Choose a reason for hiding this comment

t1gu1 commented Jun 17, 2024 • edited Loading

kkafar commented Jul 11, 2023 •

edited

Loading

tboba left a comment •

edited

Loading

t1gu1 commented Jun 17, 2024 •

edited

Loading