feat(iOS): modal detents #34834

hurali97 · 2022-10-01T09:38:03Z

Summary

As proposed in this discussion, this PR introduces feature for iOS 15+ detents with fabric support. Kudos to @barthap for his wonderful gist which introduces the feature for paper. Additional work was done to introduce this feature for fabric.

We have three values for detents and the usage is like below:

<Modal 
  detent="mediumResizable" // medium, large, mediumResizable
  presentationStyle="formSheet" // can also try pageSheet
/>

Down the road when adding support for fabric, it occurred to me that onRequestClose is not implemented, which prevents the dismiss of formSheet or PageSheet when we drag it down. So, added the support for onRequestClose as well. I am not sure why this was left out for fabric, if it was intentional, it might worth a discussion.

Since we don't have detents for android AFAIK, an empty method setDetent implemented in ReactModalHostManager to confirm to the generated interface.

Thanks to @nandorojo for his POC with the above gist and a nice tweet showcasing the demo 🚀

Changelog

[iOS] [Added] - iOS 15 detents to Modal

Test Plan

This feature is tested on rn-tester with both old and new arch, below is the output:

detent-demo.mp4

Code for rn-tester is not committed to not bloat the PR. I may address it following the design principles of rn-tester in a couple of days, once this PR is merged.

TODO

Update documentation for detent on react-native website
Update code to implement detent in rn-tester

analysis-bot · 2022-10-01T10:12:53Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,062,509	+426
android	hermes	armeabi-v7a	6,466,999	+290
android	hermes	x86	7,376,950	+272
android	hermes	x86_64	7,345,932	+458
android	jsc	arm64-v8a	8,919,615	+514
android	jsc	armeabi-v7a	7,685,963	+377
android	jsc	x86	8,869,166	+361
android	jsc	x86_64	9,460,069	+545

Base commit: 745f3ee
Branch: main

analysis-bot · 2022-10-01T11:15:57Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 4f142bf
Branch: main

nandorojo · 2022-10-01T12:15:46Z

Thoughts on calling the prop detent without the word modal? Seems a bit obvious that the modal is part of it.

hurali97 · 2022-10-03T07:06:40Z

Thoughts on calling the prop detent without the word modal? Seems a bit obvious that the modal is part of it.

Hmm. I tend to agree with keeping the prop name to detent, to match the parity with other props such as presentationStyle, animationType, etc. I will try to incorporate these changes asap. Thanks for the comment 🌟

cipolleschi

Amazing, thank you so much for doing this! 👏

I left a couple of very minor suggestions to improve a bit the codebase.

React/Fabric/Mounting/ComponentViews/Modal/RCTModalHostViewComponentView.mm

cipolleschi · 2022-10-04T15:49:15Z

Could you also rebase the PR on main? The ios errors should have been fixed now.

hurali97 · 2022-10-05T08:56:45Z

@cipolleschi Thanks for the reviews, these are now incorporated. Would be great if you could re-review 🚀

I am not sure why CI fails on test_windows.

cipolleschi · 2022-10-05T09:23:20Z

that's just a flaky test, unfortunately

facebook-github-bot · 2022-10-05T09:23:40Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

nandorojo · 2022-10-06T15:39:21Z

Awesome seeing this approved so quickly, thanks @hurali97 & @cipolleschi!

hurali97 · 2022-10-11T14:53:33Z

@cipolleschi Can you see what caused the Facebook Internal - Linter to fail? 🙂

cipolleschi · 2022-10-17T13:19:25Z

React/Fabric/Mounting/ComponentViews/Modal/RCTModalHostViewComponentView.mm

+      case ModalHostViewDetent::Large:
+        break;
+      case ModalHostViewDetent::Medium:
+        detents = @[UISheetPresentationControllerDetent.mediumDetent];


From @sammy-SC:

here it looks like we're holding some state in view controller that does not get reset when component is reused or detent value changes. Is that expected?

I think we should update the prepareForRecycle method.

I tried changing the detent values using the states and they are working as they should. Also, the prepareForRecycle method already sets the _viewController to nil which sets the detents to nil automatically. So we're in good shape I think.

Here's the attached video, on how it looks like with states:

detents.mp4

cipolleschi · 2022-10-17T13:21:01Z

React/Fabric/Mounting/ComponentViews/Modal/RCTModalHostViewComponentView.mm

