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

[WinUI] Flyout Popover doesn't reset "IsPresented" when dismissed via click on contents. #13496

Open
drasticactions opened this issue Feb 22, 2023 · 11 comments
Labels
area-controls-flyout Flyout platform/windows 🪟 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working
Milestone

Comments

@drasticactions
Copy link
Contributor

Description

When using a Popover FlyoutPage on WinUI and setting "IsPresented", the Flyout pane will show. Clicking on the main page contents will hide the popover, but "IsPresented" is still true, meaning you can't reinvoke the popover again unless you set it to false yourself.

Steps to Reproduce

  1. Run the MAUI Samples app.
  2. Go to the FlyoutPage https://github.com/dotnet/maui/blob/main/src/Controls/samples/Controls.Sample/Pages/Core/FlyoutPageGallery.xaml
  3. Set it to Popover
  4. Click on "Show", then click on the main page.

Link to public reproduction project repository

https://github.com/dotnet/maui/blob/main/src/Controls/samples/Controls.Sample

Version with bug

7.0 (current)

Last version that worked well

Unknown/Other

Affected platforms

Windows

Affected platform versions

At least WinUI, may affect other platforms.

Did you find any workaround?

Detect when the flyout is closing and handle "IsPresenting" yourself, but this may cause the flyout to close twice.

Relevant log output

No response

@drasticactions drasticactions added the t/bug Something isn't working label Feb 22, 2023
@drasticactions drasticactions changed the title [WinUI] Flyout Popover doesn't reset "IsPresented" when dismissed via click. [WinUI] Flyout Popover doesn't reset "IsPresented" when dismissed via click on contents. Feb 22, 2023
@rachelkang rachelkang added this to the Backlog milestone Feb 22, 2023
@ghost
Copy link

ghost commented Feb 22, 2023

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@Eilon Eilon added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Apr 14, 2023
@Keflon
Copy link

Keflon commented Apr 17, 2023

I just ran into this. I have a workaround that monitors IsFocused on the FlyoutPage::Flyout property.
Basically if the Flyout loses focus and the FlyoutLayoutBehavior is Popover (you may want to cover different cases as well), it assumes the Flyout has been dismissed and sets the IsPresented property to false.
It's far from ideal, so if anyone has any better ideas please share.

using System.ComponentModel;

namespace FunctionZero.Maui.MvvmZero.Workaround
{
    public class AdaptedFlyoutPage : FlyoutPage
    {
        Page _oldFlyout;
        public AdaptedFlyoutPage()
        {
#if WINDOWS
            PropertyChanged += AdaptedFlyoutPage_PropertyChanged;
#endif
            _oldFlyout = null;
        }
        private void AdaptedFlyoutPage_PropertyChanged(object sender, PropertyChangedEventArgs e)
        {
            if (e.PropertyName == nameof(FlyoutPage.Flyout))
            {
                if (_oldFlyout != null)
                    _oldFlyout.PropertyChanged -= FlyoutFlyout_PropertyChanged;

                if (Flyout != null)
                    Flyout.PropertyChanged += FlyoutFlyout_PropertyChanged;

                _oldFlyout = Flyout;
            }
        }
        private async void FlyoutFlyout_PropertyChanged(object sender, PropertyChangedEventArgs e)
        {
            if (e.PropertyName == nameof(Page.IsFocused))
            {
                if (FlyoutLayoutBehavior == FlyoutLayoutBehavior.Popover)
                {
                    if (Flyout.IsFocused == false)
                    {
                        // Delay is necessary otherwise closing the Flyout using the hamburger menu
                        // immediately repoens it, for no sane reason.
                        await Task.Delay(120);
                        IsPresented = false;
                    }
                }
            }
        }
    }
}

@Keflon
Copy link

Keflon commented Apr 17, 2023

@drasticactions I see you suggested a workaround ...

Detect when the flyout is closing and handle "IsPresenting" yourself, but this may cause the flyout to close twice.

Do you know how to detect the flyout closing? (Other than watching IsFocused)

@axa88
Copy link

axa88 commented Sep 5, 2023

@drasticactions I see you suggested a workaround ...

Do you know how to detect the flyout closing? (Other than watching IsFocused)

You could directly subscribe to each the Flyout and Detail FocusChangeRequested and Focused and Unfocused events.
A much more elegant and functioning example is posted below.

@axa88
Copy link

axa88 commented Sep 5, 2023

fwiw, seems to me a more simple description of this bug is

  • On WinUI, FlyoutPage.IsPresented property is always stuck true when the Flyout is closed and NOT presented.
  • This has many negative effects.

Seems to me such a fundamental and simple problem that should be addressed. Surprised to see it go straight to the back burner

@axa88
Copy link

axa88 commented Sep 9, 2023

I think this is a much more straightforward work around for the this issue.
Inserted into any class inheriting FlyoutPage

#if WINDOWS
	PropertyChanged += (_, propertyChangedEventHandler) =>
	{
		if (propertyChangedEventHandler.PropertyName == nameof(FlyoutLayoutBehavior) && FlyoutLayoutBehavior is not (FlyoutLayoutBehavior.Popover or FlyoutLayoutBehavior.SplitOnPortrait))
			IsPresented = true;
	};

	Flyout.Focused += (_, __) =>
	{
		if (FlyoutLayoutBehavior is FlyoutLayoutBehavior.Popover or FlyoutLayoutBehavior.SplitOnPortrait)
			IsPresented = true;
	};

	Flyout.Unfocused += (_, __) =>
	{
		if (FlyoutLayoutBehavior is FlyoutLayoutBehavior.Popover or FlyoutLayoutBehavior.SplitOnPortrait)
			IsPresented = false;
	};
