Fix for MenuFlyoutItem stops working after navigating away from and back to page

[BagavathiPerumal]

Root cause

The issue occurs because the BindingContext is set to null after navigating to another page. As a result, when returning to the page, the MenuFlyoutItem loses its binding, causing the Command to stop functioning properly.

Description of Issue Fix

The fix involves applying the BindingContext for MenuBarItem within the OnAppearing() method of the page. This ensures that the MenuBarItem's BindingContext is correctly set when the page is displayed again after navigation.

Tested the behavior in the following platforms.

• [x] Windows
• [x] Mac
• [ ] iOS
• [ ] Android

The MenuBar control is not supported on iOS and Android, so this fix was not tested on these platforms.

Issues Fixed

Fixes https://github.com/dotnet/maui/issues/21675

Output

Before Issue Fix	After Issue Fix

[dotnet-policy-service[bot]]

Hey there @BagavathiPerumal! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[jsuarezruiz]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 3 pipeline(s).

[BagavathiPerumal]

/rebase

[jsuarezruiz]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 3 pipeline(s).

[BagavathiPerumal]

/rebase

[BagavathiPerumal]

It doesn't look like this needs a UITest

Can you move this to a unit test?

https://github.com/dotnet/maui/blob/90d039638a8b58a1d5df3c5aa0babac49b1fd002/src/Controls/tests/Core.UnitTests/MenuItemTests.cs#L10

@PureWeen, we couldn't recreate the same UI test scenario in the unit test case because the command of the MenuFlyoutItem is always present after page navigation.

[Fact]
public async void UpdateMenuBarItemBindingContext()
{
	bool fired = false;
	var cmd = new Command(() => fired = true);
	var bindingContext = new
	{
		MenuItemCommand = cmd
	};
 
	var menuFlyoutItem = new MenuFlyoutItem();
	var menuItem = new MenuBarItem { menuFlyoutItem };
	var menubarItems = new List<MenuBarItem> { menuItem };
	var mainPage = new ContentPage
	{
		MenuBarItems = menubarItems,
	};
	mainPage.BindingContext = bindingContext;
	menuFlyoutItem.SetBinding(MenuFlyoutItem.CommandProperty, new Binding("MenuItemCommand"));
 
	var page = new ContentPage
	{
		BindingContext = bindingContext
	};
 
	NavigationPage nav = new NavigationPage(mainPage);
	await mainPage.Navigation.PushAsync(page);
	await page.Navigation.PopAsync();
 
	menuFlyoutItem.Command.Execute(null);
 
	Assert.True(fired);
}  

[jsuarezruiz]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 3 pipeline(s).

[jsuarezruiz]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 3 pipeline(s).

[rmarinho]

/rebase

[BagavathiPerumal]

/rebase

[BagavathiPerumal]

/rebase

[azure-pipelines[bot]]

Azure Pipelines successfully started running 3 pipeline(s).

[BagavathiPerumal]

Can we validate this one using a unit test vs a UI Test?

@PureWeen, As discussed here in this comment, we were facing difficulties in converting the test from UI to Unit testcase.

[PureWeen]

Can we validate this one using a unit test vs a UI Test?

@PureWeen, As discussed here in this comment, we were facing difficulties in converting the test from UI to Unit testcase.

If you wire up through a TestWindow does it work?

For example, if you check any of tests under NavigationModelTests

[BagavathiPerumal]

Can we validate this one using a unit test vs a UI Test?

@PureWeen, As discussed here in this comment, we were facing difficulties in converting the test from UI to Unit testcase.

If you wire up through a TestWindow does it work?

For example, if you check any of tests under NavigationModelTests

@PureWeen, As suggested, when wiring up through a TestWindow in the UnitTest case, we couldn't recreate the UI test scenario because the MenuFlyoutItem command remains active after page navigation. Could you please share your suggestions.

