From 1389095a7ea4f422caca4e95d8de34be2709fc2b Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Tue, 16 Jun 2020 20:09:56 +0200 Subject: [PATCH 1/8] Add expanded event raise --- dev/Generated/NavigationViewItem.properties.cpp | 10 +++++++++- dev/Generated/NavigationViewItem.properties.h | 4 ++++ dev/NavigationView/NavigationView.idl | 1 + dev/NavigationView/NavigationViewItem.cpp | 13 +++++++++++++ dev/NavigationView/NavigationViewItem.h | 1 + 5 files changed, 28 insertions(+), 1 deletion(-) diff --git a/dev/Generated/NavigationViewItem.properties.cpp b/dev/Generated/NavigationViewItem.properties.cpp index 05ba98900b..add4f53227 100644 --- a/dev/Generated/NavigationViewItem.properties.cpp +++ b/dev/Generated/NavigationViewItem.properties.cpp @@ -83,7 +83,7 @@ void NavigationViewItemProperties::EnsureProperties() winrt::name_of(), false /* isAttached */, ValueHelper::BoxValueIfNecessary(false), - nullptr); + winrt::PropertyChangedCallback(&OnIsExpandedPropertyChanged)); } if (!s_MenuItemsProperty) { @@ -149,6 +149,14 @@ void NavigationViewItemProperties::OnIconPropertyChanged( winrt::get_self(owner)->OnIconPropertyChanged(args); } +void NavigationViewItemProperties::OnIsExpandedPropertyChanged( + winrt::DependencyObject const& sender, + winrt::DependencyPropertyChangedEventArgs const& args) +{ + auto owner = sender.as(); + winrt::get_self(owner)->OnIsExpandedPropertyChanged(args); +} + void NavigationViewItemProperties::OnMenuItemsPropertyChanged( winrt::DependencyObject const& sender, winrt::DependencyPropertyChangedEventArgs const& args) diff --git a/dev/Generated/NavigationViewItem.properties.h b/dev/Generated/NavigationViewItem.properties.h index 09cc664b65..a34bbc035b 100644 --- a/dev/Generated/NavigationViewItem.properties.h +++ b/dev/Generated/NavigationViewItem.properties.h @@ -62,6 +62,10 @@ class NavigationViewItemProperties winrt::DependencyObject const& sender, winrt::DependencyPropertyChangedEventArgs const& args); + static void OnIsExpandedPropertyChanged( + winrt::DependencyObject const& sender, + winrt::DependencyPropertyChangedEventArgs const& args); + static void OnMenuItemsPropertyChanged( winrt::DependencyObject const& sender, winrt::DependencyPropertyChangedEventArgs const& args); diff --git a/dev/NavigationView/NavigationView.idl b/dev/NavigationView/NavigationView.idl index c9d6d95309..560b640f56 100644 --- a/dev/NavigationView/NavigationView.idl +++ b/dev/NavigationView/NavigationView.idl @@ -342,6 +342,7 @@ unsealed runtimeclass NavigationViewItem : NavigationViewItemBase [WUXC_VERSION_MUXONLY] { [MUX_DEFAULT_VALUE("false")] + [MUX_PROPERTY_CHANGED_CALLBACK(TRUE)] Boolean IsExpanded{ get; set; }; [MUX_PROPERTY_CHANGED_CALLBACK(TRUE)] [MUX_DEFAULT_VALUE("false")] diff --git a/dev/NavigationView/NavigationViewItem.cpp b/dev/NavigationView/NavigationViewItem.cpp index c929f3d918..3f0b91c480 100644 --- a/dev/NavigationView/NavigationViewItem.cpp +++ b/dev/NavigationView/NavigationViewItem.cpp @@ -240,6 +240,19 @@ void NavigationViewItem::SuggestedToolTipChanged(winrt::IInspectable const& newC m_suggestedToolTipContent.set(newToolTipContent); } +void NavigationViewItem::OnIsExpandedPropertyChanged(const winrt::DependencyPropertyChangedEventArgs& args) +{ + if (winrt::AutomationPeer peer = winrt::FrameworkElementAutomationPeer::FromElement(*this)) + { + auto navViewItemPeer = peer.as(); + winrt::get_self(navViewItemPeer)->RaiseExpandCollapseAutomationEvent( + IsExpanded() ? + winrt::ExpandCollapseState::Expanded + : winrt::ExpandCollapseState::Collapsed + ); + } +} + void NavigationViewItem::OnIconPropertyChanged(const winrt::DependencyPropertyChangedEventArgs& args) { UpdateVisualStateNoTransition(); diff --git a/dev/NavigationView/NavigationViewItem.h b/dev/NavigationView/NavigationViewItem.h index 99978e0117..a24ccb6303 100644 --- a/dev/NavigationView/NavigationViewItem.h +++ b/dev/NavigationView/NavigationViewItem.h @@ -27,6 +27,7 @@ class NavigationViewItem : void OnApplyTemplate() override; // Property change callbacks + void OnIsExpandedPropertyChanged(const winrt::DependencyPropertyChangedEventArgs& args); void OnIconPropertyChanged(const winrt::DependencyPropertyChangedEventArgs& args); void OnMenuItemsPropertyChanged(const winrt::DependencyPropertyChangedEventArgs& args); void OnMenuItemsSourcePropertyChanged(const winrt::DependencyPropertyChangedEventArgs& args); From a9de053915557043726ca0332d4eefd8e52535f7 Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Tue, 16 Jun 2020 20:39:45 +0200 Subject: [PATCH 2/8] Add code that does not update selection state --- dev/NavigationView/NavigationView.cpp | 12 ++++++++++-- .../NavigationViewAutomationPeer.cpp | 15 +++++++++++++++ dev/NavigationView/NavigationViewAutomationPeer.h | 2 ++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/dev/NavigationView/NavigationView.cpp b/dev/NavigationView/NavigationView.cpp index e656dc4e21..ada3ff531a 100644 --- a/dev/NavigationView/NavigationView.cpp +++ b/dev/NavigationView/NavigationView.cpp @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. See LICENSE in the project root for license information. #include "pch.h" @@ -1934,13 +1934,21 @@ void NavigationView::ChangeSelection(const winrt::IInspectable& prevItem, const UnselectPrevItem(prevItem, nextItem); ChangeSelectStatusForItem(nextItem, true /*selected*/); + if (winrt::AutomationPeer peer = winrt::FrameworkElementAutomationPeer::FromElement(*this)) + { + auto navViewItemPeer = peer.as(); + winrt::get_self(navViewItemPeer)->RaiseSelectionChangedEvent( + prevItem, nextItem + ); + } + RaiseSelectionChangedEvent(nextItem, isSettingsItem, recommendedDirection); AnimateSelectionChanged(nextItem); if (auto const nvi = NavigationViewItemOrSettingsContentFromData(nextItem)) { ClosePaneIfNeccessaryAfterItemIsClicked(nvi); - } + } } } diff --git a/dev/NavigationView/NavigationViewAutomationPeer.cpp b/dev/NavigationView/NavigationViewAutomationPeer.cpp index 27f913f8f6..84c0af9708 100644 --- a/dev/NavigationView/NavigationViewAutomationPeer.cpp +++ b/dev/NavigationView/NavigationViewAutomationPeer.cpp @@ -50,3 +50,18 @@ winrt::com_array GetSelection(); + void RaiseSelectionChangedEvent(winrt::IInspectable const& oldSelection, winrt::IInspectable const& newSelecttion); + private: }; From a24ede781a307bdc2e5eb3f1344e2fbbb5ad89fe Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Wed, 17 Jun 2020 14:18:26 +0200 Subject: [PATCH 3/8] CR improvements --- dev/NavigationView/NavigationViewItem.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev/NavigationView/NavigationViewItem.cpp b/dev/NavigationView/NavigationViewItem.cpp index 3f0b91c480..840ec07eb9 100644 --- a/dev/NavigationView/NavigationViewItem.cpp +++ b/dev/NavigationView/NavigationViewItem.cpp @@ -247,8 +247,8 @@ void NavigationViewItem::OnIsExpandedPropertyChanged(const winrt::DependencyProp auto navViewItemPeer = peer.as(); winrt::get_self(navViewItemPeer)->RaiseExpandCollapseAutomationEvent( IsExpanded() ? - winrt::ExpandCollapseState::Expanded - : winrt::ExpandCollapseState::Collapsed + winrt::ExpandCollapseState::Expanded : + winrt::ExpandCollapseState::Collapsed ); } } From e66ca4cc248cfb06f20c55af8c6ae226e8e329e8 Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Wed, 1 Jul 2020 12:06:38 +0200 Subject: [PATCH 4/8] Fix issue with selection update not being announced --- dev/NavigationView/NavigationView.cpp | 27 +++++++++++++++---- dev/NavigationView/NavigationView.h | 2 ++ .../NavigationViewAutomationPeer.cpp | 18 +++++++------ 3 files changed, 34 insertions(+), 13 deletions(-) diff --git a/dev/NavigationView/NavigationView.cpp b/dev/NavigationView/NavigationView.cpp index f85dd1175c..547a43c179 100644 --- a/dev/NavigationView/NavigationView.cpp +++ b/dev/NavigationView/NavigationView.cpp @@ -742,6 +742,12 @@ void NavigationView::OnNavigationViewItemInvoked(const winrt::NavigationViewItem if (updateSelection) { auto ip = GetIndexPathForContainer(nvi); + + // Determine if we will update collapse/expand which will happen on iff the item has children + if (DoesNavigationViewItemHaveChildren(nvi)) + { + m_shouldIgnoreUIASelectionRaiseAsExpandCollapseWillRaise = true; + } UpdateSelectionModelSelection(ip); } @@ -1930,13 +1936,24 @@ void NavigationView::ChangeSelection(const winrt::IInspectable& prevItem, const UnselectPrevItem(prevItem, nextItem); ChangeSelectStatusForItem(nextItem, true /*selected*/); - if (winrt::AutomationPeer peer = winrt::FrameworkElementAutomationPeer::FromElement(*this)) + // Selection changed and we need to notify UIA + // HOWEVER expand collapse can also trigger if an item can expand/collapse + // There are multiple cases when selectino changes: + // - Through click on item with no children -> No expand/collapse change + // - Through click on item with children -> Expand/collapse change + // - Through API with item without children -> No expand/collapse change + // - Through API with item with children -> No expand/collapse change + if (!m_shouldIgnoreUIASelectionRaiseAsExpandCollapseWillRaise) { - auto navViewItemPeer = peer.as(); - winrt::get_self(navViewItemPeer)->RaiseSelectionChangedEvent( - prevItem, nextItem - ); + if (winrt::AutomationPeer peer = winrt::FrameworkElementAutomationPeer::FromElement(*this)) + { + auto navViewItemPeer = peer.as(); + winrt::get_self(navViewItemPeer)->RaiseSelectionChangedEvent( + prevItem, nextItem + ); + } } + m_shouldIgnoreUIASelectionRaiseAsExpandCollapseWillRaise = false; RaiseSelectionChangedEvent(nextItem, isSettingsItem, recommendedDirection); AnimateSelectionChanged(nextItem); diff --git a/dev/NavigationView/NavigationView.h b/dev/NavigationView/NavigationView.h index 9ab5cfb55d..e574c70649 100644 --- a/dev/NavigationView/NavigationView.h +++ b/dev/NavigationView/NavigationView.h @@ -441,5 +441,7 @@ class NavigationView : bool m_isOpenPaneForInteraction{ false }; bool m_moveTopNavOverflowItemOnFlyoutClose{ false }; + + bool m_shouldIgnoreUIASelectionRaiseAsExpandCollapseWillRaise{ false }; }; diff --git a/dev/NavigationView/NavigationViewAutomationPeer.cpp b/dev/NavigationView/NavigationViewAutomationPeer.cpp index 84c0af9708..955f424c70 100644 --- a/dev/NavigationView/NavigationViewAutomationPeer.cpp +++ b/dev/NavigationView/NavigationViewAutomationPeer.cpp @@ -55,13 +55,15 @@ void NavigationViewAutomationPeer::RaiseSelectionChangedEvent(winrt::IInspectabl { if (winrt::AutomationPeer::ListenerExists(winrt::AutomationEvents::SelectionPatternOnInvalidated)) { - //RaiseAutomationEvent(winrt::AutomationEvents::SelectionPatternOnInvalidated); - //RaiseAutomationEvent(winrt::AutomationEvents::SelectionItemPatternOnElementSelected); - - // box_value(oldState) doesn't work here, use ReferenceWithABIRuntimeClassName to make Narrator can unbox it. - //RaisePropertyChangedEvent(winrt::SelectionPatternIdentifiers::SelectionProperty(), - // box_value(oldSelection), - // box_value(newSelecttion) - //); + if (auto nv = Owner().try_as()) + { + if (auto nvi = winrt::get_self(nv)->GetSelectedContainer()) + { + if (auto peer = winrt::FrameworkElementAutomationPeer::CreatePeerForElement(nvi)) + { + peer.RaiseAutomationEvent(winrt::AutomationEvents::SelectionItemPatternOnElementSelected); + } + } + } } } From 2ba1892a227475f583e3f83d462ac848e5ab9d90 Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Wed, 1 Jul 2020 12:20:13 +0200 Subject: [PATCH 5/8] Fix typo --- dev/NavigationView/NavigationView.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev/NavigationView/NavigationView.cpp b/dev/NavigationView/NavigationView.cpp index 547a43c179..419318a931 100644 --- a/dev/NavigationView/NavigationView.cpp +++ b/dev/NavigationView/NavigationView.cpp @@ -1938,7 +1938,7 @@ void NavigationView::ChangeSelection(const winrt::IInspectable& prevItem, const // Selection changed and we need to notify UIA // HOWEVER expand collapse can also trigger if an item can expand/collapse - // There are multiple cases when selectino changes: + // There are multiple cases when selection changes: // - Through click on item with no children -> No expand/collapse change // - Through click on item with children -> Expand/collapse change // - Through API with item without children -> No expand/collapse change From 94aa99056aaade0d8930dc789fcd4499ed5a7003 Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Wed, 1 Jul 2020 21:02:21 +0200 Subject: [PATCH 6/8] CR feedback --- dev/NavigationView/NavigationView.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/dev/NavigationView/NavigationView.cpp b/dev/NavigationView/NavigationView.cpp index 419318a931..2b05754f2d 100644 --- a/dev/NavigationView/NavigationView.cpp +++ b/dev/NavigationView/NavigationView.cpp @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. See LICENSE in the project root for license information. #include "pch.h" @@ -743,7 +743,7 @@ void NavigationView::OnNavigationViewItemInvoked(const winrt::NavigationViewItem { auto ip = GetIndexPathForContainer(nvi); - // Determine if we will update collapse/expand which will happen on iff the item has children + // Determine if we will update collapse/expand which will happen iff the item has children if (DoesNavigationViewItemHaveChildren(nvi)) { m_shouldIgnoreUIASelectionRaiseAsExpandCollapseWillRaise = true; @@ -1953,7 +1953,10 @@ void NavigationView::ChangeSelection(const winrt::IInspectable& prevItem, const ); } } - m_shouldIgnoreUIASelectionRaiseAsExpandCollapseWillRaise = false; + auto scopeGuard = gsl::finally([this]() + { + m_shouldIgnoreUIASelectionRaiseAsExpandCollapseWillRaise = false; + }); RaiseSelectionChangedEvent(nextItem, isSettingsItem, recommendedDirection); AnimateSelectionChanged(nextItem); From 1673c9520c802bac8457d047282f65538967e49a Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Thu, 2 Jul 2020 02:59:09 +0200 Subject: [PATCH 7/8] Move scopeguard up --- dev/NavigationView/NavigationView.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/dev/NavigationView/NavigationView.cpp b/dev/NavigationView/NavigationView.cpp index 2b05754f2d..7bc8c736b3 100644 --- a/dev/NavigationView/NavigationView.cpp +++ b/dev/NavigationView/NavigationView.cpp @@ -1936,6 +1936,11 @@ void NavigationView::ChangeSelection(const winrt::IInspectable& prevItem, const UnselectPrevItem(prevItem, nextItem); ChangeSelectStatusForItem(nextItem, true /*selected*/); + auto scopeGuard = gsl::finally([this]() + { + m_shouldIgnoreUIASelectionRaiseAsExpandCollapseWillRaise = false; + }); + // Selection changed and we need to notify UIA // HOWEVER expand collapse can also trigger if an item can expand/collapse // There are multiple cases when selection changes: @@ -1953,10 +1958,6 @@ void NavigationView::ChangeSelection(const winrt::IInspectable& prevItem, const ); } } - auto scopeGuard = gsl::finally([this]() - { - m_shouldIgnoreUIASelectionRaiseAsExpandCollapseWillRaise = false; - }); RaiseSelectionChangedEvent(nextItem, isSettingsItem, recommendedDirection); AnimateSelectionChanged(nextItem); From 1f180007df59b840b19535d7d3d6506bf7ece579 Mon Sep 17 00:00:00 2001 From: Stephen L Peters Date: Mon, 6 Jul 2020 13:43:32 -0700 Subject: [PATCH 8/8] Update NavigationView.cpp restrict the scope of the change to ensure the proper timing of scope guarded flag set. --- dev/NavigationView/NavigationView.cpp | 38 ++++++++++++++------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/dev/NavigationView/NavigationView.cpp b/dev/NavigationView/NavigationView.cpp index 7bc8c736b3..542ab60b58 100644 --- a/dev/NavigationView/NavigationView.cpp +++ b/dev/NavigationView/NavigationView.cpp @@ -1936,29 +1936,31 @@ void NavigationView::ChangeSelection(const winrt::IInspectable& prevItem, const UnselectPrevItem(prevItem, nextItem); ChangeSelectStatusForItem(nextItem, true /*selected*/); - auto scopeGuard = gsl::finally([this]() { - m_shouldIgnoreUIASelectionRaiseAsExpandCollapseWillRaise = false; - }); + auto scopeGuard = gsl::finally([this]() + { + m_shouldIgnoreUIASelectionRaiseAsExpandCollapseWillRaise = false; + }); - // Selection changed and we need to notify UIA - // HOWEVER expand collapse can also trigger if an item can expand/collapse - // There are multiple cases when selection changes: - // - Through click on item with no children -> No expand/collapse change - // - Through click on item with children -> Expand/collapse change - // - Through API with item without children -> No expand/collapse change - // - Through API with item with children -> No expand/collapse change - if (!m_shouldIgnoreUIASelectionRaiseAsExpandCollapseWillRaise) - { - if (winrt::AutomationPeer peer = winrt::FrameworkElementAutomationPeer::FromElement(*this)) + // Selection changed and we need to notify UIA + // HOWEVER expand collapse can also trigger if an item can expand/collapse + // There are multiple cases when selection changes: + // - Through click on item with no children -> No expand/collapse change + // - Through click on item with children -> Expand/collapse change + // - Through API with item without children -> No expand/collapse change + // - Through API with item with children -> No expand/collapse change + if (!m_shouldIgnoreUIASelectionRaiseAsExpandCollapseWillRaise) { - auto navViewItemPeer = peer.as(); - winrt::get_self(navViewItemPeer)->RaiseSelectionChangedEvent( - prevItem, nextItem - ); + if (winrt::AutomationPeer peer = winrt::FrameworkElementAutomationPeer::FromElement(*this)) + { + auto navViewItemPeer = peer.as(); + winrt::get_self(navViewItemPeer)->RaiseSelectionChangedEvent( + prevItem, nextItem + ); + } } } - + RaiseSelectionChangedEvent(nextItem, isSettingsItem, recommendedDirection); AnimateSelectionChanged(nextItem);