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

[Windows] Fix crash setting MenuFlyoutSubItem IconImageSource #26021

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 6 additions & 11 deletions src/Controls/src/Core/Menu/MenuFlyoutSubItem.cs
Original file line number Diff line number Diff line change
@@ -1,29 +1,24 @@
#nullable disable
using System;
using System.Collections;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.ComponentModel;
using System.Windows.Input;
using Microsoft.Maui.Controls.StyleSheets;
using Microsoft.Maui.Graphics;
using System.Linq;

namespace Microsoft.Maui.Controls
{
public partial class MenuFlyoutSubItem : MenuFlyoutItem, IMenuFlyoutSubItem
{
readonly List<IMenuElement> _menus = new List<IMenuElement>();
readonly List<IElement> _menus = new List<IElement>();

public MenuFlyoutSubItem()
{
LogicalChildrenInternalBackingStore = new CastingList<Element, IMenuElement>(_menus);
jsuarezruiz marked this conversation as resolved.
Show resolved Hide resolved
LogicalChildrenInternalBackingStore = new CastingList<Element, IElement>(_menus);
}

private protected override IList<Element> LogicalChildrenInternalBackingStore { get; }

public IMenuElement this[int index]
{
get { return _menus[index]; }
get { return _menus[index] as IMenuElement; }
set
{
RemoveAt(index);
Expand Down Expand Up @@ -60,7 +55,7 @@ public void CopyTo(IMenuElement[] array, int arrayIndex)

public IEnumerator<IMenuElement> GetEnumerator()
{
return _menus.GetEnumerator();
return _menus.Cast<IMenuElement>().GetEnumerator();
}

public int IndexOf(IMenuElement item)
Expand All @@ -87,7 +82,7 @@ public void RemoveAt(int index)
{
var item = _menus[index];
RemoveLogicalChild((Element)item, index);
NotifyHandler(nameof(IMenuFlyoutHandler.Remove), index, item);
NotifyHandler(nameof(IMenuFlyoutHandler.Remove), index, item as IMenuElement);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this will work...

I feel like we've hit a point on this one where we actually need to maintain two different internal lists.

_menus no longer represents _menus it's a mix of types.

If someone calls RemoveAt(index) they are going to expect that it removes the menu specifically at the given index (we should also make sure this removes the ImageSource added)

So, I think we have to keep _menus as a list of IMenuElement to satisfy all the IList<IMenuElement> implementations.

And then the backing store for logical children basically has to be a mix of both.

We basically do this with Layout.cs, it has an internal list of _children it uses to satisfy the direct children but then it adds each of this to the logical children on the parent

}

IEnumerator IEnumerable.GetEnumerator()
Expand Down
23 changes: 23 additions & 0 deletions src/Controls/tests/TestCases.HostApp/Issues/Issue25893.xaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?xml version="1.0" encoding="UTF-8"?>
<controls:TestContentPage xmlns:controls="clr-namespace:Maui.Controls.Sample.Issues"
xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
x:Class="Maui.Controls.Sample.Issues.Issue25893">
<Grid RowDefinitions="Auto,*,Auto">
<Label AutomationId="WaitForStubControl" x:Name="InfoLabel" />
<ContentView x:Name="ViewWithMenu" AutomationId="ViewWithMenu" Grid.Row="1" BackgroundColor="LightGray">
<FlyoutBase.ContextFlyout>
<MenuFlyout x:Name="ContextMenu">
<MenuFlyoutItem IconImageSource="dotnet_bot.png" Text="Item1" />
<MenuFlyoutSubItem IconImageSource="dotnet_bot.png" Text="SubItem1" />
<MenuFlyoutSubItem IconImageSource="dotnet_bot.png" Text="SubItem2" />
<MenuFlyoutSubItem IconImageSource="dotnet_bot.png" Text="SubItem3" />
</MenuFlyout>
</FlyoutBase.ContextFlyout>
</ContentView>
<HorizontalStackLayout Grid.Row="2">
<Button AutomationId="AddMenuItem" Text="Add MenuItem" Clicked="AddButtonClicked" />
<Button AutomationId="RemoveMenuItem" Text="Remove MenuItem" Clicked="RemoveButtonClicked" />
</HorizontalStackLayout>
</Grid>
</controls:TestContentPage>
51 changes: 51 additions & 0 deletions src/Controls/tests/TestCases.HostApp/Issues/Issue25893.xaml.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
namespace Maui.Controls.Sample.Issues
{

[Issue(IssueTracker.Github, 25893, "Setting MenuFlyoutSubItem IconImageSource throws a NullReferenceException", PlatformAffected.UWP)]
public partial class Issue25893 : TestContentPage
{
readonly MenuFlyout _contextMenu;

public Issue25893()
{
InitializeComponent();

_contextMenu = (MenuFlyout)FlyoutBase.GetContextFlyout(ViewWithMenu);

if (_contextMenu is null)
return;

InfoLabel.Text = $"{_contextMenu.Count}";
}

protected override void Init()
{

}

void AddButtonClicked(object sender, EventArgs e)
{
var itemsCount = _contextMenu.Count;
var itemNumber = itemsCount > 0 ? itemsCount : 1;
_contextMenu.Add(new MenuFlyoutSubItem { IconImageSource = "dotnet_bot.png", Text = $"Added Item {itemNumber}" });

UpdateInfoLabel();
}

void RemoveButtonClicked(object sender, EventArgs e)
{
var itemsCount = _contextMenu.Count;

if (itemsCount > 0)
_contextMenu.RemoveAt(itemsCount - 1);

UpdateInfoLabel();
}

void UpdateInfoLabel()
{
var itemsCount = _contextMenu.Count;
InfoLabel.Text = $"{itemsCount}";
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
#if MACCATALYST || WINDOWS
using NUnit.Framework;
using NUnit.Framework.Legacy;
using UITest.Appium;
using UITest.Core;

namespace Microsoft.Maui.TestCases.Tests.Issues;

public class Issue25893 : _IssuesUITest
{
public override string Issue => "Setting MenuFlyoutSubItem IconImageSource throws a NullReferenceException";

public Issue25893(TestDevice testDevice) : base(testDevice)
{
}

[Test]
[Category(UITestCategories.ContextActions)]
public void MenuFlyoutSubItemWithIconNoCrash()
{
App.WaitForElement("WaitForStubControl");

var result1 = App.FindElement("WaitForStubControl").GetText();
ClassicAssert.AreEqual("4", result1);

App.Tap("AddMenuItem");

var result2 = App.FindElement("WaitForStubControl").GetText();
ClassicAssert.AreEqual("5", result2);

App.Tap("RemoveMenuItem");
App.Tap("RemoveMenuItem");

var result3 = App.FindElement("WaitForStubControl").GetText();
ClassicAssert.AreEqual("3", result3);

// Without crashing, the test has passed.
}
}
#endif
16 changes: 16 additions & 0 deletions src/Controls/tests/Xaml.UnitTests/Issues/Issue25893.xaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?xml version="1.0" encoding="UTF-8"?>
<ContentPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
x:Class="Microsoft.Maui.Controls.Xaml.UnitTests.Issue25893">
<Grid RowDefinitions="Auto,*,Auto">
<Label AutomationId="WaitForStubControl" Text="Issue25893"/>
<ContentView x:Name="ViewWithMenu" Grid.Row="1" Grid.RowSpan="2" BackgroundColor="LightGray">
<FlyoutBase.ContextFlyout>
<MenuFlyout x:Name="ContextMenu">
<MenuFlyoutItem IconImageSource="dotnet_bot.png" Text="Item1" />
<MenuFlyoutSubItem IconImageSource="dotnet_bot.png" Text="SubItem1" />
</MenuFlyout>
</FlyoutBase.ContextFlyout>
</ContentView>
</Grid>
</ContentPage>
37 changes: 37 additions & 0 deletions src/Controls/tests/Xaml.UnitTests/Issues/Issue25893.xaml.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using Microsoft.Maui.Controls;
using Microsoft.Maui.Controls.Core.UnitTests;
using NUnit.Framework;

namespace Microsoft.Maui.Controls.Xaml.UnitTests
{
public partial class Issue25893 : ContentPage
{
public Issue25893()
{
InitializeComponent();
}

public Issue25893(bool useCompiledXaml)
{
//this stub will be replaced at compile time
}

[TestFixture]
public class Tests
{
[TestCase(false)]
[TestCase(true)]
public void MenuFlyoutSubItemWithIcon(bool useCompiledXaml)
{
var page = new Issue25893(useCompiledXaml);
var view = page.FindByName<ContentView>("ViewWithMenu");
var contextFlyout = FlyoutBase.GetContextFlyout(view);

Assert.AreEqual(contextFlyout.LogicalChildrenInternal.Count, 2);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Microsoft.Maui.Platform;
using Microsoft.UI.Xaml.Controls;
Expand Down Expand Up @@ -62,16 +63,19 @@ public static void MapSource(IMenuFlyoutSubItemHandler handler, IMenuFlyoutSubIt
view.Source?.ToIconSource(handler.MauiContext!)?.CreateIconElement();
}

public override void SetVirtualView(IElement view)
{
base.SetVirtualView(view);
PlatformView.Items.Clear();

foreach (var item in ((IMenuFlyoutSubItem)view))
{
PlatformView.Items.Add((MenuFlyoutItemBase)item.ToPlatform(MauiContext!));
}
}
public override void SetVirtualView(IElement view)
{
base.SetVirtualView(view);
PlatformView.Items.Clear();

if (view is IMenuFlyoutSubItem menuFlyoutSubItem)
{
foreach (var item in menuFlyoutSubItem.OfType<IMenuElement>())
{
PlatformView.Items.Add((MenuFlyoutItemBase)item.ToPlatform(MauiContext!));
}
}
}

// workaround WinUI bug https://github.com/microsoft/microsoft-ui-xaml/issues/7797
void Reset()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
using UIKit;
using System.Collections.Generic;
using System.Linq;
using UIKit;

namespace Microsoft.Maui.Handlers
{
Expand All @@ -15,8 +17,10 @@ protected override UIMenu CreatePlatformElement()
uIMenuBuilder = builder;
}

var menuItems = ((IMenuFlyoutSubItem)VirtualView).OfType<IMenuElement>().ToList();

var menu =
VirtualView.ToPlatformMenu(
menuItems.ToPlatformMenu(
VirtualView.Text,
VirtualView.Source,
MauiContext!,
Expand Down
Loading