react-native-screens icon indicating copy to clipboard operation
react-native-screens copied to clipboard

Native-Stack Modal height calculation is off

Open mobinni opened this issue 1 year ago β€’ 26 comments

Description

I'm having a hard time creating a stable reproduction for this as it's very specific.

What I have found is that when lazy loading modal routes with React.lazy and Suspense boundary - only the initial height calculation of the native modal is off.

When I close and re-open it, it calculates correctly to 802. If I don't lazy load the route, it calculates correctly from the get-go.

The only thing I can trace it back to is the usage of Suspense itself.

At the top-most level I have a view where I plug in

onLayout={(e) => {
      console.log(e.nativeEvent.layout.height)
    }}>

What I see in my app, is I get is 874 and it never recalculates.

However if I implement a similar UI setup in a snack I get 874 followed by a recalculation to 802. If I load it immediately without lazy loading I get 802 and it's stable.

My bug is that I get 874 as the modal height and it does not recalculate properly

Steps to reproduce

I don't have a stable reproduction of it staying stuck at 874, but the snack linked shows it calculating 874 then going back to 802.

  • If you don't lazy load it'll calculate to 802 immediately
  • If you do lazy load the route it will off shoot to 874 then recalculate

This is only on the initial render, not subsequent renders I wish I could create an actual reproduction where it gets stuck at 874, but I have had no luck

Snack or a link to a repository

https://snack.expo.dev/0UhdKCLtMNc15_r6uAHrJ

Screens version

4.3.0

React Native version

0.76.5

Platforms

iOS

JavaScript runtime

Hermes

Workflow

React Native (without Expo)

Architecture

Fabric (New Architecture)

Build type

None

Device

iOS simulator

Device model

iPhone 11,13,15,16

Acknowledgements

Yes

mobinni avatar Dec 22 '24 01:12 mobinni

Thanks for the report.

Notes for future me: I haven't looked at the report yet, but these 72 units of height indicate that this might be about measurements (not) including header.

kkafar avatar Dec 30 '24 14:12 kkafar

Accidentally closed this, sorry! πŸ˜…

mobinni avatar Jan 01 '25 09:01 mobinni

@kkafar any updates on this one πŸ˜„ I would also be happy to look into the measurement code, I just haven't been able to pin-point where the calculation happens of adding the header height

mobinni avatar Jan 09 '25 13:01 mobinni

I haven't had opportunity to work on this yet. Unfortunately the layout code & interaction between UIKit & React Native's layout is too complex to describe in few minutes, however places to start are:

  1. viewDidLayoutSubviews method of the RNSNavigationController,
  2. onLayout & adapt callbacks in RNSScreenComponentDescriptor & RNSScreenShadowNode (C++ files),
  3. Components receive frames in updateLayoutMetrics:oldMetrics callbacks from react-native

kkafar avatar Jan 09 '25 20:01 kkafar

Yah, when digging through the code with breakpoints I got the sense it got more and more complex as I made my way through, I'm willing to spend some time on this if I find something I'll update the thread. Thank you!

mobinni avatar Jan 10 '25 03:01 mobinni

Based on my debugging it looks like the layout engine itself is calculating 874 and giving that information to RNScreens, (from UIModalPresentationFormSheet?) I'm not sure why it's calculating the view height to be larger, but when I debug

  auto stateData = getStateData();
  auto contentOffset = stateData.contentOffset;

the frame size is being set to 874 there, which comes from ConcreteShadowNode Screenshot 2025-01-10 at 10 35 08β€―AM

Based on this file RNSScreenComponentDescriptor you're just setting values based on what you got back from the UIView layout engine? It's strange that it would start off calculating 792 and then jump to 874

Effectively what I'm seeing on my end is that overshoot of frame calc gets stuck and doesn't layout again πŸ˜… but I'm not seeing anywhere where on iOS we're adding height to it

mobinni avatar Jan 10 '25 15:01 mobinni

Hi there, wondering if there is any update on this thread

mobinni avatar Jan 29 '25 19:01 mobinni

Hey, unfortunately not. I haven't had opportunity to look into it yet :/

kkafar avatar Jan 29 '25 22:01 kkafar

All good! Appreciate the response I know you're super busy πŸ™‡

mobinni avatar Jan 30 '25 02:01 mobinni

Hi @mobinni are there any updates or solutions on this issue?

Kiaorra avatar Feb 14 '25 10:02 Kiaorra

