From 5fe5386b53b21e768faa22e7426684da7ea91002 Mon Sep 17 00:00:00 2001 From: Chris Glein Date: Tue, 11 Dec 2018 14:29:54 -0800 Subject: [PATCH 1/9] Check for MUX items when updating the list source. Both on initial evaluate and on any later list changes. Create a test that causes this faulre. --- dev/NavigationView/NavigationView.cpp | 63 +++++++++++++++++-- dev/NavigationView/NavigationView.h | 1 + .../NavigationViewTests.cs | 21 +++++++ 3 files changed, 80 insertions(+), 5 deletions(-) diff --git a/dev/NavigationView/NavigationView.cpp b/dev/NavigationView/NavigationView.cpp index f4d904151f..53a71d4f68 100644 --- a/dev/NavigationView/NavigationView.cpp +++ b/dev/NavigationView/NavigationView.cpp @@ -3086,17 +3086,70 @@ void NavigationView::UpdateTopNavListViewItemSource(const winrt::IInspectable& i void NavigationView::UpdateListViewItemSource() { - if (!m_appliedTemplate) - { - return; - } - auto dataSource = MenuItemsSource(); if (!dataSource) { dataSource = MenuItems(); } + auto iterable = dataSource.try_as>(); + if (iterable) + { + // Certain items are disallowed in a NavigationView's items list. Check for them. + auto checkItemIsValid = [](const winrt::IInspectable& item) + { + auto wuxItem = item.try_as(); + if (wuxItem) + { + throw winrt::hresult_invalid_argument(L"List contains Windows.UI.Xaml.Controls.NavigationViewItem. This control requires that the NavigationViewItems be of type Microsoft.UI.Xaml.Controls.NavigationViewITem"); + } + }; + + // Walk over the entire contents of the items list and validate each item. + auto checkListIsValid = [checkItemIsValid](winrt::IIterable list) + { + for (auto&& item : list) + { + checkItemIsValid(item); + } + }; + checkListIsValid(iterable); + + // If the list offers change notifications hook to validate the item contents as they're added later + auto observableVector = dataSource.try_as>(); + if (observableVector) + { + // Validate the contents now + checkListIsValid(observableVector); + + // And any future content changes + m_menuItemsVectorChangedToken = observableVector.VectorChanged({ + [checkItemIsValid, checkListIsValid](winrt::IObservableVector const& sender, winrt::IVectorChangedEventArgs const& args) + { + switch (args.CollectionChange()) + { + case winrt::Collections::CollectionChange::ItemInserted: + case winrt::Collections::CollectionChange::ItemChanged: + { + auto item = sender.GetAt(args.Index()); + checkItemIsValid(item); + break; + } + case winrt::Collections::CollectionChange::Reset: + checkListIsValid(sender); + break; + } + + } + }); + } + } + + if (!m_appliedTemplate) + { + return; + } + // Always unset the data source first from old ListView, then set data source for new ListView. if (IsTopNavigationView()) { diff --git a/dev/NavigationView/NavigationView.h b/dev/NavigationView/NavigationView.h index 1df01ef852..279387dfec 100644 --- a/dev/NavigationView/NavigationView.h +++ b/dev/NavigationView/NavigationView.h @@ -328,6 +328,7 @@ class NavigationView : winrt::SplitView::PaneOpening_revoker m_splitViewPaneOpeningRevoker{}; winrt::FrameworkElement::LayoutUpdated_revoker m_layoutUpdatedToken{}; winrt::UIElement::AccessKeyInvoked_revoker m_accessKeyInvokedRevoker{}; + winrt::event_token m_menuItemsVectorChangedToken{}; bool m_wasForceClosed{ false }; bool m_isClosedCompact{ false }; diff --git a/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs b/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs index 93ba0e7e80..5a82ae7ac1 100644 --- a/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs +++ b/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs @@ -3,6 +3,9 @@ using MUXControlsTestApp.Utilities; +using System; +using System.Collections.Generic; + using Windows.UI.Xaml; using Windows.UI.Xaml.Controls; using Windows.UI.Xaml.Markup; @@ -362,5 +365,23 @@ public void CanLoadSimpleNavigationView() } #endif + [TestMethod] + public void VerifyCanNotAddWUXItems() + { + RunOnUIThread.Execute(() => + { + var navView = new NavigationView(); + + // Nothing should go wrong adding a MUX item + var muxItem = new Microsoft.UI.Xaml.Controls.NavigationViewItem { Content = "MUX Item" }; + navView.MenuItems.Add(muxItem); + + navView.MenuItems.Add(new Microsoft.UI.Xaml.Controls.NavigationViewItemSeparator()); + + // But adding a MUX item should generate an exception + var wuxItem = new Windows.UI.Xaml.Controls.NavigationViewItem { Content = "WUX Item" }; + Verify.Throws(() => { navView.MenuItems.Add(wuxItem); }); + }); + } } } From b080d64b0253591102a0ccfc3e6b6e49ac181c20 Mon Sep 17 00:00:00 2001 From: Chris Glein Date: Tue, 11 Dec 2018 14:40:27 -0800 Subject: [PATCH 2/9] Use auto revoker --- dev/NavigationView/NavigationView.cpp | 4 ++-- dev/NavigationView/NavigationView.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dev/NavigationView/NavigationView.cpp b/dev/NavigationView/NavigationView.cpp index 53a71d4f68..819c1b99f5 100644 --- a/dev/NavigationView/NavigationView.cpp +++ b/dev/NavigationView/NavigationView.cpp @@ -3101,7 +3101,7 @@ void NavigationView::UpdateListViewItemSource() auto wuxItem = item.try_as(); if (wuxItem) { - throw winrt::hresult_invalid_argument(L"List contains Windows.UI.Xaml.Controls.NavigationViewItem. This control requires that the NavigationViewItems be of type Microsoft.UI.Xaml.Controls.NavigationViewITem"); + throw winrt::hresult_invalid_argument(L"MenuItems contains a Windows.UI.Xaml.Controls.NavigationViewItem. This control requires that the NavigationViewItems be of type Microsoft.UI.Xaml.Controls.NavigationViewItem."); } }; @@ -3123,7 +3123,7 @@ void NavigationView::UpdateListViewItemSource() checkListIsValid(observableVector); // And any future content changes - m_menuItemsVectorChangedToken = observableVector.VectorChanged({ + m_menuItemsVectorChangedRevoker = observableVector.VectorChanged(winrt::auto_revoke, { [checkItemIsValid, checkListIsValid](winrt::IObservableVector const& sender, winrt::IVectorChangedEventArgs const& args) { switch (args.CollectionChange()) diff --git a/dev/NavigationView/NavigationView.h b/dev/NavigationView/NavigationView.h index 279387dfec..0e5879a77d 100644 --- a/dev/NavigationView/NavigationView.h +++ b/dev/NavigationView/NavigationView.h @@ -328,7 +328,7 @@ class NavigationView : winrt::SplitView::PaneOpening_revoker m_splitViewPaneOpeningRevoker{}; winrt::FrameworkElement::LayoutUpdated_revoker m_layoutUpdatedToken{}; winrt::UIElement::AccessKeyInvoked_revoker m_accessKeyInvokedRevoker{}; - winrt::event_token m_menuItemsVectorChangedToken{}; + winrt::IObservableVector::VectorChanged_revoker m_menuItemsVectorChangedRevoker{}; bool m_wasForceClosed{ false }; bool m_isClosedCompact{ false }; From 53e071f6838acfb1b6d2038f56a97b9e8566ee39 Mon Sep 17 00:00:00 2001 From: Chris Glein Date: Tue, 11 Dec 2018 14:46:06 -0800 Subject: [PATCH 3/9] Detach revoker if list casts fail. --- dev/NavigationView/NavigationView.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/dev/NavigationView/NavigationView.cpp b/dev/NavigationView/NavigationView.cpp index 819c1b99f5..e546c09fda 100644 --- a/dev/NavigationView/NavigationView.cpp +++ b/dev/NavigationView/NavigationView.cpp @@ -3143,6 +3143,14 @@ void NavigationView::UpdateListViewItemSource() } }); } + else + { + m_menuItemsVectorChangedRevoker.revoke(); + } + } + else + { + m_menuItemsVectorChangedRevoker.revoke(); } if (!m_appliedTemplate) From 573745e90187ae1ac259963a9baa71dd5aad1758 Mon Sep 17 00:00:00 2001 From: Chris Glein Date: Tue, 11 Dec 2018 17:45:29 -0800 Subject: [PATCH 4/9] Don't run test pre-RS3 when NavigationView wasn't available. --- .../NavigationView_ApiTests/NavigationViewTests.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs b/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs index 5a82ae7ac1..e637e73536 100644 --- a/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs +++ b/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs @@ -368,6 +368,12 @@ public void CanLoadSimpleNavigationView() [TestMethod] public void VerifyCanNotAddWUXItems() { + if (!PlatformConfiguration.IsOsVersionGreaterThanOrEqual(OSVersion.Redstone3)) + { + Log.Warning("WUX version of NavigationViewItem only available starting in RS3."); + return; + } + RunOnUIThread.Execute(() => { var navView = new NavigationView(); From e8a00d77e6276e3e305ee2f2b23a2d97791aebd8 Mon Sep 17 00:00:00 2001 From: Chris Glein Date: Tue, 11 Dec 2018 17:47:43 -0800 Subject: [PATCH 5/9] Use API present check instead of version check --- .../NavigationView_ApiTests/NavigationViewTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs b/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs index e637e73536..ffcc396552 100644 --- a/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs +++ b/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs @@ -5,7 +5,7 @@ using System; using System.Collections.Generic; - +using Windows.Foundation.Metadata; using Windows.UI.Xaml; using Windows.UI.Xaml.Controls; using Windows.UI.Xaml.Markup; @@ -368,7 +368,7 @@ public void CanLoadSimpleNavigationView() [TestMethod] public void VerifyCanNotAddWUXItems() { - if (!PlatformConfiguration.IsOsVersionGreaterThanOrEqual(OSVersion.Redstone3)) + if (!ApiInformation.IsTypePresent("Windows.UI.Xaml.Controls.NavigationViewItem")) { Log.Warning("WUX version of NavigationViewItem only available starting in RS3."); return; From 41edfdc257e8995fc68c58250a1da6f717ff9b6e Mon Sep 17 00:00:00 2001 From: UXP Controls Automated Porting System Date: Wed, 12 Dec 2018 10:28:38 -0800 Subject: [PATCH 6/9] Disable VerifyCanNotAddWUXItems in Windows builds because it makes no sense when WUX and MUX are one. --- .../NavigationView_ApiTests/NavigationViewTests.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs b/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs index ffcc396552..0ff96a297c 100644 --- a/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs +++ b/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs @@ -365,6 +365,7 @@ public void CanLoadSimpleNavigationView() } #endif +#if !BUILD_WINDOWS [TestMethod] public void VerifyCanNotAddWUXItems() { @@ -390,4 +391,5 @@ public void VerifyCanNotAddWUXItems() }); } } +#endif } From efdb34b994310030eb41ee59f077afe9fd2ff406 Mon Sep 17 00:00:00 2001 From: UXP Controls Automated Porting System Date: Wed, 12 Dec 2018 11:20:39 -0800 Subject: [PATCH 7/9] Remove validation checks when in the Windows build. --- dev/NavigationView/NavigationView.cpp | 6 ++++-- .../NavigationView_ApiTests/NavigationViewTests.cs | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/dev/NavigationView/NavigationView.cpp b/dev/NavigationView/NavigationView.cpp index e546c09fda..b2550d75b5 100644 --- a/dev/NavigationView/NavigationView.cpp +++ b/dev/NavigationView/NavigationView.cpp @@ -3092,14 +3092,15 @@ void NavigationView::UpdateListViewItemSource() dataSource = MenuItems(); } + // The following validation is only relevant outside of the Windows build where WUXC and MUXC have distinct types. +#if !BUILD_WINDOWS auto iterable = dataSource.try_as>(); if (iterable) { // Certain items are disallowed in a NavigationView's items list. Check for them. auto checkItemIsValid = [](const winrt::IInspectable& item) { - auto wuxItem = item.try_as(); - if (wuxItem) + if (item.try_as()) { throw winrt::hresult_invalid_argument(L"MenuItems contains a Windows.UI.Xaml.Controls.NavigationViewItem. This control requires that the NavigationViewItems be of type Microsoft.UI.Xaml.Controls.NavigationViewItem."); } @@ -3152,6 +3153,7 @@ void NavigationView::UpdateListViewItemSource() { m_menuItemsVectorChangedRevoker.revoke(); } +#endif if (!m_appliedTemplate) { diff --git a/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs b/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs index 0ff96a297c..2fb8caa585 100644 --- a/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs +++ b/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs @@ -390,6 +390,6 @@ public void VerifyCanNotAddWUXItems() Verify.Throws(() => { navView.MenuItems.Add(wuxItem); }); }); } - } #endif + } } From 7526468caf7869ac94987dce66f13377834c7bbc Mon Sep 17 00:00:00 2001 From: UXP Controls Automated Porting System Date: Wed, 12 Dec 2018 16:03:22 -0800 Subject: [PATCH 8/9] Use PrepareContainerForItemOverride instead of a new VectorChanged listener. --- dev/NavigationView/NavigationView.cpp | 73 ++----------------- dev/NavigationView/NavigationView.h | 1 - dev/NavigationView/NavigationViewList.cpp | 12 ++- .../NavigationViewTests.cs | 10 ++- 4 files changed, 23 insertions(+), 73 deletions(-) diff --git a/dev/NavigationView/NavigationView.cpp b/dev/NavigationView/NavigationView.cpp index b2550d75b5..f4d904151f 100644 --- a/dev/NavigationView/NavigationView.cpp +++ b/dev/NavigationView/NavigationView.cpp @@ -3086,78 +3086,15 @@ void NavigationView::UpdateTopNavListViewItemSource(const winrt::IInspectable& i void NavigationView::UpdateListViewItemSource() { - auto dataSource = MenuItemsSource(); - if (!dataSource) - { - dataSource = MenuItems(); - } - - // The following validation is only relevant outside of the Windows build where WUXC and MUXC have distinct types. -#if !BUILD_WINDOWS - auto iterable = dataSource.try_as>(); - if (iterable) - { - // Certain items are disallowed in a NavigationView's items list. Check for them. - auto checkItemIsValid = [](const winrt::IInspectable& item) - { - if (item.try_as()) - { - throw winrt::hresult_invalid_argument(L"MenuItems contains a Windows.UI.Xaml.Controls.NavigationViewItem. This control requires that the NavigationViewItems be of type Microsoft.UI.Xaml.Controls.NavigationViewItem."); - } - }; - - // Walk over the entire contents of the items list and validate each item. - auto checkListIsValid = [checkItemIsValid](winrt::IIterable list) - { - for (auto&& item : list) - { - checkItemIsValid(item); - } - }; - checkListIsValid(iterable); - - // If the list offers change notifications hook to validate the item contents as they're added later - auto observableVector = dataSource.try_as>(); - if (observableVector) - { - // Validate the contents now - checkListIsValid(observableVector); - - // And any future content changes - m_menuItemsVectorChangedRevoker = observableVector.VectorChanged(winrt::auto_revoke, { - [checkItemIsValid, checkListIsValid](winrt::IObservableVector const& sender, winrt::IVectorChangedEventArgs const& args) - { - switch (args.CollectionChange()) - { - case winrt::Collections::CollectionChange::ItemInserted: - case winrt::Collections::CollectionChange::ItemChanged: - { - auto item = sender.GetAt(args.Index()); - checkItemIsValid(item); - break; - } - case winrt::Collections::CollectionChange::Reset: - checkListIsValid(sender); - break; - } - - } - }); - } - else - { - m_menuItemsVectorChangedRevoker.revoke(); - } - } - else + if (!m_appliedTemplate) { - m_menuItemsVectorChangedRevoker.revoke(); + return; } -#endif - if (!m_appliedTemplate) + auto dataSource = MenuItemsSource(); + if (!dataSource) { - return; + dataSource = MenuItems(); } // Always unset the data source first from old ListView, then set data source for new ListView. diff --git a/dev/NavigationView/NavigationView.h b/dev/NavigationView/NavigationView.h index 0e5879a77d..1df01ef852 100644 --- a/dev/NavigationView/NavigationView.h +++ b/dev/NavigationView/NavigationView.h @@ -328,7 +328,6 @@ class NavigationView : winrt::SplitView::PaneOpening_revoker m_splitViewPaneOpeningRevoker{}; winrt::FrameworkElement::LayoutUpdated_revoker m_layoutUpdatedToken{}; winrt::UIElement::AccessKeyInvoked_revoker m_accessKeyInvokedRevoker{}; - winrt::IObservableVector::VectorChanged_revoker m_menuItemsVectorChangedRevoker{}; bool m_wasForceClosed{ false }; bool m_isClosedCompact{ false }; diff --git a/dev/NavigationView/NavigationViewList.cpp b/dev/NavigationView/NavigationViewList.cpp index 26309b774a..6588301504 100644 --- a/dev/NavigationView/NavigationViewList.cpp +++ b/dev/NavigationView/NavigationViewList.cpp @@ -56,6 +56,16 @@ void NavigationViewList::PrepareContainerForItemOverride(winrt::DependencyObject itemContainer.UseSystemFocusVisuals(m_showFocusVisual); winrt::get_self(itemContainer)->ClearIsContentChangeHandlingDelayedForTopNavFlag(); } + + // The following validation is only relevant outside of the Windows build where WUXC and MUXC have distinct types. +#if !BUILD_WINDOWS + // Certain items are disallowed in a NavigationView's items list. Check for them. + if (element.try_as()) + { + throw winrt::hresult_invalid_argument(L"MenuItems contains a Windows.UI.Xaml.Controls.NavigationViewItem. This control requires that the NavigationViewItems be of type Microsoft.UI.Xaml.Controls.NavigationViewItem."); + } +#endif + __super::PrepareContainerForItemOverride(element, item); } @@ -133,4 +143,4 @@ void NavigationViewList::PropagateChangeToAllContainers(std::function(() => { navView.MenuItems.Add(wuxItem); }); + navView.MenuItems.Add(wuxItem); + + // But adding a WUX item should generate an exception (as soon as the new item gets processed) + Verify.Throws(() => { navView.UpdateLayout(); }); }); } #endif From 56fb0056867bcbdc8892f3596a0666a74013ade2 Mon Sep 17 00:00:00 2001 From: UXP Controls Automated Porting System Date: Fri, 14 Dec 2018 16:49:32 -0800 Subject: [PATCH 9/9] Switch from using PrepareContainerForItemOverride to IsItemItsOwnContainerOverride. --- dev/NavigationView/NavigationViewList.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/dev/NavigationView/NavigationViewList.cpp b/dev/NavigationView/NavigationViewList.cpp index 6588301504..3a0639b836 100644 --- a/dev/NavigationView/NavigationViewList.cpp +++ b/dev/NavigationView/NavigationViewList.cpp @@ -26,6 +26,15 @@ bool NavigationViewList::IsItemItsOwnContainerOverride(winrt::IInspectable const bool isItemItsOwnContainerOverride = false; if (args) { + // This validation is only relevant outside of the Windows build where WUXC and MUXC have distinct types. +#if !BUILD_WINDOWS + // Certain items are disallowed in a NavigationView's items list. Check for them. + if (args.try_as()) + { + throw winrt::hresult_invalid_argument(L"MenuItems contains a Windows.UI.Xaml.Controls.NavigationViewItem. This control requires that the NavigationViewItems be of type Microsoft.UI.Xaml.Controls.NavigationViewItem."); + } +#endif + auto nvib = args.try_as(); if (nvib && nvib != m_lastItemCalledInIsItemItsOwnContainerOverride.get()) { @@ -57,15 +66,6 @@ void NavigationViewList::PrepareContainerForItemOverride(winrt::DependencyObject winrt::get_self(itemContainer)->ClearIsContentChangeHandlingDelayedForTopNavFlag(); } - // The following validation is only relevant outside of the Windows build where WUXC and MUXC have distinct types. -#if !BUILD_WINDOWS - // Certain items are disallowed in a NavigationView's items list. Check for them. - if (element.try_as()) - { - throw winrt::hresult_invalid_argument(L"MenuItems contains a Windows.UI.Xaml.Controls.NavigationViewItem. This control requires that the NavigationViewItems be of type Microsoft.UI.Xaml.Controls.NavigationViewItem."); - } -#endif - __super::PrepareContainerForItemOverride(element, item); }