feat(iOS): modal detents

[hurali97]

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:

https://user-images.githubusercontent.com/47336142/193402450-c9da9835-9ac4-48aa-a703-6a34d08572f5.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]

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: 745f3ee8c571560406629bc7af3cf4914ef1b211 Branch: main

[analysis-bot]

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

Base commit: 4f142bf803470dc67b8d43b2c862c3743c69f1dd Branch: main

[nandorojo]

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

[hurali97]

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]

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

[hurali97]

@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]

that's just a flaky test, unfortunately

[facebook-github-bot]

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

[nandorojo]

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

[hurali97]

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

[cipolleschi]

There are some linting errors in the internal CI. The Clang format we use internally is slightly different from the Open Source one (we are working on their alignment but is not as easy as it looks, unfortunately). Don't worry too much about them: as soon as the PR is ok, I'll reimport it, fix the linting problems and land it! ;)

[hurali97]

There are some linting errors in the internal CI. The Clang format we use internally is slightly different from the Open Source one (we are working on their alignment but is not as easy as it looks, unfortunately). Don't worry too much about them: as soon as the PR is ok, I'll reimport it, fix the linting problems and land it! ;)

Perfect. I have added a few comments regarding your suggestions, would be great if you could discuss them.

[facebook-github-bot]

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

[cipolleschi]

I'm going to run the internal formatter on the diff and then we can land it. Thank you so much for the effort and the PR!

[pull-bot]

PR build artifact for 4ce436c6f571b5b442b599b83836d5096db58f38 is ready. To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[cipolleschi]

@hurali97, sorry for the delay. What's the status with this PR? Are there any other changes you'd like to add/checks to perform or do you think we are good?

[jacobp100]

@cipolleschi I think we're still waiting on changes. I can take over if needed - it'd be good to get this feature in

[thymikee]

Hey! @hurali97 is on vacation this and next week. I've asked him if he can provide you with push access to his fork @jacobp100, hopefully he sees that somehow, but I can't bother him too much 🙂

[hurali97]

@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:

https://user-images.githubusercontent.com/47336142/201321171-8a0ea4e3-02a3-4b03-97ae-625f5232c80e.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]

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

https://user-images.githubusercontent.com/7275322/202026500-41cb7785-40cb-4ffe-bfcf-d2c30fdd9e85.mp4

[hurali97]

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]

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

[hurali97]

@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]

@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]

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:

https://user-images.githubusercontent.com/47336142/204908645-f7d4baaf-0515-4fb0-ac81-694c2de858cb.mp4

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

https://user-images.githubusercontent.com/47336142/204903075-ad703ce1-bd21-442a-8360-9eefbaa90e95.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]

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]

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]

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]

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 👀

[hurali97]

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

https://user-images.githubusercontent.com/47336142/207265984-dd64c68c-a345-4233-976a-6f16e8e176d1.mp4

Resizing Slow

https://user-images.githubusercontent.com/47336142/207266332-5e1da0f6-c0cc-4ac9-b3ab-dc4f6dae9117.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

https://user-images.githubusercontent.com/47336142/207265681-24a22e18-1af0-448e-971a-cdb72726209a.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