@Kiaorra what I did in the meantime is I manually set the height for the view inside the modal for iOS only (since Android doesn't seem to have the issue)

  const height = Dimensions.get('screen').height;
  const { bottom } = useSafeAreaInsets();

    const bottomValue = bottom > 0 ? bottom : 16;

    const modalStyle = isIOS()
      ? {
          height: height - bottomValue * 2,
        }
      : {
          flex: 1,
        };

mobinni avatar Feb 19 '25 12:02 mobinni

@mobinni - thanks for the workaround! This issue is causing quite some troubles in the expo community as it makes something like having a button at the bottom of the screen hard to do.

In the expo issue: https://github.com/expo/expo/issues/34352 it was mentioned that this is a problem only with the new architecture and not with the old one. Not sure if this is a good hint, but thought to share here.

compojoom avatar Feb 19 '25 14:02 compojoom

I'm also experiencing this. Oddly the issue is sporadic in my main app, but in a minimal npx create-expo-app repro it happens every time.

Here's the minimal repro - modal.tsx is always 72 too tall: https://github.com/Jpoliachik/RNScreensModalIssueFeb2025

Jpoliachik avatar Feb 19 '25 17:02 Jpoliachik

@compojoom glad it helps! ❀ I think it's either a screens issue or an upstream RN height calculation issue, I traced it all the way back to RN's view height calculation, but I'm not sure if screens is adding something or RN is doing something odd.cj

@Jpoliachik it was sporadic for us as well, if we used suspense it was consistent, if we didn't it was sporadic

Off-topic, we also saw something similar with KeyboardAvoidingView on android with bottom buttons, where if you add autoFocus to an input and show the keyboard, but the buttons come into view later (due to some external logic) the height won't recalculate and the keyboard avoiding view will leave behind a grey area πŸ˜… the fix there was to not autoFocus until the view is finished drawing or remove it completely.

It makes me think this might be some upstream issue with view height re-calculation, but the code is complex and I lack in-depth knowledge of the Fabric render mechanism to validate

mobinni avatar Feb 19 '25 23:02 mobinni

Off-topic, we also saw something similar with KeyboardAvoidingView on android with bottom buttons, where if you add autoFocus to an input and show the keyboard, but the buttons come into view later (due to some external logic) the height won't recalculate and the keyboard avoiding view will leave behind a grey area πŸ˜… the fix there was to not autoFocus until the view is finished drawing or remove it completely.

Oh, good you are bringing this up! I also saw the same, but didn't conect the two. You are right. I had to add around 80px padding for the buttons to appear on top of the keyboard.

Image

Obviously I did it wrong as I was adding the inset.top to the padding bottom...

This 2 problems are most probably connected.

compojoom avatar Feb 20 '25 06:02 compojoom

@kkafar it seems this issue is more wide-spread I know you're super busy but I was just wondering if this warrants it to be a bigger priority πŸ€” Im also happy to show you what is happening or pair on this

mobinni avatar Feb 21 '25 14:02 mobinni

Thanks for bringing it to my attention. I'll try to look into this somewhere late next week.

I think I even know where the issue is coming from, but it won't be oneline fix

(first Yoga layout has no information of height of the native header)

kkafar avatar Feb 21 '25 15:02 kkafar

I'm running into a similar layout issues with Expo Router. Only on iOS

In my app, I have a screen configured with presentation="modal". Inside that screen is a top-level <View> set to flex: 1. When I log the onLayout callback, the reported height changes multiple times:

(NOBRIDGE) LOG {"height": 852}
(NOBRIDGE) LOG {"height": 796}
(NOBRIDGE) LOG {"height": 727}
(NOBRIDGE) LOG {"height": 796}

The value 727 is the closest to what I would expect but it still misses the safe area at the bottom.

From this modal screen, I also navigate to another screen set to presentation="formSheet". On the first render, the form sheet displays with the wrong height, but after closing and reopening, the height is correct.

I have new architecture enabled

bolyesz avatar Feb 26 '25 18:02 bolyesz

hi, are there any updates ?

alissoncunha avatar Mar 07 '25 22:03 alissoncunha

@kkafar wondering if there are any updates on this πŸ˜„

mobinni avatar Mar 17 '25 15:03 mobinni

@kkafar sorry for this being +1, but any chance this can be prioritised?

This is honestly destroying any joy of developing. It's extremely cumbersome to calculate the height on every modal screen and add correct paddings when you are using the same views in screens that are not modals :(

compojoom avatar Mar 21 '25 13:03 compojoom

Any updates on this fix? I am having this exact issue in my project. Bottom button will look correct in dev but then production build the height is correct the first load of the modal screen then every time after it’s different and the button cuts off screen.

benjeske avatar Apr 12 '25 15:04 benjeske

Any updates? Same problem here.

hamonCordova avatar Apr 16 '25 20:04 hamonCordova

Same problem :\

lorenzogonnelli avatar Apr 30 '25 07:04 lorenzogonnelli

I updated to expo 53 and RN 0.79 and it seems that I no longer experience the issue. Can someone else confirm this?

compojoom avatar Apr 30 '25 07:04 compojoom

This hotfix helps me, just wrap it around the root of the modal, the trick here is to use useAnimatedStyle with transform. translateY: 0 . You can use withSafeAreaFix or SafeAreaFix component directly, just add flex: 1 there

import React, { ComponentType } from 'react';
import { StyleProp, ViewProps, ViewStyle } from 'react-native';
import Animated, {
  AnimatedProps,
  useAnimatedStyle,
} from 'react-native-reanimated';

interface Props extends AnimatedProps<ViewProps> {
  children: React.ReactNode;
  shouldFollowKeyboard?: boolean;
  offset?: number;
  considerSafeArea?: boolean;
  style?: StyleProp<ViewStyle>;
}

/**
 * Fixes safe area jumping after opening in page sheet view
 */
export function SafeAreaFix({ children, style, ...animatedViewProps }: Props) {
  const translateStyle = useAnimatedStyle(() => {
    return {
      transform: [
        {
          translateY: 0,
        },
      ],
    };
  });

  return (
    <Animated.View style={[style, translateStyle]} {...animatedViewProps}>
      {children}
    </Animated.View>
  );
}

/**
 * HOC that wraps a component with SafeAreaFix
 *
 * @example
 * const MyScreenWithFix = withSafeAreaFix(MyScreen);
 */
export function withSafeAreaFix<P extends object>(
  Component: ComponentType<P>,
): ComponentType<P> {
  return function SafeAreaFixWrapper(props: P) {
    return (
      <SafeAreaFix style={flex}>
        <Component {...props} />
      </SafeAreaFix>
    );
  };
}

const flex = { flex: 1 };

sintylapse avatar Nov 10 '25 11:11 sintylapse