[Proposal] Add Popup.CloseAsync()

[brminnick]

Feature name

Add Popup.CloseAsync()

Link to discussion

Discussed in June 2023 Standup:

https://github.com/CommunityToolkit/Maui/wiki/2023-June-Standup

Progress tracker

• Android Implementation
• iOS Implementation
• MacCatalyst Implementation
• Windows Implementation
• Tizen Implementation
• Unit Tests
• Samples
• Documentation: Add Popup.CloseAsync() MicrosoftDocs/CommunityToolkit#285

Summary

This Proposal adds the following API to Popup:

public async Task CloseAsync(object? result = null);

CloseAsync() returns once the operating system has dismissed Popup from the page.

Motivation

Currently, Popup only offers one method to programmatically dismiss it from the screen: public void Close();.

However, on MacCatalyst and iOS, the code required to dismiss the Popup uses async/await to dismiss the UIViewController:

Maui/src/CommunityToolkit.Maui.Core/Handlers/Popup/PopupHandler.macios.cs

Lines 14 to 23 in e89e7da

	public static async void MapOnClosed(PopupHandler handler, IPopup view, object? result)
	{
	var vc = handler.PlatformView.ViewController;
	if (vc is not null)
	{
	await vc.DismissViewControllerAsync(true);
	}
	handler.DisconnectHandler(handler.PlatformView);
	}

The existing Close() API is thus acting in a fire-and-forget manner; the method is returning to the caller before the Popup has been dismissed on iOS + MacCatalyst.

Detailed Design

IPopup.shared.cs

This API update requires a TaskCompletionSource in IPopup that can be referenced by both the Handler and the Control.

This is required because the PropertyMappers / CommandMappers that .NET MAUI use for Handlers are not asynchronous (they cannot be awaitd).

We will instead tell the Control to await PopupDismissedTaskCompletionSource.Task after IPopup.Closed() has been called. Then, in PopupHandler.MapOnClosed, we will call PopupDismissedTaskCompletionSource TrySetResult() after the operating system has dismissed the Popup from the page.

public interface IPopup : IElement, IVisualTreeElement
{
    // ...
    // Existing APIs
    //..

    /// <summary>
    /// <see cref="TaskCompletionSource"/> that completes when the operating system has dismissed <see cref="IPopup"/> from the screen
    /// </summary>
    TaskCompletionSource PopupDismissedTaskCompletionSource { get; }
}

Popup.shared.cs

This Proposal also requires a new API public Task Popup.CloseAsync(object? result = null);

/// <summary>
/// Close the current popup.
/// </summary>
/// <remarks>
/// Returns once the operating system has dismissed the <see cref="IPopup"/> from the page
/// </remarks>
/// <param name="result">
/// The result to return.
/// </param>
public async Task CloseAsync(object? result = null)
{
  await OnClosed(result, false);
  taskCompletionSource.TrySetResult(result);
}

Usage Syntax

async void Button_Clicked(object? sender, EventArgs e)
{
    await CloseAsync();
    await Toast.Make("Popup Dismissed By Button").Show();
}

Drawbacks

Only iOS + MacCatalyst defer to a different thread when dismissing the Popup; Android and Windows can dismiss the Popup synchronously. Adding a method that returns Task adds a bit of overhead to our users, however, the overhead should be negligible as users typically only display, and subsequently close, one Popup at a time.

Alternatives

This Proposal can be updated to remove the fire-and-forget method, void Close().

However, removing an existing API is a breaking change.

Unresolved Questions

Should we instead return ValueTask?

public async ValueTask CloseAsync(object? result = null)

[VladislavAntonyuk]

Approve. Can we change only return type from void to Task/ValueTask and keep the name “Close”? The app will behave the same until user fix the warning and await method

[brminnick]

Thanks Vlad! That was my initial plan, but I decided against replacing the existing void Close() method for two reasons:

1. Replacing void Close() -> Task Close() would be a breaking change
2. Some users may not want to await the closure of a Popup (eg using a non-async button handler)

That being said, you know how much I love async/await and I always love to push developers to use the "better" API, so if the other maintainers agree that we should replace void Close() with Task Close(), I'm happy to commit!

[kphillpotts]

Approve. I like the solution you propose @brminnick for the following reasons:

1. It's a non-breaking change
2. It's easily understandable and consistent with other api surfaces
3. It balances out the ShowPopup/Close and the ShowPopupAsync/CloseAsync methods.

[bijington]

I'm happy to approve this also

[brminnick]

Great! Approved ✅

[marekm294]

Hey, quick question. Does it solve #1111 ?

[brminnick]

Does it solve #1111 ?

Yup! Thanks, I've added linked it to the PR.

[ghost]

Reopening Proposal.

Only Proposals moved to the Closed Project Column and Completed Project Column can be closed.