Clear DataContext of cleared elements in ItemsRepeater (microsoft/mic…

…rosoft-ui-xaml#2626)
Kinnara · Jun 14, 2020 · 5ebdc90 · 5ebdc90
1 parent 55f8b51
commit 5ebdc90
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 2 deletions.
diff --git a/ModernWpf.Controls/Repeater/ItemsRepeater/ViewManager.cs b/ModernWpf.Controls/Repeater/ItemsRepeater/ViewManager.cs
@@ -98,10 +98,31 @@ public void ClearElement(UIElement element, bool isClearedDueToCollectionChange)
             }
         }
 
+        // 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.
         public void ClearElementToElementFactory(UIElement element)
         {
             m_owner.OnElementClearing(element);
 
+            var 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 (element is FrameworkElement elementAsFE)
+                {
+                    elementAsFE.DataContext = null;
+                }
+            }
+
             if (m_owner.ItemTemplateShim != null)
             {
                 if (m_ElementFactoryRecycleArgs == null)
@@ -133,8 +154,6 @@ public void ClearElementToElementFactory(UIElement element)
                 children.RemoveAt(childIndex);
             }
 
-            var virtInfo = ItemsRepeater.GetVirtualizationInfo(element);
-            virtInfo.MoveOwnershipToElementFactory();
             if (m_lastFocusedElement == element)
             {
                 // Focused element is going away. Remove the tracked last focused element
@@ -575,6 +594,8 @@ ElementFactoryGetArgs initArgs()
                 // View obtained from ElementFactory already has a VirtualizationInfo attached to it
                 // which means that the element has been recycled and not created from scratch.
             }
+            // Clear flag
+            virtInfo.MustClearDataContext = false;
 
             if (data != element)
             {
@@ -599,6 +620,7 @@ object initElementDataContext()
                     var elementDataContext = initElementDataContext();
 
                     elementAsFE.DataContext = elementDataContext;
+                    virtInfo.MustClearDataContext = true;
                 }
                 else
                 {

diff --git a/ModernWpf.Controls/Repeater/Layouts/VirtualizationInfo.cs b/ModernWpf.Controls/Repeater/Layouts/VirtualizationInfo.cs
@@ -50,6 +50,8 @@ public VirtualizationInfo()
 
         public bool IsInUniqueIdResetPool => Owner == ElementOwner.UniqueIdResetPool;
 
+        public bool MustClearDataContext { get; set; }
+
         public void MoveOwnershipToLayoutFromElementFactory(int index, string uniqueId)
         {
             Debug.Assert(Owner == ElementOwner.ElementFactory);

diff --git a/test/ModernWpfTestApp/ApiTests/RepeaterTests/ViewManagerTests.cs b/test/ModernWpfTestApp/ApiTests/RepeaterTests/ViewManagerTests.cs
@@ -760,6 +760,47 @@ 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;