From 3514ed2b9de753e69c040778b564c94104ef11fd Mon Sep 17 00:00:00 2001 From: Tim <47110241+timunie@users.noreply.github.com> Date: Fri, 27 Feb 2026 01:53:26 +0100 Subject: [PATCH] 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 --- .../VirtualizingStackPanel.cs | 198 ++++++++++- .../ListBoxVirtualizationIssueTests.cs | 336 ++++++++++++++++++ .../VirtualizingStackPanelTests.cs | 8 +- 3 files changed, 533 insertions(+), 9 deletions(-) create mode 100644 tests/Avalonia.Controls.UnitTests/ListBoxVirtualizationIssueTests.cs diff --git a/src/Avalonia.Controls/VirtualizingStackPanel.cs b/src/Avalonia.Controls/VirtualizingStackPanel.cs index 7e79026c60..9f0446c117 100644 --- a/src/Avalonia.Controls/VirtualizingStackPanel.cs +++ b/src/Avalonia.Controls/VirtualizingStackPanel.cs @@ -8,7 +8,7 @@ using Avalonia.Controls.Utils; using Avalonia.Input; using Avalonia.Interactivity; using Avalonia.Layout; -using Avalonia.Reactive; +using Avalonia.Logging; using Avalonia.Utilities; using Avalonia.VisualTree; @@ -263,8 +263,20 @@ namespace Avalonia.Controls e.Arrange(rect); - if (_viewport.Intersects(rect)) - _scrollAnchorProvider?.RegisterAnchorCandidate(e); + if (e.IsVisible && _viewport.Intersects(rect)) + { + 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; } @@ -307,6 +319,9 @@ namespace Avalonia.Controls { InvalidateMeasure(); + // Always update special elements + UpdateSpecialElementsOnItemsChanged(e); + if (_realizedElements is null) return; @@ -332,7 +347,7 @@ namespace Avalonia.Controls if (e.NewStartingIndex > e.OldStartingIndex) { - insertIndex -= e.OldItems.Count - 1; + insertIndex -= e.OldItems!.Count - 1; } _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) { base.OnItemsControlChanged(oldValue); @@ -351,6 +492,24 @@ namespace Avalonia.Controls oldValue.PropertyChanged -= OnItemsControlPropertyChanged; if (ItemsControl is not null) 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) @@ -860,6 +1019,7 @@ namespace Avalonia.Controls var recycled = recyclePool.Pop(); recycled.SetCurrentValue(Visual.IsVisibleProperty, true); generator.PrepareItemContainer(recycled, item, index); + AddInternalChild(recycled); generator.ItemContainerPrepared(recycled, item, index); return recycled; } @@ -893,6 +1053,7 @@ namespace Avalonia.Controls if (recycleKey is null) { + ItemContainerGenerator!.ClearItemContainer(element); RemoveInternalChild(element); } else if (recycleKey == s_itemIsItsOwnContainer) @@ -909,6 +1070,7 @@ namespace Avalonia.Controls ItemContainerGenerator!.ClearItemContainer(element); PushToRecyclePool(recycleKey, element); element.SetCurrentValue(Visual.IsVisibleProperty, false); + RemoveInternalChild(element); } } @@ -920,7 +1082,12 @@ namespace Avalonia.Controls 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); } @@ -929,9 +1096,30 @@ namespace Avalonia.Controls ItemContainerGenerator!.ClearItemContainer(element); PushToRecyclePool(recycleKey, element); 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) { _recyclePool ??= new(); diff --git a/tests/Avalonia.Controls.UnitTests/ListBoxVirtualizationIssueTests.cs b/tests/Avalonia.Controls.UnitTests/ListBoxVirtualizationIssueTests.cs new file mode 100644 index 0000000000..54fec22e44 --- /dev/null +++ b/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((_, _) => new TextBlock { Height = 50 }), + Height = 100, // Show 2 items + ItemsPanel = new FuncTemplate(() => 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().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() + .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 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(() => 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() + .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() + .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(() => new VirtualizingStackPanel { CacheLength = 0 }), + Template = new FuncControlTemplate(CreateListBoxTemplate), + ItemsSource = letters, + ItemTemplate = new FuncDataTemplate((_, _) => 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().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(Enumerable.Range(0, 100).Select(i => $"Item {i}")); + + var target = new ListBox + { + Template = new FuncControlTemplate(CreateListBoxTemplate), + ItemsSource = items, + ItemTemplate = new FuncDataTemplate((_, _) => new TextBlock { Height = 50 }), + Height = 100, // Show 2 items + ItemsPanel = new FuncTemplate(() => 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(Enumerable.Range(0, 100).Select(i => $"Item {i}")); + + var target = new ListBox + { + Template = new FuncControlTemplate(CreateListBoxTemplate), + ItemsSource = items, + ItemTemplate = new FuncDataTemplate((_, _) => new TextBlock { Height = 50 }), + Height = 100, // Show 2 items + ItemsPanel = new FuncTemplate(() => 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; } + } +} diff --git a/tests/Avalonia.Controls.UnitTests/VirtualizingStackPanelTests.cs b/tests/Avalonia.Controls.UnitTests/VirtualizingStackPanelTests.cs index 0643e0758f..107d9ef9dc 100644 --- a/tests/Avalonia.Controls.UnitTests/VirtualizingStackPanelTests.cs +++ b/tests/Avalonia.Controls.UnitTests/VirtualizingStackPanelTests.cs @@ -141,8 +141,8 @@ namespace Avalonia.Controls.UnitTests } [Theory] - [InlineData(0d, 11)] - [InlineData(0.5d, 21)] + [InlineData(0d, 10)] + [InlineData(0.5d, 20)] public void Scrolling_Up_To_Index_Does_Not_Create_A_Page_Of_Unrealized_Elements(double bufferFactor, int expectedCount) { using var app = App(); @@ -1097,9 +1097,9 @@ namespace Avalonia.Controls.UnitTests itemsControl.ItemsSource = null; 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(3, target.Children.Count); + Assert.Equal(0, target.Children.Count); // Make the panel effectively invisible and set items. container.IsVisible = false;