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

BagavathiPerumal · 2024-10-10T09:52:09Z

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 the parent of the MenuBarItem only set to null when the parent of MenuBarItem is MenuBar instead of page. This prevents the unintended loss of the BindingContext, which can disrupt bindings and commands when navigating back to a page.

Tested the behavior in the following platforms.

Windows
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 #21675

Output

Before Issue Fix	After Issue Fix
BeforeFix-MenuBarItem-Windows.mp4	AfterFix-MenuBarItem-Windows.mp4

…tion.

dotnet-policy-service · 2024-10-10T09:52:27Z

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.

…fix-21675

jsuarezruiz · 2024-10-11T11:29:52Z

/azp run

azure-pipelines · 2024-10-11T11:30:11Z

Azure Pipelines successfully started running 3 pipeline(s).

BagavathiPerumal · 2024-10-14T09:15:20Z

/rebase

jsuarezruiz · 2024-10-14T10:02:33Z

/azp run

azure-pipelines · 2024-10-14T10:02:53Z

Azure Pipelines successfully started running 3 pipeline(s).

BagavathiPerumal · 2024-10-15T12:09:56Z

/rebase

…tion.

PureWeen

It doesn't look like this needs a UITest

Can you move this to a unit test?

maui/src/Controls/tests/Core.UnitTests/MenuItemTests.cs

Line 10 in 90d0396

public class MenuItemTests

BagavathiPerumal · 2024-10-16T12:58:06Z

It doesn't look like this needs a UITest

Can you move this to a unit test?

maui/src/Controls/tests/Core.UnitTests/MenuItemTests.cs

Line 10 in 90d0396

public class MenuItemTests

@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);
}

…fix-21675

azure-pipelines · 2025-01-06T13:36:24Z

Azure Pipelines successfully started running 3 pipeline(s).

PureWeen

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

BagavathiPerumal · 2025-01-09T11:33:23Z

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 · 2025-01-14T20:58:29Z

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 · 2025-01-14T21:28:12Z

src/Controls/src/Core/Page/Page.cs