#endif

The suggestion above ignores the SplitOnPortrait FlyoutLayoutBehavior which is essentially the same as Popover

@Zhanglirong-Winnie Zhanglirong-Winnie added s/verified Verified / Reproducible Issue ready for Engineering Triage s/triaged Issue has been reviewed labels Nov 24, 2023
@Zhanglirong-Winnie
Copy link

Verified this issue with Visual Studio Enterprise 17.9.0 Preview 1.0. Can repro on windows platform.

@SarthakB26
Copy link

Is this planning to be fixed any sooner ?

@SarthakB26
Copy link

SarthakB26 commented Feb 6, 2024

I think this is a much more straightforward work around for the this issue. Inserted into any class inheriting FlyoutPage

#if WINDOWS
	PropertyChanged += (_, propertyChangedEventHandler) =>
	{
		if (propertyChangedEventHandler.PropertyName == nameof(FlyoutLayoutBehavior) && FlyoutLayoutBehavior is not (FlyoutLayoutBehavior.Popover or FlyoutLayoutBehavior.SplitOnPortrait))
			IsPresented = true;
	};

	Flyout.Focused += (_, __) =>
	{
		if (FlyoutLayoutBehavior is FlyoutLayoutBehavior.Popover or FlyoutLayoutBehavior.SplitOnPortrait)
			IsPresented = true;
	};

	Flyout.Unfocused += (_, __) =>
	{
		if (FlyoutLayoutBehavior is FlyoutLayoutBehavior.Popover or FlyoutLayoutBehavior.SplitOnPortrait)
			IsPresented = false;
	};
#endif

The suggestion above ignores the SplitOnPortrait FlyoutLayoutBehavior which is essentially the same as Popover

When should we Unsubscribe to these event in deconstructor ? or when handler is changing to null ?.In Case user logged out and we are setting MainPage to login screen

@axa88
Copy link

axa88 commented Feb 6, 2024

@SarthakB26 frankly Ive found further issues with this solution.
The problem is that if you try to set IsPresented = false in a FlyoutLayoutBehavior where the framework forbids collapsing of the Flyout, it will throw an exception.
For example on WinUi (and likely iPad) you can have different FlyoutLayoutBehavior modes where the the layout is always split, so if you attempt to close the Flyout it will throw. And the problem with that is the framework can change the behavior of FlyoutLayoutBehavior depending on the size of the window and the program cant know it cuz the FlyoutLayoutBehavior value isnt changed along with it. It can even report Default which is undocumented so you wont know what it does anyway. This is only not a problem on Android as FlyoutLayoutBehavior is always the collapsible Popover

So now i do something like this, another workaround hack:

internal class FlyoutPageBase : FlyoutPage
{
	/// <summary>
	/// Require Initialization to deal with current FlyoutPage bugs
	/// </summary>
	/// <param name="flyout"></param>
	/// <param name="detail"></param>
	/// <param name="title"></param>
	internal void Initialize(Page flyout, Page detail, string title)
	{
		Flyout = flyout;
		Detail = detail;

		Flyout.Focused += (_, _) => IsPresented = true;
		Flyout.Unfocused += (_, _) => CollapseFlyoutIfSafe();

		Loaded += (_, _) =>
		{
			IsPresented = true;
			CollapseFlyoutIfSafe();
		};

		void CollapseFlyoutIfSafe()
		{
			if (DeviceInfo.Platform == DevicePlatform.WinUI
				&&
					(
						FlyoutLayoutBehavior is FlyoutLayoutBehavior.Popover
						||
						(FlyoutLayoutBehavior is FlyoutLayoutBehavior.SplitOnPortrait && Window.Width > 500)
						||
						(FlyoutLayoutBehavior is FlyoutLayoutBehavior.Default or FlyoutLayoutBehavior.SplitOnLandscape && Window.Width < 450)
					)
				)
				IsPresented = false;
		}
	}
}

The above is also brittle because depending if you resize a window from large to small, or small to large, there are different sizes to which the FlyoutLayoutBehavior will change itself... and you wont know it.
I wrote about all this in another issue
Maybe you can help upvote the issue or something...

Honestly Im not worried about unsubscribing as the page lasts the lifetime of the application.
If the Page gets unreferenced completely i expect the GC to deal with it.
Its more likely going to throw when the page is resized and its not handled by the above.

@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
@DennisWelu
Copy link

I used the native control event of the MainPage/FlyoutPage (NavigationView in WinUI) to trigger resetting of IsPresented (tested for Popover behavior):

	public MainPage()
	{
		InitializeComponent();

#if WINDOWS
		Loaded += OnLoaded;
		return;

		void OnLoaded(object? sender, EventArgs e)
		{
			Loaded -= OnLoaded;
			((NavigationView)Handler!.PlatformView!).PaneClosed += (_, _) => IsPresented = false;
		}
#endif
	}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-flyout Flyout platform/windows 🪟 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

9 participants