Skip to content

Commit

Permalink
fix(listview): managed listview doesnt render all items inside viewport
Browse files Browse the repository at this point in the history
  • Loading branch information
@Xiaoy312
Xiaoy312 committed Jul 29, 2024
1 parent 161c3c1 commit 1f60e76
Show file tree
Hide file tree
Showing 4 changed files with 158 additions and 21 deletions.
84 changes: 84 additions & 0 deletions src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_ListViewBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4678,6 +4678,90 @@ public async Task When_UpdateLayout_In_DragDropping()
Assert.AreEqual("2", textBlocks[2].Text);
}
#endif

[TestMethod]
[RunsOnUIThread]
public async Task When_ScrollIntoView_FreshlyAddedDefaultItem() // checks against #17695
{
var source = new ObservableCollection<string>();
var sut = new ListView
{
ItemsSource = source,
};

await UITestHelper.Load(sut, x => x.IsLoaded); // custom criteria to prevent empty listview failure

void AddItem(string item, bool select = false)
{
source.Add(item);
if (select)
{
sut.SelectedItem = item;
}
}

// We just assume here that there is enough space to display 3 items.
// Here we are testing if adding a new items and immediately selecting it
// doesn't result in a listview with missing items in the viewport.
AddItem($"Item 1", select: true);
await Task.Delay(10);
AddItem($"Item 2", select: false);
await Task.Delay(10);
AddItem($"Item 3", select: true);

await UITestHelper.WaitForIdle();

var count = sut.MaterializedContainers.Count();
Assert.AreEqual(3, count);
}

[TestMethod]
[RunsOnUIThread]
public async Task When_ScrollIntoView_FreshlyAddedOffscreenItem()
{
const int FixedItemHeight = 29;

var source = new ObservableCollection<string>();
var sut = new ListView
{
ItemsSource = source,
ItemTemplate = FixedSizeItemTemplate, // height=29
};

await UITestHelper.Load(sut, x => x.IsLoaded); // custom criteria to prevent empty listview failure

void AddItem(string item, bool select = false)
{
source.Add(item);
if (select)
{
sut.SelectedItem = item;
}
}

// Fill the source with enough enough items to fill the available height.
// Rounding up to the next tens, so we always start counting from XX1 XX2 XX3 for next step
var fulfillSize = Math.Round(sut.XamlRoot.Size.Height / FixedItemHeight / 10, MidpointRounding.ToPositiveInfinity) * 10 + 10;
for (int i = 0; i < fulfillSize; i++)
{
AddItem($"Item {i + 1}", select: false);
}

AddItem($"Item {fulfillSize + 1}", select: true);
await Task.Delay(10);
AddItem($"Item {fulfillSize + 2}", select: false);
await Task.Delay(10);
AddItem($"Item {fulfillSize + 3}", select: true);

await UITestHelper.WaitForIdle();

// Here we aren't verifying the viewport is entirely filled,
// But the last 3 are materialized, and that we are scrolled to the end.
Assert.IsNotNull(sut.ContainerFromIndex(source.Count - 3), "Container#n-3 is null");
Assert.IsNotNull(sut.ContainerFromIndex(source.Count - 2), "Container#n-2 is null");
Assert.IsNotNull(sut.ContainerFromIndex(source.Count - 1), "Container#n-1 is null");
Assert.AreEqual(sut.ScrollViewer.ScrollableHeight, sut.ScrollViewer.VerticalOffset, "ListView is not scrolled to the end.");
}
}

public partial class Given_ListViewBase // data class, data-context, view-model, template-selector
Expand Down
2 changes: 1 addition & 1 deletion src/Uno.UI/Extensions/ViewExtensions.visual-tree.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ static IEnumerable<string> GetDetails(object x)
}
if (x is ScrollViewer sv)
{
yield return $"Offset=({sv.HorizontalOffset},{sv.VerticalOffset}), Viewport=({sv.ViewportHeight},{sv.ViewportWidth}), Extent=({sv.ExtentHeight},{sv.ExtentWidth})";
yield return $"Offset=({sv.HorizontalOffset},{sv.VerticalOffset}), Viewport=({sv.ViewportWidth},{sv.ViewportHeight}), Extent=({sv.ExtentWidth},{sv.ExtentHeight})";
}
if (TryGetDpValue<CornerRadius>(x, "CornerRadius", out var cr)) yield return $"CornerRadius={FormatCornerRadius(cr)}";
if (TryGetDpValue<Thickness>(x, "Margin", out var margin)) yield return $"Margin={FormatThickness(margin)}";
Expand Down
84 changes: 64 additions & 20 deletions src/Uno.UI/UI/Xaml/Controls/ListViewBase/VirtualizingPanelLayout.managed.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

