Skip to content
This repository was archived by the owner on May 1, 2024. It is now read-only.

iOS13 Fix Modal lifecycle #7923

Merged
merged 7 commits into from
Mar 11, 2020
Merged

iOS13 Fix Modal lifecycle #7923

merged 7 commits into from
Mar 11, 2020

Conversation

jfversluis
Copy link
Member

@jfversluis jfversluis commented Oct 10, 2019

Description of Change

Issues Resolved

API Changes

None

Platforms Affected

  • iOS

Behavioral/Visual Changes

Modals presented as a FormSheet can now be swiped down and this will trigger the popped event for a modal within Xamarin.Forms. Because of that, the app continues to behave as intended instead of getting stuck because of the modal still being on the navigation stack.

Before/After Screenshots

Not applicable

Testing Procedure

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@jfversluis
Copy link
Member Author

jfversluis commented Oct 10, 2019

For this PR I did not revert the change in #7172 yet. That might however become part of this PR. Initially, this fixes #7878, but with that I think it also fixes the whole new iOS13 modal navigation.

I've implemented the new delegate for this and from there trigger the PopModalAsync within our codebase. This fixes the initial issue, but I think is also beneficial for handling the iOS13 modals.

I did not implement the animated bool for this, I believe swiping the FormSheet modal will always be animated anyway.

Since this might impact all of the modals in iOS, I'd like to get some eyes on this before we move forward on it. Also, additional testing with other modal presentation forms and modal presentation on iPad is still to be done.

@jfversluis jfversluis requested a review from PureWeen October 10, 2019 09:33
@samhouts samhouts self-requested a review October 10, 2019 19:15
@PureWeen PureWeen added the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Oct 14, 2019
Copy link
Contributor

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Can't use because it uses XCode 11 APIs

We should just take this PR over and get it merged
#6558

I have some suggestions in there around the life cycle bits

@samhouts samhouts removed the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Jan 15, 2020
@samhouts samhouts marked this pull request as ready for review January 15, 2020 23:07
@samhouts samhouts requested a review from PureWeen January 24, 2020 18:28
@jfversluis
Copy link
Member Author

I think this is now covered in #8551 so closing this one

@jfversluis jfversluis closed this Feb 25, 2020
@PureWeen PureWeen reopened this Feb 28, 2020
@PureWeen
Copy link
Contributor

PureWeen commented Feb 28, 2020

@jfversluis can you rebase this to 4.5 and make any fixes necessary here to the appearing behavior?

@jfversluis jfversluis changed the base branch from master to 4.5.0 March 2, 2020 12:51
@jfversluis
Copy link
Member Author

@PureWeen done!

Extended the 7878 issue page a little to show it all works as expected now

Copy link
Contributor

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Looks like something didn't go quite right with the rebase

@jfversluis
Copy link
Member Author

🤔 should be fixed now

Copy link
Contributor

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

So, now it's failing from the VS 2017 checks we put in

So you'll need to if def for xcode11/xcode10

@jfversluis
Copy link
Member Author

OK for real this time!

@PureWeen PureWeen removed the request for review from StephaneDelcroix March 2, 2020 22:04
@samhouts samhouts added the Core label Mar 2, 2020
@matll42
Copy link

matll42 commented Mar 10, 2020

Hi, great thing that it is solved, I was getting my nerves on this since few days. Any idea, when we can except to get this deployed? (pre or not)

@jfversluis
Copy link
Member Author

@matll42 we need one more approval for it to get merged. After that it should be available pretty soon in the next 4.5 service release and up

@jfversluis jfversluis merged commit fd77991 into 4.5.0 Mar 11, 2020
@jfversluis jfversluis deleted the fix-7878 branch March 11, 2020 20:11
@matll42
Copy link

matll42 commented Mar 12, 2020

Sorry to insist
Is that present in nightly build? latest 4.5.0 NB is 4.5.0.125
I assume it's prior to 4.5.0.356 (stable)

In .125 modal opens with iOS12 behavior (and works)

@jfversluis
Copy link
Member Author

I'm not sure if I understand you correctly, but the nightly only holds the vNext+1 version, which is the master branch AFAIK. So that would be 4.7 at this point in time. This fix is only merged in the 4.5 branch right now and wasn't merged up all the way to master yet.

Where did you see the 4.5 version that you are mentioning?

@matll42
Copy link

matll42 commented Mar 12, 2020

Too bad, I have to wait so.
I see it in Visual Studio when I upgrade packages. I added Xamarin-CI nugget source as in https://github.com/xamarin/Xamarin.Forms/wiki/Nightly-Builds

image

@jfversluis
Copy link
Member Author

Great that you did and I love your enthusiasm, but unfortunately you will have to wait for the nightlies. Typically we merge up everything during Mondays. So come Tuesday there should be a nightly build with this fix.

You should be able to get the NuGet associated to this build, with the fix, here. You can install a NuGet package from disk as well :)

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

Successfully merging this pull request may close these issues.

[Bug] OnAppearing does not fire after a UIModalPresentationStyle.FormSheet Page is Dismissed [Bug] Page not popped on iOS 13 FormSheet swipe down
4 participants