Prevent a tap inside the content from closing a popup

[bijington]

Description of Change

Linked Issues

• Fixes [BUG] PopupV2 CanBeDismissedByTappingOutsideOfPopup=true still dismisses the popup if tap INSIDE the popup #2730
• Fixes [BUG] [iOS] CollectioView hosted in PopupV2 doesn't select an item unless you hold a row for longer than 4 second #2727

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

Additional information

[Copilot]

Pull Request Overview

This PR updates the popup page layout to only close when tapping outside the popup content, rather than anywhere on the page.

• Removed the old gesture recognizer on the entire content and now injects a tap command into a dedicated background grid.
• Updated PopupPageLayout constructor to accept the outside-tap command and add a transparent grid covering the full page behind the popup.
• Kept existing behaviors by only setting content when not already provided by the base constructor.

Comments suppressed due to low confidence (1)

src/CommunityToolkit.Maui/Views/Popup/PopupPage.shared.cs:188

• Add unit or UI tests to confirm that taps inside the popup content do not close the popup, and that taps outside do trigger the close behavior.

			var backgroundGrid = new Grid();

[TheCodeTraveler]

Thanks Shaun! I don't love the idea of putting a Grid inside of a Grid. Perhaps we could put an empty TapGestureRegcognizer on Border? Would that override the underlying TapGestureRegcognizer on the page?

If we do go this route with a Grid inside of a Grid, please be sure to set its BackgroundColor = null. Currently the BackgroundColor of the additional Grid is White (at least on iOS):

This PR	Main

[bijington]

Yeah I wasn't keen on this approach but hadn't considered your suggestion. I'll try that out

[TheCodeTraveler]

Update: I confirmed that we can fix #2730 by just adding an empty TapGestureRecognizer onto Border in PopupPageLayout

        internal sealed partial class PopupPageLayout : Grid
	{
		public PopupPageLayout(in Popup popupContent, in IPopupOptions options)
		{
			Background = BackgroundColor = null;

			var border = new Border
			{
				BackgroundColor = popupContent.BackgroundColor ??= PopupDefaults.BackgroundColor,
				Content = popupContent
			};
			border.GestureRecognizers.Add(new TapGestureRecognizer()); // < --- This resolves the problem without modifying any other code in `PopupPage`.

			// Bind `Popup` values through to Border using OneWay Bindings 
			border.SetBinding(Border.MarginProperty, static (Popup popup) => popup.Margin, source: popupContent, mode: BindingMode.OneWay);
			border.SetBinding(Border.PaddingProperty, static (Popup popup) => popup.Padding, source: popupContent, mode: BindingMode.OneWay);
			border.SetBinding(Border.BackgroundProperty, static (Popup popup) => popup.Background, source: popupContent, mode: BindingMode.OneWay);
			border.SetBinding(Border.BackgroundColorProperty, static (Popup popup) => popup.BackgroundColor, source: popupContent, mode: BindingMode.OneWay);
			border.SetBinding(Border.VerticalOptionsProperty, static (Popup popup) => popup.VerticalOptions, source: popupContent, mode: BindingMode.OneWay);
			border.SetBinding(Border.HorizontalOptionsProperty, static (Popup popup) => popup.HorizontalOptions, source: popupContent, mode: BindingMode.OneWay);

			// Bind `PopupOptions` values through to Border using OneWay Bindings
			border.SetBinding(Border.ShadowProperty, static (IPopupOptions options) => options.Shadow, source: options, mode: BindingMode.OneWay);
			border.SetBinding(Border.StrokeProperty, static (IPopupOptions options) => options.Shape, source: options, converter: new BorderStrokeConverter(), mode: BindingMode.OneWay);
			border.SetBinding(Border.StrokeShapeProperty, static (IPopupOptions options) => options.Shape, source: options, mode: BindingMode.OneWay);
			border.SetBinding(Border.StrokeThicknessProperty, static (IPopupOptions options) => options.Shape, source: options, converter: new BorderStrokeThicknessConverter(), mode: BindingMode.OneWay);

			Children.Add(border);
		}

		sealed partial class BorderStrokeThicknessConverter : BaseConverterOneWay<Shape?, double>
		{
			public override double DefaultConvertReturnValue { get; set; } = PopupOptionsDefaults.BorderStrokeThickness;

			public override double ConvertFrom(Shape? value, CultureInfo? culture) => value?.StrokeThickness ?? 0;
		}

		sealed partial class BorderStrokeConverter : BaseConverterOneWay<Shape?, Brush?>
		{
			public override Brush? DefaultConvertReturnValue { get; set; } = PopupOptionsDefaults.BorderStroke;

			public override Brush? ConvertFrom(Shape? value, CultureInfo? culture) => value?.Stroke;
		}
	}

[bijington]

Thanks I can apply that change now.

Out of interest do you know why we have this line:

Maui/src/CommunityToolkit.Maui/Views/Popup/PopupPage.shared.cs

Line 45 in 2217d08

base.Content ??= new PopupPageLayout(popup, popupOptions);

I can't think of a scenario when Content is not null?

[bijington]

Although given we have a strange selection bug reported with CollectionView I am keen to see if this proposal plays well with that scenario.

UPDATE: The proposal of the empty TapGestureRecognizer leaves us with the CollectionView selection bug so it's not viable sadly

[TheCodeTraveler]

I can't think of a scenario when Content is not null?

Ah - yea, we can update that to replace ??= with =. That was leftover from when the constructor of Popup<T> was assigning base.Content and I used ??= in the constructor of Popup to ensure we didn't overwrite base.Content if it was already set by Popup<T>.

[TheCodeTraveler]

Thanks Shaun! Approved ✅

FYI - I add public Border PopupBorder { get; } to PopupPageLayout to help us simplify the Unit Tests a bit.