NavigationView: Fix regression where navigating to the different pane items via the Tab key would crash the app

[Felix-Dev]

Description

This PR fixes a recently introduced WinUI 2.5 preview regression where putting a NavigationViewItem inside the NavigationView.PaneFooter area as the last item and then moving focus to the NavigationView's in-built Settings item via the Tab key causes the app to crash. A menu item has to be selected at the time of tabbing to the Settings item from the pane footer.

Motivation and Context

Fixes #3445

How Has This Been Tested?

Tested manually and modified interaction tests. The modified test TabNavigationTest() now verifies that this tabbing scenario does not crash the app any longer and now works as intended. The test has also been modified to cover the different tab focus behaviors for the menu items list: If a selected menu item exists and focus is set to the menu items list, focus will be set to the selected menu item and not the first/last menu item of the list. These different focus behaviors weren't being fully tested before from what I could tell looking through the existing interaction tests.

[StephenLPeters]

@ranjeshj lets make sure this makes it in the 2.5 release

[ranjeshj]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Felix-Dev]

Summary of the different test failures:

PaneBehaviorTests.VerifySettingsWidthOnLeftNavMediumMode():

• Fails on RS3, RS4
• On RS3, it has been run 10 times and succeeded 4 attempts in total
• Runs fine for me locally (also no MUXControlsTestApp crash)

EventTests.MenuItemInvokedTest():

• Fails on RS4
• Runs fine for me locally and no app crash

FocusBehaviorTests.EnsureTopSettingsRetainsFocusAfterOrientationChanges():

• Fails on 19H1
• Runs fine for me locally and no app crash

There apparently are no test failures on RS2 and RS5.

I'm not sure why these tests fail and unfortunately none of the Log.Comment() calls or the different Verify() fail messages are listed in Azure Pipelines.

[ranjeshj]

@StephenLPeters can you help debug these failures ?

[StephenLPeters]

@Felix-Dev RS2 and RS5 are the flavors that we run the tests with a debug build, where rs3, rs4 and 19h1 we use the release build. Can you try running the tests locally with a release build of the mux controls test app?

[Felix-Dev]

@StephenLPeters I ran all three failing tests on a local release build and every test passed. EventViewer also showed no errors for the test app.

Perhaps the failing tests don't like me having added another item to the pane footer in the test UI, though lookign at the test code, I'm not sure why that would be the case.

Looking at the test pipeline report again, I can see that all three failing tests succeded at least once during the 10 attempts fot each test.

[ranjeshj]

@Felix-Dev My guess is that the NavigationViewItem that you added to the test page caused layout to change and maybe some of the MenuItems got pushed out into the overflow menu. Now tests that assumed to see those items in the UIA tree are not seeing it anymore. a simple fix might be to make the footer item smaller in size so that items don't get pushed out. Trying out this hypothesis by updating the content to be something smaller.

[ranjeshj]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ranjeshj]

Looks like we are down to one test failing now (consistently across all runs).

VerifyNavigationViewItemContentPresenterMargin

Error log:
[Error]: AreEqual(0,0,20,0, 0,0,0,0) [File C:\a\1\s\dev\NavigationView\NavigationView_InteractionTests\CommonTests.cs Line: 1709]

[Felix-Dev]

@ranjeshj Thanks for fixing the previous test failures.

First observation about the new failing test: Runs fine for me locally.

This is getting midly annoying now. While I run the test on 20H2 and the test pipeline only tests up to 19H1, I have the suspicion the test pipeline would fail on 20H2 as well. Yet locally, I keep seeing "test passed", even if I run it multiple times (and the test fails consistently in the pipeline).

[ranjeshj]

@ranjeshj Thanks for fixing the previous test failures.

First observation about the new failing test: Runs fine for me locally.

This is getting midly annoying now. While I run the test on 20H2 and the test pipeline only tests up to 19H1, I have the suspicion the test pipeline would fail on 20H2 as well. Yet locally, I keep seeing "test passed", even if I run it multiple times (and the test fails consistently in the pipeline).

