NavigationView: Fix SelectedItem not being unset when no NavigationVi…

…ewItem is selected (#2329) * Fix SelectedItem not being unset when no NavigationViewItem is selected * Update comment * Add API test for the SelectionModel * Comment formatting Co-Authored-By: Marcel Wagner <[email protected]> * Comment formatting (2) * Update SelectionModel API test Co-authored-by: Marcel Wagner <[email protected]>
microsoft · Apr 28, 2020 · 9fd0861 · 9fd0861
1 parent 41c7fa3
commit 9fd0861
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 3 deletions.
diff --git a/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs b/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs
@@ -653,5 +653,37 @@ public void VerifyMenuItemAndContainerMappingMenuItems()
                 MUXControlsTestApp.App.TestContentRoot = null;
             });
         }
+
+        [TestMethod]
+        public void VerifySelectedItemIsNullWhenNoItemIsSelected()
+        {
+            RunOnUIThread.Execute(() =>
+            {
+                var navView = new NavigationView();
+                Content = navView;
+
+                var menuItem1 = new NavigationViewItem();
+                menuItem1.Content = "Item 1";
+
+                navView.MenuItems.Add(menuItem1);
+                navView.Width = 1008; // forces the control into Expanded mode so that the menu renders
+                Content.UpdateLayout();
+
+                Verify.IsFalse(menuItem1.IsSelected);
+                Verify.AreEqual(null, navView.SelectedItem);
+
+                menuItem1.IsSelected = true;
+                Content.UpdateLayout();
+
+                Verify.IsTrue(menuItem1.IsSelected);
+                Verify.AreEqual(menuItem1, navView.SelectedItem);
+
+                menuItem1.IsSelected = false;
+                Content.UpdateLayout();
+
+                Verify.IsFalse(menuItem1.IsSelected);
+                Verify.AreEqual(null, navView.SelectedItem, "SelectedItem should have been [null] as no item is selected");
+            });
+        }
     }
 }
diff --git a/dev/Repeater/APITests/SelectionModelTests.cs b/dev/Repeater/APITests/SelectionModelTests.cs
@@ -53,15 +53,56 @@ public void ValidateOneLevelSingleSelection()
                 SelectionModel selectionModel = new SelectionModel() { SingleSelect = true };
                 Log.Comment("Set the source to 10 items");
                 selectionModel.Source = Enumerable.Range(0, 10).ToList();
+
+                // Check index selection
                 Select(selectionModel, 3, true);
                 ValidateSelection(selectionModel, new List<IndexPath>() { Path(3) }, new List<IndexPath>() { Path() });
                 Select(selectionModel, 3, false);
                 ValidateSelection(selectionModel, new List<IndexPath>() { });
+
+                // Check index path selection
+                Select(selectionModel, Path(4), true);
+                ValidateSelection(selectionModel, new List<IndexPath>() { Path(4) }, new List<IndexPath>() { Path() });
+                Select(selectionModel, Path(4), false);
+                ValidateSelection(selectionModel, new List<IndexPath>() { });
+            });
+        }
+
+        [TestMethod]
+        public void ValidateSelectionChangedEventSingleSelection()
+        {
+            RunOnUIThread.Execute(() =>
+            {
+                SelectionModel selectionModel = new SelectionModel() { SingleSelect = true };
+                selectionModel.Source = Enumerable.Range(0, 10).ToList();
+
+                bool select = true;
+                int selectionChangedFiredCount = 0;
+                selectionModel.SelectionChanged += delegate (SelectionModel sender, SelectionModelSelectionChangedEventArgs args) {
+                    selectionChangedFiredCount++;
+
+                    // Verify SelectionChanged was raised after selection state was changed in the SelectionModel
+                    if (select)
+                    {
+                        ValidateSelection(selectionModel, new List<IndexPath>() { Path(4) }, new List<IndexPath>() { Path() });
+                    }
+                    else
+                    {
+                        ValidateSelection(selectionModel, new List<IndexPath>() { });
+                    }
+                };
+
+                Select(selectionModel, Path(4), select);
+                Verify.AreEqual(1, selectionChangedFiredCount);
+
+                select = false;
+                Select(selectionModel, Path(4), select);
+                Verify.AreEqual(2, selectionChangedFiredCount);
             });
         }
 
         [TestMethod]
-        public void ValidateSelectionChangedEvent()
+        public void ValidateSelectionChangedEventMultipleSelection()
         {
             RunOnUIThread.Execute(() =>
             {
@@ -72,11 +113,12 @@ public void ValidateSelectionChangedEvent()
                 selectionModel.SelectionChanged += delegate (SelectionModel sender, SelectionModelSelectionChangedEventArgs args) 
                 {
                     selectionChangedFiredCount++;
+
+                    // Verify SelectionChanged was raised after selection state was changed in the SelectionModel
                     ValidateSelection(selectionModel, new List<IndexPath>() { Path(4) }, new List<IndexPath>() { Path() });
                 };
 
                 Select(selectionModel, 4, true);
-                ValidateSelection(selectionModel, new List<IndexPath>() { Path(4) }, new List<IndexPath>() { Path() });
                 Verify.AreEqual(1, selectionChangedFiredCount);
             });
         }

diff --git a/dev/Repeater/SelectionModel.cpp b/dev/Repeater/SelectionModel.cpp
@@ -647,7 +647,13 @@ void SelectionModel::SelectWithPathImpl(const winrt::IndexPath& index, bool sele
         // If we unselect something, raise event any way, otherwise changedSelection is false
         bool changedSelection = false;
 
-        if (m_singleSelect)
+        // We only need to clear selection by walking the data structure from the beginning when:
+        // - we are in single selection mode and 
+        // - want to select something.
+        // 
+        // If we want to unselect something we unselect it directly in TraverseIndexPath below and raise the SelectionChanged event
+        // if required.
+        if (m_singleSelect && select)
         {
             ClearSelection(true /*resetAnchor*/, false /* raiseSelectionChanged */);
         }