Browse Source

Fix for GhostItems in virtualized ItemsControls like ListBox (#20700)

* Add failing test for #20688

Unit test created from issue description

* Add a failing test for wrong CacheLength handling

All items would be realized which is obviously wrong

* fix test setup used StackPanel instead of expcected

VirtualizingStackPanel

* Make sure the test actually fails

* update comment

* Fix for focusedElement and focusedIndex

* add another unit test

* Fixes for new test cases

* Addressing Review

* Update tests to match new behavior

* only recycle focused element if it is not null

* Address review

* Address copilot review
pull/20773/head
Tim 4 weeks ago
committed by GitHub
parent
commit
3514ed2b9d
No known key found for this signature in database GPG Key ID: B5690EEEBB952194
  1. 198
      src/Avalonia.Controls/VirtualizingStackPanel.cs
  2. 336
      tests/Avalonia.Controls.UnitTests/ListBoxVirtualizationIssueTests.cs
  3. 8
      tests/Avalonia.Controls.UnitTests/VirtualizingStackPanelTests.cs

198
src/Avalonia.Controls/VirtualizingStackPanel.cs

@ -8,7 +8,7 @@ using Avalonia.Controls.Utils;
using Avalonia.Input; using Avalonia.Input;
using Avalonia.Interactivity; using Avalonia.Interactivity;
using Avalonia.Layout; using Avalonia.Layout;
using Avalonia.Reactive; using Avalonia.Logging;
using Avalonia.Utilities; using Avalonia.Utilities;
using Avalonia.VisualTree; using Avalonia.VisualTree;
@ -263,8 +263,20 @@ namespace Avalonia.Controls
e.Arrange(rect); e.Arrange(rect);
if (_viewport.Intersects(rect)) if (e.IsVisible && _viewport.Intersects(rect))
_scrollAnchorProvider?.RegisterAnchorCandidate(e); {
try
{
_scrollAnchorProvider?.RegisterAnchorCandidate(e);
}
catch (InvalidOperationException ex)
{
// Element might have been removed/reparented during virtualization; ignore but log for diagnostics.
Logger.TryGet(LogEventLevel.Verbose, LogArea.Layout)?.Log(this,
"RegisterAnchorCandidate ignored for {Element}: not a descendant of ScrollAnchorProvider. {Message}",
e, ex.Message);
}
}
u += orientation == Orientation.Horizontal ? rect.Width : rect.Height; u += orientation == Orientation.Horizontal ? rect.Width : rect.Height;
} }
@ -307,6 +319,9 @@ namespace Avalonia.Controls
{ {
InvalidateMeasure(); InvalidateMeasure();
// Always update special elements
UpdateSpecialElementsOnItemsChanged(e);
if (_realizedElements is null) if (_realizedElements is null)
return; return;
@ -332,7 +347,7 @@ namespace Avalonia.Controls
if (e.NewStartingIndex > e.OldStartingIndex) if (e.NewStartingIndex > e.OldStartingIndex)
{ {
insertIndex -= e.OldItems.Count - 1; insertIndex -= e.OldItems!.Count - 1;
} }
_realizedElements.ItemsInserted(insertIndex, e.NewItems!.Count, _updateElementIndex); _realizedElements.ItemsInserted(insertIndex, e.NewItems!.Count, _updateElementIndex);
@ -343,6 +358,132 @@ namespace Avalonia.Controls
} }
} }
private void UpdateSpecialElementsOnItemsChanged(NotifyCollectionChangedEventArgs e)
{
switch (e.Action)
{
case NotifyCollectionChangedAction.Add:
if (_focusedElement is not null && e.NewStartingIndex <= _focusedIndex)
{
var oldIndex = _focusedIndex;
_focusedIndex += e.NewItems!.Count;
_updateElementIndex(_focusedElement, oldIndex, _focusedIndex);
}
if (_scrollToElement is not null && e.NewStartingIndex <= _scrollToIndex)
{
_scrollToIndex += e.NewItems!.Count;
}
break;
case NotifyCollectionChangedAction.Remove:
if (_focusedElement is not null)
{
if (e.OldStartingIndex <= _focusedIndex && _focusedIndex < e.OldStartingIndex + e.OldItems!.Count)
{
RecycleFocusedElement();
}
else if (e.OldStartingIndex < _focusedIndex)
{
var oldIndex = _focusedIndex;
_focusedIndex -= e.OldItems!.Count;
_updateElementIndex(_focusedElement, oldIndex, _focusedIndex);
}
}
if (_scrollToElement is not null)
{
if (e.OldStartingIndex <= _scrollToIndex && _scrollToIndex < e.OldStartingIndex + e.OldItems!.Count)
{
RecycleScrollToElement();
}
else if (e.OldStartingIndex < _scrollToIndex)
{
_scrollToIndex -= e.OldItems!.Count;
}
}
break;
case NotifyCollectionChangedAction.Replace:
if (_focusedElement is not null && e.OldStartingIndex <= _focusedIndex && _focusedIndex < e.OldStartingIndex + e.OldItems!.Count)
{
RecycleFocusedElement();
}
if (_scrollToElement is not null && e.OldStartingIndex <= _scrollToIndex && _scrollToIndex < e.OldStartingIndex + e.OldItems!.Count)
{
RecycleScrollToElement();
}
break;
case NotifyCollectionChangedAction.Move:
if (e.OldStartingIndex < 0)
{
goto case NotifyCollectionChangedAction.Reset;
}
if (_focusedElement is not null)
{
if (e.OldStartingIndex <= _focusedIndex && _focusedIndex < e.OldStartingIndex + e.OldItems!.Count)
{
var oldIndex = _focusedIndex;
_focusedIndex = e.NewStartingIndex + (_focusedIndex - e.OldStartingIndex);
_updateElementIndex(_focusedElement, oldIndex, _focusedIndex);
}
else
{
var newFocusedIndex = _focusedIndex;
if (e.OldStartingIndex < _focusedIndex)
{
newFocusedIndex -= e.OldItems!.Count;
}
if (e.NewStartingIndex <= newFocusedIndex)
{
newFocusedIndex += e.NewItems!.Count;
}
if (newFocusedIndex != _focusedIndex)
{
var oldIndex = _focusedIndex;
_focusedIndex = newFocusedIndex;
_updateElementIndex(_focusedElement, oldIndex, _focusedIndex);
}
}
}
if (_scrollToElement is not null)
{
if (e.OldStartingIndex <= _scrollToIndex && _scrollToIndex < e.OldStartingIndex + e.OldItems!.Count)
{
_scrollToIndex = e.NewStartingIndex + (_scrollToIndex - e.OldStartingIndex);
}
else
{
var newScrollToIndex = _scrollToIndex;
if (e.OldStartingIndex < _scrollToIndex)
{
newScrollToIndex -= e.OldItems!.Count;
}
if (e.NewStartingIndex <= newScrollToIndex)
{
newScrollToIndex += e.NewItems!.Count;
}
_scrollToIndex = newScrollToIndex;
}
}
break;
case NotifyCollectionChangedAction.Reset:
if (_focusedElement is not null)
{
RecycleFocusedElement();
}
if (_scrollToElement is not null)
{
RecycleScrollToElement();
}
break;
}
}
protected override void OnItemsControlChanged(ItemsControl? oldValue) protected override void OnItemsControlChanged(ItemsControl? oldValue)
{ {
base.OnItemsControlChanged(oldValue); base.OnItemsControlChanged(oldValue);
@ -351,6 +492,24 @@ namespace Avalonia.Controls
oldValue.PropertyChanged -= OnItemsControlPropertyChanged; oldValue.PropertyChanged -= OnItemsControlPropertyChanged;
if (ItemsControl is not null) if (ItemsControl is not null)
ItemsControl.PropertyChanged += OnItemsControlPropertyChanged; ItemsControl.PropertyChanged += OnItemsControlPropertyChanged;
_realizedElements?.ResetForReuse();
_measureElements?.ResetForReuse();
if (ItemsControl is not null && _focusedElement is not null)
{
RecycleFocusedElement();
}
if (ItemsControl is not null && _scrollToElement is not null)
{
RecycleScrollToElement();
}
if (ItemsControl is null)
{
_focusedElement = null;
_scrollToElement = null;
}
_focusedIndex = -1;
_scrollToIndex = -1;
} }
protected override IInputElement? GetControl(NavigationDirection direction, IInputElement? from, bool wrap) protected override IInputElement? GetControl(NavigationDirection direction, IInputElement? from, bool wrap)
@ -860,6 +1019,7 @@ namespace Avalonia.Controls
var recycled = recyclePool.Pop(); var recycled = recyclePool.Pop();
recycled.SetCurrentValue(Visual.IsVisibleProperty, true); recycled.SetCurrentValue(Visual.IsVisibleProperty, true);
generator.PrepareItemContainer(recycled, item, index); generator.PrepareItemContainer(recycled, item, index);
AddInternalChild(recycled);
generator.ItemContainerPrepared(recycled, item, index); generator.ItemContainerPrepared(recycled, item, index);
return recycled; return recycled;
} }
@ -893,6 +1053,7 @@ namespace Avalonia.Controls
if (recycleKey is null) if (recycleKey is null)
{ {
ItemContainerGenerator!.ClearItemContainer(element);
RemoveInternalChild(element); RemoveInternalChild(element);
} }
else if (recycleKey == s_itemIsItsOwnContainer) else if (recycleKey == s_itemIsItsOwnContainer)
@ -909,6 +1070,7 @@ namespace Avalonia.Controls
ItemContainerGenerator!.ClearItemContainer(element); ItemContainerGenerator!.ClearItemContainer(element);
PushToRecyclePool(recycleKey, element); PushToRecyclePool(recycleKey, element);
element.SetCurrentValue(Visual.IsVisibleProperty, false); element.SetCurrentValue(Visual.IsVisibleProperty, false);
RemoveInternalChild(element);
} }
} }
@ -920,7 +1082,12 @@ namespace Avalonia.Controls
var recycleKey = element.GetValue(RecycleKeyProperty); var recycleKey = element.GetValue(RecycleKeyProperty);
if (recycleKey is null || recycleKey == s_itemIsItsOwnContainer) if (recycleKey is null)
{
ItemContainerGenerator!.ClearItemContainer(element);
RemoveInternalChild(element);
}
else if (recycleKey == s_itemIsItsOwnContainer)
{ {
RemoveInternalChild(element); RemoveInternalChild(element);
} }
@ -929,9 +1096,30 @@ namespace Avalonia.Controls
ItemContainerGenerator!.ClearItemContainer(element); ItemContainerGenerator!.ClearItemContainer(element);
PushToRecyclePool(recycleKey, element); PushToRecyclePool(recycleKey, element);
element.SetCurrentValue(Visual.IsVisibleProperty, false); element.SetCurrentValue(Visual.IsVisibleProperty, false);
RemoveInternalChild(element);
} }
} }
private void RecycleFocusedElement()
{
if (_focusedElement != null)
{
RecycleElementOnItemRemoved(_focusedElement);
}
_focusedElement = null;
_focusedIndex = -1;
}
private void RecycleScrollToElement()
{
if (_scrollToElement != null)
{
RecycleElementOnItemRemoved(_scrollToElement);
}
_scrollToElement = null;
_scrollToIndex = -1;
}
private void PushToRecyclePool(object recycleKey, Control element) private void PushToRecyclePool(object recycleKey, Control element)
{ {
_recyclePool ??= new(); _recyclePool ??= new();

336
tests/Avalonia.Controls.UnitTests/ListBoxVirtualizationIssueTests.cs

@ -0,0 +1,336 @@
using System.Collections.ObjectModel;
using System.Linq;
using Avalonia.Controls.Presenters;
using Avalonia.Controls.Primitives;
using Avalonia.Controls.Templates;
using Avalonia.Input;
using Avalonia.UnitTests;
using Xunit;
namespace Avalonia.Controls.UnitTests;
public class ListBoxVirtualizationIssueTests : ScopedTestBase
{
[Fact]
public void Replaced_ItemsSource_Should_Not_Show_Old_Selected_Item_When_Scrolled_Back()
{
using (UnitTestApplication.Start(TestServices.MockPlatformRenderInterface))
{
var letters = "ABCDEFGHIJ".Select(c => c.ToString()).ToList();
var numbers = "0123456789".Select(c => c.ToString()).ToList();
var target = new ListBox
{
Template = new FuncControlTemplate(CreateListBoxTemplate),
ItemsSource = letters,
ItemTemplate = new FuncDataTemplate<string>((_, _) => new TextBlock { Height = 50 }),
Height = 100, // Show 2 items
ItemsPanel = new FuncTemplate<Panel?>(() => new VirtualizingStackPanel { CacheLength = 0 }),
};
Prepare(target);
// 1. Select a ListBoxItem
target.SelectedIndex = 0;
Assert.True(((ListBoxItem)target.Presenter!.Panel!.Children[0]).IsSelected);
// 2. Scroll until the selected ListBoxItem is no longer visible
target.ScrollIntoView(letters.Count - 1); // Scroll down to the last item
// Verify that the first item is no longer realized
var realizedContainers = target.GetRealizedContainers().Cast<ListBoxItem>().ToList();
Assert.DoesNotContain(realizedContainers, x => x.Content as string == "A");
// 3. Change the ItemsSource
target.ItemsSource = numbers;
// 4. Scroll to the top
target.ScrollIntoView(0);
// 5. The previously selected ListBoxItem should NOT appear in the ListBox
var realizedItems = target.GetRealizedContainers()
.Cast<ListBoxItem>()
.Select(x => x.Content?.ToString())
.ToList();
Assert.All(realizedItems, item => Assert.DoesNotContain(item, letters));
Assert.Equal("0", realizedItems[0]);
}
}
[Fact]
public void AddingItemsAtTopShouldNotCreateGhostItems()
{
using (UnitTestApplication.Start(TestServices.MockPlatformRenderInterface))
{
ObservableCollection<Item> items = new();
for (int i = 0; i < 100; i++)
{
items.Add(new Item(i));
}
var target = new ListBox
{
Template = new FuncControlTemplate(CreateListBoxTemplate),
ItemsSource = items,
Height = 100, // Show 2 items
ItemsPanel = new FuncTemplate<Panel?>(() => new VirtualizingStackPanel()),
};
Prepare(target);
// Scroll to some position
var scrollViewer = (ScrollViewer)target.VisualChildren[0];
scrollViewer.Offset = new Vector(0, 500); // Scrolled down
target.UpdateLayout();
// Add items at the top multiple times
for (int i = 0; i < 5; i++)
{
for (int j = 0; j < 10; j++)
{
items.Insert(0, new Item(1000 + (i * 100 + j)));
}
target.UpdateLayout();
// Randomly select something
target.SelectedIndex = items.Count - 1;
target.UpdateLayout();
// Scroll a bit
scrollViewer.ScrollToEnd();
scrollViewer.ScrollToEnd();
target.UpdateLayout();
// Check for ghost items during the process
var p = target.Presenter!.Panel!;
var visibleChildren = p.Children.Where(c => c.IsVisible).ToList();
var realizedContainers = target.GetRealizedContainers()
.Cast<ListBoxItem>()
.ToList();
// Only visible children should be considered. Invisible children may be recycled items kept for reuse.
Assert.Equal(realizedContainers.Count, visibleChildren.Count);
foreach (var child in visibleChildren)
{
Assert.Contains(child, realizedContainers);
}
var realizedItems = realizedContainers
.Select(x => x.Content)
.Cast<Item>()
.ToList();
// Check for duplicates in realized items
var duplicateIds = realizedItems.GroupBy(x => x.Id).Where(g => g.Count() > 1).Select(g => g.Key)
.ToList();
Assert.Empty(duplicateIds);
// Check if all realized items are actually in the items source
foreach (var item in realizedItems)
{
Assert.Contains(item, items);
}
// Check if realized items are in the correct order
int lastIndex = -1;
foreach (var item in realizedItems)
{
int currentIndex = items.IndexOf(item);
Assert.True(currentIndex > lastIndex,
$"Item {item.Id} is at index {currentIndex}, but previous item was at index {lastIndex}");
lastIndex = currentIndex;
}
// New check: verify that all visual children of the panel are accounted for in realizedContainers
var panel = target.Presenter!.Panel!;
var visualChildren = panel.Children.ToList();
// Realized containers should match exactly the visual children of the panel
// (VirtualizingStackPanel manages its children such that they should be the realized containers)
// We also check if all children are visible, if not they might be "ghosts"
foreach (var child in visualChildren)
{
Assert.True(child.IsVisible, $"Child {((ListBoxItem)child).Content} should be visible");
}
Assert.Equal(realizedContainers.Count, visualChildren.Count);
foreach (var child in visualChildren)
{
Assert.Contains(child, realizedContainers);
}
}
}
}
[Fact]
public void RealizedContainers_Should_Only_Include_Visible_Items_With_CacheLength_Zero()
{
using (UnitTestApplication.Start(TestServices.MockPlatformRenderInterface))
{
var letters = "ABCDEFGHIJ".Select(c => c.ToString()).ToList();
var target = new ListBox
{
ItemsPanel = new FuncTemplate<Panel?>(() => new VirtualizingStackPanel { CacheLength = 0 }),
Template = new FuncControlTemplate(CreateListBoxTemplate),
ItemsSource = letters,
ItemTemplate = new FuncDataTemplate<string>((_, _) => new TextBlock { Height = 50 }),
Height = 100, // Show 2 items (100 / 50 = 2)
};
Prepare(target);
// At the top, only 2 items should be visible (items at index 0 and 1)
var realizedContainers = target.GetRealizedContainers().Cast<ListBoxItem>().ToList();
// With CacheLength = 0, we should only have the visible items realized
Assert.Equal(2, realizedContainers.Count);
Assert.Equal("A", realizedContainers[0].Content?.ToString());
Assert.Equal("B", realizedContainers[1].Content?.ToString());
}
}
[Fact]
public void GhostItemTest_FocusManagement()
{
using (UnitTestApplication.Start(TestServices.MockPlatformRenderInterface))
{
var items = new ObservableCollection<string>(Enumerable.Range(0, 100).Select(i => $"Item {i}"));
var target = new ListBox
{
Template = new FuncControlTemplate(CreateListBoxTemplate),
ItemsSource = items,
ItemTemplate = new FuncDataTemplate<string>((_, _) => new TextBlock { Height = 50 }),
Height = 100, // Show 2 items
ItemsPanel = new FuncTemplate<Panel?>(() => new VirtualizingStackPanel { CacheLength = 0 }),
};
Prepare(target);
// 1. Get the first container and focus it
var container = (ListBoxItem)target.Presenter!.Panel!.Children[0];
KeyboardNavigation.SetTabOnceActiveElement(target, container);
// 2. Scroll down so the first item is recycled
target.ScrollIntoView(10);
target.UpdateLayout();
// 3. Verify it is now _focusedElement in the panel
var panel = (VirtualizingStackPanel)target.Presenter!.Panel!;
var realizedContainers = target.GetRealizedContainers().ToList();
// The focused container should still be in Children, but NOT in realizedContainers
Assert.Contains(container, panel.Children);
Assert.DoesNotContain(container, realizedContainers);
// Now scroll back to top.
target.ScrollIntoView(0);
target.UpdateLayout();
// Check if we have two containers for the same item or other weirdness
var visibleChildren = panel.Children.Where(c => c.IsVisible).ToList();
// If it was a ghost, it might still be there or we might have two items for the same thing
Assert.Equal(target.GetRealizedContainers().Count(), visibleChildren.Count);
// 4. Test: Re-insert at top might cause issues if _focusedElement is not updated correctly
items.Insert(0, "New Item");
target.UpdateLayout();
visibleChildren = panel.Children.Where(c => c.IsVisible).ToList();
Assert.Equal(target.GetRealizedContainers().Count(), visibleChildren.Count);
// 5. Remove the focused item while it's recycled
target.ScrollIntoView(10);
target.UpdateLayout();
Assert.Contains(container, panel.Children);
items.RemoveAt(1); // Item 0 was at index 1 because of Insert(0, "New Item")
target.UpdateLayout();
// container should be removed from children because RecycleElementOnItemRemoved is called
Assert.DoesNotContain(container, panel.Children);
Assert.False(container.IsVisible);
}
}
[Fact]
public void GhostItemTest_ScrollToManagement()
{
using (UnitTestApplication.Start(TestServices.MockPlatformRenderInterface))
{
var items = new ObservableCollection<string>(Enumerable.Range(0, 100).Select(i => $"Item {i}"));
var target = new ListBox
{
Template = new FuncControlTemplate(CreateListBoxTemplate),
ItemsSource = items,
ItemTemplate = new FuncDataTemplate<string>((_, _) => new TextBlock { Height = 50 }),
Height = 100, // Show 2 items
ItemsPanel = new FuncTemplate<Panel?>(() => new VirtualizingStackPanel { CacheLength = 0 }),
};
Prepare(target);
// 1. ScrollIntoView to trigger _scrollToElement
// We use a high index and don't call UpdateLayout immediately if we want to catch it in between?
// Actually ScrollIntoView calls layout internally.
target.ScrollIntoView(50);
var panel = (VirtualizingStackPanel)target.Presenter!.Panel!;
// 2. Remove the item we just scrolled to
items.RemoveAt(50);
target.UpdateLayout();
// If it was kept in _scrollToElement and not recycled, it might be a ghost.
var visibleChildren = panel.Children.Where(c => c.IsVisible).ToList();
Assert.Equal(target.GetRealizedContainers().Count(), visibleChildren.Count);
}
}
private Control CreateListBoxTemplate(TemplatedControl parent, INameScope scope)
{
return new ScrollViewer
{
Name = "PART_ScrollViewer",
Template = new FuncControlTemplate(CreateScrollViewerTemplate),
Content = new ItemsPresenter
{
Name = "PART_ItemsPresenter",
[~ItemsPresenter.ItemsPanelProperty] =
((ListBox)parent).GetObservable(ItemsControl.ItemsPanelProperty).ToBinding(),
}.RegisterInNameScope(scope)
}.RegisterInNameScope(scope);
}
private Control CreateScrollViewerTemplate(TemplatedControl parent, INameScope scope)
{
return new ScrollContentPresenter
{
Name = "PART_ContentPresenter",
[~ContentPresenter.ContentProperty] =
parent.GetObservable(ContentControl.ContentProperty).ToBinding(),
}.RegisterInNameScope(scope);
}
private static void Prepare(ListBox target)
{
target.Width = target.Height = 100;
var root = new TestRoot(target);
root.LayoutManager.ExecuteInitialLayoutPass();
}
private class Item
{
public Item(int id)
{
Id = id;
}
public int Id { get; }
}
}

8
tests/Avalonia.Controls.UnitTests/VirtualizingStackPanelTests.cs

@ -141,8 +141,8 @@ namespace Avalonia.Controls.UnitTests
} }
[Theory] [Theory]
[InlineData(0d, 11)] [InlineData(0d, 10)]
[InlineData(0.5d, 21)] [InlineData(0.5d, 20)]
public void Scrolling_Up_To_Index_Does_Not_Create_A_Page_Of_Unrealized_Elements(double bufferFactor, int expectedCount) public void Scrolling_Up_To_Index_Does_Not_Create_A_Page_Of_Unrealized_Elements(double bufferFactor, int expectedCount)
{ {
using var app = App(); using var app = App();
@ -1097,9 +1097,9 @@ namespace Avalonia.Controls.UnitTests
itemsControl.ItemsSource = null; itemsControl.ItemsSource = null;
root.LayoutManager.ExecuteLayoutPass(); root.LayoutManager.ExecuteLayoutPass();
// Should have no realized elements and 3 unrealized elements. // Should have no realized elements and no unrealized elements.
Assert.Equal(0, target.GetRealizedElements().Count); Assert.Equal(0, target.GetRealizedElements().Count);
Assert.Equal(3, target.Children.Count); Assert.Equal(0, target.Children.Count);
// Make the panel effectively invisible and set items. // Make the panel effectively invisible and set items.
container.IsVisible = false; container.IsVisible = false;

Loading…
Cancel
Save