Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Page not popped on iOS 13 FormSheet swipe down #7878

Open
breyed opened this issue Oct 8, 2019 · 11 comments · May be fixed by #7923

Comments

@breyed
Copy link
Contributor

@breyed breyed commented Oct 8, 2019

Description

On iOS 13, when the user swipes down on a modal page with a FormSheet presentation style to dismiss it, the NavigationPage does not raise a Popped event. Instead, the page is gone, but the NavigationPage's state acts as if the page is still being displayed.

Steps to Reproduce

  1. Push a FormSheet style modal page, for example, using an extension method like this:
public static NavigationPage PushModal(this Page source, Page modalRoot)
{
	var navigationPage = new NavigationPage(modalRoot);
	Xamarin.Forms.PlatformConfiguration.iOSSpecific.Page.SetModalPresentationStyle(navigationPage.On<Xamarin.Forms.PlatformConfiguration.iOS>(), Xamarin.Forms.PlatformConfiguration.iOSSpecific.UIModalPresentationStyle.FormSheet);
	source.Navigation.PushModalAsync(navigationPage);
	return navigationPage;
}
  1. Run the app. Don't close the page by pressing a button or other action that causes NavigationController.PopModalAsync to be called. Instead, swipe down on the page.

Expected Behavior

The page is popped from the navigation stack, and NavigationController.Popped is raised.

Actual Behavior

The page remains on the navigation stack, and NavigationController.Popped is not raised.

Basic Information

  • Version with issue: XF 4.3.0.851321-pre3
  • Last known good version: none
  • IDE: VS 16.3.2
  • Platform Target Frameworks:
    • iOS: 13.0
  • Affected Devices: iPhone X
@pauldipietro pauldipietro added this to New in Triage Oct 8, 2019
@jfversluis jfversluis linked a pull request that will close this issue Oct 10, 2019
1 of 2 tasks complete
@jfversluis

This comment has been minimized.

Copy link
Member

@jfversluis jfversluis commented Oct 10, 2019

This is also why #7878 was merged for now I think as a result of #7145. I've created a (draft) PR that I think fixes this, along with the whole dismissal of modals in iOS 13. Since using a modal is pretty common, I do want to have some other people looking at this before we revert back the pinning of the default modal style.

@jfversluis jfversluis moved this from New to Ready For Work in Triage Oct 10, 2019
@samhouts samhouts added this to In Progress in vNext (4.4.0) Oct 10, 2019
@samhouts samhouts removed this from Ready For Work in Triage Oct 10, 2019
@brminnick

This comment has been minimized.

Copy link
Member

@brminnick brminnick commented Oct 19, 2019

Workaround

As a workaround, you can call PopModalAsync in OnDisappearing.

This ensures that, when the user dismisses the Page manually, Xamarin.Forms still pops the Page from Navigation.ModalStack.

protected override async void OnDisappearing()
{
    base.OnDisappearing();

    if (Navigation.ModalStack.Count > 0)
        await Navigation.PopModalAsync();
}
@breyed

This comment has been minimized.

Copy link
Contributor Author

@breyed breyed commented Oct 20, 2019

@brminnick Are there any situations that OnDisappearing could be called besides the user swiping down the modal page? Switching to another app? Going to the home screen? Pushing another page? Locking the phone? PopModalAsync called by the app? Phone call received?

@brminnick

This comment has been minimized.

Copy link
Member

@brminnick brminnick commented Oct 26, 2019

@breyed ContentPage.Disappearing doesn't fire on iOS when the app is backgrounded.

To handle other scenarios where we push another page, we can unsubscribe/resubscribe to ContentPage.Disappearing to ensure PopModalAsync doesn't fire unexpectedly.

Here's an example to handle when the user launches a camera view:

public class MyPage : ContentPage
{
    public MyPage()
    {
        Disappearing += HandlePageDisappearing;
    }

    async void HandleTakePhotoButtonClicked(object sender, EventArgs e)
    {
        Disappearing -= HandlePageDisappearing;
        //Display Camera View
        Disappearing += HandlePageDisappearing;
    }

    async void HandlePageDisappearing(object sender, EventArgs e)
    {
        if (Navigation.ModalStack.Count > 0)
            await Navigation.PopModalAsync();
    }
}

