Skip to content

feat(iOS): make fitToContents sheet height react to dynamic content #2877

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

Merged
merged 11 commits into from
May 5, 2025

Conversation

kligarski
Copy link
Contributor

@kligarski kligarski commented Apr 17, 2025

Description

Allows fitToContents sheet's height to adapt to dynamic content on iOS.

contentWrapper:(RNSScreenContentWrapper *)contentWrapper receivedReactFrame:(CGRect)reactFrame receives many frames, some of them contain the same dimensions. Calling [self setAllowedDetentsForSheet:sheetController to:detents animate:YES] mutliple times, even with the same detents, causes visual glitches (dimming behing formSheet on iPhone/iPad and header on iPad have very odd-looking animations).

Before, we would set _didSetSheetAllowedDetentsOnController to YES after setting detents for the first time and then ignore all incoming frames - including those with new height after content changed its height. Now, we remember frame height we received, and when we receive a new frame, we compare new height to the previous value. If it is different, we update detents using setAllowedDetentsForSheet.

Supersedes #2741.

Fixes #2688.

Changes

  • replace boolean property _didSetSheetAllowedDetentsOnController with float _sheetFrameHeight
  • add Test2877 test screen

Screenshots / GIFs

Before After
2877_before.mp4
2877_after.mp4

Test code and steps to reproduce

Enter Test2877 screen and open form sheet. Its height should adapt to displayed content every second.

Checklist

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

@kligarski
Copy link
Contributor Author

In the future, it might be necessary to investigate the issue in more detail: why updating detents causes the glitches to appear and why new frames have same height but different width on an iPad (it sometimes alternates between 2 values, e.g. 1300 -> 500 -> 1300 -> 500).

@kligarski kligarski marked this pull request as ready for review April 17, 2025 13:38
@kligarski kligarski requested a review from kkafar April 17, 2025 13:38
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great.

I will want to put some time into debugging here, so that we understand the changes 😅

I'll try to find some time next week.

@lodev09
Copy link

lodev09 commented Apr 28, 2025

In the future, it might be necessary to investigate the issue in more detail: why updating detents causes the glitches to appear and why new frames have same height but different width on an iPad (it sometimes alternates between 2 values, e.g. 1300 -> 500 -> 1300 -> 500).

This might not be related but I found a bug while working on new features for the formSheet presentation. In New Arch, every layout changes triggers updateFormSheetPresentationStyle and causing visual glitches. This is due to the method being called in finalizeUpdates block.

If you move the prop update to updateProps, it fixes the issue.

@kkafar
Copy link
Member

kkafar commented May 5, 2025

@lodev09 thanks! Do you have any more specific information, e.g. when this issue is triggered? How can we observe the visual glitches?

Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks solid. I've debugged it for few minutes now & seems that all frame updates are sensible & for a known reason.

I've allowed myself to improve example & do a small variable renaming.

@kligarski kligarski merged commit d1e9aaf into main May 5, 2025
8 checks passed
@kligarski kligarski deleted the @kligarski/fix-ios-form-sheet-fit-to-contents branch May 5, 2025 12:35
@lodev09
Copy link

lodev09 commented May 5, 2025

@kkafar my fork has some added features like the translateY event among others. while working on this, I found a performance issue in New Arc in this block:

- (void)finalizeUpdates:(RNComponentViewUpdateMask)updateMask
{
[super finalizeUpdates:updateMask];
#if !TARGET_OS_TV && !TARGET_OS_VISION
[self updateFormSheetPresentationStyle];
#endif // !TARGET_OS_TV
}

Every time the sheet is dragged, this block is being called. I have a fix for it in my fork, together with some other features.
I will submit a PR soon, currently trying to find solution on the footer issue on IOS with New Arc.

https://github.com/lodev09/react-native-screens

For the context, had to implement these changes due to requirement for our app. Feel free to check it out in the meantime when you get the chance.

Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2025-04-26.at.05.42.34.mp4

@lodev09
Copy link

lodev09 commented May 5, 2025

Created draft PR here:
#2902

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fitToContents does not fit to contents
3 participants