Fix NavView issue with SelectionChanged being raised upon displaymode change

dev/NavigationView/NavigationView.cpp

@@ -773,7 +773,12 @@ void NavigationView::CreateAndHookEventsToSettings(std::wstring_view settingsNam
         bool shouldSelectSetting = selectedItem && IsSettingsItem(selectedItem);
 
         if (shouldSelectSetting)
-        { 
+        {
+            auto scopeGuard = gsl::finally([this]()
+            {
+                    m_shouldIgnoreNextSelectionChangeBecauseSettingsRestore = false;
+            });
+            m_shouldIgnoreNextSelectionChangeBecauseSettingsRestore = true;
             SetSelectedItemAndExpectItemInvokeWhenSelectionChangedIfNotInvokedFromAPI(nullptr);
         }
 
@@ -806,7 +811,12 @@ void NavigationView::CreateAndHookEventsToSettings(std::wstring_view settingsNam
         SetValue(s_SettingsItemProperty, settingsItem);
 
         if (shouldSelectSetting)
-        { 
+        {
+            auto scopeGuard = gsl::finally([this]()
+            {
+                    m_shouldIgnoreNextSelectionChangeBecauseSettingsRestore = false;
+            });
+            m_shouldIgnoreNextSelectionChangeBecauseSettingsRestore = true;
             SetSelectedItemAndExpectItemInvokeWhenSelectionChangedIfNotInvokedFromAPI(m_settingsItem.get());
         }
     }
@@ -1538,6 +1548,11 @@ void NavigationView::RaiseSelectionChangedEvent(winrt::IInspectable const& nextI
 // If nextItem is selectionsuppressed, we should undo the selection. We didn't undo it OnSelectionChange because we want change by API has the same undo logic.
 void NavigationView::ChangeSelection(const winrt::IInspectable& prevItem, const winrt::IInspectable& nextItem)
 {
+    // Selection changed event was requested to be ignored by settings item restoration, so let's do that
+    if (m_shouldIgnoreNextSelectionChangeBecauseSettingsRestore) {
+        return;
+    }
+
     bool isSettingsItem = IsSettingsItem(nextItem);
 
     if (IsSelectionSuppressed(nextItem))
@@ -2264,9 +2279,21 @@ void NavigationView::UpdateSingleSelectionFollowsFocusTemplateSetting()
 
 void NavigationView::OnSelectedItemPropertyChanged(winrt::DependencyPropertyChangedEventArgs const& args)
 {
-    auto newItem = args.NewValue();
 
-    ChangeSelection(args.OldValue(), newItem);
+    const auto newItem = args.NewValue();
+    const auto oldItem = args.OldValue();
+
+    ChangeSelection(oldItem, newItem);
+
+    // When we do not raise a "SelectItemChanged" event, the selection does not get animated.
+    // To prevent faulty visual states, we will animate that here
+    // Since we only do this for the settings item, check if the old item is our settings item
+    if (oldItem != newItem && m_shouldIgnoreNextSelectionChangeBecauseSettingsRestore)
+    {
+        ChangeSelectStatusForItem(oldItem, false /*selected*/);
+        ChangeSelectStatusForItem(newItem, true /*selected*/);
+        AnimateSelectionChanged(oldItem, newItem);
+    }
 
     if (m_appliedTemplate && IsTopNavigationView())
     {
@@ -2306,7 +2333,6 @@ void NavigationView::SetSelectedItemAndExpectItemInvokeWhenSelectionChangedIfNot
 
         m_indexOfLastSelectedItemInTopNav = m_topDataProvider.IndexOf(item); // for the next time we animate
     }
-
     SelectedItem(item);
 }
 

dev/NavigationView/NavigationView.h

@@ -365,7 +365,8 @@ class NavigationView :
     // Customer select an item from SelectedItem property->ChangeSelection update ListView->LIstView raise OnSelectChange(we want stop here)->change property do do animation again.
     // Customer clicked listview->listview raised OnSelectChange->SelectedItem property changed->ChangeSelection->Undo the selection by SelectedItem(prevItem) (we want it stop here)->ChangeSelection again ->...
     bool m_shouldIgnoreNextSelectionChange{ false };
-
+    // Used to disable raising selection change iff settings item gets restored because of displaymode change
+    bool m_shouldIgnoreNextSelectionChangeBecauseSettingsRestore{ false };
     // A flag to track that the selectionchange is caused by selection a item in topnav overflow menu
     bool m_selectionChangeFromOverflowMenu{ false };
 

dev/NavigationView/NavigationView_InteractionTests/NavigationViewTests.cs

@@ -2947,6 +2947,41 @@ public void KeyboardFocusToolTipTest() // Verify tooltips appear when Keyboard f
             }
         }
 
+        [TestMethod]
+        [TestProperty("TestSuite", "C")]
+        public void DisplayModeChangeSelectionEventTest()
+        {
+            var testScenarios = RegressionTestScenario.BuildLeftNavRegressionTestScenarios();
+            foreach (var testScenario in testScenarios)
+            {
+                using (var setup = new TestSetupHelper(new[] { "NavigationView Tests", "NavigationView Test" }))
+                {
+                    if (!PlatformConfiguration.IsOsVersionGreaterThanOrEqual(OSVersion.Redstone3))
+                    {
+                        Log.Warning("Test is disabled on RS2 and earlier because SplitView lacks the requisite events.");
+                        return;
+                    }
+                    Button clearSelectedItem = new Button(FindElement.ById("ClearSelectionChangeIndicatorButton"));
+                    TextBlock selectionRaisedIndicator = new TextBlock(FindElement.ById("SelectionChangedRaised"));
+
+                    ComboBox selectedItem = new ComboBox(FindElement.ById("SelectedItemCombobox"));
+                    selectedItem.SelectItemByName("Settings");
+                    Verify.AreEqual("True", selectionRaisedIndicator.GetText());
+
+                    ComboBox displayMode = new ComboBox(FindElement.ById("PaneDisplayModeCombobox"));
+                    clearSelectedItem.InvokeAndWait();
+                    displayMode.SelectItemByName("Top");
+                    Verify.AreEqual("False", selectionRaisedIndicator.GetText());
+                    Wait.ForIdle();
+
+
+                    displayMode.SelectItemByName("Left");
+                    Wait.ForIdle();
+                    Verify.AreEqual("False", selectionRaisedIndicator.GetText());
+                }
+            }
+        }
+
         [TestMethod]
         [TestProperty("TestSuite", "C")]
         public void PaneOpenCloseEventsTest()

dev/NavigationView/TestUI/NavigationViewPage.xaml

@@ -178,6 +178,9 @@
                             <ComboBoxItem Content="LeftCompact" Tag="LeftCompact"/>
                             <ComboBoxItem Content="LeftMinimal" Tag="LeftMinimal"/>
                         </ComboBox>
+                        <Button x:Name="ClearSelectionChangeIndicatorButton" AutomationProperties.Name="ClearSelectionChangeIndicatorButton"  Click="ClearSelectionChangeBlock">Clear selection changed indicator</Button>
+                        <TextBlock VerticalAlignment="Center">Selection was raised:  </TextBlock>
+                        <TextBlock VerticalAlignment="Center" x:Name="SelectionChangedRaised" AutomationProperties.Name="SelectionChangedRaised"/>
                     </StackPanel>
 
                     <StackPanel Orientation="Horizontal">

dev/NavigationView/TestUI/NavigationViewPage.xaml.cs

@@ -351,6 +351,7 @@ private void NavView_ItemInvoked(NavigationView sender, NavigationViewItemInvoke
 
         private void NavView_SelectionChanged(NavigationView sender, NavigationViewSelectionChangedEventArgs args)
         {
+            SelectionChangedRaised.Text = "True";
             // Reset argument type indicatiors
             SelectionChangedItemType.Text = "null";
             SelectionChangedItemContainerType.Text = "null";
@@ -380,6 +381,11 @@ private void NavView_SelectionChanged(NavigationView sender, NavigationViewSelec
             }
         }
 
+        private void ClearSelectionChangeBlock(object sender,RoutedEventArgs e)
+        {
+            SelectionChangedRaised.Text = "False";
+        }
+
         private void MoviesEnabledCheckbox_Checked(object sender, RoutedEventArgs e)
         {
             MoviesItem.IsEnabled = true;