This code snippet is taken from my app where I've implemented this work-around:
https://github.com/brminnick/AzureBlobStorageSampleApp/blob/6d8c0f7cb5b86e6cea3ad352f5b6ec761a461962/AzureBlobStorageSampleApp/Pages/AddPhotoPage.cs#L106-L116

@breyed

This comment has been minimized.

Copy link
Contributor Author

@breyed breyed commented Oct 28, 2019

I used this extension method as a workaround:

public static void PushModal(this Page source, Page modalRoot)
{
	var navigationPage = new NavigationPage(modalRoot);
	if (Device.RuntimePlatform == Device.iOS) {
		modalRoot.Disappearing += (sender, e) => {
			if (source.Navigation.ModalStack.Count > 0)
				source.Navigation.PopModalAsync(); // Works around a XF bug on iOS 13: https://github.com/xamarin/Xamarin.Forms/issues/7878
		};
		Xamarin.Forms.PlatformConfiguration.iOSSpecific.Page.SetModalPresentationStyle(navigationPage.On<Xamarin.Forms.PlatformConfiguration.iOS>(), Xamarin.Forms.PlatformConfiguration.iOSSpecific.UIModalPresentationStyle.FormSheet);
	}
	source.Navigation.PushModalAsync(navigationPage);
}
@brminnick

This comment has been minimized.

Copy link
Member

@brminnick brminnick commented Oct 28, 2019

@breyed There are two gotchyas in your code:

  1. Always unsubscribe Event Handlers
    • Don't subscribe to events using lambdas because they cannot be unsubscribed, leading to memory leaks
      • For each lambda, the compiler generates a class in which local variables, like Page source and navigationPage in your example, are stored as fields.
      • Because we cannot unsubscribe the compiler-generated class from the event, it will remain subscribed to the event and the garbage collector won't dispose of it
    • Here is more information: http://alookonthecode.blogspot.com/2011/05/lambda-expressions-anonymous-classes.html
  2. Always await every Task

Here is a solution where we unsubscribe from Disappearing to avoid memory leaks, and we use async/await to ensure .NET rethrows exceptions if they occur:

using System;
using System.Threading.Tasks;
using Xamarin.Forms;
using Xamarin.Forms.PlatformConfiguration;
using Xamarin.Forms.PlatformConfiguration.iOSSpecific;

namespace MyNamespace
{
    public static class NavigationExtensions
    {
        public static async Task PushModal(this Xamarin.Forms.Page source, Xamarin.Forms.Page modalRoot)
        {
            modalRoot.On<iOS>().SetUseSafeArea(true);

            var navigationPage = new Xamarin.Forms.NavigationPage(modalRoot);

            if (Device.RuntimePlatform is Device.iOS)
            {
                modalRoot.Disappearing += HandleModalPageDisappearing;
            }

            Xamarin.Forms.PlatformConfiguration.iOSSpecific.Page.SetModalPresentationStyle(navigationPage.On<iOS>(), UIModalPresentationStyle.FormSheet);

            await source.Navigation.PushModalAsync(navigationPage);
        }

        static async void HandleModalPageDisappearing(object sender, EventArgs e)
        {
            var modalPage = (Xamarin.Forms.Page)sender;

            if (modalPage.Navigation.ModalStack.Count > 0)
                await modalPage.Navigation.PopModalAsync(); // Works around a XF bug on iOS 13: https://github.com/xamarin/Xamarin.Forms/issues/7878

            modalPage.Disappearing -= HandleModalPageDisappearing;
        }
    }
}
@breyed

This comment has been minimized.

Copy link
Contributor Author

@breyed breyed commented Oct 28, 2019

@brminnick While those gotchas apply in many cases generally, neither apply in this case:

  1. Event handler lifetime: In this case, the lifetime of the event handler matches the lifetime of the modal page. After the modal page is no longer referenced, it will be GCed, at which time, nothing will reference the event handler, which will also be GCed. So no memory leak. If the modal page were longer lived than the event handler, then unsubscription would be necessary.

  2. Navigation exceptions: By avoiding the awaits on PushModalAsync and PopModalAsync, you tell the compiler to not generate code to look for and propagate async exceptions. That optimization is a good thing. The generated code would just make the app bigger and slower. Per the interface contract, the returned tasks always complete successfully. Moreover, even if there were a bug in the framework such that they didn't, ignoring the error may be preferable to having to write an exception handler or having an unhandled exception.