using DirectUI;
using Microsoft.UI.Xaml.Controls.Primitives;
using Microsoft.UI.Xaml.Documents;
using Windows.Foundation;

using Uno.Extensions;
Expand All @@ -20,12 +21,6 @@
using Uno.UI.Xaml.Controls;
using Uno.Foundation.Logging;

using IndexPath = Uno.UI.IndexPath;
using static System.Math;
using static Microsoft.UI.Xaml.Controls.Primitives.GeneratorDirection;
using Microsoft.UI.Xaml.Documents;


#if __MACOS__
using AppKit;
#elif __IOS__
Expand All @@ -37,6 +32,10 @@
using _Panel = Microsoft.UI.Xaml.Controls.Panel;
#endif

using static System.Math;
using static Microsoft.UI.Xaml.Controls.Primitives.GeneratorDirection;
using IndexPath = Uno.UI.IndexPath;

namespace Microsoft.UI.Xaml.Controls
{
#if __IOS__ || __ANDROID__
Expand Down Expand Up @@ -81,6 +80,7 @@ private protected VirtualizingPanelGenerator Generator
/// The previous item to the old first visible item, used when a lightweight layout rebuild is called.
/// </summary>
private Uno.UI.IndexPath? _dynamicSeedIndex;

/// <summary>
/// Start position of the old first group, used when a lightweight layout rebuild is called.
/// </summary>
Expand All @@ -91,6 +91,15 @@ private protected VirtualizingPanelGenerator Generator
/// </summary>
private readonly Queue<CollectionChangedOperation> _pendingCollectionChanges = new Queue<CollectionChangedOperation>();

/// <summary>
/// Pending <see cref="ScrollIntoView(object, ScrollIntoViewAlignment)"/> request to be handled when the ScrollViewer's extent is updated.
/// </summary>
/// <remarks>
/// At MeasureOverride/ArrangeOverride the SV isn't updated yet. If we attempt handle it in there,
/// the SV will clamp the scrolling to its current content size, potentially leaving the scroll target out-of-sight should it be the last item.
/// </remarks>
private (int Index, ScrollIntoViewAlignment Alignment)? _pendingScrollIntoViewRequest;

/// <summary>
/// Pending scroll adjustment, if an item has been added/removed backward of the current visible viewport by a collection change.
/// </summary>
Expand Down Expand Up @@ -150,14 +159,17 @@ private double ViewportExtent
/// The start of the visible viewport, relative to the start of the panel.
/// </summary>
private double ViewportStart => ScrollOffset;

/// <summary>
/// The end of the visible viewport, relative to the end of the panel.
/// </summary>
private double ViewportEnd => ScrollOffset + ViewportExtent;

/// <summary>
/// The additional length in pixels for which to create buffered views.
/// </summary>
private double ViewportExtension => CacheLength * ViewportExtent * 0.5;

/// <summary>
/// The start of the 'extended viewport,' the area of the visible viewport plus the buffer area defined by <see cref="CacheLength"/>.
/// </summary>
Expand Down Expand Up @@ -204,6 +216,7 @@ private void OnLoaded(object sender, RoutedEventArgs e)
ScrollViewer = scrollViewer;
ScrollViewer.ViewChanged += OnScrollChanged;
ScrollViewer.SizeChanged += OnScrollViewerSizeChanged;
ScrollViewer.ExtentSizeChanged += OnScrollViewerExtentSizeChanged;
}
else if (parent is ItemsControl itemsControl)
{
Expand Down Expand Up @@ -242,6 +255,7 @@ private void OnUnloaded(object sender, RoutedEventArgs e)
{
ScrollViewer.ViewChanged -= OnScrollChanged;
ScrollViewer.SizeChanged -= OnScrollViewerSizeChanged;
ScrollViewer.ExtentSizeChanged -= OnScrollViewerExtentSizeChanged;
}

ScrollViewer = null;
Expand Down Expand Up @@ -282,8 +296,7 @@ private void OnScrollChanged(object? sender, ScrollViewerViewChangedEventArgs e)
// Set the seed start to use the approximate position of
// the line based on the average line height.
var index = (int)(ScrollOffset / _averageLineHeight);
_dynamicSeedStart = index * _averageLineHeight;
_dynamicSeedIndex = Uno.UI.IndexPath.FromRowSection(index - 1, 0);
UpdateDynamicSeed(Uno.UI.IndexPath.FromRowSection(index - 1, 0), index * _averageLineHeight);
}

