feat(iOS, FormSheet v5): Add basic setup for standalone FormSheet native component#3947
feat(iOS, FormSheet v5): Add basic setup for standalone FormSheet native component#3947
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an iOS-only FormSheet (v5) component to the gamma/experimental API surface, backed by a new Fabric component view and supporting native controller/state plumbing, plus manual test scenarios in the test app.
Changes:
- Introduces a new
FormSheetJS component (with Android/Web stubs) and exports it viareact-native-screens/experimental. - Adds iOS native implementation (
RNSFormSheetComponentView+RNSFormSheetController) and registers the component for codegen. - Adds C++ shadow node/state/descriptor glue and adds new test-app scenarios for manual validation.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/fabric/gamma/modals/form-sheet/FormSheetNativeComponent.ts | New codegen native component interface for RNSFormSheet (iOS-only). |
| src/components/gamma/modals/form-sheet/* | JS public API wrapper (FormSheet), types, and Android/Web fallbacks. |
| src/components/gamma/modals/form-sheet/index.ts | Barrel export for the new component. |
| src/experimental/index.ts | Exposes FormSheet from the experimental entrypoint. |
| package.json | Registers RNSFormSheet in codegenConfig.ios.components. |
| ios/gamma/modals/form-sheet/* | Native controller, component view, and event emitter for iOS FormSheet presentation + events. |
| common/cpp/react/renderer/components/rnscreens/RNSFormSheet* | New shadow node/state/component descriptor for layout + origin offset handling. |
| apps/src/tests//form-sheet/ | Adds new manual scenarios for single-feature and integration testing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
t0maboro
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
t0maboro
requested review from
LKuchno,
kkafar,
kligarski,
kmichalikk and
sgaczol
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kkafar
left a comment
kkafar
left a comment
There was a problem hiding this comment.
Nice job!
I've left remarks below. Please answer them before we proceed.
| if (stateData.frameSize.width >= 0 && stateData.frameSize.height >= 0) { | ||
| layoutableShadowNode.setSize( | ||
| Size{stateData.frameSize.width, stateData.frameSize.height}); | ||
| } |
There was a problem hiding this comment.
Is this ever not true?
I need to read the code further leaving notes for myself.
There was a problem hiding this comment.
Usually what I've done recently is to initialise the frameSize with special empty value and then compare against it here.
| @@ -0,0 +1,43 @@ | |||
| #pragma once | |||
|
|
|||
| #ifndef ANDROID | |||
There was a problem hiding this comment.
It's okay. I recently started preferring #if !defined(ANDROID) as more readable. Up to you, I don't mind either. Just showing an alternative.
|
|
||
| class JSI_EXPORT RNSFormSheetState final { | ||
| public: | ||
| using Shared = std::shared_ptr<const RNSFormSheetState>; |
There was a problem hiding this comment.
This is something IIRC react-native got rid of & we didn't yet. Likely we don't use it anywhere, so we can safely remove it. Worth trying imo.
There was a problem hiding this comment.
Talking about using Shared = ... here.
|
|
||
| @interface RNSFormSheetComponentEventEmitter : NSObject | ||
|
|
||
| - (BOOL)emitOnDismiss; |
There was a problem hiding this comment.
General comment: we need naming convention to determine whether we should use in such scenarios onDismiss or onDismissed. We should be consistent.
There was a problem hiding this comment.
I was following stack v5 convention, I see -ed suffix rather in v4 impl., for now I'll leave it as it is, but creating a ticket to revisit it: https://github.com/software-mansion/react-native-screens-labs/issues/1230
| @end | ||
|
|
||
| @implementation RNSFormSheetController { | ||
| CGRect _lastNotifiedFrame; |
There was a problem hiding this comment.
The name here conveys very little information. We don't know whether we (the controller) have been notified of that frame or we notified some one.
Also from it's current usage, I don't see how is it different from self.view.frame?
There was a problem hiding this comment.
similarly as above, it describes the frame relatively to window coordinates, not parent coordinates, allow me to gather all this changes together after fixing an issue with non-zero FormSheet offset reported below
| - (void)viewWillAppear:(BOOL)animated | ||
| { | ||
| [super viewWillAppear:animated]; | ||
| self.presentationController.delegate = self; | ||
| } |
There was a problem hiding this comment.
We'd like in the future to forward these via the delegate -> host -> event emitter, since we'd like to notify JS of these lifecycle events.
There was a problem hiding this comment.
writing down, I'm planning to start creating tickets for FormSheet soon, so I'll include that there, for now I'm keeping it in my local notes
| @property (nonatomic, weak, nullable) id<RNSFormSheetControllerDelegate> delegate; | ||
|
|
||
| - (void)insertReactSubview:(UIView *)subview atIndex:(NSInteger)index; | ||
| - (void)removeReactSubview:(UIView *)subview; |
There was a problem hiding this comment.
Subview manipulation methods shouldn't leave on controller. I'd suggest manipulating it's view directly.
What do you think?
| import type { ViewProps } from 'react-native'; | ||
|
|
||
| export interface FormSheetProps extends ViewProps { | ||
| /** | ||
| * Determines whether the form sheet is currently visible. | ||
| * When `true`, the sheet is presented. When `false`, it is dismissed. | ||
| */ | ||
| isOpen: boolean; | ||
|
|
||
| /** | ||
| * An array of fractional screen heights (ranging from `0` to `1`) that define | ||
| * the resting positions of the sheet. | ||
| * | ||
| * On iOS, these map directly to `UISheetPresentationController` detents. | ||
| */ | ||
| detents?: number[] | undefined; | ||
|
|
||
| /** | ||
| * Called when the sheet is dismissed. | ||
| * It is highly recommended to use this callback to synchronize | ||
| * your local state to prevent UI mismatch (e.g., updating `isOpen` to `false`). | ||
| */ | ||
| onDismiss?: (() => void) | undefined; | ||
| } |
There was a problem hiding this comment.
We need to work on an document describing the complete API surface we plan here.
There was a problem hiding this comment.
I'll add it to my notes to include it while planning the scope & tasks
kligarski
left a comment
kligarski
left a comment
There was a problem hiding this comment.
What about this situation:
<View style={{ position: 'absolute', top: 200, left: 100 }}>
<FormSheet
isOpen={isOpen}
onDismiss={() => setIsOpen(false)}
detents={[0.6, 1.0]}>
<View style={styles.sheetContent}>
<Text style={styles.sheetTitle}>FormSheet content</Text>
<View style={styles.spacing} />
<Button
title="Dismiss from JS"
color={Colors.primary}
onPress={() => setIsOpen(false)}
/>
</View>
</FormSheet>
</View>absolute.mov
LKuchno
left a comment
LKuchno
left a comment
There was a problem hiding this comment.
Screen and scenarios looks good :)
I added few comments about naming conventions and directory structure and small suggestion about scenario to be consider.
|
|
||
| const scenarioDescription: ScenarioDescription = { | ||
| name: 'Basic functionality', | ||
| key: 'test-formsheet-base', |
There was a problem hiding this comment.
Following naming convention for directories I would set key to 'test-form-sheet-base-ios'
|
|
||
| const scenarioDescription: ScenarioDescription = { | ||
| name: 'FormSheet with Nested Stack v5', | ||
| key: 'test-formsheet-nested-stack-v5', |
There was a problem hiding this comment.
Since this is a CIT scenario, the directory name should look slightly different. I would propose: test-form-sheet-in-stack-v5-ios. The platform must be included as this test is iOS-only.
@kkafar , what do you think about the naming? Since no additional props are being tested, I wouldn't include the <component-a>-<component-b>-<test-case-name> part here, as the test case is basically a <component-a>-in-<component-b> check.
kligarski
left a comment
kligarski
left a comment
There was a problem hiding this comment.
Something seems off regarding the shadow tree offset synchronization.
First, I wondered why we're using [self convertPoint:CGPointZero toView:formSheetContentView] and then sending offset as negative instead of [formSheetContentView convertPoint:CGPointZero toView:self] which makes more sense to me (no "-" necessary). But after I compared both, I get different (absolute) values:
[dbg123] original: {-2.9599811375705726e-14, -388.80829015544043} alt: {2.8421709430404007e-14, 373.33333333333337}
I decided to check with devtools what's going on and I discovered that the offset is incorrect on first update and after trying to change the size of the sheet, it's still incorrect but it's different. You can see that pressable loses focus earlier when I swipe at the top.
original.mov
Then I used the second version. The offset is incorrect on first render as well but after trying to resize the sheet, it looks correct.
modified.mov
I think that we should debug this further. Maybe due to sheet padding on iOS 26 there are some transforms applied and they mess with the calculations (maybe also timing-related?)?
Native code
// For Yoga, we need to apply the offset in RNSFormSheetHostContentView coordinates
auto formSheetContentView = (RNSFormSheetContentView *)_controller.view;
CGPoint hostOriginInContentViewSpace = [self convertPoint:CGPointZero toView:formSheetContentView];
CGPoint contentViewOriginInHostSpace = [formSheetContentView convertPoint:CGPointZero toView:self];
NSLog(@"[dbg123] original: %@ alt: %@", NSStringFromCGPoint(hostOriginInContentViewSpace), NSStringFromCGPoint(contentViewOriginInHostSpace));
CGPoint contentOriginOffset = CGPointMake(-hostOriginInContentViewSpace.x, -hostOriginInContentViewSpace.y);
// CGPoint contentOriginOffset = CGPointMake(contentViewOriginInHostSpace.x, contentViewOriginInHostSpace.y);JS test (note the disabled hitSlop for debugging!)
```tsx import React, { useState } from 'react'; import { Button, StyleSheet, Text, View } from 'react-native'; import { FormSheet } from 'react-native-screens/experimental'; import type { ScenarioDescription } from '@apps/tests/shared/helpers'; import { createScenario } from '@apps/tests/shared/helpers'; import { Colors } from '@apps/shared/styling'; import PressableWithFeedback from '@apps/shared/PressableWithFeedback';const scenarioDescription: ScenarioDescription = {
name: 'Basic functionality',
key: 'test-form-sheet-base-ios',
details: 'Allows to test the basic functionality of FormSheet component.',
platforms: ['ios'],
};
export function App() {
const [isOpen, setIsOpen] = useState(false);
return (
FormSheet Test
<Button
title="Open FormSheet"
color={Colors.primary}
onPress={() => setIsOpen(true)}
/>
<FormSheet
isOpen={isOpen}
onNativeDismiss={() => setIsOpen(false)}
detents={[0.6]}>
{/FormSheet content
/}
{/<Button
title="Dismiss from JS"
color={Colors.primary}
onPress={() => setIsOpen(false)}
/>/}
<View style={{ width: 100, height: 50 }} />
);
}
const styles = StyleSheet.create({
container: {
flex: 1,
justifyContent: 'center',
alignItems: 'center',
backgroundColor: Colors.offBackground,
},
title: {
fontSize: 20,
fontWeight: 'bold',
marginBottom: 20,
color: Colors.text,
},
sheetContent: {
flex: 1,
backgroundColor: Colors.background,
// padding: 24,
// justifyContent: 'center',
// alignItems: 'center',
},
sheetTitle: {
fontSize: 22,
fontWeight: '600',
marginBottom: 12,
color: Colors.text,
},
spacing: {
height: 32,
},
});
export default createScenario(App, scenarioDescription);
</details>
|
|
||
| #pragma mark - Presentation Logic | ||
|
|
||
| - (void)updatePresentationState |
There was a problem hiding this comment.
I wasn't sure about how we handle isOpen from JS and native, I asked LLM for some edge cases and I tried quick change false -> open from JS:
Code and logs
<FormSheet
isOpen={isOpen}
onNativeDismiss={() => {
console.log('[dbg123] onNativeDismiss');
setIsOpen(false);
}}
detents={[0.6, 1.0]}>
<View style={styles.sheetContent}>
<Text style={styles.sheetTitle}>FormSheet content</Text>
<View style={styles.spacing} />
<Button
title="Dismiss from JS"
color={Colors.primary}
onPress={() => {
console.log('[dbg123] Dismiss From JS (setIsOpen(false))');
setIsOpen(false);
setTimeout(() => {
console.log(
'[dbg123] Dismiss From JS - Timeout (setIsOpen(true))',
);
setIsOpen(true);
}, 500);
}}
/>
</View>
</FormSheet>The sheet doesn't reappear:
JS-native-desync.mov
This is what LLM suspected but I didn't check if that's true:
- The "Animation In-Flight" Trap (Rapid Toggle Desync)
The Scenario: JS rapidly toggles isOpen from true -> false -> true.
The Problem: UIKit takes time to animate dismissals, but React updates props almost instantly.
JS sets isOpen={false}.
Native updateProps triggers updatePresentationState.
Native calls [_controller dismissViewControllerAnimated:YES completion:...]. The dismissal animation begins.
While the animation is running, JS sets isOpen={true}.
Native updateProps triggers updatePresentationState again.
The code checks isPresented = _controller.presentingViewController != nil. Because UIKit is still in the middle of animating the dismissal, presentingViewController is still not nil, so isPresented evaluates to YES.
Your logic hits this block:
// 2. _isOpen == YES and isPresented == YES: This occurs when the sheet is already visible ... require no action
The native code does nothing.
A fraction of a second later, the UIKit dismissal animation completes, and the sheet disappears from the screen.
The Result: JS thinks the sheet is open (isOpen={true}). Native _isOpen is YES. But the physical UI is completely gone. Total desynchronization.
There was a problem hiding this comment.
thanks for catching that, let me add it to my issue tracker, this might be more complicated issue to fix, so let me fix it in the followup PR: https://github.com/software-mansion/react-native-screens-labs/issues/1420
There was a problem hiding this comment.
Yeah. I do not know how we'll control the sheet yet. It's down to the discussion in the RFC. For now this is fine as PoC.
Tbh, I haven't tested ScrollView integration yet, but I have that in mind: https://github.com/software-mansion/react-native-screens-labs/issues/1275 |
Found that while working on props: https://developer.apple.com/documentation/uikit/uisheetpresentationcontroller/prefersscrollingexpandswhenscrolledtoedge cc @kmichalikk |
Yep, we must write that down as a recommendation in our yet-to-be-created documentation. We should recommend having stable frame relative to window of the host & provider. |
kkafar
left a comment
kkafar
left a comment
There was a problem hiding this comment.
Looks really good. I've left few remarks. These are mostly naming-related and few questions. Let's answer them before proceeding.
There was a problem hiding this comment.
Test name here does not conform with CIT naming convention introduced in https://github.com/software-mansion/react-native-screens-labs/commit/b73db82e771f5b287f09e860a855e7ca339d6414.
Please remember also to update the scenario key.
| ); | ||
| } | ||
|
|
||
| function StackSetup() { |
There was a problem hiding this comment.
Rebase the PR on top of main, I believe you're missing exports here. Do not recall whether @sgaczol PR has already landed or not.
There was a problem hiding this comment.
PR with local ESLint rule is open, rebasing won't work, so I'll add exports to all components manually
| layoutableShadowNode.setSize( | ||
| Size{stateData.frameSize.width, stateData.frameSize.height}); |
There was a problem hiding this comment.
That code will be problematic when implementing fitToContents. Also,
during the very first layout you'll set this to (0, 0), right? Is that intentional?
Now, back to the original problem.
Each time you setSize on a shadow node, this size is hard-coded. Yoga does not compute dimensions for that particular node then, no matter the layout constraints. Please see: #3295. The issue had arisen multiple times (e.g. in header subviews), but I feel like @kmichalikk described it well.
Please see also descriptions of the PRs I link below, this knowledge will be handy later:
- fix(Fabric,iOS): header subviews do not support dynamic content changes #2905
- fix(Fabric,Android): header subviews do not support dynamic content changes #2910
- fix(Android,Fabric): prevent Yoga from stretch-fitting height of header subview #2811
- fix(Android,Fabric): prevent another infinite state update loop for header subviews with zero size #2696
I don't know yet how to solve this 100% correctly, but I think that right now, for starters, you should guard against applying setSize before you receive first state from the host realm. I think I do that in one of above PRs by adding a helper method isStateEmpty or something.
Unless you did this intentionally, let me know what do you think here.
There was a problem hiding this comment.
I found a better way to handle that probably, so let me explain the intention I had before:
- I was rendering the FormSheet (Host) in JS with styles forcing a 0x0 frame so it wouldn't disrupt the React layout hierarchy.
- Upon mounting, it would stay 0x0 until we actually wanted to show the content.
- When
isOpenchanged fromfalsetotrue, a synchronous state update was sent with the actual frame based on the sheet's detents (W x H). - When we dismissed the
FormSheet, theHostinstance remained alive in the React tree, still holding thatW x Hframe. Because we don't unmount the Host from the hierarchy, I had to send a state update on dismiss to reset its size back to 0x0 manually.
The refined approach I'm implementing now:
- To support
fitToContents, theHostactually needs to have a non-zero frame from Yoga. In this setup, the Host may even act asScreenContentWrapperfrom v4. - To prevent this non-zero Host from affecting the layout of its siblings in the React tree, it is now styled with
position: 'absolute', top: 0, left: 0. - Because we let Yoga compute the Host's size, reading the Host's size gives us the exact content size needed to cover the
fitToContentslogic. - Since the Host now has a non-zero frame in the Element tree, it would normally block touch events for the screen underneath it. To prevent this, I overrode
hitTest:withEvent:on theHostview to always returnnil. - The touch events for the
Host'schildren are safe, as they are handled by a separateRCTSurfaceTouchHandlerattached directly to theContentView(where the React children are natively re-parented).
| - (instancetype)initWithFrame:(CGRect)frame | ||
| { | ||
| if (self = [super initWithFrame:frame]) { | ||
| self.backgroundColor = [UIColor clearColor]; |
There was a problem hiding this comment.
Why clearColor here? What's the default?
There was a problem hiding this comment.
nil, I want to have more control over the ContentView (UIView) in the background and do not rely on UIView defaults, because this one is attached manually to the hierarchy by me, so I don't want anything, especially React View to sample anything from that view under some conditions
There was a problem hiding this comment.
In other components we have EventEmitter, w/o additional Component infix, e.g. RNSTabsHostEventEmitter or RNSStackScreenEventEmitter.
I don't this this one is worse or better I just want it to be consistent.
|
|
||
| // Touch handler requires absolute positioning coordinates, relatively to root (UIWindow) | ||
| CGPoint contentViewOriginInWindow = [_controller.view convertPoint:CGPointZero toView:nil]; | ||
| [self updateTouchHandlerWithOrigin:contentViewOriginInWindow]; |
There was a problem hiding this comment.
I bit bigger nit-pick: I don't like the fact that the touch handler update "is hidden" in syncShadowNodeState method. Seems like it is outside of the scope fo this method. I'd suggest renaming it to updateOnLayoutInavlidation or something. Don't have a great suggestion, but we need something not misleading.
| { | ||
| RCTAssert([self.view isKindOfClass:[RNSFormSheetContentView class]], | ||
| @"[RNScreens] ContentView must be of type RNSFormSheetContentView"); | ||
| return (RNSFormSheetContentView *)self.view; |
| - (void)viewWillAppear:(BOOL)animated | ||
| { | ||
| [super viewWillAppear:animated]; | ||
| self.presentationController.delegate = self; |
There was a problem hiding this comment.
Why is the delegate initialised here? Why not in initialiser? I don't think we've done something like this in v4 implementation.
There was a problem hiding this comment.
My point is: view lifecycle method does not seem like the right place for the delegate initialisation, unless you plan to clean it up in viewDidDisappear. In any other case, this should likely be done in initialiser, OR , if the self.presentationController is still null there follow the approach in RNSScreen?
Haven't verified what do we do in v4, hopefully not exactly the same 😅 Please check it out.
There was a problem hiding this comment.
sorry, I forgot about the issue I had with native dismissal, because UISheetPresentationController was recreated for every presentation, while RNSFormSheetContentController is living longer, because the associated Host is still living in the React hierarchy, just not presenting the content, so I wanted to ensure, that the delegate, which will allow detecting the native dismissal will be always available, when we'd like to present the content
There was a problem hiding this comment.
reverting faf0d58 & adding cleanup in viewDidDisappear, let me know if this approach is okay for you
| const styles = StyleSheet.create({ | ||
| host: { | ||
| position: 'absolute', | ||
| top: 0, | ||
| left: 0, | ||
| width: 0, | ||
| height: 0, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
These styles require in-code explanation IMO. It's vastly not clear when reading the code why you want to do it like that.
|
|
||
| interface NativeProps extends ViewProps { | ||
| isOpen?: CT.WithDefault<boolean, false>; | ||
| detents?: CT.Double[] | undefined; |
There was a problem hiding this comment.
Okay, I don't mind that much here.
FYI: You can make _detents as std::vector<CGFloat> + actually copying values over from one vector to another would be cheaper, than copy-constructing a new vector on each assignment. C++ stdlib has functions for this.
… sending zeroed frames
This reverts commit faf0d58.
Description
Introducing the iOS implementation of the standalone FormSheet component.
Key Architectural Decisions:
UIModalPresentationFormSheetpresentation.RNSFormSheetHostController. This controller creates a transparent top-level UIView and is presented modally over the current UIWindow hierarchy via the closest validpresentingViewController.isOpenprop. This declarative boolean is translated into imperativepresentViewControlleranddismissViewControllercalls. When a user natively dismisses the sheet (e.g., via a swipe-down gesture), theRNSFormSheetHostControllerDelegatetriggers theRNSFormSheetHostComponentEventEmitterto fire theonNativeDismissevent, allowing the JS to synchronize its local state.viewDidLayoutSubviewsinside the presented controller. When the native bounds change, the new size and origin offset are captured and dispatched via a C++ state update. This explicitly forces Yoga to synchronously recalculate the content layout using the sheet's actual native dimensions.RNSFormSheetContentView.Implementation details:
RCTSurfaceTouchHandlerattached to the controller's view, utilizingviewOriginOffsetto correctly align the React touch coordinate space with the natively presented window.Caution
Dynamic content origin updates aren't supported in the context of synchronization with the ShadowTree state. If an ancestor offset changes, the frame of our Host view might not be updated at all, because from the Host perspective, the frame doesn't change, so there's no need to re-layout the content.
Caution
Regarding the layout, it may currently become slightly desynchronized, as noted in the discussion: #3947 (review)
The root cause is that
UIDropShadowView(an ancestor ofContentView) applies a transform: scale, which is not straightforward to detect. As a result, the bounds ofContentViewremain unchanged, and layout updates triggered byUIDropShadowVieware not observable from our components.This can lead to a minor offset during the initial layout pass on iOS 26, since the
FormSheetis not anchored to the leading and trailing edges of the screen.I wouldn’t classify this as a regression - we observed similar behaviors in the v4 implementation, and it did not raise any reported issues. For now, I prefer to leave it as-is rather than introduce fragile workarounds. I'll open a follow-up ticket to track this in case the desynchronization becomes problematic.
(ik that too many tasks were covered in this single PR, I'm refining this approach for other modal components)
Closes https://github.com/software-mansion/react-native-screens-labs/issues/1254
Closes https://github.com/software-mansion/react-native-screens-labs/issues/1255
Closes https://github.com/software-mansion/react-native-screens-labs/issues/1256
Closes https://github.com/software-mansion/react-native-screens-labs/issues/1257
Closes https://github.com/software-mansion/react-native-screens-labs/issues/1258
Changes
RNSFormSheetHostComponentViewto handle the Fabric lifecycle.RNSFormSheetHostControllerto manage modal presentation.RNSFormSheetHostContentViewto isolate react subviews reparenting.RNSFormSheetHostComponentEventEmitterand updated the JS spec to emit theonNativeDismissevent.RNSFormSheetHostShadowNodedefinitions for synchronous layout updates via C++ state proxy.Before & after - visual documentation
basic-layout.mov
formsheet-with-stack-v5.mov
Test plan
isOpenis set totrue.StackContainerfills the FormSheet's boundsChecklist