@@ -258,6 +293,7 @@ - (void)updateProps:(Props::Shared const &)props oldProps:(Props::Shared const &
  self.viewController.modalPresentationStyle = presentationConfiguration(newProps);

  _shouldPresent = newProps.visible;
+  [self handleDetent: newProps.detent];


From @sammy-SC

should we check if detent changes?

I think we should check whether the new detent is different from the old one and proceed updating the props only if the two differs.

What do you think?

Hey so I tested by comparing old and new props but it turns out we can’t really add that guard. The reason is, whenever the modal closes, prepareForRecycle gets called and it sets _viewController to nil. With this we loses the detents we set on the _viewController. So when the next time we open the modal,
detents aren’t applied due to the guard we set previously and _viewController has already been reset and lost the detents on it.

Here's how the guard is added:

Thank you for checking this!

…react-native into feat/ios/modal-detents

hurali97 · 2022-11-11T10:31:02Z

@jacobp100 @cipolleschi So the final state of this PR is such that it allows the user to set medium, large or custom sized detents.

  <Modal
        detents={['120', 'medium', 'large']}
        presentationStyle='pageSheet'
     />

This works as expected and produces the following output:

ios-detents.mp4

However there's one case where I think we could investigate. When we supply a value of 100 or less, I have noticed that the modal doesn't dismiss as presentationControllerDidAttemptToDismiss isn't called. See more details in my previous comment

  <Modal
        detents={['100', 'medium', 'large']}
        presentationStyle='pageSheet'
     />

@jacobp100 I am sending you the access rights to my fork, since I am on vacation and sadly can't find enough time right now.

jacobp100 · 2022-11-15T21:17:22Z

I've got a branch cleaning up the conversions here - https://github.com/hurali97/react-native/tree/detents-converters

I can't get Fabric working - I think that will be more difficult because it won't allow mixed array types

I did notice one thing - when resizing it, the content jumps. I don't think we can ship it like this - at minimum we need to set the background colour so the gap isn't visible

I did spend a fair amount of time @grahammendick trying to fix this exact issue - but I don't think we ever found a full solution. I think if we can force a synchronous re-layout, we might be able to get iOS to animate it properly - but I don't know enough about fabric to do that

Simulator.Screen.Recording.-.iPhone.SE.3rd.generation.-.2022-11-15.at.21.11.23.mp4

hurali97 · 2022-11-21T09:25:42Z

Hey @jacobp100,

Thanks for your work, appreciated.

You're right Fabric doesn't work with mixed array types that's why I used detents?: ?$ReadOnlyArray<string> to make it work with fabric. I tend to agree with this approach because we can then support medium, large and custom size. But if we do like detents?: WithDefault<$ReadOnlyArray<'medium' | 'large'>, 'large'> the signature doesn't say we can pass any other custom size. WDYT here?

And regarding the jumping layout, you're right and this happens for both fabric and paper, we need to handle this behaviour. I can take a look at this ✍️

jacobp100 · 2022-11-21T09:51:43Z

@hurali97 I think we should just support the medium and large sizes for now - until Fabric adds support for mixed types

hurali97 · 2022-11-21T10:04:01Z

@hurali97 I think we should just support the medium and large sizes for now - until Fabric adds support for mixed types

Understandable. So that leaves us with handling the jumping layout. I will pull your changes to my branch and start on top of it.

jacobp100 · 2022-11-21T10:23:25Z

@hurali97 it might be possible to get numbers working by removing the codegen for that prop, and manually doing it in the updateProps method. You get a JSValue class with has methods like isNumber etc.

The layout jumping is probably the bigger issue though!

hurali97 · 2022-11-30T21:06:26Z

Guys @jacobp100 @cipolleschi here's an update on this, sorry for being late.

A quick recap of the problem we were facing with layout. So I tried debugging the cause for the jump in layout or the glitch at bottom of the modal. It turns out to me that there might be some underlying in the RCTViewComponentView which causes this issue (I might be wrong), since there's some state being set using the view bounds.

- (void)boundsDidChange:(CGRect)newBounds
{
  auto eventEmitter = [self modalEventEmitter];
  if (eventEmitter) {
    eventEmitter->onOrientationChange(onOrientationChangeStruct(newBounds));
  }

  if (_state != nullptr) {
    auto newState = ModalHostViewState{RCTSizeFromCGSize(newBounds.size)};
    _state->updateState(std::move(newState));
  }
}

RCTModalHostViewComponentView

- (void)viewDidLayoutSubviews
{
  [super viewDidLayoutSubviews];
  if (!CGRectEqualToRect(_lastViewBounds, self.view.bounds)) {
    [_delegate boundsDidChange:self.view.bounds];
    _lastViewBounds = self.view.bounds;
  }
}

RCTFabricModalHostViewController

So I tried injecting a UIView with a UILabel directly as a subview of RCTFabricViewModalHostViewController in mountChildComponentView. This experiment goes well I think, as we see no glitch while resizing the VC. Thanks to @troZee for helping me out 🌟

Here's a video experimenting UIView as subview of VC:

native-view.mp4

Here's a video with the default RCTViewComponentView as subview of VC:

rct-view.mp4

@cipolleschi @jacobp100 If you guys can provide some more details here or have some other suggestions that would be great in moving forward. Apart from this, as a temporary fix we may go with setting the backgroundColor to the view of VC, it may fix the glitch but not the layout jump of content inside. Also, with this approach we might need to get a hold of the RCTView in order to read the backgroundColor and then set it on the VC's view, as I think taking backgroundColor from user isn't right in such case.

jacobp100 · 2022-11-30T23:27:29Z

Is it possible to force a synchronous layout? I’ve not done enough fabric to know how - but if we do that in the resize delegate method, I think iOS should animate the resizing

hurali97 · 2022-12-01T07:28:05Z

Is it possible to force a synchronous layout? I’ve not done enough fabric to know how - but if we do that in the resize delegate method, I think iOS should animate the resizing

Yes, we can try that too. We will need to look for pointers in the other RN components to get a hold of how it will be done in fabric. Maybe @cipolleschi have some info on this🧐.

cipolleschi · 2022-12-01T09:17:28Z

Hi there!

Small PSA: I'll be on PTO for a week, from today to the next Thursday.

On this specific matter, I think @sammy-SC could have more information on how to ask Fabric to execute a sync layout.

hurali97 · 2022-12-09T09:24:44Z

Hi there!

Small PSA: I'll be on PTO for a week, from today to the next Thursday.

On this specific matter, I think @sammy-SC could have more information on how to ask Fabric to execute a sync layout.

Hi @sammy-SC,

A quick recap can be seen here. What we basically want is to ask Fabric to execute layout synchronously. Before diving in myself to see how we can achieve this, I can really use some pointers here 👀

…t-native into feat/ios/modal-detents

hurali97 · 2022-12-13T08:41:36Z

Update about my recent findings.

I tried to find how we can cause fabric to do synchronous layout. But before that, I wanted to make sure whether it is really related to fabric synchronous layout or otherwise. For that, I again analysed this PR on both old paper and new fabric architecture. And on the both archs, we have the same behaviour of layout jump. So it's hard to say it can be fixed by synchronous layout using fabric. I decided to dive a bit deeper in the C++ implementation of ModalHostView. In ModalHostViewComponentDescriptor I stumbled upon shadowNode accessing state for screenSize. I realised this is the shared state that we have in ModalHostViewComponent. Now, in ModalHostViewComponentDescriptor we have layoutableShadowNode calling setSize which sets height and width style on the yoga node.

I tried to experiment setSize method by passing hardcoded height and width, and the layout jump was gone however, it resulted in content being not centred, which makes sense.

There's another observation, when we resize the modal slowly, the layout doesn't jump however when we do it fast, it causes jump.

Resizing Fast

with-out-bg-fast.mp4

Resizing Slow

with-out-bg-slow.mp4

These observations tends me to think it's related to how Yoga is trying to layout this underneath. Since, we don't know the height of the modal and Yoga tries to calculate it upon resizing, as we update the shared state in boundsDidChange, which causes the layoutableShadowNode to set the size on yoga node and then yoga does some additional calculations underneath which is quite complicated to get a hold off.

As we also have this same behaviour on old paper architecture, I think it maybe known to the core team. If such is the case, we can consider testing the second option that we had, to set the backgroundColor on the VC.

The point is how do we approach it. I tried to get the backgroundColor of the childComponentView that we get as a parameter of mountChildComponentView but the backgroundColor is null. Then, I tried accessing the backgroundColor from _props which is basically ViewProps, but no luck there as well. Also, I tried reactSubviews but no luck in that department as well. So I tried using drawViewHeirarchyInRect only for the first time when modal resizes, and draw it to an image using UIGraphics, then get the pixel colour from the image. Then set it to the VC as backgroundColor.

Now with this approach, we get the same color set by the user without implementation differences on both old and new architectures. I haven't yet measured the performance this process may cause. I used guard to calculate it only once and for the minimum rect from the bottom of screen, but still need to measure it.

If there's any other more efficient way we can get the backgroundColor from the underlying view, I am more than happy to discuss it.

With background color

with-bg.mp4

I haven't pushed the code for backgroundColor stuff, if we opt-in going this path, I will do it and we can then review it.

@cipolleschi @jacobp100 fyi

cipolleschi · 2022-12-15T13:38:40Z

Wow @hurali97, thank you so much for this detailed explanation! 😮

Unfortunately, I don't have clear answers and a full context for you. I'll try to route this PR to people working on Yoga and which have more context on how the renderers work. It could be tricky as the Christmas is coming, but I'll try. Thank you again for this.

nandorojo · 2023-02-05T21:51:46Z

Hey guys, hate to be the one pinging PRs, but alas...does it seem like this is close to merging? I'm still using the original patch from the linked tweet, would be awesome to have it built in! Thanks for all the work here.

cipolleschi · 2023-02-07T09:51:40Z

@nandorojo, sorry but with those glitches we can't really merge it. I'll try to ping some other people to have more clarity on how to move forward!

github-actions · 2023-08-13T05:30:59Z

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

github-actions · 2023-09-05T05:13:15Z

This PR was closed because it has been stalled for 7 days with no activity.

grahammendick · 2023-11-10T17:54:20Z

Hey, I just added Bottom Sheet support to my Navigation router on iOS. I removed the flicker you're seeing when the sheet is dropped. Details in the PR but let me know if you want to know more

## 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

hurali97 added 5 commits October 1, 2022 03:08

chore(types): added modalDetent type

47b95af

feat(ios): support for modal detent in paper

7dd8ec7

fix: match the java delegate generated by codegen

6c149e4

feat(ios): support for modal detent in fabric

c3b6f9b

refactor: remove console.log

a1d071b

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Oct 1, 2022

fix(prettier): fix formatting

a4abe50

react-native-bot added Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Oct 1, 2022

fix(prettier): fix Modal.d.ts formatting

5a7a9e2

hurali97 added 3 commits October 3, 2022 13:08

refactor(JS): changed type for updated prop detent

3bdd88b

refactor(iOS): changed type for updated prop detent

639065f

refactor(android): changed type for updated prop detent

826d4c9

nandorojo mentioned this pull request Oct 4, 2022

⭐️ Support Detents for Page Sheet for Improved Bottom Sheet UX dominicstop/react-native-ios-modal#7

Open

cipolleschi requested changes Oct 4, 2022

View reviewed changes

React/Fabric/Mounting/ComponentViews/Modal/RCTModalHostViewComponentView.mm Outdated Show resolved Hide resolved

React/Fabric/Mounting/ComponentViews/Modal/RCTModalHostViewComponentView.mm Outdated Show resolved Hide resolved

hurali97 added 2 commits October 5, 2022 10:53

Merge branch 'facebook:main' into feat/ios/modal-detents

37cc67f

refactor: fixed indentation and early exit if nil

aef05d2

cipolleschi approved these changes Oct 5, 2022

View reviewed changes

cipolleschi requested changes Oct 17, 2022

View reviewed changes

Merge branch 'feat/ios/modal-detents' of https://github.com/hurali97/…

5f8c366

…react-native into feat/ios/modal-detents

Detents WIP

1b45c43

hurali97 added 2 commits December 1, 2022 16:34

Merge branch 'facebook:main' into feat/ios/modal-detents

c615745

Merge branch 'facebook:main' into feat/ios/modal-detents

38acf4a

Merge branch 'detents-converters' of https://github.com/hurali97/reac…

e70f593

…t-native into feat/ios/modal-detents

grahammendick mentioned this pull request Feb 11, 2023

Bottom Sheet sheet collapsed as soon as you start scrolling down ? grahammendick/navigation#666

Closed

github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Aug 13, 2023

github-actions bot closed this Sep 5, 2023

grahammendick mentioned this pull request Oct 17, 2023

Bottom Sheet (IOS) grahammendick/navigation#726

Closed

kkafar mentioned this pull request Nov 15, 2023

fix(iOS): modal view flickering software-mansion/react-native-screens#1870

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iOS): modal detents #34834