[Fact]
public async void UpdateMenuBarItemBindingContext()
{
	bool fired = false;
	var cmd = new Command(() => fired = true);
	var bindingContext = new
	{
		MenuItemCommand = cmd
	};
 
	MenuFlyoutItem flyout;
	MenuBarItem menuItem;
	var mainPage = new ContentPage
	{
		MenuBarItems =
		{
			(menuItem = new MenuBarItem
			{
				(flyout = new MenuFlyoutItem { })
			})
		}
	};
	mainPage.BindingContext = bindingContext;
	flyout.SetBinding(MenuFlyoutItem.CommandProperty, new Binding("MenuItemCommand"));
 
	var page = new ContentPage
	{
		BindingContext = bindingContext
	};
 
	TestWindow testWindow = new TestWindow(mainPage);
	NavigationPage nav = new NavigationPage(mainPage);
	await mainPage.Navigation.PushAsync(page);
	await page.Navigation.PopAsync();
 
	flyout.Command.Execute(null);
 
	Assert.True(fired);
}

[PureWeen]

Can we validate this one using a unit test vs a UI Test?

@PureWeen, As discussed here in this comment, we were facing difficulties in converting the test from UI to Unit testcase.

If you wire up through a TestWindow does it work? For example, if you check any of tests under NavigationModelTests

@PureWeen, As suggested, when wiring up through a TestWindow in the UnitTest case, we couldn't recreate the UI test scenario because the MenuFlyoutItem command remains active after page navigation. Could you please share your suggestions.

[Fact]
public async void UpdateMenuBarItemBindingContext()
{
	bool fired = false;
	var cmd = new Command(() => fired = true);
	var bindingContext = new
	{
		MenuItemCommand = cmd
	};
 
	MenuFlyoutItem flyout;
	MenuBarItem menuItem;
	var mainPage = new ContentPage
	{
		MenuBarItems =
		{
			(menuItem = new MenuBarItem
			{
				(flyout = new MenuFlyoutItem { })
			})
		}
	};
	mainPage.BindingContext = bindingContext;
	flyout.SetBinding(MenuFlyoutItem.CommandProperty, new Binding("MenuItemCommand"));
 
	var page = new ContentPage
	{
		BindingContext = bindingContext
	};
 
	TestWindow testWindow = new TestWindow(mainPage);
	NavigationPage nav = new NavigationPage(mainPage);
	await mainPage.Navigation.PushAsync(page);
	await page.Navigation.PopAsync();
 
	flyout.Command.Execute(null);
 
	Assert.True(fired);
}

@BagavathiPerumal in your test you aren't assigning the right thing to the window

You need to change those lines to this

NavigationPage nav = new NavigationPage(mainPage);
			TestWindow testWindow = new TestWindow(nav);
			await mainPage.Navigation.PushAsync(page);
			await page.Navigation.PopAsync();
		

[PureWeen]

/rebase

[BagavathiPerumal]

Can we validate this one using a unit test vs a UI Test?

@PureWeen, As discussed here in this comment, we were facing difficulties in converting the test from UI to Unit testcase.

If you wire up through a TestWindow does it work? For example, if you check any of tests under NavigationModelTests

@PureWeen, As suggested, when wiring up through a TestWindow in the UnitTest case, we couldn't recreate the UI test scenario because the MenuFlyoutItem command remains active after page navigation. Could you please share your suggestions.

[Fact]
public async void UpdateMenuBarItemBindingContext()
{
	bool fired = false;
	var cmd = new Command(() => fired = true);
	var bindingContext = new
	{
		MenuItemCommand = cmd
	};
 
	MenuFlyoutItem flyout;
	MenuBarItem menuItem;
	var mainPage = new ContentPage
	{
		MenuBarItems =
		{
			(menuItem = new MenuBarItem
			{
				(flyout = new MenuFlyoutItem { })
			})
		}
	};
	mainPage.BindingContext = bindingContext;
	flyout.SetBinding(MenuFlyoutItem.CommandProperty, new Binding("MenuItemCommand"));
 
	var page = new ContentPage
	{
		BindingContext = bindingContext
	};
 
	TestWindow testWindow = new TestWindow(mainPage);
	NavigationPage nav = new NavigationPage(mainPage);
	await mainPage.Navigation.PushAsync(page);
	await page.Navigation.PopAsync();
 
	flyout.Command.Execute(null);
 
	Assert.True(fired);
}

