Binding an Empty Collection to MenuItemSource of NavigationViewItem still shows a Chevron

[MarkIvanDev]

Describe the bug

Binding an Empty Collection to the MenuItemSource property of NavigationViewItem still shows a Chevron.

Steps to reproduce the bug

Steps to reproduce the behavior:

1. Use the model

public class Item
{
    public string Content { get; set; }
    public Collection<Item> Children { get; set; }
}

2. Using the object
   var item = new Item { Content = "Sample", Children = new Collection<Item>() };

And bind as

<winui:NavigationViewItem
    Content="{x:Bind Content}"
    MenuItemsSource="{x:Bind Children, Mode=OneWay}">
</winui:NavigationViewItem>

Expected behavior
Currently, the check is only being made if the MenuItemsSource is null.

microsoft-ui-xaml/dev/NavigationView/NavigationViewItem.cpp

Line 483 in f41ff56

return MenuItems().Size() > 0 || MenuItemsSource() != nullptr || HasUnrealizedChildren();

It should treat an empty collection bound to MenuItemsSource as no Children, thus not showing a Chevron.

Screenshots

Version Info

NuGet package version:

[Microsoft.UI.Xaml 2.4.2]

Windows 10 version	Saw the problem?
Insider Build (xxxxx)
May 2020 Update (19041)	Yes
November 2019 Update (18363)
May 2019 Update (18362)
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)

Device form factor	Saw the problem?
Desktop	Yes
Xbox
Surface Hub
IoT

Additional context

I don't know if this is by design. And if it is, what could be made to have a workaround this. Thank you

[Felix-Dev]

This sounds like a bug to me. There is no point showing the chevron when there are no children and it actually can create confusion instead. If the team confirms this is a bug, would you like to provide a fix seeing as you already did some investigations?

[MarkIvanDev]

@Felix-Dev TBH, I do not have experience coding in C++. So i don't have the confidence to fix this if this happens to be a bug. 😅

[marcelwgn]

I think replacing the line you linked @MarkIvanDev with the following:

    return MenuItems().Size() > 0 
        || (MenuItemsSource() != nullptr && m_repeater.get().ItemsSourceView().Count() > 0)
        || HasUnrealizedChildren();

would fix it since the snippet also check if there are items, in the collection, instead of just checking that it is not null.

I think this is everything you would need to do in C++ land here. Would you then like to create a PR if this is indeed a bug (which I certainly think it is)? If not, that's fine too :)

[Felix-Dev]

That line above is not enough to fix the issue. We also need to update the chevron visual state when the number of child items changes from 0 to > 0 and vice-versa.

If you don't feel confident enough yet to take on this PR we'll be happy to step in here 😁 That said, it is definitely a fun experience to work on your first PR for WinUI and figure out stuff along the way, especially when you are fixing an issue you yourself reported. The team and the community will gladly help you out if you decide to go on this journey and need some help :)

[marcelwgn]

That line above is not enough to fix the issue. We also need to update the chevron visual state when the number of child items changes from 0 to > 0 and vice-versa.

Oh I thought this method was being called when the number of items changes, but it seems that we don't listen to that at all. Yes in that case we need more logic for this.

[MarkIvanDev]

@Felix-Dev Thank you for the encouragement. I think this would be a great learning experience for me. But as @chingucoding said, this may need a lot more code to fix.

I checked the code again and UpdateVisualStateForChevron is only called when MenuItems, MenuItemsSource, and HasUnrealizedChildren are changed (it is also called in the various Pointer event handlers). Will it be enough to check if MenuItemsSource implements INotifyCollectionChanged (i don't know if this interface exists in this context) when it is set and attach a CollectionChanged event and remove the previous event handler each time and call UpdateVisualStateForChevron there?

[marcelwgn]

Yes you would need to hook into the collection changed event. You can do this by listening to the ItemsRepeaters ItemsSourceView CollectionChanged. An example where we do this is here:

microsoft-ui-xaml/dev/Repeater/ItemsRepeater.cpp

Line 525 in c5fa8a6

m_itemsSourceViewChanged = newValue.CollectionChanged(winrt::auto_revoke, { this, &ItemsRepeater::OnItemsSourceViewChanged });

That way, you only have to register/unregister and don't have to think about whether the collection is actually INotifyCollectionChanged or not. If it is not, nothing happens, if it is, you will get notified if the collection changes.

[Felix-Dev]

@MarkIvanDev

But as @chingucoding said, this may need a lot more code to fix.

The code needed to add here should just be a few lines. You might need to write a test as well to cover this case but those can be written in C# :)

[ranjeshj]

This is not by design. It looks like a bug for sure.

[MarkIvanDev]

I started making the changes in my local branch of the repo. Will try to submit a pull request later.

[MarkIvanDev]

Sorry if the pull request is taking a long time. I had some problems with setting my environment again as when I was finished making the changes, the build broke and there are errors everywhere. 😅

[MarkIvanDev]

Thank you @Felix-Dev and @chingucoding for helping me through my first pull request.