NavigationView: Fix content clipping regression

[Felix-Dev]

Description

This PR fixes a NavigationView content clipping regression in closed compact mode introduced with WinUI 2.4.

Motivation and Context

Fixes #2541.

How Has This Been Tested?

Added interaction test + visually

Screenshots:

Current cutoff text in closed compact mode excluding this PR:

WinUI 2.3 closed compact look restored with this PR:

(The cutoff NavigationViewItem content is "Menu Item".)

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Felix-Dev]

Hmmm...the two failing tests in Azure Pipelines work fine for me locally and looking at their test code I also can't see anything which would make them fail based on my additions in this PR. I'm also up-to-date with the master branch except for PR c5fa8a6 which does not touch the NavigationView. The two failing tests in question failed already a few days ago too when we first ran Azure Pipelines on this PR.

Thoughts?

[StephenLPeters]

The tests are only failing on RS2 with the following errors:
NavigationViewTests.SelectionTests.SelectionFollowFocusTest:
[Error]: IsTrue [File d:\a\1\s\dev\NavigationView\NavigationView_InteractionTests\SelectionTests.cs Line: 141]

NavigationViewTests.SelectionTests.MenuItemAutomationSelectionTest:
[Error]: IsTrue [File d:\a\1\s\dev\NavigationView\NavigationView_InteractionTests\SelectionTests.cs Line: 178]

[StephenLPeters]

Sorry this info isn't available to you, the Azure Dev Ops team says they are looking at the issue.

[StephenLPeters]

@Felix-Dev the Azure Dev Ops issue has been resolved, you should now be able to see the test failures yourself.

[Felix-Dev]

@StephenLPeters I am looking at the tests in question and I cannot notice anything here why my additional test code added to the test UI would cause these tests to fail.

NavigationViewTests.SelectionTests.SelectionFollowFocusTest:
[Error]: IsTrue [File d:\a\1\s\dev\NavigationView\NavigationView_InteractionTests\SelectionTests.cs Line: 141]

I took a look at the line and this is the failing code:

microsoft-ui-xaml/dev/NavigationView/NavigationView_InteractionTests/SelectionTests.cs

Line 141 in 3834f4d

Verify.IsTrue(secondItem.HasKeyboardFocus);

The error above says "IsTrue". How am I supposed to read that? Does it mean Verify.IsTrue(secondItem.HasKeyboardFocus); is actually false, thus secondItem.HasKeyboardFocus is false? Otherwise, "IsTrue" here is the expected value and shouldn't cause the test to fail.

And again, I don't have an idea right now why my addition of another NavigationViewItem as the new last one of the NavigationViewItems would cause this tests to fail here. The "Apps" and "Games" NavigationViewItems are untouched by me in this PR.

Similar story as for the other failing test.

I suppose I could cut out my added test code and add it to one of the other many NavigationView test pages in the MUXControlsTestApp, though that would just be me randomly trying to fix failing tests I don't know why they are failing in the first place.

How should we proceed here then?

[StephenLPeters]

The test output indicates the verify that failed so in this case it indicates that the Verify.IsTrue(secondItem.HasKeyboardFocus) failed indicating secondItem.HasKeyboardFocus is false. These tests are only failing on RS2, so make sense that they don't fail locally. I haven't had time to look at why that might be the case.

[Felix-Dev]

Seems like we have to wait for someone form the team then to look at the test failures on RS2 more closely as I won't be able to set up an environment here where I could inspect those myself.

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

@Felix-Dev are you still blocked here?

[Felix-Dev]

@StephenLPeters I will hopefully be able to take a final look at this today and move the failing tests to another existing test page for now. I think I will go with the existing "NavigationVew Regression Test" test page as that page will do it (instead of setting up a new test page). Does that sound reasonable to you?

[StephenLPeters]

@StephenLPeters I will hopefully be able to take a final look at this today and move the failing tests to another existing test page for now. I think I will go with the existing "NavigationVew Regression Test" test page as that page will do it (instead of setting up a new test page). Does that sound reasonable to you?

