diff --git a/src/Controls/src/Core/Handlers/Items/Android/MauiCarouselRecyclerView.cs b/src/Controls/src/Core/Handlers/Items/Android/MauiCarouselRecyclerView.cs index 24b18159ca99..c3e62cbbbb90 100644 --- a/src/Controls/src/Core/Handlers/Items/Android/MauiCarouselRecyclerView.cs +++ b/src/Controls/src/Core/Handlers/Items/Android/MauiCarouselRecyclerView.cs @@ -18,6 +18,7 @@ public class MauiCarouselRecyclerView : MauiRecyclerView _oldViews; CarouselViewOnGlobalLayoutListener _carouselViewLayoutListener; @@ -223,6 +224,9 @@ void CollectionItemsSourceChanged(object sender, System.Collections.Specialized. if (!(ItemsViewAdapter.ItemsSource is IItemsViewSource observableItemsSource)) return; + // Set flag to disable animation during collection changes + _isInternalPositionUpdate = true; + var carouselPosition = Carousel.Position; var currentItemPosition = observableItemsSource.GetPosition(Carousel.CurrentItem); var count = observableItemsSource.Count; @@ -267,6 +271,7 @@ void CollectionItemsSourceChanged(object sender, System.Collections.Specialized. if (removingAnyPrevious) { + _isInternalPositionUpdate = false; return; } @@ -294,23 +299,32 @@ void CollectionItemsSourceChanged(object sender, System.Collections.Specialized. GetDispatcher() .Dispatch(() => { - // If someone called explicit ScrollTo before the dispatched - // callback was delivered then don't override it. - if (_scrollToCounter == savedScrollToCounter) + try { - SetCurrentItem(carouselPosition); - UpdatePosition(carouselPosition); - ScrollToPosition(carouselPosition); + // If someone called explicit ScrollTo before the dispatched + // callback was delivered then don't override it. + if (_scrollToCounter == savedScrollToCounter) + { + SetCurrentItem(carouselPosition); + UpdatePosition(carouselPosition); + //If we are adding or removing the last item we need to update + //the inset that we give to items so they are centered + if (e.NewStartingIndex == count - 1 || removingLastElement) + { + UpdateItemDecoration(); + } + + UpdateVisualStates(); + + ScrollToPosition(carouselPosition); + } } - - //If we are adding or removing the last item we need to update - //the inset that we give to items so they are centered - if (e.NewStartingIndex == count - 1 || removingLastElement) + finally { - UpdateItemDecoration(); + // Reset flag after collection operations complete, + // always reset even if ScrollTo was called or an exception occurred + _isInternalPositionUpdate = false; } - - UpdateVisualStates(); }); } @@ -450,6 +464,11 @@ void CarouselViewScrolled(object sender, ItemsViewScrolledEventArgs e) if (!_initialized || !_isVisible) return; + // Do not process scroll events triggered by internal collection changes + // (e.g. item inserted at index 0 shifts RecyclerView scroll offset) + if (_isInternalPositionUpdate) + return; + _noNeedForScroll = false; var index = e.CenterItemIndex; if (Carousel?.Loop == true) @@ -510,7 +529,7 @@ void IMauiCarouselRecyclerView.UpdateFromCurrentItem() if (_gotoPosition == -1 && currentItemPosition != carouselPosition) { _gotoPosition = currentItemPosition; - ItemsView.ScrollTo(currentItemPosition, position: Microsoft.Maui.Controls.ScrollToPosition.Center, animate: Carousel.AnimateCurrentItemChanges); + ScrollToItemPosition(currentItemPosition, Carousel.AnimateCurrentItemChanges); } } @@ -531,7 +550,6 @@ void IMauiCarouselRecyclerView.UpdateFromPosition() return; } - if (carouselPosition >= itemCount || carouselPosition < 0) throw new IndexOutOfRangeException($"Can't set CarouselView to position {carouselPosition}. ItemsSource has {itemCount} items."); @@ -548,12 +566,21 @@ void IMauiCarouselRecyclerView.UpdateFromPosition() if (_gotoPosition == -1 && !Carousel.IsDragging && !Carousel.IsScrolling && centerPosition != carouselPosition) { _gotoPosition = carouselPosition; - - ItemsView.ScrollTo(carouselPosition, position: Microsoft.Maui.Controls.ScrollToPosition.Center, animate: Carousel.AnimatePositionChanges); + ScrollToItemPosition(carouselPosition, Carousel.AnimatePositionChanges); } SetCurrentItem(carouselPosition); } + void ScrollToItemPosition(int position, bool shouldAnimate) + { + if (position < 0 || position >= (ItemsViewAdapter?.ItemsSource?.Count ?? 0)) + return; + + // Disable animation during collection changes to prevent cascading scroll events + var animate = shouldAnimate && !_isInternalPositionUpdate; + ItemsView.ScrollTo(position, position: Microsoft.Maui.Controls.ScrollToPosition.Center, animate: animate); + } + void AddLayoutListener() { if (_carouselViewLayoutListener is not null) diff --git a/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs b/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs index 624a1db7025a..a9fdd3d35b10 100644 --- a/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs +++ b/src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs @@ -26,6 +26,7 @@ public partial class CarouselViewHandler : ItemsViewHandler WScrollBarVisibility? _verticalScrollBarVisibilityWithoutLoop; Size _currentSize; bool _isCarouselViewReady; + bool _isInternalPositionUpdate; int _gotoPosition = -1; NotifyCollectionChangedEventHandler _collectionChanged; readonly WeakNotifyCollectionChangedProxy _proxy = new(); @@ -334,9 +335,7 @@ bool IsValidPosition(int position) void SetCarouselViewPosition(int position) { if (ItemCount == 0) - { return; - } if (!IsValidPosition(position)) return; @@ -418,7 +417,9 @@ void UpdateCurrentItem() return; } - ItemsView.ScrollTo(currentItemPosition, position: ScrollToPosition.Center, animate: ItemsView.AnimateCurrentItemChanges); + // Disable animation during collection changes to prevent cascading scroll events + var animate = ItemsView.AnimateCurrentItemChanges && !_isInternalPositionUpdate; + ItemsView.ScrollTo(currentItemPosition, position: ScrollToPosition.Center, animate: animate); } void UpdatePosition() @@ -566,34 +567,53 @@ void OnScrollViewChanged(object sender, ScrollViewerViewChangedEventArgs e) void OnCollectionItemsSourceChanged(object sender, NotifyCollectionChangedEventArgs e) { - var carouselPosition = ItemsView.Position; - var currentItemPosition = GetItemPositionInCarousel(ItemsView.CurrentItem); - var count = (sender as IList).Count; + // Set flag to disable animation during collection changes + _isInternalPositionUpdate = true; + + try + { + var carouselPosition = ItemsView.Position; + var currentItemPosition = GetItemPositionInCarousel(ItemsView.CurrentItem); + var count = (sender as IList).Count; - bool removingCurrentElement = currentItemPosition == -1; - bool removingLastElement = e.OldStartingIndex == count; - bool removingFirstElement = e.OldStartingIndex == 0; - bool removingCurrentElementButNotFirst = removingCurrentElement && removingLastElement && ItemsView.Position > 0; + bool removingCurrentElement = currentItemPosition == -1; + bool removingLastElement = e.OldStartingIndex == count; + bool removingFirstElement = e.OldStartingIndex == 0; + bool removingCurrentElementButNotFirst = removingCurrentElement && removingLastElement && ItemsView.Position > 0; - if (removingCurrentElementButNotFirst) - { - carouselPosition = ItemsView.Position - 1; + if (removingCurrentElementButNotFirst) + { + carouselPosition = ItemsView.Position - 1; + } + else if (removingFirstElement && !removingCurrentElement) + { + carouselPosition = currentItemPosition; + } - } - else if (removingFirstElement && !removingCurrentElement) - { - carouselPosition = currentItemPosition; - } + // If we are adding a new item make sure to maintain the CurrentItemPosition + else if (e.Action == NotifyCollectionChangedAction.Add + && currentItemPosition != -1) + { + carouselPosition = currentItemPosition; + } + + if (ItemsView.ItemsUpdatingScrollMode == ItemsUpdatingScrollMode.KeepLastItemInView) + { + carouselPosition = count == 0 ? 0 : count - 1; + } + else if (ItemsView.ItemsUpdatingScrollMode == ItemsUpdatingScrollMode.KeepItemsInView) + { + carouselPosition = 0; + } - // If we are adding a new item make sure to maintain the CurrentItemPosition - else if (e.Action == NotifyCollectionChangedAction.Add - && currentItemPosition != -1) + SetCarouselViewCurrentItem(carouselPosition); + SetCarouselViewPosition(carouselPosition); + } + finally { - carouselPosition = currentItemPosition; + // Reset flag after collection operations complete + _isInternalPositionUpdate = false; } - - SetCarouselViewCurrentItem(carouselPosition); - SetCarouselViewPosition(carouselPosition); } void OnListViewSizeChanged(object sender, SizeChangedEventArgs e) => Resize(e.NewSize); diff --git a/src/Controls/src/Core/Handlers/Items2/iOS/CarouselViewController2.cs b/src/Controls/src/Core/Handlers/Items2/iOS/CarouselViewController2.cs index e302b76fec97..639f2c450a82 100644 --- a/src/Controls/src/Core/Handlers/Items2/iOS/CarouselViewController2.cs +++ b/src/Controls/src/Core/Handlers/Items2/iOS/CarouselViewController2.cs @@ -16,6 +16,7 @@ public class CarouselViewController2 : ItemsViewController2 { bool _isRotating = false; bool _isUpdating = false; + bool _isInternalCollectionUpdate = false; int _section = 0; bool _wasDetachedFromWindow = false; CarouselViewLoopManager _carouselViewLoopManager; @@ -209,6 +210,7 @@ void TearDown(CarouselView carouselView) _carouselViewLoopManager = null; _isUpdating = false; _isRotating = false; + _isInternalCollectionUpdate = false; } internal void UpdateScrollingConstraints() @@ -319,14 +321,21 @@ void CollectionViewUpdating(object sender, NotifyCollectionChangedEventArgs e) { _positionAfterUpdate = GetPositionWhenAddingItems(carouselPosition, currentItemPosition); } + + // Suppress any scroll-driven SetPosition calls that UIKit fires during the batch update + _isInternalCollectionUpdate = true; } [UnconditionalSuppressMessage("Memory", "MEM0003", Justification = "Proven safe in test: MemoryTests.HandlerDoesNotLeak")] void CollectionViewUpdated(object sender, NotifyCollectionChangedEventArgs e) { + // Clear before anything else so SetPosition/SetCurrentItem called from this method are not suppressed + _isInternalCollectionUpdate = false; + int targetPosition; if (_positionAfterUpdate == -1) { + _isUpdating = false; return; } @@ -353,7 +362,6 @@ void CollectionViewUpdated(object sender, NotifyCollectionChangedEventArgs e) } } - _isUpdating = false; ScrollToPosition(targetPosition, targetPosition, false, true); } @@ -364,7 +372,7 @@ int GetPositionWhenAddingItems(int carouselPosition, int currentItemPosition) return currentItemPosition != -1 ? currentItemPosition : carouselPosition; } - private int GetTargetPosition() + int GetTargetPosition() { if (ItemsSource.ItemCount == 0) { @@ -486,6 +494,12 @@ void ScrollToPosition(int goToPosition, int carouselPosition, bool animate, bool internal void SetPosition(int position) { + // Suppress spurious calls from UIKit scroll callbacks during a collection batch update + if (_isInternalCollectionUpdate) + { + return; + } + if (ItemsView is not CarouselView carousel) { return; diff --git a/src/Controls/tests/TestCases.HostApp/Issues/Issue29529.cs b/src/Controls/tests/TestCases.HostApp/Issues/Issue29529.cs new file mode 100644 index 000000000000..14ffbcd056c7 --- /dev/null +++ b/src/Controls/tests/TestCases.HostApp/Issues/Issue29529.cs @@ -0,0 +1,113 @@ +using System.Collections.ObjectModel; + +namespace Maui.Controls.Sample.Issues; + +[Issue(IssueTracker.Github, 29529, "CurrentItemChangedEventArgs and PositionChangedEventArgs Not Updating Correctly in CarouselView", PlatformAffected.UWP | PlatformAffected.Android)] +public class Issue29529 : ContentPage +{ + int _positionChangedCount = 0; + int _currentItemChangedCount = 0; + + public Issue29529() + { + var verticalStackLayout = new VerticalStackLayout(); + var carouselItems = new ObservableCollection + { + "Item 1", + "Item 2", + "Item 3", + "Item 4", + "Item 5", + "Item 6", + }; + + CarouselView carouselView = new CarouselView + { + ItemsSource = carouselItems, + AutomationId = "carouselview", + Position = 3, + ItemsUpdatingScrollMode = ItemsUpdatingScrollMode.KeepItemsInView, + Loop = false, + HeightRequest = 300, + ItemTemplate = new DataTemplate(() => + { + var grid = new Grid + { + Padding = 10 + }; + + var label = new Label + { + VerticalOptions = LayoutOptions.Center, + HorizontalOptions = LayoutOptions.Center, + FontSize = 18, + }; + label.SetBinding(Label.TextProperty, "."); + label.SetBinding(Label.AutomationIdProperty, "."); + + grid.Children.Add(label); + return grid; + }), + HorizontalOptions = LayoutOptions.Fill, + }; + + var positionLabel = new Label + { + AutomationId = "positionLabel", + Text = $"Current Position: {carouselView.Position}", + HorizontalOptions = LayoutOptions.Center, + Padding = new Thickness(20), + }; + + var itemLabel = new Label + { + AutomationId = "itemLabel", + Text = $"Current Item: {carouselView.CurrentItem}", + HorizontalOptions = LayoutOptions.Center, + Padding = new Thickness(20), + }; + + var eventCountLabel = new Label + { + AutomationId = "eventCountLabel", + Text = "PositionChanged: 0, CurrentItemChanged: 0", + HorizontalOptions = LayoutOptions.Center, + Padding = new Thickness(20), + }; + + carouselView.PositionChanged += (s, e) => + { + _positionChangedCount++; + positionLabel.Text = $"Current Position: {e.CurrentPosition}, Previous Position: {e.PreviousPosition}"; + eventCountLabel.Text = $"PositionChanged: {_positionChangedCount}, CurrentItemChanged: {_currentItemChangedCount}"; + }; + + carouselView.CurrentItemChanged += (s, e) => + { + _currentItemChangedCount++; + itemLabel.Text = $"Current Item: {e.CurrentItem}, Previous Item: {e.PreviousItem}"; + eventCountLabel.Text = $"PositionChanged: {_positionChangedCount}, CurrentItemChanged: {_currentItemChangedCount}"; + }; + + var insertButton = new Button + { + Text = "Insert item at index 0", + AutomationId = "InsertButton", + Margin = new Thickness(20), + }; + + insertButton.Clicked += (sender, e) => + { + _positionChangedCount = 0; + _currentItemChangedCount = 0; + carouselItems.Insert(0, "Item 0"); + }; + + verticalStackLayout.Children.Add(carouselView); + verticalStackLayout.Children.Add(insertButton); + verticalStackLayout.Children.Add(positionLabel); + verticalStackLayout.Children.Add(itemLabel); + verticalStackLayout.Children.Add(eventCountLabel); + Content = verticalStackLayout; + } +} diff --git a/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue29529.cs b/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue29529.cs new file mode 100644 index 000000000000..a1bd51fb3d79 --- /dev/null +++ b/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue29529.cs @@ -0,0 +1,28 @@ +using NUnit.Framework; +using UITest.Appium; +using UITest.Core; + +namespace Microsoft.Maui.TestCases.Tests.Issues; + +public class Issue29529 : _IssuesUITest +{ + public override string Issue => "CurrentItemChangedEventArgs and PositionChangedEventArgs Not Updating Correctly in CarouselView"; + + public Issue29529(TestDevice device) + : base(device) + { } + + [Test] + [Category(UITestCategories.CarouselView)] + public void Issue29529VerifyPreviousPositionOnInsert() + { + App.WaitForElement("carouselview"); + App.Tap("InsertButton"); + var text = App.FindElement("positionLabel").GetText(); + Assert.That(text, Is.EqualTo("Current Position: 0, Previous Position: 3")); + text = App.FindElement("itemLabel").GetText(); + Assert.That(text, Is.EqualTo("Current Item: Item 0, Previous Item: Item 4")); + text = App.FindElement("eventCountLabel").GetText(); + Assert.That(text, Is.EqualTo("PositionChanged: 1, CurrentItemChanged: 1")); + } +}