@brminnick

This comment has been minimized.

Copy link
Member

@brminnick brminnick commented Oct 28, 2019

@breyed I'm happy to chat more with you about this offline so that we don't clutter this Issue.

In your example, yes the garbage collector will dispose of the resources properly. I elaborated on your example, using subscribing/unsubscribing event handlers, as best-practices for other devs who would like to implement your sample, but are unaware of the dangers of using lambdas; it ensures that they don't accidentally introduce memory leaks when they modify your example for their specific use-case.

You should be using await for both PopModalAsync and PushModalAsync, because neither are guaranteed to return a successful Task:

If either of these methods throw an exception, and you did not use the await keyword when you called the method, the .NET runtime will not rethrow your exception.

Yes, async/await does add overhead to the app size (each async method increases the app size by ~100 bytes), and yes it adds overhead to the runtime execution due to context switching, however the overhead for this extension method is negligible.

If you are concerned about this overhead, you can avoid increasing the app size and avoid the extra context switching by returning the Task in the PushModal extension method instead of awaiting it:

public static Task PushModal(this Xamarin.Forms.Page source, Xamarin.Forms.Page modalRoot)
{
    modalRoot.On<iOS>().SetUseSafeArea(true);

    var navigationPage = new Xamarin.Forms.NavigationPage(modalRoot);

    if (Device.RuntimePlatform is Device.iOS)
    {
        modalRoot.Disappearing += HandleModalPageDisappearing;
    }

    Xamarin.Forms.PlatformConfiguration.iOSSpecific.Page.SetModalPresentationStyle(navigationPage.On<iOS>(), UIModalPresentationStyle.FormSheet);

    return source.Navigation.PushModalAsync(navigationPage);
}
@breyed

This comment has been minimized.

Copy link
Contributor Author

@breyed breyed commented Oct 28, 2019

@brminnick If INaviation.PushModalAsync or INaviation.PopModalAsync return tasks that may not complete successfully, it would be good catch and indicate a need to update their docs accordingly. However, I don't see based on your analysis of the iOS adapter code, how they can fault. PresentViewControllerAsync wraps presentViewController:animated:completion:, which doesn't pass an error parameter to completion; thus it cannot cause the Task to fault. Likewise for dismiss(animated:completion:).

@Thanghand

This comment has been minimized.

Copy link

@Thanghand Thanghand commented Oct 30, 2019

It is so dangerous when pop modal on disappearing. For example: Your modal has button. It can navigate to another page and event OnDisappearing will fire. It will remove your current modal page. Therefore, it is not good behavior. My solution that, I make custom NavigationPage. When i push new modal page, I always use navigationPage wrap it because it has a navigation bar. Example:
var newModalPage = new CustomNavigationPage(new TestPage);
await Navigation.PushModalAsync(newModalPage);

I make CustomNavigationPageRenderer and override the ViewDidDisappear
something like this:

Screen Shot 2019-10-30 at 2 15 18 PM

It is more safe, when modal page navigate to another page. ViewDidDisappear will not call when your modal page navigate to another page. It just call when you swipe down or Pop

@breyed

This comment has been minimized.

Copy link
Contributor Author

@breyed breyed commented Nov 7, 2019

Per @Thanghand's observation that ViewDidDisappear isn't called when navigating forward, below is a custom-renderer-based workaround. A nice aspect is that it involves no bookkeeping. Here's the code, copy-paste friendly, and everything. :-)

public class ModalNavigationPage : NavigationPage
{
	public ModalNavigationPage(Page root) : base(root) { }
}

[assembly: ExportRenderer(typeof(ModalNavigationPage), typeof(ModalNavigationPageRenderer))]
public class ModalNavigationPageRenderer : NavigationRenderer
{
	public override void ViewDidDisappear(bool animated)
	{
		base.ViewDidDisappear(animated);
		if (Element.Navigation.ModalStack.Count > 0)
			Element.Navigation.PopModalAsync();
	}
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
vNext (4.4.0)
  
In Progress
5 participants
You can’t perform that action at this time.