Clear DataContext of cleared elements in ItemsRepeater (#2626)

* Add clearing of datacontext upon element clearing * Update comment * CR feedback * Add unit test * Update order and clean up test
microsoft · Jun 12, 2020 · e9bd88e · e9bd88e
1 parent 1b42ebb
commit e9bd88e
Show file tree

Hide file tree

Showing 3 changed files with 72 additions and 4 deletions.
diff --git a/dev/Repeater/APITests/ViewManagerTests.cs b/dev/Repeater/APITests/ViewManagerTests.cs
@@ -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.
 
 using Windows.UI.Xaml.Tests.MUXControls.ApiTests.RepeaterTests.Common;
@@ -759,6 +759,48 @@ public void ValidateFocusMoveOnElementClearedWithUniqueIds()
                     () => { ValidateCurrentFocus(repeater, 0 /*expectedIndex */, "3" /* expectedContent */); }
                 });
         }
+
+        [TestMethod]
+        // Why does this test work?
+        // When the elements get created from the RecyclingElementFactory, we get already "existing" data templates.
+        // However, the reason for the crash in #2384 is that those "empty" data templates actually still had their data context
+        // If that data context is not null, that means it did not get cleared when the element was recycled, which is the wrong behavior.
+        // To check if the clearing is working correctly, we are checking this inside the ElementFactory's RecycleElement function.
+        public void ValidateElementClearingClearsDataContext()
+        {
+            ItemsRepeater repeater = null;
+            MockElementFactory elementFactory = null;
+            int elementClearingRaisedCount = 0;
+            Log.Comment("Initialize ItemsRepeater");
+            RunOnUIThread.Execute(() =>
+            {
+                elementFactory = new MockElementFactory() {
+                    GetElementFunc = delegate (int index, UIElement owner) {
+                        return new Button() { Content = index };
+                           },
+
+                    ClearElementFunc = delegate (UIElement element, UIElement owner) {
+                        elementClearingRaisedCount++;
+                        Verify.IsNull((element as FrameworkElement).DataContext);
+                    }
+                };
+
+                repeater = CreateRepeater(Enumerable.Range(0, 100),
+                    elementFactory);
+
+                repeater.Layout = new StackLayout();
+
+                Content = repeater;
+                repeater.UpdateLayout();
+
+                repeater.ItemsSource = null;
+
+                Log.Comment("Verify ItemsRepeater cleared data contexts correctly");
+                Verify.IsTrue(elementClearingRaisedCount > 0, "ItemsRepeater should have cleared some elements");
+            });
+        }
+
+
         private void MoveFocusToIndex(ItemsRepeater repeater, int index)
         {
             var element = repeater.TryGetElement(index) as Control;

diff --git a/dev/Repeater/ViewManager.cpp b/dev/Repeater/ViewManager.cpp
@@ -97,10 +97,31 @@ void ViewManager::ClearElement(const winrt::UIElement& element, bool isClearedDu
     }
 }
 
+// We need to clear the datacontext to prevent crashes from happening,
+//  however we only do that if we were the ones setting it.
+// That is when one of the following is the case (numbering taken from line ~642):
+// 1.2    No ItemTemplate, data is not a UIElement
+// 2.1    ItemTemplate, data is not FrameworkElement
+// 2.2.2  Itemtemplate, data is FrameworkElement, ElementFactory returned Element different to data
+//
+// In all of those three cases, we the ItemTemplateShim is NOT null.
+// Luckily when we create the items, we store whether we were the once setting the DataContext.
 void ViewManager::ClearElementToElementFactory(const winrt::UIElement& element)
 {
     m_owner->OnElementClearing(element);
 
+    auto virtInfo = ItemsRepeater::GetVirtualizationInfo(element);
+    virtInfo->MoveOwnershipToElementFactory();
+
+    // During creation of this object, we were the one setting the DataContext, so clear it now.
+    if (virtInfo->MustClearDataContext())
+    {
+        if (const auto elementAsFE = element.try_as<winrt::FrameworkElement>())
+        {
+            elementAsFE.DataContext(nullptr);
+        }
+    }
+
     if (m_owner->ItemTemplateShim())
     {
         if (!m_ElementFactoryRecycleArgs)
@@ -132,8 +153,6 @@ void ViewManager::ClearElementToElementFactory(const winrt::UIElement& element)
         children.RemoveAt(childIndex);
     }
 
-    auto virtInfo = ItemsRepeater::GetVirtualizationInfo(element);
-    virtInfo->MoveOwnershipToElementFactory();
     m_phaser.StopPhasing(element, virtInfo);
     if (m_lastFocusedElement == element)
     {
@@ -693,6 +712,8 @@ winrt::UIElement ViewManager::GetElementFromElementFactory(int index)
         // which means that the element has been recycled and not created from scratch.
         REPEATER_TRACE_PERF(L"ElementRecycled");
     }
+    // Clear flag
+    virtInfo->MustClearDataContext(false);
 
     if (data != element)
     {
@@ -731,6 +752,7 @@ winrt::UIElement ViewManager::GetElementFromElementFactory(int index)
             }();
 
             elementAsFE.DataContext(elementDataContext);
+            virtInfo->MustClearDataContext(true);
         }
         else
         {

diff --git a/dev/Repeater/VirtualizationInfo.h b/dev/Repeater/VirtualizationInfo.h
@@ -48,6 +48,9 @@ class VirtualizationInfo : public winrt::implements<VirtualizationInfo, winrt::I
     winrt::IInspectable Data() const { return m_data.get(); }
     winrt::IDataTemplateComponent DataTemplateComponent() const { return m_dataTemplateComponent.get(); }
 
+    bool MustClearDataContext() const { return m_mustClearDataContext; }
+    void MustClearDataContext(bool mustClearDataContext) { m_mustClearDataContext = mustClearDataContext; }
+
     static constexpr int PhaseNotSpecified = std::numeric_limits<int>::min();
     static constexpr int PhaseReachedEnd = -1;
 
@@ -90,7 +93,8 @@ class VirtualizationInfo : public winrt::implements<VirtualizationInfo, winrt::I
     int m_phase{ PhaseNotSpecified };
     bool m_keepAlive{ false };
     bool m_autoRecycleCandidate{ false };
+    bool m_mustClearDataContext{ false };
 
     weak_ref<winrt::IInspectable> m_data;
     weak_ref<winrt::IDataTemplateComponent> m_dataTemplateComponent;
-};
+};