[iOS & Mac] Fix for Incorrect ItemsViewScrolledEventArgs Values in CollectionView with Grouped Items#34240
Conversation
|
Hey there @@SyedAbdulAzeemSF4852! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect ItemsViewScrolledEventArgs visible-item indices for grouped CollectionView on iOS/Mac by ensuring visible NSIndexPaths are ordered consistently across section boundaries, and adds a HostApp repro page + Appium UI test for issue #17664.
Changes:
- iOS (legacy
Items/) and iOS/MacCatalyst (currentItems2/) delegators now sort visibleNSIndexPaths bySectionthenRow. - Added HostApp issue page (
Issue17664) that displays the “last visible item” text while scrolling grouped content. - Added an Appium UI test (
Issue17664) validating the label updates after a programmatic scroll.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/Controls/src/Core/Handlers/Items2/iOS/ItemsViewDelegator2.cs | Updates visible index-path ordering for grouped sources (Items2 iOS/MacCatalyst handler). |
| src/Controls/src/Core/Handlers/Items/iOS/ItemsViewDelegator.cs | Updates visible index-path ordering for grouped sources (legacy iOS handler). |
| src/Controls/tests/TestCases.HostApp/Issues/Issue17664.cs | Adds repro page using grouped CollectionView + Scrolled handler output. |
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue17664.cs | Adds Appium UI regression test for the issue. |
| // Sort visible item index paths by section and then by row for consistent order in both grouped and ungrouped sources | ||
| var indexPathsForVisibleItems = collectionView.IndexPathsForVisibleItems.OrderBy(x => x.Section).ThenBy(x => x.Row).ToList(); | ||
|
|
There was a problem hiding this comment.
GetVisibleItemsIndexPath now orders IndexPathsForVisibleItems by Section/Row, but GetCenteredIndexPath still orders only by Row. If IndexPathForItemAtPoint returns null (e.g., center point lands between cells/headers), the fallback uses the incorrectly-sorted "first" index path, which can still produce wrong CenterItemIndex for grouped sources. Update GetCenteredIndexPath to use the same Section/Row ordering for consistency.
| // Sort visible item index paths by section and then by row for consistent order in both grouped and ungrouped sources | ||
| var indexPathsForVisibleItems = collectionView.IndexPathsForVisibleItems.OrderBy(x => x.Section).ThenBy(x => x.Row).ToList(); | ||
|
|
There was a problem hiding this comment.
GetVisibleItemsIndexPath now orders IndexPathsForVisibleItems by Section/Row, but GetCenteredIndexPath still orders only by Row. If IndexPathForItemAtPoint returns null (e.g., center point lands between cells/headers), the fallback uses the incorrectly-sorted "first" index path, which can still produce wrong CenterItemIndex for grouped sources. Update GetCenteredIndexPath to use the same Section/Row ordering for consistency.
| @@ -0,0 +1,31 @@ | |||
| #if TEST_FAILS_ON_ANDROID // PR Link - https://github.com/dotnet/maui/pull/31437 | |||
There was a problem hiding this comment.
This test file is wrapped in #if TEST_FAILS_ON_ANDROID, which means it will never run in the Android UI test project (where TEST_FAILS_ON_ANDROID is not defined). If the intent is to validate the cross-platform fix (and especially if the linked Android fix has landed), remove this guard or switch to a runtime skip so Android gets regression coverage too.
| #if WINDOWS | ||
| Thread.Sleep(1000); | ||
| #endif | ||
|
|
||
| var resultItem = App.WaitForElement("Issue17664DescriptionLabel").GetText(); | ||
| Assert.That(resultItem, Is.EqualTo("Category C item #2")); |
There was a problem hiding this comment.
Avoid platform-specific #if WINDOWS + Thread.Sleep in the test body; it’s brittle and can still be flaky. Prefer waiting for the expected UI state (e.g., wait until the label text becomes the expected value using an existing wait helper) so the test is stable across platforms without conditional compilation.
| _collectionView.Scrolled += (s, e) => | ||
| { | ||
| var flatItems = _groupedItems.SelectMany(group => group).ToList(); | ||
| if (e.LastVisibleItemIndex < flatItems.Count) | ||
| { | ||
| descriptionLabel.Text = flatItems[e.LastVisibleItemIndex]; | ||
| } | ||
| }; |
There was a problem hiding this comment.
The Scrolled handler assumes LastVisibleItemIndex is always non-negative. On some platforms/conditions ItemsViewScrolledEventArgs can report -1, and the current check (< flatItems.Count) would still pass and then index flatItems[-1], causing an exception. Add a >= 0 guard (or otherwise handle -1) before indexing into flatItems.
|
|
||
| namespace Maui.Controls.Sample.Issues; | ||
|
|
||
| [Issue(IssueTracker.Github, 17664, "Incorrect ItemsViewScrolledEventArgs in CollectionView when IsGrouped is set to true", PlatformAffected.iOS | PlatformAffected.Android)] |
There was a problem hiding this comment.
The IssueAttribute marks this as affecting iOS/Android only, but this PR targets iOS + Mac as well. Consider adding PlatformAffected.macOS (and any other affected platforms) so the metadata matches where this scenario is expected to reproduce/be validated.
| [Issue(IssueTracker.Github, 17664, "Incorrect ItemsViewScrolledEventArgs in CollectionView when IsGrouped is set to true", PlatformAffected.iOS | PlatformAffected.Android)] | |
| [Issue(IssueTracker.Github, 17664, "Incorrect ItemsViewScrolledEventArgs in CollectionView when IsGrouped is set to true", PlatformAffected.iOS | PlatformAffected.Android | PlatformAffected.macOS)] |
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
Description of Change
iOS (ItemsViewDelegator.cs and ItemsViewDelegator2.cs)
Issues Fixed
Fixes #17664
Validated the behaviour in the following platforms
Android fix PR: #31437
Output
iOS_Before.mov
iOS_After.mov