+			{
+				if (menuBarItem.BindingContext is null)
+				{
+					SetInheritedBindingContext(menuBarItem, BindingContext);


I think that this is where the actual bug is @BagavathiPerumal

The fix here might be to check if the Parent is MenuBar and if it's not then don't set it to null.

The Parent of MenuBarItem is the Page that it's on so the Page needs to control when the parent is set to null

BagavathiPerumal · 2025-01-22T14:47:39Z

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 · 2025-01-29T17:06:11Z

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 · 2025-01-29T17:18:08Z

/rebase

BagavathiPerumal · 2025-01-30T06:57:00Z

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.

PureWeen

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

#25170 (comment)

BagavathiPerumal · 2025-01-30T14:09:58Z

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

#25170 (comment)

Hi @PureWeen, Could you kindly check this response?

#25170 (comment)

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

PureWeen · 2025-02-03T22:57:30Z

/rebase

PureWeen · 2025-02-03T23:10:37Z

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

AFAICT this code is wrong

maui/src/Controls/src/Core/Menu/MenuBar.cs

Lines 116 to 117 in 2478901

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

and should be changed to this

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

What I'm thinking here is

remove the code you've added
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 · 2025-02-04T10:39:22Z

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

AFAICT this code is wrong

maui/src/Controls/src/Core/Menu/MenuBar.cs

Lines 116 to 117 in 2478901

if (item is Element e)

e.Parent = null;

and should be changed to this
			if (item is Element e && e.Parent == this)
				e.Parent = null;
What I'm thinking here is

remove the code you've added

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 · 2025-02-04T17:43:49Z

@BagavathiPerumal Apologies but I'm having trouble following the thread on this one
AFAICT this code is wrong

maui/src/Controls/src/Core/Menu/MenuBar.cs

Lines 116 to 117 in 2478901

if (item is Element e)

e.Parent = null;

and should be changed to this
			if (item is Element e && e.Parent == this)
				e.Parent = null;
What I'm thinking here is

remove the code you've added

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

maui/src/Controls/tests/Core.UnitTests/Menu/MenuTestBase.cs

Lines 94 to 100 in a5553ad

    
           if (typeof(TChildType) == typeof(MenuBarItem)) 
        
           { 
        
           	Assert.True(child1.Parent is ContentPage); 
        
           } 
        
           else 
        
           { 
        
           	Assert.Same(menuBar, child1.Parent);

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

…nuBar instead of page.

BagavathiPerumal · 2025-02-05T10:17:28Z

@BagavathiPerumal Apologies but I'm having trouble following the thread on this one
AFAICT this code is wrong

maui/src/Controls/src/Core/Menu/MenuBar.cs

Lines 116 to 117 in 2478901

if (item is Element e)

e.Parent = null;

and should be changed to this
			if (item is Element e && e.Parent == this)
				e.Parent = null;
What I'm thinking here is

remove the code you've added

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

maui/src/Controls/tests/Core.UnitTests/Menu/MenuTestBase.cs

Lines 94 to 100 in a5553ad

if (typeof(TChildType) == typeof(MenuBarItem))

{

Assert.True(child1.Parent is ContentPage);

}

else

{

Assert.Same(menuBar, child1.Parent);

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 · 2025-02-05T15:29:32Z

/azp run

azure-pipelines · 2025-02-05T15:29:53Z

Azure Pipelines successfully started running 3 pipeline(s).

HimanshuBansal112 · 2025-02-11T13:35:03Z

Does it fix #13848, repository for my issue is at https://github.com/HimanshuBansal112/NetMauiIssue for testing?

MartyIX · 2025-02-11T13:38:34Z

Does it fix #13848, repository for my issue is at https://github.com/HimanshuBansal112/NetMauiIssue for testing?

You can try a nightly build to figure out if the issue is fixed or not.

BagavathiPerumal added 3 commits October 10, 2024 14:58

fix-21675-Updated the BindingContext of MenuBarItem after page naviga…

21da0c2

…tion.

fix-21675- Added UI testcase.

aab1a4b

fix-21675-Removed unwanted space.

7d2da5b

dotnet-policy-service bot added the community ✨ Community Contribution label Oct 10, 2024

BagavathiPerumal added 3 commits October 10, 2024 15:43

Merge branch 'main' of https://github.com/BagavathiPerumal/maui into …

e762e8a

…fix-21675

fix-21675-removed unwanted spacing.

7904dd6

fix-21675-Updated testcase.

5a8bc16

BagavathiPerumal marked this pull request as ready for review October 11, 2024 09:30

BagavathiPerumal requested a review from a team as a code owner October 11, 2024 09:30

BagavathiPerumal requested review from jfversluis and mattleibow October 11, 2024 09:30

github-actions bot force-pushed the fix-21675 branch from 8d7eea9 to cba919e Compare October 14, 2024 09:15

BagavathiPerumal added 6 commits October 15, 2024 12:10

fix-21675-Updated the BindingContext of MenuBarItem after page naviga…

0818f26

…tion.

fix-21675- Added UI testcase.

d8f4b2d

fix-21675-Removed unwanted space.

ecc6ea6

fix-21675-removed unwanted spacing.

13bb22f

fix-21675-Updated testcase.

6e0ffd1

fix-21675-Updated testcase script.

4c04284

github-actions bot force-pushed the fix-21675 branch from cba919e to 4c04284 Compare October 15, 2024 12:10

PureWeen requested changes Oct 15, 2024

View reviewed changes

BagavathiPerumal added 2 commits October 17, 2024 11:58

fix-21675-Updated testcase which only execute for the Windows and MacOS

9be1bdc

Merge branch 'main' of https://github.com/BagavathiPerumal/maui into …

11661bb

…fix-21675

PureWeen added this to the .NET 9 SR4 milestone Jan 5, 2025

Merge branch 'main' of https://github.com/BagavathiPerumal/maui into …

801b6ce

…fix-21675

PureWeen requested changes Jan 8, 2025

View reviewed changes

PureWeen requested changes Jan 14, 2025

View reviewed changes

fix-21675-Added UnitTestcase and removed UI testcase.

76bfa3d

PureWeen requested changes Jan 30, 2025

View reviewed changes

jfversluis requested a review from PureWeen January 31, 2025 14:40

PureWeen modified the milestones: .NET 9 SR4, .NET 9 SR5 Feb 4, 2025

fix-21675- Parent of the MenuBarItems only null when the parent is Me…

af8c599

…nuBar instead of page.

PureWeen approved these changes Feb 7, 2025

View reviewed changes

PureWeen merged commit 8000048 into dotnet:main Feb 9, 2025
117 of 124 checks passed

HimanshuBansal112 mentioned this pull request Feb 11, 2025

Page changing using menu bar stops command in menu bar from working #13848

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

BagavathiPerumal commented Oct 10, 2024 •

edited

Loading

dotnet-policy-service bot commented Oct 10, 2024

jsuarezruiz commented Oct 11, 2024

azure-pipelines bot commented Oct 11, 2024

BagavathiPerumal commented Oct 14, 2024

jsuarezruiz commented Oct 14, 2024

azure-pipelines bot commented Oct 14, 2024

BagavathiPerumal commented Oct 15, 2024

PureWeen left a comment

BagavathiPerumal commented Oct 16, 2024 •

edited

Loading

azure-pipelines bot commented Jan 6, 2025

PureWeen left a comment

BagavathiPerumal commented Jan 9, 2025

PureWeen commented Jan 14, 2025

PureWeen Jan 14, 2025

BagavathiPerumal commented Jan 22, 2025

PureWeen commented Jan 29, 2025

PureWeen commented Jan 29, 2025

BagavathiPerumal commented Jan 30, 2025

PureWeen left a comment

BagavathiPerumal commented Jan 30, 2025

PureWeen commented Feb 3, 2025

PureWeen commented Feb 3, 2025

BagavathiPerumal commented Feb 4, 2025

PureWeen commented Feb 4, 2025

BagavathiPerumal commented Feb 5, 2025

jsuarezruiz commented Feb 5, 2025

azure-pipelines bot commented Feb 5, 2025

HimanshuBansal112 commented Feb 11, 2025

MartyIX commented Feb 11, 2025

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

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

Conversation

BagavathiPerumal commented Oct 10, 2024 • edited Loading

Root cause

Description of Issue Fix

Issues Fixed

Output

dotnet-policy-service bot commented Oct 10, 2024

jsuarezruiz commented Oct 11, 2024

azure-pipelines bot commented Oct 11, 2024

BagavathiPerumal commented Oct 14, 2024

jsuarezruiz commented Oct 14, 2024

azure-pipelines bot commented Oct 14, 2024

BagavathiPerumal commented Oct 15, 2024

PureWeen left a comment

Choose a reason for hiding this comment

BagavathiPerumal commented Oct 16, 2024 • edited Loading

azure-pipelines bot commented Jan 6, 2025

PureWeen left a comment

Choose a reason for hiding this comment

BagavathiPerumal commented Jan 9, 2025

PureWeen commented Jan 14, 2025

PureWeen Jan 14, 2025

Choose a reason for hiding this comment

BagavathiPerumal commented Jan 22, 2025

PureWeen commented Jan 29, 2025

PureWeen commented Jan 29, 2025

BagavathiPerumal commented Jan 30, 2025

PureWeen left a comment

Choose a reason for hiding this comment

BagavathiPerumal commented Jan 30, 2025

PureWeen commented Feb 3, 2025

PureWeen commented Feb 3, 2025

BagavathiPerumal commented Feb 4, 2025

PureWeen commented Feb 4, 2025

BagavathiPerumal commented Feb 5, 2025

jsuarezruiz commented Feb 5, 2025

azure-pipelines bot commented Feb 5, 2025

HimanshuBansal112 commented Feb 11, 2025

MartyIX commented Feb 11, 2025

BagavathiPerumal commented Oct 10, 2024 •

edited

Loading

BagavathiPerumal commented Oct 16, 2024 •

edited

Loading