feat(iOS): modal detents #34834

hurali97 commented Oct 1, 2022 •

edited

Loading

analysis-bot commented Oct 1, 2022 •

edited

Loading

analysis-bot commented Oct 1, 2022 •

edited

Loading

nandorojo commented Oct 1, 2022

hurali97 commented Oct 3, 2022 •

edited

Loading

cipolleschi left a comment

cipolleschi commented Oct 4, 2022

hurali97 commented Oct 5, 2022

cipolleschi commented Oct 5, 2022

facebook-github-bot commented Oct 5, 2022

nandorojo commented Oct 6, 2022

hurali97 commented Oct 11, 2022

cipolleschi Oct 17, 2022

hurali97 Oct 18, 2022

cipolleschi Oct 17, 2022

hurali97 Oct 18, 2022

cipolleschi Oct 20, 2022

hurali97 commented Nov 11, 2022

jacobp100 commented Nov 15, 2022 •

edited

Loading

hurali97 commented Nov 21, 2022 •

edited

Loading

jacobp100 commented Nov 21, 2022

hurali97 commented Nov 21, 2022

jacobp100 commented Nov 21, 2022

hurali97 commented Nov 30, 2022 •

edited

Loading

jacobp100 commented Nov 30, 2022

hurali97 commented Dec 1, 2022

