feat: correct measure with native header

[WoLewicki]

Description

PR adding the getContentOriginOffset to Screen's ShadowNode in order to account for native header on both platforms. It seems like for the different screen orientations don't work correctly on Android so it needs to be investigated too. It also adds ModalScreen concept for now only to have different shadow node there. We need it for different traits since the non-modal Screen can be placed below some other views, so it should not be treated as the root of the view hierarchy by yoga.

Changes

• Added new prop to Screen's state which is used in getContentOriginOffset method which is in turn used by LayoutableShadowNode when adding offsets to elements: https://github.com/facebook/react-native/blob/8da1da2e7320e23341e024a855e87c4762b220cc/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp#L54.
• Added ModalScreen

Screenshots / GIFs

Here you can add screenshots / GIFs documenting your change.

You can add before / after section if you're changing some behavior.

Before

After

-->

Test code and steps to reproduce

It needs to be tested thoroughly with different scenarios of having headers in nested stacks etc.

Checklist

• Included code example that can be used to test this change
• Updated TS types
• Updated documentation:
  • https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
  • https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
  • https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
  • https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
• Ensured that CI passes

[j-piasecki]

I'll leave it here so it's not lost to time. We believe this trait is necessary for modals, but it breaks touchables (measure to be more precise) in every case where screen is not positioned at 0,0. We'd need to remove this trait here and make new shadow node specific for modals.

[tboba]

Hi @j-piasecki, do you think this PR is ready to check? I see that our Android builds are failing on CI at the moment and I think this is not expected 🤔

[j-piasecki]

There were some things left to do, like for example compiling it on the old arch, where Wojti forgot to update the method signature 😄. The thing that needs to be for sure checked is the behavior of nested stacks with different header settings.

I think I will be able to look into it tomorrow, no promises though so if you want to do it, go ahead.

[j-piasecki]

This breaks on the js stack, it returns the height of the header that doesn't exist

[tboba]

Hi @WoLewicki, could you merge the changes from main to this PR? I see that you have also bumped the version to RN 0.73.4, which is already done on main 😄

[cortinico]

I'm also wondering what's the status here. Can we get this merged in a version of screens that comes out before RN hits 0.74 stable?

[tboba]

Hi @cortinico, just wanted to ask - are there any estimates when RN 0.74 will be officially published?

[cortinico]

Hi @cortinico, just wanted to ask - are there any estimates when RN 0.74 will be officially published?

We're planning to cut the branch next week. Stable release will follow in the coming weeks so it's not immediate future but still quite high on the agenda. I just wanted to make sure this PR doesn't get forgotten

[tboba]

Just left some comments and questions before testing the PR. Please answer them 🎉

[tboba]

Why do we need to have statusbar and orientation oriented methods in RNSModalScreen?

[WoLewicki]

They come with RCTConvert. We could try and see if those are really needed in a follow up PR.

[tboba]

I would add some comment here, that because how iOS modals are being mounted in view hierarchy, we need to use separate component. Also, in the future we need to check if for sure Android modals are being mounted in the same view group as usual screens (and not in the separate view, same as on iOS).

[WoLewicki]

Also, in the future we need to check if for sure Android modals are being mounted in the same view group as usual screens (and not in the separate view, same as on iOS).

You mean in the future as when the new modals will be merged or do you mean the current situation? Currently, modals on Android are just transparent fragments with the previous ones not being detached so they are in the same view hierarchy.

[tboba]

I mean the modals, that will be introduced in Screens, since they may be pushed into the separate dialog hierarchy, but we'll check that, when the modal PR will be opened

[tboba]

@WoLewicki also, I think I've found another issue that isn't covered with this fix. When there's a headerLargeTitle and the content is wrapped with the scrollview, touchable's target is still in the incorrect place 🤔

Screen.Recording.2024-02-23.at.17.07.10.mov

This time I see that the button is under the whole content. As I've printed calculated header's height, it gives the correct value (96, as it does not count status bar). Do you know about that case?

Repro

/**
 * Sample React Native App
 * https://github.com/facebook/react-native
 *
 * @format
 */

import React from 'react';
import { Button, SafeAreaView, View, ScrollView } from 'react-native';

import { createNativeStackNavigator } from '@react-navigation/native-stack';
import { NavigationContainer } from '@react-navigation/native';

const Stack = createNativeStackNavigator();

const TestScreen = ({ navigation }): React.JSX.Element => {
  return (
    <ScrollView contentInsetAdjustmentBehavior="automatic">
      <Button
        title={
          'Click me and drag around a bit and I should log something still'
        }
        onPress={() => {
          console.log(Date.now());
        }}
      />
      <Button
        title={'Navigate to modal'}
        onPress={() => {
          navigation.navigate('Test2');
        }}
      />
    </ScrollView>
  );
};
function App(): React.JSX.Element {
  return (
    <>
      <View style={{ width: 100, height: 200, backgroundColor: 'red' }} />
      <NavigationContainer>
        <Stack.Navigator
          initialRouteName="Test"
          screenOptions={{ presentation: 'modal' }}>
          <Stack.Screen
            name="Test"
            component={TestScreen}
            options={{
              headerShown: true,
              headerLargeTitle: true,
            }}
          />
          <Stack.Screen
            name="Test2"
            component={TestScreen}
            options={{ headerShown: false }}
          />
        </Stack.Navigator>
      </NavigationContainer>
    </>
  );
}

export default App;

[tboba]

I think we should also check if new architecture is enabled in other places where we're declaring initWithBridge (e.g. RNSFullWindowOverlay, RNSSearchBar). I also see some inconsistency here - in RNSScreenStackHeaderSubview we're declaring this initWithBridge into the #pragma mark - Paper specific declaration. Maybe we should move this initializer into the pragma marks everywhere? What do you think?

[tboba]

Let's leave that on a separate PR 👍

[WoLewicki]

Yeah, we can do it in some follow-up for sure. It does not change anything on new arch since this method is just never called.

[tboba]

LGTM! 👍