Skip to content

Popup V2 #1581

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

Open
wants to merge 153 commits into
base: main
Choose a base branch
from
Open

Conversation

bijington
Copy link
Contributor

@bijington bijington commented Nov 29, 2023

Description of Change

This is primarily aimed at playing around but also with the aim of exposing the implementation to see what everything thinks.

The popupcontainer is an internal ContentPage that is used to display content. there are 2 ways user can create popup. It can be a simple view or for more complex tasks user has to inherit from Popup. What are the complex tasks? In case you want to Close popup from the control or you want to get the result from the popup or you want to subscribe on OnOpened/OnClosed.

For MVVM users there is a PopupService. Pay attention all popups must be registered. Also you can close popup from PopupService only if you opened it using PopupService. DO NOT combine extension method and PopupService.

Now all configurations live in PopupOptions. You can configure BackgroundColor (Dim), popup size and popup layout position. Dissmiss by outside tap also exist in PopupOptions.

The BindingContext is set automatically to PopupContainer, Popup and View.

As the new Popup uses only MAUI Controls, we expect you get all benefits of HotReload. All Controls including Popup in Popup and MediaElement should work without issues.

Features currently removed: AnchorView.

Linked Issues

PR Checklist

  • Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Migration Guide

Popup XAML/CS

  • Remove CanBeDismissedByTappingOutsideOfPopup and pass parameter using PopupOptions
  • Color is replaced with Background Color
  • Size is replaced with WidthRequest and HeightRequest

You can now use any View as Popup Content without inheriting it from Popup. But if you need to return a result from the popup, you have to inherit from Popup.

Popup Service

  • All popup configurations can be configured using PopupOptions and passed as a parameter
  • You need to pass INavigation to make sure the popup is displayed on the correct window.

@ne0rrmatrix
Copy link
Contributor

ne0rrmatrix commented Dec 1, 2023

I have ended up rewriting at least 3 or 4 samples submitted as bugs using standard DI pattern because the code would crash in IOS 17.x. I wanted to verify the issues that were being reported. If I see a bunch of different reports I put extra effort in.

Simplifying implementation would be great. I am all for that.

@bijington bijington added the needs discussion Discuss it on the next Monthly standup label Dec 5, 2023
@TheCodeTraveler TheCodeTraveler removed the needs discussion Discuss it on the next Monthly standup label Jan 4, 2024
@bijington
Copy link
Contributor Author

There are currently a couple of breaking changes to the IPopupService that I think are still good but we should consider documenting them to aid developers upgrading:

await popupService.ShowPopupAsync<MockPageViewModel>(CancellationToken.None);

becomes

await popupService.ShowPopupAsync<MockPageViewModel>(token: CancellationToken.None);
await Assert.ThrowsAsync<TaskCanceledException>(() => popupService.ShowPopupAsync<MockPageViewModel>(viewModel => viewModel.HasLoaded = true, cts.Token));

becomes

await Assert.ThrowsAsync<TaskCanceledException>(() => popupService.ShowPopupAsync<MockPageViewModel>(viewModel => viewModel.HasLoaded = true, token: cts.Token));

It also involves removing the ability to supply a viewModel instance to the Show methods to remove the confusion we have seen around developers expecting the BindingContext to automatically be set. I am happy to move this into a separate PR if we think it if worthwhile?

@bijington bijington marked this pull request as ready for review February 11, 2024 14:26
@TheCodeTraveler
Copy link
Collaborator

@bijington FYI - we now have a few merge conflicts on this PR after merging a bunch of Popup PRs today

@dotnet-policy-service dotnet-policy-service bot added stale The author has not responded in over 30 days help wanted This proposal has been approved and is ready to be implemented labels Apr 27, 2024
@bijington bijington removed help wanted This proposal has been approved and is ready to be implemented stale The author has not responded in over 30 days labels Jun 5, 2024
@bijington bijington requested a review from a team June 9, 2024 20:43
@bijington bijington changed the title Initial musings on showing popups without having to subclass Popup Provide the ability to show popups without having to subclass Popup Jun 20, 2024
@dotnet-policy-service dotnet-policy-service bot added stale The author has not responded in over 30 days help wanted This proposal has been approved and is ready to be implemented labels Jul 22, 2024
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 11 changed files in this pull request and generated no suggestions.

Files not reviewed (6)
  • samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/MultiplePopupPage.xaml: Language not supported
  • samples/CommunityToolkit.Maui.Sample/Views/Popups/PopupContentView.xaml: Language not supported
  • samples/CommunityToolkit.Maui.Sample/MauiProgram.cs: Evaluated as low risk
  • samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/MultiplePopupViewModel.cs: Evaluated as low risk
  • samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/PopupContentViewModel.cs: Evaluated as low risk
  • samples/CommunityToolkit.Maui.Sample/Views/Popups/PopupContentView.xaml.cs: Evaluated as low risk