Awesome! that does sound reasonable.

[Felix-Dev]

Merged with master.

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

@Felix-Dev the change to use lab test frame dimensions locally just got checked in. Could you merge in master and try running the failing tests again? I'm hoping that accounts for the difference in environments that is causing the lab failures

[Felix-Dev]

@StephenLPeters Merged with master.

Unfortunately, I couldn't find all of the failing tests again (previous Azure Pipelines build failures say they are no longer available), but thatnks to the comversattion here, at least one failing test in the pipeline but succeeding for me locally was SettingsCanBeUnselected(). After merging with master, I can now now verify that it fails locally for me too.

It just fails (and presumably all the other failing tests) because of my addition of two additional NavigationViewItems on the used test page here. If I remove these two items again, SettingsCanBeUnselected() succeeds as expected.

Honestly, since the addition of two more NavigationViewItems to the main NavigationView test page (NavigationViewTestPage) appears to create so much "pain" unrelated specifically to this issue, I am considering moving my added interaction test (which also checks the ContentPresenter margin of child menu item) to a different test page, untouched by the majority of the tests. Not yet sure which page (I'll have to take a closer look).

[StephenLPeters]

Does it fail because the test is trying to click a button that is off screen? can we just call invoke instead, or if clicking is important call BringIntoView first?

[Felix-Dev]

Yep, the unrelated test fails because the addition of two more NavigationViewItems pushed the settings button off the screen (prior to the CI frame PR, the settings button still fit into the screen area so it ran fine for me). I can take a look at your suggestion here.

[Felix-Dev]

@StephenLPeters
The culprit in the failing SettingsCanBeUnselected() test is the Settings NavigationViewItem:

microsoft-ui-xaml/dev/NavigationView/NavigationView_InteractionTests/SelectionTests.cs

Line 216 in 647cb4d

settings.Click();

I can not call Invoke() instead of Click() on it because the InvokePattern is not implemented for the NavigationViewItem.

I did not find a BringIntoView() method but there is a UIElement.StartBringIntoView() method. That is asynchronous though so I looked for an alternative here to scroll the Settings NavigationViewItem into view. My idea was to grab the ScrollViewer responsible for scrolling the NavigationViewItems and tell it to scroll to the bottom - thus scrolling Settings NavigationViewItem into view:

private void BringSettingsIntoViewButton_Click(object sender, RoutedEventArgs e)
{
    if (VisualTreeUtils.FindElementOfTypeInSubtree<ItemsRepeaterScrollHost>(NavView) is ItemsRepeaterScrollHost scrollHost)
    {
        scrollHost.ScrollViewer.ChangeView(null, double.MaxValue, null);
    }
}

Unfortunately, while the ChangeView() method is indeed executed with each button press here, no scrolling happens:

Am I missing something here?

[StephenLPeters]

does FrameworkElement.BringIntoView not work? https://docs.microsoft.com/en-us/dotnet/api/system.windows.frameworkelement.bringintoview?view=netcore-3.1

[Felix-Dev]

@StephenLPeters Unfortunately, that method is currently not available in UWP.

[marcelwgn]

It is available in UWP: https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.uielement.startbringintoview?view=winrt-19041

Also looks like there are merge conflicts you need to resolve @Felix-Dev .

[Felix-Dev]

StartBringIntoView() != BringIntoView() (it's asynchronous). That threw me a loop yesterday night but it seems that adding a Wait.ForIdle() call in the interaction test is enough to have the test reliably scroll Settings into view.

[marcelwgn]

Beside the asynchronity BringIntoView and StartBringIntoView should be the same though, and Wait.ForIdle shouldn't care whether it's async or not. But if it works now, that's great.

[Felix-Dev]

@StephenLPeters Merged with master and fixed failing SettingsCanBeUnselected() interaction test.

Will look into the other failing tests the pipeline should throw out.

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Felix-Dev]

@StephenLPeters Fixed remaining failing interaction tests.

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).