cipolleschi commented Dec 1, 2022

hurali97 commented Dec 9, 2022

hurali97 commented Dec 13, 2022

cipolleschi commented Dec 15, 2022

nandorojo commented Feb 5, 2023

cipolleschi commented Feb 7, 2023

github-actions bot commented Aug 13, 2023

github-actions bot commented Sep 5, 2023

grahammendick commented Nov 10, 2023

feat(iOS): modal detents #34834

feat(iOS): modal detents #34834

Conversation

hurali97 commented Oct 1, 2022 • edited Loading

Summary

Changelog

Test Plan

TODO

analysis-bot commented Oct 1, 2022 • edited Loading

analysis-bot commented Oct 1, 2022 • edited Loading

nandorojo commented Oct 1, 2022

hurali97 commented Oct 3, 2022 • edited Loading

cipolleschi left a comment

Choose a reason for hiding this comment

cipolleschi commented Oct 4, 2022

hurali97 commented Oct 5, 2022

cipolleschi commented Oct 5, 2022

facebook-github-bot commented Oct 5, 2022

nandorojo commented Oct 6, 2022

hurali97 commented Oct 11, 2022

cipolleschi Oct 17, 2022

Choose a reason for hiding this comment

hurali97 Oct 18, 2022

Choose a reason for hiding this comment

