feat(iOS): support for custom detents with `formSheet` presentation #1852

kkafar · 2023-08-01T13:56:01Z

Description

This PR adds support for custom detents with formSheet modal presentation on iOS >= 16.0.

This new feature can be used with native-stack v5 (react-native-screens/native-stack) via sheetAllowedDetents prop by passing an array of values to it (see prop description in docs).

I plan on exposing it in v6 (@react-navigation/native-stack) in react-navigation/react-navigation#11516 or in a follow up PR.

Note: I'm looking for any suggestions on better prop naming :D

See #1870 for view flickering issue description & solution

Resolves #1772

Changes

Implementation notes

Please look below for my review.

Known issues

View flickering on Paper and Fabric (the exact cause is not determined yet, but the code responsible for this is in updateBounds method which is called on every layout recalculation and it reports current view size to react (/ updates shadow node).

See:

Sheet / detent buggy jumping behavior on iOS #1722

Test code and steps to reproduce

Test1649 in both TestsExample and FabricTestExample apps.

Checklist

Included code example that can be used to test this change
Updated TS types
Updated documentation:
Ensured that CI passes

ferrannp · 2023-08-02T14:58:56Z

Do you want some help to test this @kkafar ? Also, if you make this native for Android... you will be a hero 😅.

kkafar · 2023-08-03T06:30:51Z

Hey @ferrannp,
thank you and yes -- I would really use a hand in testing this stuff, any feedback is welcome.

Currently I'm working on Fabric implementation which has some intricacies and it is not ready yet.
Paper, however should work just fine (but I have not tested how this behaves on iPad yet).

Also I'm still thinking on prop naming and best way of exposing this functionality, any suggestions would also be welcome. At the moment I'm using dedicated prop temporarily named sheetCustomDetents which has effect only if:

stackPresentation == 'formSheet'
sheetAllowedDetents == 'custom'

But I also consider accepting an array with sheetAllowedDetents prop (so the declaration would be sheetAllowedDetents: SheetDetentTypes | number[] and dispatching this internally.

ferrannp · 2023-08-04T11:37:43Z

I will test it then @kkafar 👍 . For me this one sheetAllowedDetents: SheetDetentTypes | number[] makes more sense 👍 . Is there a limit of amount of detents do you know? Cause if there is a limit maybe we can be more explicit in the type number[].

kkafar · 2023-08-08T09:53:39Z

I'm not aware of any limitation on number of detents.
Only restriction I'm aware of right now is that the array has to be sorted in ascending order.

kkafar

General remarks:

Prop definitions differ for native stack v5 and native component ScreenProps.

In native stack we have:

sheetAllowedDetents: SheetDetentTypes | number[],
sheetLargestUndimmedDetent: SheetDetentTypes | number,

while native component goes with four distinct props:

sheetAllowedDetents: SheetDetentTypes,
sheetLargestUndimmedDetent: SheetDetentTypes,
sheetCustomDetents: number[],
sheetCustomLargestUndimmedDetent: number,

It made native implementation much easier. Alternatively we could take an array of strings:

sheetAllowedDetents: (SheetDetentTypes | string)[],
sheetLargestUndimmedDetent: SheetDetentTypes | string,

where string is a percentage, e.g. '50%', and parse it on native side?

Looking for any suggestions

Either native detent levels ('medium', 'large', 'all') or custom ones can be used, but not both at the same time.

This is because the values I receive must be sorted & exact height of native detents is not known and it is context dependent, thus I reject mixing them.

kkafar · 2023-08-09T13:58:32Z

FabricTestExample/package.json

@@ -35,6 +35,7 @@
    "babel-jest": "^29.2.1",
    "eslint": "^8.19.0",
    "jest": "^29.2.1",
+    "jotai": "^2.2.3",


Added it to both TestsExample & FabricTestExample to make my life much easier (it is a simple state management library, 399kB unpacked).

kkafar · 2023-08-09T14:22:20Z

ios/RNSScreen.h

@@ -85,9 +85,11 @@ namespace react = facebook::react;
 // Props controlling UISheetPresentationController
 @property (nonatomic) RNSScreenDetentType sheetAllowedDetents;
 @property (nonatomic) RNSScreenDetentType sheetLargestUndimmedDetent;
+@property (nonatomic) NSNumber *sheetCustomLargestUndimmedDetent;


sheetUndimmedDetentIndex?

I would choose even sheetLargestUndimmedDetent without Custom keyword. On the first look, the name sheetUndimmedDetentIndex isn't quite clear for me - what is Index there? 🤔

kkafar · 2023-08-09T14:23:16Z

ios/RNSScreen.h

 @property (nonatomic) BOOL sheetGrabberVisible;
 @property (nonatomic) CGFloat sheetCornerRadius;
 @property (nonatomic) BOOL sheetExpandsWhenScrolledToEdge;
+@property (nonatomic) NSArray<NSNumber *> *sheetCustomDetents;


sheetDetentValues, sheetMaxDetentValueFractions or some variation of these?

I would stay with sheetCustomDetents, as sheetAllowedDetents is already there and it would be confusing to use justsheetDetentValues. Also, sheetMaxDetentValueFractions seems a bit odd for me 🤔

kkafar · 2023-08-09T14:25:12Z

ios/RNSScreen.mm

+                                 customDetentWithIdentifier:ident
+                                                   resolver:^CGFloat(
+                                                       id<UISheetPresentationControllerDetentResolutionContext> ctx) {
+                                                     return ctx.maximumDetentValue * frac.floatValue;


I'm not sure yet whether I should use ctx.maximumDetentValue or try using large system detent height (by using resolveValueInContext:). Needs to be tested.

kkafar · 2023-08-09T14:30:52Z

ios/RNSScreen.mm

+      [NSMutableArray arrayWithCapacity:fractions.count];
+  int detentIndex = 0;
+  for (NSNumber *frac in fractions) {
+    NSString *ident = [[NSNumber numberWithInt:detentIndex] stringValue];


System API is organised is such way, that if I want to refer to particular detent I need to do it by its identifier (unique string). Thus I'm using indexes here. I rejected doing it by detent value (height) because the value is context dependent.

Alternative would be to make user define detents as objects with id / name field, e.g.:

[{ id: "3/5", value: 0.6 }]

But I did not like this idea even more so.

This is why sheetCustomLargestUndimmedDetent (the name must be changed) takes an index as a value.

What about if someone provided 2 same values?

iOS will treat them as a single detent. The resolved value in such case is rather iOS implementation specific and not documented. It's better to treat such case as undefined behaviour.

I see your point though.

I guess we will stick to using indices.

ios/utils/NSArray+RNSUtil.h

ios/RNSScreen.mm

WoLewicki

Just a quick review with some questions. Looks good overall 🎉

WoLewicki · 2023-08-09T15:47:30Z

ios/RNSScreen.mm

+      [NSMutableArray arrayWithCapacity:fractions.count];
+  int detentIndex = 0;
+  for (NSNumber *frac in fractions) {
+    NSString *ident = [[NSNumber numberWithInt:detentIndex] stringValue];


What about if someone provided 2 same values?

ios/utils/NSArray+RNSUtil.h

native-stack/README.md

src/native-stack/types.tsx

tboba

Great job with this one 🥳 However, I've got some suggestions that should be detented (haha) 😂😂🤣

android/src/paper/java/com/facebook/react/viewmanagers/RNSScreenManagerDelegate.java

FabricTestExample/src/Test1649.tsx

ios/RNSScreen.mm

ferrannp · 2023-08-10T21:10:55Z

I am testing this and TBH, being able to do things like:

sheetCornerRadius: 20,
sheetAllowedDetents: [0.4, 0.9],
sheetLargestUndimmedDetent: 0,

is just amazing 🔥 .

However:

#1722 This bug is very bad. You do not notice it in a modal somehow but here is very noticeable see:

jumping.mov

There is an opened PR... but IMO we need to fix this. It kinda breaks the whole experience.

kkafar · 2023-08-11T05:38:06Z

I'm aware of this flickering (on Fabric it gets even worse).

#1767 can't be merged unfortunately, because while it helps with this particular problem, it brings back other already solved issues.

I will see what can be done here.

ansh · 2023-10-18T19:40:38Z

Will I be able to use a FlatList or ScrollView inside the modal with formSheet presentation?

So that the method's name better describes its purpose

Test example was much more easier to write with some kind of state management library.

kkafar · 2023-12-13T11:16:08Z

Hey @ansh,

This PR is held back by #1974, as I want to have largest common API surface between platforms.

Also I'm still fighting with the "clipping" effect that was reported above. Haven't found a solution without tradeoffs yet (or maybe I just did, as I'm pushing right now).

…n presentation

## Description Should be merged to #1852 or first rebased and then merged to main after #1852. Closes #1722 So the exact roots of the issue are unclear & obfuscated. It seems that it might be related to following (not 100% sure): 1. It looks like during animation `viewDidLayoutSubviews` is being called which in turn calls `setSize:forView:` on UIManager. This triggers Yoga layout underneath which sets view dimensions to the target values (end animation values) resulting in view clipping during the animation. There are two more important facts: a. its hard to determine whether its Yoga who sets invalid value or it gets invalid value from us (passed in `updateBounds` method after `viewDidLayoutSubviews` is triggered by system. b. when uimanager is not notified of bounds size change everything works fine I've considered various approaches: 1. Do not pass the value to UIManager when animation is ongoing. Presence of animation was detected by checking `animationKeys` property of view's layer. This still resulted in visual glitches. Moreover if I sent the final value after animation finish (via completion block) it resulted in content jumping (see [here](facebook/react-native#34834 (comment))). 2. Use `CADisplayLink` & report to UIManager bounds size from `presentationLayer` (that should be (almost) accurate value), but it still resulted in visual glitches (even when sliding down), both flickering and content jumping or just content had wrong top offset / padding. 3. Do not notify UIManager at all on bounds change. ## Changes I went with this approach for now. That is: I do not notify uimanager on bounds change when `stackPresentation == formSheet`. This is wild I know. I experimented a bit trying to find out what did it broke, but I did not find anything on my toy example (Test1649), however it is highly unlikely that such approach does not have negative impact, but I believe it is better that way, than having formsheets unusable due to this flickering. ## Test code and steps to reproduce Test1649 ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes

kkafar · 2024-10-08T21:57:10Z

Superseded by #2045

kkafar changed the title ~~feat(iOS): support for custom detens on formSheet presentation~~ feat(iOS): support for custom detents on formSheet presentation Aug 1, 2023

kkafar changed the title ~~feat(iOS): support for custom detents on formSheet presentation~~ feat(iOS): support for custom detents with formSheet presentation Aug 2, 2023

kkafar commented Aug 9, 2023

View reviewed changes

kkafar requested review from kacperkapusciak, tboba and WoLewicki and removed request for kacperkapusciak August 9, 2023 14:58

kkafar commented Aug 9, 2023

View reviewed changes

ios/RNSScreen.mm Outdated Show resolved Hide resolved

WoLewicki reviewed Aug 9, 2023

View reviewed changes

tboba requested changes Aug 10, 2023

View reviewed changes

kkafar mentioned this pull request Aug 17, 2023

fix(iOS): modal view flickering #1870

Merged

2 tasks

kkafar mentioned this pull request Aug 24, 2023

feat(iOS,v6): support for form sheet medium detent react-navigation/react-navigation#11516

Draft

3 tasks

kkafar added 10 commits November 14, 2023 12:42

Add PoC

65a3ac1

Reorganise native code & rename the prop to sheetCustomDetents

894c45a

Set default for sheetCustomDetents in InnerScreen

d04c354

Pass the new prop through NativeStackView

1f9499e

Remove debug logging

0fcad06

Add stubs for type documentation

bd556e0

Cleanup native code a bit

9c2636f

Rename updatePresentationStyle to updateFormSheetPresentationStyle

1ce34bc

So that the method's name better describes its purpose

Add dev dependency on jotai

3db72e7

Test example was much more easier to write with some kind of state management library.

Refactor test example using jotai

b34b6e5

kkafar added 7 commits December 13, 2023 18:55

Start cleaning up

eec733d

Cleanup updateBounds, animationDidStart, animationDidStop

3edede5

Update jotai

2f4743f

Remove setFrame & setBounds methods

597d082

Unify example between platforms

59008f8

Remove fixed backgorund style from screen component

90b2639

Do not use custom shadow view (yet)

c9ffb76

kkafar added the Feature impl PR with feature implementation label Dec 14, 2023

kkafar self-assigned this Dec 14, 2023

kkafar added 17 commits December 14, 2023 16:28

Properly call updateFormSheetPresentationStyle

abd6c03

Cleanup updateFormSheetPresentation code & animate prop value changes

b8abf7b

REVERT: Stash debug setup

a7bd623

Flickering sheet fix v30 (partial)

a8015a9

Try to fix flickering with setting full height for view when dragging up

282eacc

iOS code for avoiding reactSetFrame on screens subviews

5cc8e41

JS code to avoid reactSetFrame on screens content when formSheet is i…

6be17f2

…n presentation

Update example in TE

6b33387

Allow for passing screen style (naively, NEED TO FILTER THIS)

8233c3f

Patch for scrollview

7e2b131

Cleanup 1

d0800d1

Cleanup 2

9f6f0a9

Cleanup 3

5ac1072

Add important comments on the reasons why the code looks like it does

52c6353

Remove custom shadow view code

0391257

Improve KVO observer handling to balance out add/remove calls

9af41ba

Update TE

1ae540e

kkafar closed this Oct 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iOS): support for custom detents with `formSheet` presentation #1852

feat(iOS): support for custom detents with `formSheet` presentation #1852

kkafar commented Aug 1, 2023 •

edited

Loading

ferrannp commented Aug 2, 2023

kkafar commented Aug 3, 2023

ferrannp commented Aug 4, 2023

kkafar commented Aug 8, 2023

kkafar left a comment

kkafar Aug 9, 2023

kkafar Aug 9, 2023

tboba Aug 10, 2023

kkafar Aug 9, 2023

tboba Aug 10, 2023

kkafar Aug 9, 2023

kkafar Aug 9, 2023

WoLewicki Aug 9, 2023

kkafar Aug 9, 2023

WoLewicki left a comment

WoLewicki Aug 9, 2023

tboba left a comment

ferrannp commented Aug 10, 2023

kkafar commented Aug 11, 2023

ansh commented Oct 18, 2023

kkafar commented Dec 13, 2023 •

edited

Loading

kkafar commented Oct 8, 2024

feat(iOS): support for custom detents with formSheet presentation #1852

feat(iOS): support for custom detents with formSheet presentation #1852

Conversation

kkafar commented Aug 1, 2023 • edited Loading

Description

Changes

Implementation notes

Known issues

Test code and steps to reproduce

Checklist

ferrannp commented Aug 2, 2023

kkafar commented Aug 3, 2023

ferrannp commented Aug 4, 2023

kkafar commented Aug 8, 2023

kkafar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WoLewicki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tboba left a comment

Choose a reason for hiding this comment

ferrannp commented Aug 10, 2023

kkafar commented Aug 11, 2023

ansh commented Oct 18, 2023

kkafar commented Dec 13, 2023 • edited Loading

kkafar commented Oct 8, 2024

feat(iOS): support for custom detents with `formSheet` presentation #1852

feat(iOS): support for custom detents with `formSheet` presentation #1852

kkafar commented Aug 1, 2023 •

edited

Loading

kkafar commented Dec 13, 2023 •

edited

Loading