@VladislavAntonyuk VladislavAntonyuk force-pushed the feature/sl/remove-popup-constraint-from-popup-service branch from c1488d0 to ebc39f2 Compare January 15, 2025 00:11
@VladislavAntonyuk VladislavAntonyuk self-assigned this Jan 15, 2025
@VladislavAntonyuk VladislavAntonyuk force-pushed the feature/sl/remove-popup-constraint-from-popup-service branch from aeb9020 to 94bee57 Compare January 15, 2025 18:52
Copy link
Member

@pictos pictos left a comment

Choose a reason for hiding this comment

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

Good job! I didn't run locally yet, just reviewed the code.

I saw that you removed the old popups reference, and I think we should mark them as obsolete and let them live for a while before removing everything. At least is what I remember from the last standup.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 81 out of 96 changed files in this pull request and generated no comments.

Files not reviewed (15)
  • samples/CommunityToolkit.Maui.Sample/App.xaml: Language not supported
  • samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/MultiplePopupPage.xaml: Language not supported
  • samples/CommunityToolkit.Maui.Sample/Resources/Styles/Styles.xaml: Language not supported
  • samples/CommunityToolkit.Maui.Sample/Models/PopupSize.cs: Evaluated as low risk
  • samples/CommunityToolkit.Maui.Sample/AppShell.xaml.cs: Evaluated as low risk
  • samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/PopupSizingIssuesViewModel.cs: Evaluated as low risk
  • samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/StylePopupViewModel.cs: Evaluated as low risk
  • samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/PopupPositionViewModel.cs: Evaluated as low risk
  • samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/CustomSizeAndPositionPopupViewModel.cs: Evaluated as low risk
  • samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/UpdatingPopupViewModel.cs: Evaluated as low risk
  • samples/CommunityToolkit.Maui.Sample/Pages/Views/MediaElement/MediaElementPage.xaml.cs: Evaluated as low risk
  • samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/PopupLayoutAlignmentPage.xaml.cs: Evaluated as low risk
  • samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/ShowPopupInOnAppearingPage.xaml.cs: Evaluated as low risk
  • samples/CommunityToolkit.Maui.Sample/ViewModels/Views/ViewsGalleryViewModel.cs: Evaluated as low risk
  • samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/PopupAnchorViewModel.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)

samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/PopupContentViewModel.cs:8

  • The property name 'message' should be renamed to 'Message' to follow PascalCase convention.
string message = "";

@VladislavAntonyuk VladislavAntonyuk added pending documentation This feature requires documentation and removed help wanted This proposal has been approved and is ready to be implemented stale The author has not responded in over 30 days labels Jan 15, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors and modernizes popup handling in the .NET MAUI Toolkit samples by replacing legacy direct popup calls with a centralized IPopupService and by removing deprecated popup dependencies. Key changes include:

  • Replacing usages of PopupSizeConstants with IPopupService injection and flag-based popup display control.
  • Adding new sample pages (e.g. PopupsPage) that demonstrate various popup behaviors.
  • Removing obsolete popup pages and adjusting popup registrations and view model mappings accordingly.

Reviewed Changes

Copilot reviewed 112 out of 120 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/ShowPopupInOnAppearingPage.xaml.cs Updated to inject IPopupService and display a popup once upon appearing using a flag.
samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/PopupsPage.xaml.cs Introduces multiple async handlers to demonstrate various popup scenarios.
samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/PopupOnDisappearingPage.cs Displays a popup on page disappearance using ShowPopupAsync.
samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/PopupLayoutAlignmentPage.xaml.cs Refactored to use local layout option variables and streamlined popup display.
samples/CommunityToolkit.Maui.Sample/Pages/Views/MediaElement/MediaElementPage.xaml.cs Modified the popup display in media element scenarios to use ShowPopupAsync.
samples/CommunityToolkit.Maui.Sample/MauiProgram.cs Removed deprecated PopupSizeConstants registration and updated popup registrations.
samples/CommunityToolkit.Maui.Sample/AppShell.xaml.cs Adjusted view model mappings to reflect new sample page structure.
Files not reviewed (8)
  • Directory.Build.props: Language not supported
  • samples/CommunityToolkit.Maui.Sample/App.xaml: Language not supported
  • samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/CustomSizeAndPositionPopupPage.xaml: Language not supported
  • samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/PopupAnchorPage.xaml: Language not supported
  • samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/PopupSizingIssuesPage.xaml: Language not supported
  • samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/PopupsPage.xaml: Language not supported
  • samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/ShowPopupInOnAppearingPage.xaml: Language not supported
  • samples/CommunityToolkit.Maui.Sample/Resources/Styles/Styles.xaml: Language not supported

