From 602ea10e03fe2b5117d34cb27f49a9391c35bc5a Mon Sep 17 00:00:00 2001 From: David Date: Tue, 19 Mar 2024 17:18:13 -0400 Subject: [PATCH] fix(itemsRepeater): Fix an issue where IR might re-position first items if items below are significantly higher --- .../UITests.Shared/ItemExclusions.props | 4 - .../Repeater/Given_ItemsRepeater.cs | 92 ++++++++++++++++--- .../UI/Xaml/Controls/Repeater/StackLayout.cs | 6 +- 3 files changed, 85 insertions(+), 17 deletions(-) diff --git a/src/SamplesApp/UITests.Shared/ItemExclusions.props b/src/SamplesApp/UITests.Shared/ItemExclusions.props index 53e1b5f030b4..e430fdf6370f 100644 --- a/src/SamplesApp/UITests.Shared/ItemExclusions.props +++ b/src/SamplesApp/UITests.Shared/ItemExclusions.props @@ -17,10 +17,6 @@ - - - - diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Repeater/Given_ItemsRepeater.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Repeater/Given_ItemsRepeater.cs index 6381eeead23e..f5a331b2eb0e 100644 --- a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Repeater/Given_ItemsRepeater.cs +++ b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Repeater/Given_ItemsRepeater.cs @@ -430,13 +430,82 @@ public async Task When_ClearItemsWhileUnloaded_Then_MaterializeItems() sut.Materialized.Should().Be(0); } - private record SUT(Border Root, ScrollViewer Scroller, ItemsRepeater Repeater, ObservableCollection Source) + + [TestMethod] + [RunsOnUIThread] +#if __MACOS__ + [Ignore("Currently fails on macOS, part of #9282 epic")] +#endif + public async Task When_ItemSignificantlyHigher_Then_VirtualizeProperly() { - public static SUT Create(int itemsCount = 3, Size? viewport = default) + var sut = SUT.Create( + new ObservableCollection + { + new (0, 200, Colors.FromARGB("#FF0000")), + new (1, 400, Colors.FromARGB("#FF8000")), + new (2, 200, Colors.FromARGB("#FFFF00")), + new (3, 5000, Colors.FromARGB("#008000")), + new (4, 100, Colors.FromARGB("#0000FF")), + new (5, 100, Colors.FromARGB("#A000C0")) + }, + new DataTemplate(() => new Border + { + Width = 100, + Margin = new Thickness(10), + Child = new TextBlock().Apply(tb => tb.SetBinding(TextBlock.TextProperty, new Binding())) + } + .Apply(b => b.SetBinding(FrameworkElement.HeightProperty, new Binding{Path = nameof(MyItem.Height)})) + .Apply(b => b.SetBinding(FrameworkElement.BackgroundProperty, new Binding { Path = nameof(MyItem.Color) }))), + new Size(100, 500) + ); + + await sut.Load(); + sut.Scroller.ViewChanged += (s, e) => Console.WriteLine($"Vertical: {sut.Scroller.VerticalOffset}"); + + var originalEstimatedExtent = sut.Scroller.ExtentHeight; + + sut.Scroller.ChangeView(null, 800, null, disableAnimation: true); // First scroll enough to get item #3 to be materialized + await TestServices.WindowHelper.WaitForIdle(); + + sut.MaterializedItems.Should().Contain(sut.Source[3]); // Confirm that item has been materialized! + sut.Scroller.ExtentHeight.Should().BeGreaterThan(originalEstimatedExtent); // Confirm that the extent has increased due to item #3 + + var item3OriginalVerticalOffset = sut.Repeater.Children.First(elt => ReferenceEquals(elt.DataContext, sut.Source[3])).ActualOffset.Y; + + sut.Scroller.ChangeView(null, 1500, null, disableAnimation: true); // Then scroll enough for first items to be DE-materialized + await TestServices.WindowHelper.WaitForIdle(); + + sut.MaterializedItems.Should().NotContain(sut.Source[0]); // Confirm that first items has been removed! + sut.MaterializedItems.Should().NotContain(sut.Source[1]); + + var item3UpdatedVerticalOffset = sut.Repeater.Children.First(elt => ReferenceEquals(elt.DataContext, sut.Source[3])).ActualOffset.Y; + + item3UpdatedVerticalOffset.Should().Be(item3OriginalVerticalOffset); // Confirm that item #3 has not been moved down + var result = await UITestHelper.ScreenShot(sut.Repeater); + ImageAssert.HasColorAt(result, 10, 10, Colors.FromARGB("#008000")); // For safety also check it's effectively the item 3 that is visible + } + + private record MyItem(int Id, double Height, Color Color) + { + public string Title => $"Item {Id}"; + } + +#nullable enable + private static class SUT + { + public static SUT Create(ObservableCollection source, DataTemplate? itemTemplate = null, Size? viewport = default) { + itemTemplate ??= new DataTemplate(() => new Border + { + Width = 100, + Height = 100, + Background = new SolidColorBrush(Colors.DeepSkyBlue), + Margin = new Thickness(10), + Child = new TextBlock().Apply(tb => tb.SetBinding(TextBlock.TextProperty, new Binding())) + }); + var repeater = default(ItemsRepeater); var scroller = default(ScrollViewer); - var source = new ObservableCollection(Enumerable.Range(0, itemsCount).Select(i => $"Item #{i}")); var root = new Border { BorderThickness = new Thickness(5), @@ -447,14 +516,7 @@ public static SUT Create(int itemsCount = 3, Size? viewport = default) { ItemsSource = source, Layout = new StackLayout(), - ItemTemplate = new DataTemplate(() => new Border - { - Width = 100, - Height = 100, - Background = new SolidColorBrush(Colors.DeepSkyBlue), - Margin = new Thickness(10), - Child = new TextBlock().Apply(tb => tb.SetBinding(TextBlock.TextProperty, new Binding())) - }) + ItemTemplate = itemTemplate }) }) }; @@ -468,9 +530,15 @@ public static SUT Create(int itemsCount = 3, Size? viewport = default) return new(root, scroller, repeater, source); } + public static SUT Create(int itemsCount = 3, Size? viewport = default) + => Create(new ObservableCollection(Enumerable.Range(0, itemsCount).Select(i => $"Item #{i}")), viewport: viewport); + } + + private record SUT(Border Root, ScrollViewer Scroller, ItemsRepeater Repeater, ObservableCollection Source) + { public int Materialized => Repeater.Children.Count(elt => elt.ActualOffset.X >= 0); - public IEnumerable MaterializedItems => Repeater.Children.Where(elt => elt.ActualOffset.X >= 0).Select(elt => elt.DataContext?.ToString()); + public IEnumerable MaterializedItems => Repeater.Children.Where(elt => elt.ActualOffset.X >= 0).Select(elt => (T)elt.DataContext); public async ValueTask Load() { diff --git a/src/Uno.UI/Microsoft/UI/Xaml/Controls/Repeater/StackLayout.cs b/src/Uno.UI/Microsoft/UI/Xaml/Controls/Repeater/StackLayout.cs index e5202a572d59..b75edce5f96b 100644 --- a/src/Uno.UI/Microsoft/UI/Xaml/Controls/Repeater/StackLayout.cs +++ b/src/Uno.UI/Microsoft/UI/Xaml/Controls/Repeater/StackLayout.cs @@ -176,7 +176,11 @@ Rect GetExtent( if (firstRealized != null) { MUX_ASSERT(lastRealized != null); - SetMajorStart(ref extent, (float)(MajorStart(firstRealizedLayoutBounds) - firstRealizedItemIndex * averageElementSize)); + var firstRealizedMajor = (float)(MajorStart(firstRealizedLayoutBounds) - firstRealizedItemIndex * averageElementSize); + // Uno workaround [BEGIN]: Make sure to not move items above the viewport. This can be the case if an items is significantly higher than previous items (will increase the average items size) + firstRealizedMajor = Math.Max(0.0f, firstRealizedMajor); + // Uno workaround [END] + SetMajorStart(ref extent, firstRealizedMajor); var remainingItems = itemsCount - lastRealizedItemIndex - 1; SetMajorSize(ref extent, MajorEnd(lastRealizedLayoutBounds) - MajorStart(extent) + (float)(remainingItems * averageElementSize)); }