while (unappliedDelta > 0)
Expand Down Expand Up @@ -328,6 +341,14 @@ private void OnScrollViewerSizeChanged(object sender, SizeChangedEventArgs args)
OwnerPanel?.InvalidateMeasure();
}

private void OnScrollViewerExtentSizeChanged(object sender, SizeChangedEventArgs args)
{
if (_pendingScrollIntoViewRequest is { } request)
{
ScrollIntoViewCore(request.Index, request.Alignment);
}
}

/// <summary>
/// Get the increment to consume scroll changes, based on the size of the disappearing view.
/// </summary>
Expand Down Expand Up @@ -441,6 +462,7 @@ private void UpdateLayout(double? extentAdjustment, bool isScroll)

UnfillLayout(extentAdjustment ?? 0);
FillLayout(extentAdjustment ?? 0);
UpdateDynamicSeed(null, null);

CorrectForEstimationErrors();

Expand Down Expand Up @@ -477,7 +499,8 @@ private void UpdateCompleted()
/// <param name="extentAdjustment">Adjustment to apply when calculating fillable area.</param>
private void FillLayout(double extentAdjustment)
{
if (!_dynamicSeedStart.HasValue) // Don't fill backward if we're doing a light rebuild (since we are starting from nearest previously-visible item)
// Don't fill backward if we're doing a light rebuild (since we are starting from nearest previously-visible item)
if (!_dynamicSeed.Start.HasValue)
{
FillBackward();
}
Expand All @@ -490,9 +513,6 @@ private void FillLayout(double extentAdjustment)
AddLine(Forward, reorderIndex);
}

_dynamicSeedIndex = null;
_dynamicSeedStart = null;

