From 9eb316105f1cde829a776c445b6f389d55cdcddf Mon Sep 17 00:00:00 2001 From: "E.Z. Hart" Date: Mon, 17 Jun 2019 15:07:32 -0600 Subject: [PATCH] [iOS] Fix missing partial columns in horizontal grid layout (#6241) * Create repro * Work around UICollectionViewFlowLayout bug to display partial columns; fixes #6077 Fix content inset behaviors when dealing with safe area on newer iOS versions * Add check for case where there is only a single partial column * Works better if I don't forget the return statement * Split up tests to make it easier to determine what failed * Fix bounds exception when changing to a smaller ItemsSource * Add preserve attributes so the linker won't ruin my UI test * Fix missing using after rebase --- .../Issue6077.cs | 139 ++++++++++++++++++ ...rin.Forms.Controls.Issues.Shared.projitems | 1 + .../Tests/CollectionViewUITests.cs | 27 ++-- .../CollectionView/GridViewLayout.cs | 106 ++++++++++++- .../CollectionView/ItemsViewLayout.cs | 9 ++ .../CollectionView/ItemsViewRenderer.cs | 10 ++ 6 files changed, 278 insertions(+), 14 deletions(-) create mode 100644 Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue6077.cs diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue6077.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue6077.cs new file mode 100644 index 00000000000..9d11195217a --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue6077.cs @@ -0,0 +1,139 @@ +using System; +using System.Collections.Generic; +using System.Collections.ObjectModel; +using System.ComponentModel; +using System.Runtime.CompilerServices; +using Xamarin.Forms.CustomAttributes; +using Xamarin.Forms.Internals; +using Xamarin.Forms.PlatformConfiguration; +using Xamarin.Forms.PlatformConfiguration.iOSSpecific; + +#if UITEST +using Xamarin.Forms.Core.UITests; +using Xamarin.UITest; +using NUnit.Framework; +#endif + +namespace Xamarin.Forms.Controls.Issues +{ +#if UITEST + [NUnit.Framework.Category(UITestCategories.CollectionView)] +#endif + [Preserve(AllMembers = true)] + [Issue(IssueTracker.Github, 6077, "CollectionView (iOS) using horizontal grid does not display last column of uneven item count", PlatformAffected.iOS)] + public class Issue6077 : TestNavigationPage + { + [Preserve(AllMembers = true)] + public class MainViewModel : INotifyPropertyChanged + { + readonly IList _items; + public ObservableCollection Items { get; private set; } + + public MainViewModel() + { + _items = new List(); + CreateItemsCollection(); + } + + void CreateItemsCollection(int items = 5) + { + for (int n = 0; n < items; n++) + { + _items.Add(new ItemModel + { + Title = $"Item {n + 1}", + }); + } + + Items = new ObservableCollection(_items); + } + + protected bool SetProperty(ref T backingStore, T value, + [CallerMemberName]string propertyName = "", + Action onChanged = null) + { + if (EqualityComparer.Default.Equals(backingStore, value)) + return false; + + backingStore = value; + onChanged?.Invoke(); + OnPropertyChanged(propertyName); + return true; + } + + #region INotifyPropertyChanged + public event PropertyChangedEventHandler PropertyChanged; + protected void OnPropertyChanged([CallerMemberName] string propertyName = "") + { + var changed = PropertyChanged; + if (changed == null) + return; + + changed.Invoke(this, new PropertyChangedEventArgs(propertyName)); + } + #endregion + } + + [Preserve(AllMembers = true)] + public class ItemModel + { + public string Title { get; set; } + } + + ContentPage CreateRoot() + { + var page = new ContentPage { Title = "Issue6077" }; + + var cv = new CollectionView { ItemSizingStrategy = ItemSizingStrategy.MeasureAllItems }; + + var itemsLayout = new GridItemsLayout(3, ItemsLayoutOrientation.Horizontal); + + + cv.ItemsLayout = itemsLayout; + + var template = new DataTemplate(() => { + var grid = new Grid { HeightRequest = 100, WidthRequest = 50, BackgroundColor = Color.AliceBlue }; + + grid.RowDefinitions = new RowDefinitionCollection { new RowDefinition { Height = new GridLength(100) } }; + grid.ColumnDefinitions = new ColumnDefinitionCollection { new ColumnDefinition { Width = new GridLength(50) } }; + + var label = new Label { }; + + label.SetBinding(Label.TextProperty, new Binding("Title")); + + var content = new ContentView { Content = label }; + + grid.Children.Add(content); + + return grid; + }); + + cv.ItemTemplate = template; + cv.SetBinding(ItemsView.ItemsSourceProperty, new Binding("Items")); + + page.Content = cv; + + BindingContext = new MainViewModel(); + + return page; + } + + protected override void Init() + { +#if APP + Device.SetFlags(new List(Device.Flags ?? new List()) { "CollectionView_Experimental" }); + + PushAsync(CreateRoot()); +#endif + } + +#if UITEST + [Test] + public void LastColumnShouldBeVisible() + { + // If the partial column shows up, then Item 5 will be in it + RunningApp.WaitForElement("Item 5"); + } +#endif + } +} diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems index b768f3d93a6..ff68fde6904 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems @@ -964,6 +964,7 @@ + diff --git a/Xamarin.Forms.Core.UITests.Shared/Tests/CollectionViewUITests.cs b/Xamarin.Forms.Core.UITests.Shared/Tests/CollectionViewUITests.cs index 1a9a2792b95..271b3fea73c 100644 --- a/Xamarin.Forms.Core.UITests.Shared/Tests/CollectionViewUITests.cs +++ b/Xamarin.Forms.Core.UITests.Shared/Tests/CollectionViewUITests.cs @@ -49,21 +49,24 @@ protected override void TestTearDown() // "ScrollToItemCode,HorizontalList", "ScrollToItemCode,VerticalList", "ScrollToItemCode,HorizontalGrid", "ScrollToItemCode,VerticalGrid", // }, 19, 3)] //[TestCase("Snap Points", new string[] { "SnapPointsCode,HorizontalList", "SnapPointsCode,VerticalList", "SnapPointsCode,HorizontalGrid", "SnapPointsCode,VerticalGrid" }, 19, 2)] - [TestCase("Observable Collection", new string[] { "Add/RemoveItemsList", "Add/RemoveItemsGrid" }, 19, 6)] - [TestCase("Default Text", new string[] { "VerticalListCode", "HorizontalListCode", "VerticalGridCode", "HorizontalGridCode" }, 101, 11)] - [TestCase("DataTemplate", new string[] { "VerticalListCode", "HorizontalListCode", "VerticalGridCode", "HorizontalGridCode" }, 19, 6)] - public void VisitAndUpdateItemsSource(string collectionTestName, string[] subGalleries, int firstItem, int lastItem) + [TestCase("Observable Collection", "Add/RemoveItemsList", 19, 6)] + [TestCase("Observable Collection", "Add/RemoveItemsGrid", 19, 6)] + + [TestCase("Default Text", "VerticalListCode", 101, 11)] + [TestCase("Default Text", "HorizontalListCode", 101, 11)] + [TestCase("Default Text", "VerticalGridCode", 101, 11)] + [TestCase("Default Text", "HorizontalGridCode", 101, 11)] + + [TestCase("DataTemplate", "VerticalListCode", 19, 6)] + [TestCase("DataTemplate", "HorizontalListCode", 19, 6)] + [TestCase("DataTemplate", "VerticalGridCode", 19, 6)] + [TestCase("DataTemplate", "HorizontalGridCode", 19, 6)] + public void VisitAndUpdateItemsSource(string collectionTestName, string subGallery, int firstItem, int lastItem) { VisitInitialGallery(collectionTestName); - foreach (var gallery in subGalleries) - { - if (gallery == "FilterItems") - continue; - - VisitSubGallery(gallery, !gallery.Contains("Horizontal"), $"Item: {firstItem}", $"Item: {lastItem}", lastItem - 1, true, false); - App.NavigateBack(); - } + VisitSubGallery(subGallery, !subGallery.Contains("Horizontal"), $"Item: {firstItem}", $"Item: {lastItem}", lastItem - 1, true, false); + App.NavigateBack(); } //[TestCase("ScrollTo", new string[] { diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/GridViewLayout.cs b/Xamarin.Forms.Platform.iOS/CollectionView/GridViewLayout.cs index 263c9cc3822..4a892c756f0 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/GridViewLayout.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/GridViewLayout.cs @@ -1,7 +1,7 @@ using System; using System.ComponentModel; -using System.Diagnostics; using CoreGraphics; +using Foundation; using UIKit; namespace Xamarin.Forms.Platform.iOS @@ -43,7 +43,7 @@ public override void ConstrainTo(CGSize size) ConstrainedDimension = (availableSpace - spacing) / _itemsLayout.Span; - // TODO hartez 2018/09/12 14:52:24 We need to truncate the decimal part of ConstrainedDimension + // We need to truncate the decimal part of ConstrainedDimension // or we occasionally run into situations where the rows/columns don't fit // But this can run into situations where we have an extra gap because we're cutting off too much // and we have a small gap; need to determine where the cut-off is that leads to layout dropping a whole row/column @@ -58,5 +58,107 @@ public override void ConstrainTo(CGSize size) ConstrainedDimension = (int)ConstrainedDimension; DetermineCellSize(); } + + /* `CollectionViewContentSize` and `LayoutAttributesForElementsInRect` are overridden here to work around what + * appears to be a bug in the UICollectionViewFlowLayout implementation: for horizontally scrolling grid + * layouts with auto-sized cells, trailing items which don't fill out a column are never displayed. + * For example, with a span of 3 and either 4 or 5 items, the resulting layout looks like + * + * Item1 + * Item2 + * Item3 + * + * But with 6 items, it looks like + * + * Item1 Item4 + * Item2 Item5 + * Item3 Item6 + * + * IOW, if there are not enough items to fill out the last column, the last column is ignored. + * + * These overrides detect and correct that situation. + */ + + public override CGSize CollectionViewContentSize + { + get + { + if (!NeedsPartialColumnAdjustment()) + { + return base.CollectionViewContentSize; + } + + var contentSize = base.CollectionViewContentSize; + + // Add space for the missing column at the end + var correctedSize = new CGSize(contentSize.Width + EstimatedItemSize.Width, contentSize.Height); + + return correctedSize; + } + } + + public override UICollectionViewLayoutAttributes[] LayoutAttributesForElementsInRect(CGRect rect) + { + if (!NeedsPartialColumnAdjustment()) + { + return base.LayoutAttributesForElementsInRect(rect); + } + + // When we implement Groups, we'll have to iterate over all of them to adjust and this will + // be a lot more complicated. But until then, we only have to worry about section 0 + int section = 0; + + var fullColumns = base.LayoutAttributesForElementsInRect(rect); + + var itemCount = CollectionView.NumberOfItemsInSection(section); + + if (fullColumns.Length == itemCount) + { + return fullColumns; + } + + var missingCellCount = itemCount % _itemsLayout.Span; + + UICollectionViewLayoutAttributes[] allCells = new UICollectionViewLayoutAttributes[fullColumns.Length + missingCellCount]; + fullColumns.CopyTo(allCells, 0); + + for (int n = fullColumns.Length; n < allCells.Length; n++) + { + allCells[n] = LayoutAttributesForItem(NSIndexPath.FromItemSection(n, section)); + } + + return allCells; + } + + bool NeedsPartialColumnAdjustment(int section = 0) + { + if (ScrollDirection == UICollectionViewScrollDirection.Vertical) + { + // The bug only occurs with Horizontal scrolling + return false; + } + + if (EstimatedItemSize.IsEmpty) + { + // The bug only occurs when using Autolayout; with a set ItemSize, we don't have to worry about it + return false; + } + + var itemCount = CollectionView.NumberOfItemsInSection(section); + + if (itemCount < _itemsLayout.Span) + { + // If there is just one partial column, no problem; UICollectionViewFlowLayout gets it right + return false; + } + + if (itemCount % _itemsLayout.Span == 0) + { + // All of the columns are full; the bug only occurs when we have a partial column + return false; + } + + return true; + } } } \ No newline at end of file diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewLayout.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewLayout.cs index b938199bd22..0d41d818e67 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewLayout.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewLayout.cs @@ -27,6 +27,15 @@ protected ItemsViewLayout(ItemsLayout itemsLayout) : UICollectionViewScrollDirection.Vertical; Initialize(scrollDirection); + + if (Forms.IsiOS11OrNewer) + { + // `ContentInset` is actually the default value, but I'm leaving this here as a note to + // future maintainers; it's likely that someone will want a Platform Specific to change this behavior + // (Setting it to `SafeArea` lets you do the thing where the header/footer of your UICollectionView + // fills the screen width in landscape while your items are automatically shifted to avoid the notch) + SectionInsetReference = UICollectionViewFlowLayoutSectionInsetReference.ContentInset; + } } protected override void Dispose(bool disposing) diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewRenderer.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewRenderer.cs index 8a70494b9d4..8edb1210d51 100644 --- a/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewRenderer.cs +++ b/Xamarin.Forms.Platform.iOS/CollectionView/ItemsViewRenderer.cs @@ -85,6 +85,16 @@ protected virtual void SetUpNewElement(ItemsView newElement) UpdateLayout(); ItemsViewController = CreateController(newElement, _layout); + + if (Forms.IsiOS11OrNewer) + { + // We set this property to keep iOS from trying to be helpful about insetting all the + // CollectionView content when we're in landscape mode (to avoid the notch) + // The SetUseSafeArea Platform Specific is already taking care of this for us + // That said, at some point it's possible folks will want a PS for controlling this behavior + ItemsViewController.CollectionView.ContentInsetAdjustmentBehavior = UIScrollViewContentInsetAdjustmentBehavior.Never; + } + SetNativeControl(ItemsViewController.View); ItemsViewController.CollectionView.BackgroundColor = UIColor.Clear; ItemsViewController.UpdateEmptyView();