@BagavathiPerumal in your test you aren't assigning the right thing to the window

You need to change those lines to this

NavigationPage nav = new NavigationPage(mainPage);
			TestWindow testWindow = new TestWindow(nav);
			await mainPage.Navigation.PushAsync(page);
			await page.Navigation.PopAsync();
		

@PureWeen, Thank you for the suggestion. I have added the unit test case accordingly. Please review and let me know if you have any further concerns.

[BagavathiPerumal]

My comment here still applies to the PR in its current state

#25170 (comment)

Hi @PureWeen, Could you kindly check this response?

https://github.com/dotnet/maui/pull/25170#discussion_r1919870488

We have attempted this approach and encountered a related test failure.

[PureWeen]

/rebase

[PureWeen]

@BagavathiPerumal Apologies but I'm having trouble following the thread on this one

AFAICT this code is wrong https://github.com/dotnet/maui/blob/2478901a5b74b4298f7397e8554d897bdd04effe/src/Controls/src/Core/Menu/MenuBar.cs#L116-L117

and should be changed to this

			if (item is Element e && e.Parent == this)
				e.Parent = null;

What I'm thinking here is

1. remove the code you've added
2. make the change I link above

And then that should be the proper fix here.

If I'm still missing something here can you summarize it all in a comment so it's a bit easier to follow where I'm not understanding?

[BagavathiPerumal]

@BagavathiPerumal Apologies but I'm having trouble following the thread on this one

AFAICT this code is wrong

https://github.com/dotnet/maui/blob/2478901a5b74b4298f7397e8554d897bdd04effe/src/Controls/src/Core/Menu/MenuBar.cs#L116-L117

and should be changed to this

			if (item is Element e && e.Parent == this)
				e.Parent = null;

What I'm thinking here is

1. remove the code you've added
2. make the change I link above

And then that should be the proper fix here.

If I'm still missing something here can you summarize it all in a comment so it's a bit easier to follow where I'm not understanding?

@PureWeen, As suggested, setting the parent of a MenuBarItem to null when its parent is MenuBar resolves the actual issue. However, it causes the following test case to fail.

ClearUpdatesParent()

In this test case, the ContentPage's MenuBarItems are cleared. At that time, the parent of the MenuBarItem is ContentPage. Therefore, when MenuBarItems are cleared, the parent of the MenuBarItem must also be cleared and set to null. This seems to be a valid case. However, due to your suggested fix, these test cases have failed.

The actual issue is that the MenuBarItem's BindingContext is reset to null after navigating to another page due to the previous page's MenuBarItem parent being set to null. To resolve this, I added a fix where the BindingContext for the MenuBarItem is reapplied within the OnAppearing() method of the page. This ensures that the MenuBarItem's BindingContext is correctly set when the page is displayed again after navigation, without affecting any previous logic.

[PureWeen]

@BagavathiPerumal Apologies but I'm having trouble following the thread on this one AFAICT this code is wrong https://github.com/dotnet/maui/blob/2478901a5b74b4298f7397e8554d897bdd04effe/src/Controls/src/Core/Menu/MenuBar.cs#L116-L117

and should be changed to this

			if (item is Element e && e.Parent == this)
				e.Parent = null;

What I'm thinking here is

1. remove the code you've added
2. make the change I link above

And then that should be the proper fix here. If I'm still missing something here can you summarize it all in a comment so it's a bit easier to follow where I'm not understanding?