void FillBackward()
{
if (GetItemsStart() > ExtendedViewportStart + extentAdjustment)
Expand Down Expand Up @@ -631,7 +651,7 @@ private void ApplyCollectionChanges()
var updated = CollectionChangedOperation.Offset(dynamicSeedIndex, _pendingCollectionChanges);
if (updated is Uno.UI.IndexPath updatedValue)
{
_dynamicSeedIndex = updated;
UpdateDynamicSeed(updated, _dynamicSeedStart);

var itemOffset = updatedValue.Row - dynamicSeedIndex.Row; // TODO: This will need to change when grouping is supported
var scrollAdjustment = itemOffset * _averageLineHeight; // TODO: not appropriate for ItemsWrapGrid
Expand All @@ -653,7 +673,7 @@ private void ApplyScrollAdjustment(double scrollAdjustment)
return;
}

_dynamicSeedStart += scrollAdjustment;
UpdateDynamicSeed(_dynamicSeedIndex, _dynamicSeedStart + scrollAdjustment);

if ((scrollAdjustment < 0 && IsScrolledToStart()) ||
(scrollAdjustment > 0 && IsScrolledToEnd())
Expand Down Expand Up @@ -868,8 +888,7 @@ private void ScrapLayout()
firstVisibleItem = _materializedLines.SelectMany(line => line.Items).Skip(1).FirstOrDefault().index;
}

_dynamicSeedIndex = GetDynamicSeedIndex(firstVisibleItem);
_dynamicSeedStart = GetContentStart();
UpdateDynamicSeed(GetDynamicSeedIndex(firstVisibleItem), GetContentStart());

if (this.Log().IsEnabled(LogLevel.Debug))
{
Expand Down Expand Up @@ -1214,10 +1233,29 @@ private void ResetReorderingIndex()
internal void ScrollIntoView(object item, ScrollIntoViewAlignment alignment = ScrollIntoViewAlignment.Default)
{
var index = ItemsControl?.IndexFromItem(item) ?? -1;
var path = Uno.UI.IndexPath.FromRowSection(index, 0);

ScrollIntoViewCore(index, alignment);
}
internal void ScrollIntoViewCore(int index, ScrollIntoViewAlignment alignment = ScrollIntoViewAlignment.Default)
{
if (index == -1) { return; }

var path = Uno.UI.IndexPath.FromRowSection(index, 0);

if (_pendingCollectionChanges.Any(x =>
x.Action == NotifyCollectionChangedAction.Add &&
x.StartingIndex.Row <= index && index < (x.StartingIndex.Row + x.Range)
))
{
// We must wait until the ScrollViewer content size to be updated, before attempting to scroll.
// Otherwise, the clamping mechanism here or from SV will clamp the scroll, so the item is left outside of the viewport.
_pendingScrollIntoViewRequest = (index, alignment);
return;
}

var pending = _pendingScrollIntoViewRequest;
_pendingScrollIntoViewRequest = null;

var extent = Extent;
var viewportExtent = ViewportExtent;
var initialOffset = ScrollOffset;
Expand All @@ -1227,8 +1265,8 @@ internal void ScrollIntoView(object item, ScrollIntoViewAlignment alignment = Sc
if (FindViewByIndexPath(path) is not { } targetView)
{
// skip to an estimate offset of where the target could be
_dynamicSeedStart = adjustedOffset = index * _averageLineHeight;
_dynamicSeedIndex = Uno.UI.IndexPath.FromRowSection(index - 1, 0);
adjustedOffset = index * _averageLineHeight;
UpdateDynamicSeed(Uno.UI.IndexPath.FromRowSection(index - 1, 0), adjustedOffset);
UpdateLayout(adjustedOffset - initialOffset, isScroll: true);

// scroll forward or backward as needed
Expand Down Expand Up @@ -1285,7 +1323,7 @@ internal void ScrollIntoView(object item, ScrollIntoViewAlignment alignment = Sc
}

// clamp offset within valid range
adjustedOffset = Clamp(adjustedOffset, 0, extent - viewportExtent);
adjustedOffset = Clamp(adjustedOffset, 0, Max(0, extent - viewportExtent));

ApplyScrollAdjustment(adjustedOffset - initialOffset);
}
Expand All @@ -1306,6 +1344,12 @@ internal void ScrollIntoView(object item, ScrollIntoViewAlignment alignment = Sc
return null;
}

private void UpdateDynamicSeed(Uno.UI.IndexPath? index, double? start)
{
_dynamicSeedIndex = index;
_dynamicSeedStart = start;
}

/// <summary>
/// Represents a single row in a vertically-scrolling panel, or a column in a horizontally-scrolling panel.
/// </summary>
Expand Down
9 changes: 9 additions & 0 deletions src/Uno.UI/UI/Xaml/Controls/ScrollViewer/ScrollViewer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ public static class ScrollBarsSeparator
/// </summary>
public event EventHandler<ScrollViewerViewChangedEventArgs>? ViewChanged;

internal event SizeChangedEventHandler? ExtentSizeChanged;

static ScrollViewer()
{
#if !IS_UNIT_TESTS
Expand Down Expand Up @@ -749,6 +751,8 @@ void UpdateDimensionProperties()
ViewportHeight = (_presenter as IFrameworkElement)?.ActualHeight ?? ActualHeight;
ViewportWidth = (_presenter as IFrameworkElement)?.ActualWidth ?? ActualWidth;

var oldSize = new Size(ExtentWidth, ExtentHeight);

if (_presenter?.CustomContentExtent is { } customExtent)
{
ExtentHeight = customExtent.Height;
Expand Down Expand Up @@ -833,6 +837,11 @@ void UpdateDimensionProperties()

TrimOverscroll(Orientation.Vertical);
TrimOverscroll(Orientation.Horizontal);

if (new Size(ExtentWidth, ExtentWidth) is { } newSize && oldSize != newSize)
{
ExtentSizeChanged?.Invoke(this, new(this, oldSize, newSize));
}
}

private void UpdateComputedVerticalScrollability(bool invalidate)
Expand Down

0 comments on commit 1f60e76

Please sign in to comment.