-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[iOS] Fix for Incorrect Scroll in Loop Mode When CurrentItem Is Not Found in ItemsSource #32141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[iOS] Fix for Incorrect Scroll in Loop Mode When CurrentItem Is Not Found in ItemsSource #32141
Conversation
…and CurrentItem is set to an item not in the ItemsSource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug where CarouselView incorrectly scrolls to the last item in loop mode when CurrentItem is set to a value not present in ItemsSource. The fix adds early return checks when an invalid item index (-1) is detected, preventing the unwanted scroll behavior.
Key Changes:
- Added validation to prevent scrolling when target position is invalid (< 0)
- Implemented UI tests to verify the fix works correctly
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Controls/src/Core/Handlers/Items/iOS/CarouselViewController.cs | Added guard clause to return early if goToPosition is negative |
| src/Controls/src/Core/Handlers/Items2/iOS/CarouselViewController2.cs | Added guard clause to return early if goToPosition is negative |
| src/Controls/tests/TestCases.HostApp/Issues/Issue32139.cs | Created UI test page demonstrating the issue with CarouselView loop mode |
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue32139.cs | Created NUnit test to validate no scroll occurs when selecting invalid item |
|
|
||
| void ScrollToPosition(int goToPosition, int carouselPosition, bool animate, bool forceScroll = false) | ||
| { | ||
| if (goToPosition < 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is < 0? "When CurrentItem is not found in ItemsSource, GetIndexForItem returns -1; in loop mode, CarouselViewLoopManager.GetCorrectedIndexPathFromIndex(-1) adds 1, incorrectly converting it to 0, which results in an unintended scroll to the last duplicated item." Could we apply fixes in the GetCorrectedIndexPathFromIndex directly?
Could we include logging? Silent failures are hard to debug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsuarezruiz , I’ve updated the PR template for CV1.
We can handle this in GetCorrectedIndexPathFromIndex by creating an invalid NSIndexPath with the invalid index. However, I think it’s better for both CV1 and CV2 to return early from ScrollToPosition when goToPosition < 0, rather than allowing the invalid value to propagate through the scrolling call chain. This helps prevent unintended behavior earlier in the flow.
As for logging, I’ve added a debug log
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Issue Details
Root Cause
CarouselViewHandler1 :
CarouselViewHandler2 :
Description of Change
Issues Fixed
Fixes #32139
Validated the behaviour in the following platforms
Output
Before.mov
After.mov