Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Native-Stack Modal height calculation is off #2587

Open
mobinni opened this issue Dec 22, 2024 · 9 comments
Open

Native-Stack Modal height calculation is off #2587

mobinni opened this issue Dec 22, 2024 · 9 comments
Assignees
Labels
Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided

Comments

@mobinni
Copy link

mobinni commented Dec 22, 2024

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

@github-actions github-actions bot added Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided labels Dec 22, 2024
@kkafar
Copy link
Member

kkafar commented Dec 30, 2024

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 kkafar self-assigned this Dec 30, 2024
@mobinni mobinni closed this as completed Dec 31, 2024
@mobinni mobinni reopened this Jan 1, 2025
@mobinni
Copy link
Author

mobinni commented Jan 1, 2025

Accidentally closed this, sorry! 😅

@mobinni
Copy link
Author

mobinni commented Jan 9, 2025

@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

@kkafar
Copy link
Member

kkafar commented Jan 9, 2025

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

@mobinni
Copy link
Author

mobinni commented Jan 10, 2025

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
Copy link
Author

mobinni commented Jan 10, 2025

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
Copy link
Author

mobinni commented Jan 29, 2025

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

@kkafar
Copy link
Member

kkafar commented Jan 29, 2025

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

@mobinni
Copy link
Author

mobinni commented Jan 30, 2025

All good! Appreciate the response I know you're super busy 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided
Projects
None yet
Development

No branches or pull requests

2 participants