cipolleschi Oct 17, 2022

Choose a reason for hiding this comment

hurali97 Oct 18, 2022

Choose a reason for hiding this comment

cipolleschi Oct 20, 2022

Choose a reason for hiding this comment

hurali97 commented Nov 11, 2022

jacobp100 commented Nov 15, 2022 • edited Loading

hurali97 commented Nov 21, 2022 • edited Loading

jacobp100 commented Nov 21, 2022

hurali97 commented Nov 21, 2022

jacobp100 commented Nov 21, 2022

hurali97 commented Nov 30, 2022 • edited Loading

jacobp100 commented Nov 30, 2022

hurali97 commented Dec 1, 2022

cipolleschi commented Dec 1, 2022

hurali97 commented Dec 9, 2022

hurali97 commented Dec 13, 2022

cipolleschi commented Dec 15, 2022

nandorojo commented Feb 5, 2023

cipolleschi commented Feb 7, 2023

github-actions bot commented Aug 13, 2023

github-actions bot commented Sep 5, 2023

grahammendick commented Nov 10, 2023

hurali97 commented Oct 1, 2022 •

edited

Loading

analysis-bot commented Oct 1, 2022 •

edited

Loading

analysis-bot commented Oct 1, 2022 •

edited

Loading

hurali97 commented Oct 3, 2022 •

edited

Loading

jacobp100 commented Nov 15, 2022 •

edited

Loading

hurali97 commented Nov 21, 2022 •

edited

Loading

hurali97 commented Nov 30, 2022 •

edited

Loading