From 6a5bbf0806a8c031639614f5867631c9ab17a053 Mon Sep 17 00:00:00 2001 From: "E.Z. Hart" Date: Wed, 22 Nov 2023 14:54:31 -0700 Subject: [PATCH] Prevent iOS CollectionView size shifts from clearing the cell size cache (#18464) * Prevent iOS CollectionView size shifts from clearing the cell size cache Fixes #17890 * Try to exclude tests from macOS --- .../Handlers/Items/iOS/ItemsViewController.cs | 23 +-- .../Handlers/Items/iOS/ItemsViewLayout.cs | 38 +---- .../Core/Handlers/Items/iOS/SizeExtensions.cs | 42 +++++ .../Core/Handlers/Items/iOS/TemplatedCell.cs | 22 +-- .../CollectionViewCacheTests.ios.cs | 152 ++++++++++++++++++ .../CollectionView/CollectionViewTests.cs | 4 + .../CollectionView/CollectionViewTests.iOS.cs | 2 - 7 files changed, 210 insertions(+), 73 deletions(-) create mode 100644 src/Controls/src/Core/Handlers/Items/iOS/SizeExtensions.cs create mode 100644 src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewCacheTests.ios.cs diff --git a/src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs b/src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs index 222b6e9aa9d6..a52511b47e62 100644 --- a/src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs +++ b/src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs @@ -7,7 +7,6 @@ using Foundation; using Microsoft.Maui.Controls.Internals; using Microsoft.Maui.Graphics; -using ObjCRuntime; using UIKit; namespace Microsoft.Maui.Controls.Handlers.Items @@ -178,12 +177,12 @@ public override void ViewDidLoad() public override void ViewWillAppear(bool animated) { base.ViewWillAppear(animated); - ConstrainToItemsView(); + ConstrainItemsToBounds(); } public override void ViewWillLayoutSubviews() { - ConstrainToItemsView(); + ConstrainItemsToBounds(); base.ViewWillLayoutSubviews(); InvalidateMeasureIfContentSizeChanged(); LayoutEmptyView(); @@ -243,7 +242,7 @@ void InvalidateMeasureIfContentSizeChanged() internal Size? GetSize() { - if (_emptyViewDisplayed) + if (_emptyViewDisplayed) { return _emptyUIView.Frame.Size.ToSize(); } @@ -251,18 +250,11 @@ void InvalidateMeasureIfContentSizeChanged() return CollectionView.CollectionViewLayout.CollectionViewContentSize.ToSize(); } - void ConstrainToItemsView() + void ConstrainItemsToBounds() { - var itemsViewWidth = ItemsView.Width; - var itemsViewHeight = ItemsView.Height; - - if (itemsViewHeight < 0 || itemsViewWidth < 0) - { - ItemsViewLayout.UpdateConstraints(CollectionView.Bounds.Size); - return; - } - - ItemsViewLayout.UpdateConstraints(new CGSize(itemsViewWidth, itemsViewHeight)); + var contentBounds = CollectionView.AdjustedContentInset.InsetRect(CollectionView.Bounds); + var constrainedSize = contentBounds.Size; + ItemsViewLayout.UpdateConstraints(constrainedSize); } void EnsureLayoutInitialized() @@ -321,7 +313,6 @@ public virtual void UpdateFlowDirection() Layout.InvalidateLayout(); } - public override nint NumberOfSections(UICollectionView collectionView) { CheckForEmptySource(); diff --git a/src/Controls/src/Core/Handlers/Items/iOS/ItemsViewLayout.cs b/src/Controls/src/Core/Handlers/Items/iOS/ItemsViewLayout.cs index 59428bed0d62..87911c109ef9 100644 --- a/src/Controls/src/Core/Handlers/Items/iOS/ItemsViewLayout.cs +++ b/src/Controls/src/Core/Handlers/Items/iOS/ItemsViewLayout.cs @@ -5,8 +5,6 @@ using CoreGraphics; using Foundation; using Microsoft.Extensions.Logging; -using Microsoft.Maui.Controls.Internals; -using ObjCRuntime; using UIKit; namespace Microsoft.Maui.Controls.Handlers.Items @@ -21,9 +19,7 @@ public abstract class ItemsViewLayout : UICollectionViewFlowLayout CGSize _currentSize; WeakReference> _getPrototype; - const double ConstraintSizeTolerance = 0.00001; - - Dictionary _cellSizeCache = new Dictionary(); + readonly Dictionary _cellSizeCache = new(); public ItemsUpdatingScrollMode ItemsUpdatingScrollMode { get; set; } @@ -102,7 +98,7 @@ protected virtual void HandlePropertyChanged(PropertyChangedEventArgs propertyCh internal virtual void UpdateConstraints(CGSize size) { - if (!RequiresConstraintUpdate(size, _currentSize)) + if (size.IsCloseTo(_currentSize)) { return; } @@ -568,23 +564,12 @@ static void ForceScrollToLastItem(UICollectionView collectionView, ItemsLayout i public override bool ShouldInvalidateLayoutForBoundsChange(CGRect newBounds) { - if (newBounds.Size == _currentSize) + if(newBounds.Size == _currentSize) { return base.ShouldInvalidateLayoutForBoundsChange(newBounds); } - if (OperatingSystem.IsIOSVersionAtLeast(11) || OperatingSystem.IsMacCatalystVersionAtLeast(11) -#if TVOS - || OperatingSystem.IsTvOSVersionAtLeast(11) -#endif - ) - { - UpdateConstraints(CollectionView.AdjustedContentInset.InsetRect(newBounds).Size); - } - else - { - UpdateConstraints(CollectionView.Bounds.Size); - } + UpdateConstraints(CollectionView.AdjustedContentInset.InsetRect(newBounds).Size); return true; } @@ -610,20 +595,5 @@ internal void ClearCellSizeCache() { _cellSizeCache.Clear(); } - - bool RequiresConstraintUpdate(CGSize newSize, CGSize current) - { - if (Math.Abs(newSize.Width - current.Width) > ConstraintSizeTolerance) - { - return true; - } - - if (Math.Abs(newSize.Height - current.Height) > ConstraintSizeTolerance) - { - return true; - } - - return false; - } } } diff --git a/src/Controls/src/Core/Handlers/Items/iOS/SizeExtensions.cs b/src/Controls/src/Core/Handlers/Items/iOS/SizeExtensions.cs new file mode 100644 index 000000000000..36399c2a7636 --- /dev/null +++ b/src/Controls/src/Core/Handlers/Items/iOS/SizeExtensions.cs @@ -0,0 +1,42 @@ +#nullable disable +using System; +using CoreGraphics; +using Microsoft.Maui.Graphics; + +namespace Microsoft.Maui.Controls.Handlers.Items +{ + internal static class SizeExtensions + { + const double Tolerance = 0.001; + + public static bool IsCloseTo(this CGSize sizeA, CGSize sizeB) + { + if (Math.Abs(sizeA.Height - sizeB.Height) > Tolerance) + { + return false; + } + + if (Math.Abs(sizeA.Width - sizeB.Width) > Tolerance) + { + return false; + } + + return true; + } + + public static bool IsCloseTo(this CGSize sizeA, Size sizeB) + { + if (Math.Abs(sizeA.Height - sizeB.Height) > Tolerance) + { + return false; + } + + if (Math.Abs(sizeA.Width - sizeB.Width) > Tolerance) + { + return false; + } + + return true; + } + } +} diff --git a/src/Controls/src/Core/Handlers/Items/iOS/TemplatedCell.cs b/src/Controls/src/Core/Handlers/Items/iOS/TemplatedCell.cs index 857aa1fb8794..919866f1186b 100644 --- a/src/Controls/src/Core/Handlers/Items/iOS/TemplatedCell.cs +++ b/src/Controls/src/Core/Handlers/Items/iOS/TemplatedCell.cs @@ -5,7 +5,6 @@ using Foundation; using Microsoft.Maui.Controls.Internals; using Microsoft.Maui.Graphics; -using ObjCRuntime; using UIKit; namespace Microsoft.Maui.Controls.Handlers.Items @@ -66,7 +65,7 @@ public override UICollectionViewLayoutAttributes PreferredLayoutAttributesFittin var preferredSize = preferredAttributes.Frame.Size; - if (SizesAreSame(preferredSize, _size) + if (preferredSize.IsCloseTo(_size) && AttributesConsistentWithConstrainedDimension(preferredAttributes)) { return preferredAttributes; @@ -79,8 +78,6 @@ public override UICollectionViewLayoutAttributes PreferredLayoutAttributesFittin OnLayoutAttributesChanged(preferredAttributes); - //_isMeasured = true; - return preferredAttributes; } @@ -290,23 +287,6 @@ protected void OnLayoutAttributesChanged(UICollectionViewLayoutAttributes newAtt protected abstract bool AttributesConsistentWithConstrainedDimension(UICollectionViewLayoutAttributes attributes); - bool SizesAreSame(CGSize preferredSize, Size elementSize) - { - const double tolerance = 0.000001; - - if (Math.Abs(preferredSize.Height - elementSize.Height) > tolerance) - { - return false; - } - - if (Math.Abs(preferredSize.Width - elementSize.Width) > tolerance) - { - return false; - } - - return true; - } - void UpdateVisualStates() { if (PlatformHandler?.VirtualView is VisualElement element) diff --git a/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewCacheTests.ios.cs b/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewCacheTests.ios.cs new file mode 100644 index 000000000000..ba213dd12f44 --- /dev/null +++ b/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewCacheTests.ios.cs @@ -0,0 +1,152 @@ +using System.Collections.Generic; +using System.Threading.Tasks; +using CoreGraphics; +using Foundation; +using Microsoft.Maui.Controls; +using Microsoft.Maui.Controls.Handlers.Items; +using UIKit; +using Xunit; + +namespace Microsoft.Maui.DeviceTests +{ +#if !MACCATALYST + public partial class CollectionViewTests + { + public class CacheTestCollectionView : CollectionView { } + + class CacheTestCollectionViewHandler : ReorderableItemsViewHandler + { + protected override ItemsViewController CreateController(CacheTestCollectionView itemsView, ItemsViewLayout layout) + { + return new CacheTestItemsViewController(itemsView, layout); + } + } + + class CacheTestItemsViewController : ItemsViewController + { + protected override bool IsHorizontal { get; } + + public UICollectionViewDelegateFlowLayout DelegateFlowLayout { get; private set; } + + public CacheTestItemsViewController(CacheTestCollectionView reorderableItemsView, ItemsViewLayout layout) : base(reorderableItemsView, layout) + { + } + + protected override UICollectionViewDelegateFlowLayout CreateDelegator() + { + DelegateFlowLayout = new CacheMissCountingDelegate(ItemsViewLayout, this); + return DelegateFlowLayout; + } + } + + internal class CacheMissCountingDelegate : ItemsViewDelegator> + { + public int CacheMissCount { get; set; } + + public CacheMissCountingDelegate(ItemsViewLayout itemsViewLayout, ItemsViewController itemsViewController) : base(itemsViewLayout, itemsViewController) + { + } + + public override CGSize GetSizeForItem(UICollectionView collectionView, UICollectionViewLayout layout, NSIndexPath indexPath) + { + var itemSize = base.GetSizeForItem(collectionView, layout, indexPath); + + if (ViewController.Layout is UICollectionViewFlowLayout flowLayout) + { + // This is totally a cheat from a unit-testing perspective; we know how the item size cache + // functions internally (that it will return the estimated size if the item size is not found in the cache), + // and we're relying on that for this test. Normally we wouldn't do this, but we need to ensure that further + // code changes don't break the cache again, and the only observable outside consequence is that the scrolling + // for the CollectionView becomes "janky". We don't want to rely entirely on human perception of "jankiness" to + // catch this problem. + + // If the size comes back as EstimatedItemSize, we'll know that the actual value was not found in the cache. + + if (itemSize == flowLayout.EstimatedItemSize) + { + CacheMissCount += 1; + } + } + + return itemSize; + } + } + + internal class ItemModel + { + public int Index { get; set; } + public double HeightRequest => 40 * (Index + 1); + } + + [Fact] + public async Task EnsureCellSizesAreCached() + { + SetupBuilder(); + + var collectionView = new CacheTestCollectionView() + { + // Deliberately choosing a height which will cause rounding issues which can caused pathological + // sizing/layout loops if handled wrong + HeightRequest = 654.66666 + }; + + var template = new DataTemplate(() => { + var content = new Label() { Text = "Howdy" }; + content.SetBinding(VisualElement.HeightRequestProperty, new Binding(nameof(VisualElement.HeightRequest))); + return content; + }); + + // Build up a view model that's got enough items to ensure scrolling and template reuse + int itemCount = 15; + var source = new List(); + for (int n = 0; n < itemCount; n++) + { + source.Add(new ItemModel() { Index = n }); + } + + collectionView.ItemTemplate = template; + collectionView.ItemsSource = source; + + var frame = collectionView.Frame; + + await CreateHandlerAndAddToWindow(collectionView, async handler => + { + // Wait until the CollectionView is actually rendering + await WaitForUIUpdate(frame, collectionView); + + // Tell it to scroll to the bottom (and give it time to do so) + collectionView.ScrollTo(itemCount - 1); + await Task.Delay(1000); + + // Now back to the top + collectionView.ScrollTo(0); + await Task.Delay(1000); + + if (handler.Controller is CacheTestItemsViewController controller + && controller.DelegateFlowLayout is CacheMissCountingDelegate cacheMissCounter) + { + // Different screen sizes and timings mean this isn't 100% predictable. But we can work out some conditions + // which will tell us that the cache is completely broken and test for those. + + // With 15 items in the list, we can assume a minimum of 15 size cache misses until the cache is populated. + // Plus at least one for the initial proxy item measurement. + + // The bugs we are trying to avoid clear the cache prematurely and cause _every_ GetSizeForItem call + // to miss the cache, so scrolling top to bottom and back with 15 items we would see at _least_ 30 + // cache misses, likely more (since item sizes will be retrieved more than once as items scroll). + + // If we have fewer than 20 cache misses, we at least know that the cache isn't being wiped as we scroll. + int missCountThreshold = 20; + int actualMissCount = cacheMissCounter.CacheMissCount; + Assert.True(actualMissCount < missCountThreshold, $"Cache miss count {actualMissCount} was higher than the threshold value of {missCountThreshold}"); + } + else + { + // Something went wrong with this test + Assert.Fail("Wrong controller type in the test; is the handler registration broken?"); + } + }); + } + } +#endif +} diff --git a/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.cs b/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.cs index 7dae9210f4b6..08af68584c2b 100644 --- a/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.cs +++ b/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.cs @@ -34,6 +34,10 @@ void SetupBuilder() handlers.AddHandler(); handlers.AddHandler(); handlers.AddHandler(); + +#if IOS && !MACCATALYST + handlers.AddHandler(); +#endif }); }); } diff --git a/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs b/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs index 932f0bdd9bfc..6a2eced0a556 100644 --- a/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs +++ b/src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs @@ -1,9 +1,7 @@ using System; using System.Collections.Generic; using System.Collections.ObjectModel; -using System.Reflection; using System.Threading.Tasks; -using System.Windows.Input; using CoreGraphics; using Microsoft.Maui.Controls; using Microsoft.Maui.Controls.Handlers.Items;