Copy link
Member

@pictos pictos left a comment

Choose a reason for hiding this comment

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

I observe that the PR removes the outdated Popup implementation, and the main file lacks the Obsolete attribute. Consequently, this constitutes a significant breaking change that will not provide other developers with sufficient time to migrate to the new Popup implementation. Furthermore, this action isn't the pattern followed by the official .NET libraries. Therefore, I strongly recommend reverting the deleted files and adding the Obsolete attribute.


if (popupPageToClose is null)
{
throw new InvalidOperationException($"Unable to close popup: could not locate {nameof(PopupPage)}. If using a custom implementation of {nameof(Popup)}, override the {nameof(Close)} method");
Copy link
Member

Choose a reason for hiding this comment

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

not sure if should throw for here, I can't see good reason to do an exception if the popup can't be closed and not given a way for devs to leave here. In other words, let's say that we have 3 popups and for some reason I can't close the last one, then my app will crash (if the exception isn't handled) or will be blocked on those popups forever.

Maybe we can throw and add a method CloseAllPopups that will close all popups opened popups?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The Exception is valid.

When the developer calls Close(), we have two options:

  1. Close the current Popup
  2. Throw an exception telling them why we were unable to close the Popup

Copy link
Member

@pictos pictos Apr 22, 2025

Choose a reason for hiding this comment

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

Yeap, but in a production code, the app will be blocked forever on the popup. So I would say that we should provide a panic mode, a method that will close all popups.

I believe it's easy to implement a method like that and expose them for all devs, that way they can decide how to handle this situation better

Copy link
Collaborator

@TheCodeTraveler TheCodeTraveler Apr 23, 2025

Choose a reason for hiding this comment

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

The scenarios where we throw an InvalidOperationException are very rare and I doubt many (if any) developers will run into either.

the app will be blocked forever on the popup

The good news is that both scenarios have work-arounds ensuring that we don't block developers:

  1. Developers have implemented their own implementation of Popup that bypasses PopupPage
    • The solution here, as the Exception message explains, is to override the Popup.Close() method
  2. A developer has pushed a Modal Page on top of the Popup
    • The solution here, as the Exception message explains, is to ensure all modal pages are dismissed before calling .Close() (example solution below)
while(Navigation.ModalStack.Count > 1)
{
    await Navigation.PopModalAsync();
}

await popup.Close();

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll be sure to work with Shaun to add this to the docs 👍

@bijington
Copy link
Contributor Author

bijington commented Apr 27, 2025

Amazing work team! The docs PR should be coming this week too

@TheCodeTraveler TheCodeTraveler added the approved This Proposal has been approved and is ready to be added to the Toolkit label Apr 27, 2025
@VladislavAntonyuk VladislavAntonyuk removed the needs discussion Discuss it on the next Monthly standup label May 1, 2025
@daltzctr
Copy link

daltzctr commented May 2, 2025

Is there a way to utilize PR artifacts in a local build (similar to how .NET MAUI does it)?

@pictos
Copy link
Member

pictos commented May 2, 2025

@daltzctr yes, go here, scroll until the bottom of the page and you see the nugets there for download

/// <summary>
/// Gets or sets the result that will return when the user taps outside the Popup.
/// </summary>
protected object? ResultWhenUserTapsOutsideOfPopup { get; set; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did we really want to remove the ability to set a result for this scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we reinstate this then we could potentially remove the need for the WasDismissedByTappingOutsideOfPopup property in PopupResult

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not needed because when the user taps outside of Popup, the result is always null.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The only way to set the result of a Popup is to call Popup.Close(). When the user taps outside of the popup, the developer does not call Popup.Close() and therefore never provides us the result.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be honest, if I could wave a magic wand, I would remove Popup<T> completely because it just turns Popup into a Messaging Service which is entirely unnecessary. There already exists a million ways for developers to propagate data between the View and ViewModel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The developer doesn't call close but our code does? Could we somehow retrieve the default value from PopupOptions and return that value. Writing this it sounds a little involved and the only way I can think to do it is via an attached property

Copy link
Collaborator

@TheCodeTraveler TheCodeTraveler May 7, 2025

Choose a reason for hiding this comment

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

The developer doesn't call close but our code does

Correct. We add a GestureRecognizer to the PopupPage that fires await Close(new PopupResult(true)); when the user taps outside of the popup.

public PopupPage(Popup popup, IPopupOptions popupOptions)
{
ArgumentNullException.ThrowIfNull(popup);
ArgumentNullException.ThrowIfNull(popupOptions);
this.popup = popup;
this.popupOptions = popupOptions;
// Only set the content if the parent constructor hasn't set the content already; don't override content if it already exists
base.Content ??= new PopupPageLayout(popup, popupOptions);
tapOutsideOfPopupCommand = new Command(async () =>
{
popupOptions.OnTappingOutsideOfPopup?.Invoke();
await Close(new PopupResult(true));
}, () => popupOptions.CanBeDismissedByTappingOutsideOfPopup);
Content.GestureRecognizers.Add(new TapGestureRecognizer { Command = tapOutsideOfPopupCommand });
if (popupOptions is BindableObject bindablePopupOptions)
{
bindablePopupOptions.PropertyChanged += HandlePopupPropertyChanged;
}
this.SetBinding(BindingContextProperty, static (Popup x) => x.BindingContext, source: popup, mode: BindingMode.OneWay);
this.SetBinding(BackgroundColorProperty, static (IPopupOptions options) => options.PageOverlayColor, source: popupOptions, mode: BindingMode.OneWay);
Shell.SetPresentationMode(this, PresentationMode.ModalNotAnimated);
On<iOS>().SetModalPresentationStyle(UIModalPresentationStyle.OverFullScreen);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh this could be an easy solve then... we could sub class TapGestureRecognizer to take the default return value and return the value when it's actioned?

If it's something we want to retain I'm happy to make the changes.

I'm just working through all the current breaks and trying to see what future bug reports we can avoid

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry - are we still talking about ResultWhenUserTapsOutsideOfPopup here, or did we cross threads?

It's a good breaking change to remove ResultWhenUserTapsOutsideOfPopup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was still talking about that property. OK I can update the docs to reflect this change

/// <summary>
/// Provides a mechanism for displaying Popups based on the underlying view model.
/// </summary>
public interface IPopupService
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have lost the ability to pass data across to the view model when it is being presented (the old onPresenting parameter). Do we have an alternative here or is this another breaking change to document?

Copy link
Collaborator

@TheCodeTraveler TheCodeTraveler May 7, 2025

Choose a reason for hiding this comment

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

Could you explain more about what you mean?

When the developer calls Popup.Close() in the ViewModel, they pass in the result as a parameter to us.

That result is then returned to the code that is await'ing Popup.ShowAsync() (likely the View).

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the user calls the Popup from the View, they don't need to pass in a result that we'll pass directly back to them.

Copy link
Collaborator

@TheCodeTraveler TheCodeTraveler May 7, 2025

Choose a reason for hiding this comment

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

To be honest, if I could wave a magic wand, I would remove Popup<T> completely because it just turns Popup into a Messaging Service which is entirely unnecessary. There already exists a million ways for developers to propagate data between the View and ViewModel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually talking about the other direction here. Setting values on the view model behind the popup, a bit like what can be done with IQueryAttributable in shell.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it. Yup, let's delete that section from the docs.

There's no reason for this because everything in Popup is Bindable including PopupOptions. We shouldn't be reinventing the wheel for passing data back-and-forth between a View and ViewModel for this narrow use case.

If the user wants to do something when the Popup opens, they can override Popup.OnAppearing() like they would for any View.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to delete the section but I'll see if I can rework it to use PopupOptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this isn't reinventing the wheel... let me try to explain:

We have a view model ExamResultsViewModel which shows exam result for a student. When the user taps on a result they can see some more information in a popup, let's call it ExamResultsDetailPopup which has a backing view model called ExamResuktsDetailsPopupViewModel). The scenario I'm talking about wants to get the data from ExamResultsViewModel to ExamResuktsDetailsPopupViewModel.

Using messaging would be a messy because you want to get the data to the view model before the popup is displayed. So this scenario is trying to match what is available with IQueryAttributable when navigating between pages in Maui

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah - ViewModel to ViewModel. Yea, that makes sense. Got it.

Yea, let's still move forward with this release and add that in if we get feedback from the community, similar to Anchor.

I do wonder (not for this PR) if we could implement IQueryable for Popup so that it "just works".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's fair. I'm happy to try and follow up with IQueryAttributable support in a separate PR. It'll be nice to unify with the current APIs in MAUI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This Proposal has been approved and is ready to be added to the Toolkit area/views Issue/Discussion/PR that has to do with Views breaking change This label is used for PRs that include a breaking change pending documentation This feature requires documentation
Projects
None yet
7 participants