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

NavigationView: Throw exception when a WUX NavigationViewItem is added to it as a menu item #3232

Conversation

Felix-Dev
Copy link
Contributor

@Felix-Dev Felix-Dev commented Sep 2, 2020

Description

With PR #69, logic was added to throw an exception with a helpful error message when a developer accidentally adds a WUX NavigationViewItem to a MUX NavigationView. Unfortunately, with the move to ItemsRepeater in PR #1683, this logic did not survive and thus starting with WinUI 2.4, WinUI no longer guards against adding WUX NavigationViewItems.

This PR adds back this logic when a developer adds a WUX NavigationViewItem to a NavigationView (via the APIs MenuItems\MenuItemsSource or FooterMenuItems\FooterMenuItemsSource). The exception looks like this in a WinUI C# app:
image

Motivation and Context

Prevents developer confusion when they accidentally add a WUX NavigationViewItem to a NavigationView and see "weird" rendering behavior as a result. The image below shows the rendering difference between a MUX NavigationViewItem and a WUX NavigationViewItem when added to a MUX NavigationView:
image

How Has This Been Tested?

Tested manually. Unfortunately, while an API test for this scenario exists, it appears we cannot use a simple API test here because adding a WUX NavigationViewItem to the UI will lead to multiple calls of the NavigationViewItemsFactory::GetElementCore method during the UI update phase. Each of these calls will throw our added exception and I was unable to catch all of them and have the API test succeed. I tried adding an additional Application.Current.UnhandledException handler in the API test to catch all thrown exceptions in addition to the one we catch in

Verify.Throws<Exception>(() => { navView.UpdateLayout(); });

but the VS test explorer still showed the test as failing because they apparently still managed to slip through my added unhandled exception handling (and I set UnhandledExceptionEventArgs.Handled to true).

Any other ideas here? If we cannot get this API test to work, we should add the reason why it is disabled as a comment (or alternatively just remove it, though keeping it will act as a documentation for this test case).

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Sep 2, 2020
@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters StephenLPeters added area-NavigationView NavView control team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Sep 2, 2020
@StephenLPeters
Copy link
Contributor

@Felix-Dev would you mind making an issue to track removing this code in winui 3 where this problem would no longer exist?

@Felix-Dev
Copy link
Contributor Author

@StephenLPeters Created tracking issue #3236.

@StephenLPeters StephenLPeters merged commit 5e503fa into microsoft:master Sep 8, 2020
@Felix-Dev Felix-Dev deleted the user/Felix-Dev/navview-wux-navviewitem-handling branch September 8, 2020 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NavigationView NavView control team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants