Hide NavigationViewItem Chevron if Children is empty #2759
Conversation
Attach a CollectionChanged to ItemsRepeater ItemsViewSource whenever the MenuItems or MenuItemsSource changes. Add check to MenuItemsSource count when checking for children. Fixes microsoft#2743
|
|
msft-github-bot
added
the
needs-triage
|
I think we should a test here for the chevron visibility. You can find the NavigationView Interaction tests here. I'm thinking we could add two tests here - one for when NavigationView uses XAML Markup to set the child items (via NavigationView.MenuItems) and one test to handle the data binding case where the source can also be set to You can find interaction test examples fo many of the WinUI-developed controls, for example TreeView if you need some help setting them up. |
|
At the end of the day you will need to walk the visual tree to get to the chevron presenter in your test. Maybe an API test would be sufficient for this? Generally API tests are faster and less likely to be unreliable. I think in this an API test would be simpler since it's quite easy to repro the issue with just code behind. If you want @MarkIvanDev I can help you get started with the test. |
|
@chingucoding I suggested an interaction test because I used those for when I had to walk the visual tree and I was under the impression that using an interaction test instead of an API test seemed to be much better suited for those kind of tests. If we can, however, walk the visual tree rather easily in an API test as well, then yeah, that sounds better. |
|
The general pattern I have seen with interaction tests is the following:
In a lot of cases this makes sense, however in this case I think we could skip this "UIA interop" by just creating a NavigationView(item) and running the visual tree walking inside the test. By that we skip this "read text X from page Y" that comes from having an interaction test. |
|
Yeah, thinking about it further, not sure why I thought an interaction test here would be better suited than an API test. @MarkIvanDev you can find the API tests for the NavigationView here. |
|
In general if your test does not require interacting with the control, via touch, or mouse, or keyboard interaction, then you should attempt to test the functionality via an API test. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
StephenLPeters
added
area-NavigationView
|
@chingucoding making an API test involves making a new TestMethod right? Can you help me set that up. |
|
@MarkIvanDev In NavigationViewTests.cs you could add the following code: [TestMethod]
public void VerifyExpandCollapseChevronVisibility()
{
NavigationView navView = null;
NavigationViewItem parentItem = null;
ObservableCollection<string> children = null;
RunOnUIThread.Execute(() =>
{
navView = new NavigationView();
Content = navView;
children = new ObservableCollection<string>();
parentItem = new NavigationViewItem() { Content = "ParentItem", MenuItemsSource = children };
navView.MenuItems.Add(parentItem);
navView.Width = 1008; // forces the control into Expanded mode so that the menu renders
Content.UpdateLayout();
UIElement chevronUIElement = (UIElement)FindVisualChildByName(parentItem, "ExpandCollapseChevron");
Verify.IsTrue(chevronUIElement.Visibility == Visibility.Collapsed, "chevron should have been collapsed as NavViewItem has no children");
// Add a child to our NavigationView parentItem. This should make the chevron visible.
children.Add("Child");
Content.UpdateLayout();
Verify.IsTrue(chevronUIElement.Visibility == Visibility.Visible, "chevron should have been visible as NavViewItem now has children");
// Remove all children from our NavigationView parentItem. This should collapse the chevron
children.Clear();
Content.UpdateLayout();
Verify.IsTrue(chevronUIElement.Visibility == Visibility.Collapsed, "chevron should have been collapsed as NavViewItem no longer has children");
});
}
private static DependencyObject FindVisualChildByName(FrameworkElement parent, string name)
{
if (parent.Name == name)
{
return parent;
}
int childrenCount = VisualTreeHelper.GetChildrenCount(parent);
for (int i = 0; i < childrenCount; i++)
{
FrameworkElement childAsFE = VisualTreeHelper.GetChild(parent, i) as FrameworkElement;
if (childAsFE != null)
{
DependencyObject result = FindVisualChildByName(childAsFE, name);
if (result != null)
{
return result;
}
}
}
return null;
}
This test method might not cover everything we want to check here so feel free to add more test cases or treat data bound menu items (NavigationViewItem.MenuItemsSource) different to XAML markup added items (NavigationViewItem.MenuItems). It should give you a good starting block though and a feeling how to write these API tests. It's also helpful to just check other API tests as often times, they can give you sample code how to write your API test (like walking the visual tree or updating the UI). |
|
Thanks @Felix-Dev for answering! Adding to @Felix-Dev s answer: In some cases calling "Content.UpdateLayout()" might not have finished doing all changes while the next line of code is already being evaluated. For example, new items might not have updated correctly while the verification code is already running. In those cases, you need to close the "RunUIThread.Execute" block and add "IdleSynchronizer.WaitForIdle();" to ensure all updates to the UI have been done. |
|
@chingucoding @Felix-Dev I am having trouble restoring packages. I just performed a clean and then when I try to rebuild or even restore packages, there are packages that cannot be restored. I am dealing with this for the past few days so I can't get anything running. |
|
That's probably a stupid question on my side, but does building the solution work though? (BTW You don't need to build all projects, you only need a few of them). |
|
Try deleting the BuildOutput folder in the root of the repository. |
|
@chingucoding These are the errors when doing a package restore. |
|
Finally, I was able to run the test. I tried to do everything for the second time and it ran at last.
What will be the next step? |
|
Those errors you got seems more like network issues as it fails to download the packages. But seems that you are able to test your changes now so, yay! Next steps are:
If there are no requested changes during code review, there is nothing else you need to do here. Thank you for contributing this fix! |
| @@ -144,9 +144,16 @@ void NavigationViewItem::UpdateRepeaterItemsSource() | |||
| } | |||
| return MenuItems().as<winrt::IInspectable>(); | |||
| }(); | |||
| m_itemsSourceViewChanged.revoke(); | |||
There was a problem hiding this comment.
#Fixed I think you need to revoke this listener in UnhookEventsAndClearFields too:
microsoft-ui-xaml/dev/NavigationView/NavigationViewItem.cpp
Lines 918 to 928 in 50577c7
There was a problem hiding this comment.
Do I also need to rename the variable into m_itemsSourceViewCollectionChangedRevoker to be inline with the other revokers?
There was a problem hiding this comment.
Good point, yes that would be great!
There was a problem hiding this comment.
Just pushed a commit doing this.
| navView.Width = 1008; // forces the control into Expanded mode so that the menu renders | ||
| Content.UpdateLayout(); | ||
|
|
||
| UIElement chevronUIElement = (UIElement)FindVisualChildByName(parentItem, "ExpandCollapseChevron"); |
There was a problem hiding this comment.
We can use VisualTreeUtils.FindVisualChildByName() here instead of having to provide an own implementation.
There was a problem hiding this comment.
I also removed the FindVisualChildByName method.
| @@ -759,5 +759,70 @@ public void VerifyNavigationViewItemInFooterDoesNotCrash() | |||
| Verify.IsTrue(true); | |||
| }); | |||
| } | |||
|
|
|||
| [TestMethod] | |||
| public void VerifyExpandCollapseChevronVisibility() | |||
There was a problem hiding this comment.
-
For now we check if the chevron is collapsed when the bound data collection is empty. We should probably also check in this test if the chevron visibility is updated accordingly when the NavigationViewItem.MenuItemsSource is set to
null. (I.e. you could add an item again to the bound collection after our final visibility check after clearing the collection, then setparentItem.MenuItemsSource = null;and verify again if the chevron is collapsed.) -
Right now we only use the
NavigationViewItem.MenuItemsSourceAPI to verify the correctness of the chevron visibility. For completeness, we could also add testing if the chevron visibility is updated correctly when using theNavigationViewItem.MenuItemsAPI instead. Testing with this API would simply add/remove items to/from MenuItems (as MenuItems cannot be set tonull) and check the chevron visibility accordingly. These checks could be added inside this method below our tests for theMenuItemsSourceAPI (for example after clearing the bound collection or setting MenuItemsSource to null) so that we start testing using the MenuItems API with the chevron in collapsed state.
There was a problem hiding this comment.
Are there no other tests that would verify the other branches of the HasChildren function? I thought that some other tests verified this implicitly.
There was a problem hiding this comment.
@chingucoding I haven't immediately found any other test (API/Interaction test) which we could fall back to here.
Edit: Just noticed there is no public NavigationViewItem.HasChildren API so ignore what I previously wrote here about having to create a test for it too 😅
There was a problem hiding this comment.
It seems that are no tests that test collection changes in the MenuItems API. Would be best if we add those here or if we create a tracking issue to do that later.
There was a problem hiding this comment.
I am adding the tests for the MenuItems API now. Will push it once it passes the tests.
There was a problem hiding this comment.
All tests have passed even the tests for the MenuItems API.
|
|
||
| // Add a child to parentItem and set the MenuItemsSource as null. This should collapse the chevron | ||
| children.Add("Child 2"); | ||
| parentItem.MenuItemsSource = null; |
There was a problem hiding this comment.
Do we need a Content.UpdateLayout() call here to make sure the chevron is visible again before setting the MenuItemsSource to null and verifying that it actually collapses the chevron? (Before adding "Child 2" item, the chevron is collapsed.)
There was a problem hiding this comment.
But we have already checked earlier at Line 787 that adding an item to children does make the chevron visible right? I think that it would be redundant to make that check again here?
There was a problem hiding this comment.
My thinking here is not to verify that adding an item actually makes the chevron visible (by adding an additional Verify.IsTrue(chevronUIElement.Visibility == Visibility.Visible)) but making sure that by the time we set parentItem.MenuItemsSource = null; the chevron is actually visible. That way, we can make sure that setting MenuItemsSource to null will indeed collapse the chevron.
In other words, my concern here is the following: Before we add our child item (Child 2) the chevron is collapsed. We then add this child and immediately after that set MenuItemsSource to null. I'm concerned that without the Content.UpdateLayout() call after adding the child, the chevron might not actually be made visible again, thus we are not actually verifying whether setting MenuItemsSource to null collapses the chevron. (Since the chevron might still be collapsed when we set the source to null and thus the Verify.IsTrue() call below succeeds, even though MenuItemsSource = null might not actually change the chevron visibility state at all.)
@StephenLPeters @chingucoding Your thoughts here?
There was a problem hiding this comment.
I understand now. Thanks. Will add those now.
There was a problem hiding this comment.
Valid concern, and another Verify would guarantee that was the case. Without it it could be the case that a bug causes the chevron to never show which wouldn't be caught without the "redundant" verify.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Looks good to me 🙂 |
|
Thanks @Felix-Dev and @chingucoding for helping me through this. |
Attach a CollectionChanged to ItemsRepeater ItemsViewSource
whenever the MenuItems or MenuItemsSource changes.
Add check to MenuItemsSource count when checking for children.
Fixes #2743
Description
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):