Fix text toolbar item icon not removed when navigating on Android

[Adam--]

Description of Change

Fixes an issue on Android when there are multiple pages with toolbar items and on one page the toolbar item has an icon and on another page it doesn't, the icon is not removed when navigating to the page without the icon.

When updating the Android MenuItem icon and the base drawable is null, the icon is set to null. On Android, this clears out the previous icon.

Issues Fixed

Fixes #7823

[ghost]

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

[jfichtner]

This is a much needed fix! Hopefully this gets reviewed and approved quickly!

[mattleibow]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[mattleibow]

This all looks great, but we probably should add some tests.

@PureWeen is there a good example - or even possible - to have a test? I know we have device tests, but we also now have the image comparison tests.

[Adam--]

@mattleibow, @PureWeen

Adding tests sounds like a great idea.

It seems that if we were to write a test similar (or just use the same test) to ToolbarItemsOrderUpdatesCorrectlyAfterNavigation in \src\Controls\tests\DeviceTests\Elements\Toolbar\ToolbarTests.cs but with toolbar items with icons on one of the pages and not the other then check if those icons are correct on the platform after navigation this would test the fix.

Such as:

[Fact(DisplayName = "Toolbar Items Order Updates Correctly After Navigation")]
public async Task ToolbarItemsOrderUpdatesCorrectlyAfterNavigation()
{
	SetupBuilder();
	var firstPageToolbarItem = new ToolbarItem() { Text = "Toolbar Item First Page" };
	var secondPagePrimaryToolbarItem = new ToolbarItem() { Text = "Toolbar Item Second Page", IconImageSource = "SecondPageIcon" };
	var secondPageSecondaryToolbarItem = new ToolbarItem() { Text = "Secondary Toolbar Item Second Page", Order = ToolbarItemOrder.Secondary };

	var navPage = new NavigationPage(new ContentPage()
	{
		ToolbarItems =
		{
			firstPageToolbarItem
		}
	});

	await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), async (handler) =>
	{
		await navPage.PushAsync(new ContentPage()
		{
			ToolbarItems =
			{
				secondPagePrimaryToolbarItem,
				secondPageSecondaryToolbarItem
			}
		});

		ToolbarItemsMatch(handler, GetExpectedToolbarItems(navPage));
		ToolbarItemsMatch(handler, secondPagePrimaryToolbarItem, secondPageSecondaryToolbarItem);
		await navPage.PopAsync();
		ToolbarItemsMatch(handler, GetExpectedToolbarItems(navPage));
		ToolbarItemsMatch(handler, firstPageToolbarItem);
	});
}

ToolbarItemsMatch would need to be updated to check the toolbar IconImageSource to the platform's element icon in the platforms' ControlsHandlerTestBase.cs. I don't know how to do this on Android (how can you get the image source from the Icon drawable?) and even less on the other platforms.

Would you be able to help out?

[Adam--]

Hey, I'd like to get this merged. Can you review the code?

[DP-Technology-LLC]

We agree with @jfichtner, this is a much needed fix!

[PureWeen]

ToolbarItemsOrderUpdatesCorrectlyAfterNavigation

Let me know if you want me to take this one over.

I think for the device tests on this one, you could grab the handler from the toolbar which will be of type MaterialToolbar

toolbar.Handler.PlatformView as MaterialToolbar

And then retrieve the menu items from there to see if the icon has been removed.

[jsuarezruiz]

Added UI Test

[mattleibow]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[Adam--]

Thanks all, I appreciate the help on this!

[jsuarezruiz]

/azp run

[PureWeen]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[PureWeen]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).