The first issue seemed to be related to the resolution/size of the window. @chingucoding added the button to switch the app to test machine resolution. I'm curious if we are switching to that resolution when running locally. I agree, anytime there is a difference between the lab run and local runs, it is not ideal.

[marcelwgn]

The first issue seemed to be related to the resolution/size of the window. @chingucoding added the button to switch the app to test machine resolution. I'm curious if we are switching to that resolution when running locally. I agree, anytime there is a difference between the lab run and local runs, it is not ideal.

@StephenLPeters Updated the test app to run all tests in CI dimensions. I know there was one test that had problems with that, I am not sure if we fixed that test or if we made that test an exception. So in theory CI runs and local runs shouldn't behave differently if we look at sizing of test content.

[Felix-Dev]

Yeah, I am seeing the correct CI dimensions being used locally by default, so that shouldn't be the issue.

[ranjeshj]

Yeah, I am seeing the correct CI dimensions being used locally by default, so that shouldn't be the issue.

This might be a timing issue with clicking on the flyout and then reading the child (which is in the flyout) properties. If the flyout did not open in time, the test just no-ops and we will end up reading the previous value for the margin. Trying a minor fix to delay by a little bit to see if that is the case. If it is a timing issue, then it explains why you are not seeing it on your dev box which is probably more performant than the lab machines.

[ranjeshj]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ranjeshj]

@ranjeshj Thanks for trying to fix the test failures.

The test could also fail at this check:

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

Line 1717 in e90f6f8

getTopLevelContentPresenterMarginButton.InvokeAndWait();

Running the test locally, I noticed the following:

• The child flyout we opened immediately before is still open (we do not close it). This might interfere with some of the following tets code (like switching the pane display mode from Compact to Expanded, so that the pane remains in Compact mode and thus we get the wrong ContentPresenter margin for top-level items).

I don't know if that is cause for the test failures we are still seeing but I'm a bit shooting in the dark here anyway so might be worth running the pipeline with that code change as well (we now close the flyout before attempting to switch the pane display mode).

Its worth a try. I've kicked off a run. The NavView test page is getting pretty complex, perhaps there is opportunity to simplify the test page a bit. For starters, content gets clipped right now for smaller sizes.

[Felix-Dev]

So that didn't solve the test failure either....I didn't add a small timer delay after clicking to close the flyout. We can give that a try as well but that's just me running out of options here 😑

[ranjeshj]

So that didn't solve the test failure either....I didn't add a small timer delay after clicking to close the flyout. We can give that a try as well but that's just me running out of options here 😑

I think the simplest option might be to move the test out of this page.

[Felix-Dev]

Since we about tried everything in the test implementation to fix the failing test, yet without success, this sounds reasonable to me. I will take a look at moving the test to another test page.

[Felix-Dev]

@ranjeshj I think I have found the reason for the test failures we were seeing. PR #3602 changed the pane menu item layout behavior and causes the menu item HasChildItem to not be visible on the screen by the time we try to open its flyout. Since it isn't on the screen, the child menu flyout is not opened and we are seeing the test failure. I had yet to merge the above PR into my local branch so I was not yet testing against the updated behavior, whereas the test pipeline did.

Adding the second pane footer item as part of this PR caused the HasChildItem menu item to be pushed out of the viewport, thus we are seeing the test fail, whereas it succeeded as part of the test run for PR #3602 because the test UI was yet lacking that second pane footer item.

Instead of moving the failing test to another page, I will fix the failing test in an upcoming PR.

[Felix-Dev]

@ranjeshj You can start a test pipeline run again to see if test runs fine now.

[Felix-Dev]

The BringSettingsIntoViewButton ui element can now be retired again as PR #3602 restored the Settings item's pinned viewport behavior. This button can now be removed in the test UI, however, as multiple unrelated interaction tests are using it, I would suggest that if we remove that button, we can do that in a follow-up PR. Let's first bring this PR over the finish line so it won't block the WinUI 2.5 release.

(Alternatively, the functionality of this button could be integrated into the newly introduced ScrollItemIntoViewComboBox (as part of this PR), though it shouldn't be required currently.)

[ranjeshj]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).