Extend the fix for Recycling the focused element to non virtualized l…

…ayouts (#1724) * Ensure that we set the m_processingItesmSourceChange flag for non-virtualizing layouts as well as virtualizing ones. * update the corresponding test * Fix the test
microsoft · Dec 11, 2019 · 99e9caa · 99e9caa
1 parent ea9292e
commit 99e9caa
Show file tree

Hide file tree

Showing 2 changed files with 84 additions and 74 deletions.
diff --git a/dev/Repeater/APITests/RepeaterTests.cs b/dev/Repeater/APITests/RepeaterTests.cs
@@ -20,6 +20,7 @@
 #endif
 
 using ItemsRepeater = Microsoft.UI.Xaml.Controls.ItemsRepeater;
+using Layout = Microsoft.UI.Xaml.Controls.Layout;
 using ItemsSourceView = Microsoft.UI.Xaml.Controls.ItemsSourceView;
 using RecyclingElementFactory = Microsoft.UI.Xaml.Controls.RecyclingElementFactory;
 using RecyclePool = Microsoft.UI.Xaml.Controls.RecyclePool;
@@ -267,52 +268,63 @@ public void NestedRepeaterWithDataTemplateScenario()
         [TestMethod]
         public void VerifyFocusedItemIsRecycledOnCollectionReset()
         {
-            List<string> items = new List<string> { "item0", "item1", "item2", "item3", "item4", "item5", "item6", "item7", "item8", "item9" };
-            ItemsRepeater repeater = null;
-            const int targetIndex = 4;
-            string targetItem = items[targetIndex]; 
-
+            List<Layout> layouts = new List<Layout>();
             RunOnUIThread.Execute(() =>
             {
-                repeater = new ItemsRepeater() {
-                    ItemsSource = items,
-                    ItemTemplate = CreateDataTemplateWithContent(@"<Button Content='{Binding}'/>")
-                };
-                Content = repeater;
+                layouts.Add(new MyCustomNonVirtualizingStackLayout());
+                layouts.Add(new StackLayout());
             });
-
-            IdleSynchronizer.Wait();
 
-            RunOnUIThread.Execute(() =>
+            foreach (var layout in layouts)
             {
-                Log.Comment("Setting Focus on item " + targetIndex);
-                Button toFocus = (Button)repeater.TryGetElement(targetIndex);
-                Verify.AreEqual(targetItem, toFocus.Content as string);
-                toFocus.Focus(FocusState.Keyboard);
-            });
+                List<string> items = new List<string> { "item0", "item1", "item2", "item3", "item4", "item5", "item6", "item7", "item8", "item9" };
+                const int targetIndex = 4;
+                string targetItem = items[targetIndex];
+                ItemsRepeater repeater = null;
 
-            IdleSynchronizer.Wait();
+                RunOnUIThread.Execute(() =>
+                {
+                    repeater = new ItemsRepeater() {
+                        ItemsSource = items,
+                        ItemTemplate = CreateDataTemplateWithContent(@"<Button Content='{Binding}'/>"),
+                        Layout = layout
+                    };
+                    Content = repeater;
+                });
 
-            RunOnUIThread.Execute(() =>
-            { 
-                Log.Comment("Removing focused element from collection");
-                items.Remove(targetItem);
+                IdleSynchronizer.Wait();
 
-                Log.Comment("Reset the collection with an empty list");
-                repeater.ItemsSource = new List<string>() ;
-            });
+                RunOnUIThread.Execute(() =>
+                {
+                    Log.Comment("Setting Focus on item " + targetIndex);
+                    Button toFocus = (Button)repeater.TryGetElement(targetIndex);
+                    Verify.AreEqual(targetItem, toFocus.Content as string);
+                    toFocus.Focus(FocusState.Keyboard);
+                });
 
-            IdleSynchronizer.Wait();
+                IdleSynchronizer.Wait();
 
-            RunOnUIThread.Execute(() => 
-            { 
-                Log.Comment("Verify new elements");
-                for (int i = 0; i < items.Count; i++)
+                RunOnUIThread.Execute(() =>
                 {
-                    Button currentButton = (Button)repeater.TryGetElement(i);
-                    Verify.IsNull(currentButton);
-                }
-            });
+                    Log.Comment("Removing focused element from collection");
+                    items.Remove(targetItem);
+
+                    Log.Comment("Reset the collection with an empty list");
+                    repeater.ItemsSource = new List<string>();
+                });
+
+                IdleSynchronizer.Wait();
+
+                RunOnUIThread.Execute(() =>
+                {
+                    Log.Comment("Verify new elements");
+                    for (int i = 0; i < items.Count; i++)
+                    {
+                        Button currentButton = (Button)repeater.TryGetElement(i);
+                        Verify.IsNull(currentButton);
+                    }
+                });
+            }
         }
 
         private DataTemplate CreateDataTemplateWithContent(string content)