@PureWeen, As suggested, setting the parent of a MenuBarItem to null when its parent is MenuBar resolves the actual issue. However, it causes the following test case to fail.

ClearUpdatesParent()

In this test case, the ContentPage's MenuBarItems are cleared. At that time, the parent of the MenuBarItem is ContentPage. Therefore, when MenuBarItems are cleared, the parent of the MenuBarItem must also be cleared and set to null. This seems to be a valid case. However, due to your suggested fix, these test cases have failed.

The actual issue is that the MenuBarItem's BindingContext is reset to null after navigating to another page due to the previous page's MenuBarItem parent being set to null. To resolve this, I added a fix where the BindingContext for the MenuBarItem is reapplied within the OnAppearing() method of the page. This ensures that the MenuBarItem's BindingContext is correctly set when the page is displayed again after navigation, without affecting any previous logic.

Yea, but the problem is that the MenuBarItems parent should never be set to null. It belongs to the the Page so it isn't the responsibility of the MenuBar to clear out the parent.

The tests inside ClearUpdatesParent() attempt to be generalized across all the different types of menu since they apply to a lot of the same scenarios but there are cases where the logic differs.

For example, if you check this test https://github.com/dotnet/maui/blob/a5553ad0f101b93d00ce67a6c2a25ccecb47dad4/src/Controls/tests/Core.UnitTests/Menu/MenuTestBase.cs#L94-L100

you'll see the logic deviates based on the type of item, so I'm pretty sure this is the same case. The ClearUpdatesParent needs to be fixed so that it's actually checking that the parent isn't removed when clearing the MenuBar

[BagavathiPerumal]

@BagavathiPerumal Apologies but I'm having trouble following the thread on this one AFAICT this code is wrong https://github.com/dotnet/maui/blob/2478901a5b74b4298f7397e8554d897bdd04effe/src/Controls/src/Core/Menu/MenuBar.cs#L116-L117

and should be changed to this

			if (item is Element e && e.Parent == this)
				e.Parent = null;

What I'm thinking here is

1. remove the code you've added
2. make the change I link above

And then that should be the proper fix here. If I'm still missing something here can you summarize it all in a comment so it's a bit easier to follow where I'm not understanding?

@PureWeen, As suggested, setting the parent of a MenuBarItem to null when its parent is MenuBar resolves the actual issue. However, it causes the following test case to fail. ClearUpdatesParent() In this test case, the ContentPage's MenuBarItems are cleared. At that time, the parent of the MenuBarItem is ContentPage. Therefore, when MenuBarItems are cleared, the parent of the MenuBarItem must also be cleared and set to null. This seems to be a valid case. However, due to your suggested fix, these test cases have failed. The actual issue is that the MenuBarItem's BindingContext is reset to null after navigating to another page due to the previous page's MenuBarItem parent being set to null. To resolve this, I added a fix where the BindingContext for the MenuBarItem is reapplied within the OnAppearing() method of the page. This ensures that the MenuBarItem's BindingContext is correctly set when the page is displayed again after navigation, without affecting any previous logic.

Yea, but the problem is that the MenuBarItems parent should never be set to null. It belongs to the the Page so it isn't the responsibility of the MenuBar to clear out the parent.

The tests inside ClearUpdatesParent() attempt to be generalized across all the different types of menu since they apply to a lot of the same scenarios but there are cases where the logic differs.

For example, if you check this test

https://github.com/dotnet/maui/blob/a5553ad0f101b93d00ce67a6c2a25ccecb47dad4/src/Controls/tests/Core.UnitTests/Menu/MenuTestBase.cs#L94-L100

you'll see the logic deviates based on the type of item, so I'm pretty sure this is the same case. The ClearUpdatesParent needs to be fixed so that it's actually checking that the parent isn't removed when clearing the MenuBar

@PureWeen, As suggested, I have modified the fix and the test case(ClearUpdatesParent()). Could you please share your concerns about this.

[jsuarezruiz]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 3 pipeline(s).