diff --git a/dev/Repeater/ItemsRepeater.cpp b/dev/Repeater/ItemsRepeater.cpp
@@ -516,26 +516,26 @@ void ItemsRepeater::OnDataSourcePropertyChanged(const winrt::ItemsSourceView& ol
         m_itemsSourceViewChanged = newValue.CollectionChanged(winrt::auto_revoke, { this, &ItemsRepeater::OnItemsSourceViewChanged });
     }
 
-    if (auto layout = Layout())
-    {
-        if (auto virtualLayout = layout.try_as<winrt::VirtualizingLayout>())
-        {
-            auto args = winrt::NotifyCollectionChangedEventArgs(
-                winrt::NotifyCollectionChangedAction::Reset,
-                nullptr /* newItems */,
-                nullptr /* oldItems */,
-                -1 /* newIndex */,
-                -1 /* oldIndex */);
-            args.Action();
-            m_processingItemsSourceChange.set(args);
-            auto processingChange = gsl::finally([this]()
-                {
-                    m_processingItemsSourceChange.set(nullptr);
-                });
+    if (auto const layout = Layout())
+    {
+        auto const args = winrt::NotifyCollectionChangedEventArgs(
+            winrt::NotifyCollectionChangedAction::Reset,
+            nullptr /* newItems */,
+            nullptr /* oldItems */,
+            -1 /* newIndex */,
+            -1 /* oldIndex */);
+        args.Action();
+        auto const processingChange = gsl::finally([this]()
+            {
+                m_processingItemsSourceChange.set(nullptr);
+            });
+        m_processingItemsSourceChange.set(args);
 
+        if (auto const virtualLayout = layout.try_as<winrt::VirtualizingLayout>())
+        {
             virtualLayout.OnItemsChangedCore(GetLayoutContext(), newValue, args);
         }
-        else if (auto nonVirtualLayout = layout.try_as<winrt::NonVirtualizingLayout>())
+        else if (auto const nonVirtualLayout = layout.try_as<winrt::NonVirtualizingLayout>())
         {
             // Walk through all the elements and make sure they are cleared for
             // non-virtualizing layouts.
@@ -565,36 +565,34 @@ void ItemsRepeater::OnItemTemplateChanged(const winrt::IElementFactory& oldValue
     // have already been created and are now in the tree. The easiest way to do that
     // would be to do a reset.. Note that this has to be done before we change the template
     // so that the cleared elements go back into the old template.
-    if (auto layout = Layout())
-    {
-        if (auto virtualLayout = layout.try_as<winrt::VirtualizingLayout>())
-        {
-            auto args = winrt::NotifyCollectionChangedEventArgs(
-                winrt::NotifyCollectionChangedAction::Reset,
-                nullptr /* newItems */,
-                nullptr /* oldItems */,
-                -1 /* newIndex */,
-                -1 /* oldIndex */);
-            args.Action();
-            m_processingItemsSourceChange.set(args);
-            auto processingChange = gsl::finally([this]()
-                {
-                    m_processingItemsSourceChange.set(nullptr);
-                });
+    if (auto const layout = Layout())
+    {
+        auto const args = winrt::NotifyCollectionChangedEventArgs(
+            winrt::NotifyCollectionChangedAction::Reset,
+            nullptr /* newItems */,
+            nullptr /* oldItems */,
+            -1 /* newIndex */,
+            -1 /* oldIndex */);
+        args.Action();
+        auto const processingChange = gsl::finally([this]()
+            {
+                m_processingItemsSourceChange.set(nullptr);
+            });
+        m_processingItemsSourceChange.set(args);
 
+        if (auto const virtualLayout = layout.try_as<winrt::VirtualizingLayout>())
+        {
             virtualLayout.OnItemsChangedCore(GetLayoutContext(), newValue, args);
         }
-        else if (auto nonVirtualLayout = layout.try_as<winrt::NonVirtualizingLayout>())
+        else if (auto const nonVirtualLayout = layout.try_as<winrt::NonVirtualizingLayout>())
         {
             // Walk through all the elements and make sure they are cleared for
             // non-virtualizing layouts.
-            auto children = Children();
-            for (unsigned i = 0u; i < children.Size(); ++i)
+            for (auto const& child : Children())
             {
-                auto element = children.GetAt(i);
-                if (GetVirtualizationInfo(element)->IsRealized())
+                if (GetVirtualizationInfo(child)->IsRealized())
                 {
-                    ClearElementImpl(element);
+                    ClearElementImpl(child);
                 }
             }
         }