[iOS] CollectionView: Fix drag-and-drop reordering into empty groups#34151
[iOS] CollectionView: Fix drag-and-drop reordering into empty groups#34151SuthiYuvaraj wants to merge 5 commits intodotnet:mainfrom
Conversation
- Add empty group validation in ReorderableItemsViewController MoveItem - Add bounds checking for source/destination indices - Handle empty group insertion (toItemIndex clamping) - Rewrite GetTargetIndexPathForMove in ReorderableItemsViewDelegator to support redirecting drops to empty groups via FindFirstEmptyGroup - Apply same changes to Items2 (CollectionView2) handlers - Add Issue12008 test host app page (PlatformAffected.iOS) - Add UI tests excluding Windows and Android Fixes dotnet#12008 (iOS portion)
- Bind to explicit ItemCount property instead of ObservableCollection.Count, since CollectionView group headers may not pick up PropertyChanged from the explicitly-implemented INotifyPropertyChanged on ObservableCollection - Override OnCollectionChanged to raise PropertyChanged for ItemCount - Add ReorderCompleted event handler to update status label - Add reorder-completed assertion in UI test
…into fix-12008-iOS
dotnet-policy-service
bot
added
the
partner/syncfusion
sheiksyedm
added
platform/ios
area-controls-collectionview
|
@SuthiYuvaraj could you please duplicate tests? The reason is that the PR agent needs them to validate and explore alternative fixes :) |
There was a problem hiding this comment.
Pull request overview
Fixes iOS/MacCatalyst grouped CollectionView drag-and-drop reordering so items can be dropped into empty groups, and ensures group header supplementary views (e.g., count labels) refresh after a reorder completes.
Changes:
- Update
GetTargetIndexPathForMoveto handle UICollectionView’s fallback behavior when hovering over empty-group header areas and to better constrain target index paths. - Reload grouped sections after
EndInteractiveMovement()so supplementary headers reflect updated data. - Add additional validation/clamping in
MoveItemfor grouped moves (section bounds, empty-group insertion index, row bounds).
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/ReorderableItemsViewDelegator2.cs | Adds iOS/MacCatalyst (Items2) logic to redirect drops into empty groups and constrain target index paths. |
| src/Controls/src/Core/Handlers/Items2/iOS/ReorderableItemsViewController2.cs | Reloads sections after interactive movement to refresh headers; adds grouped move validation/clamping. |
| src/Controls/src/Core/Handlers/Items/iOS/ReorderableItemsViewDelegator.cs | Mirrors the empty-group drop targeting logic for the legacy Items iOS handler. |
| src/Controls/src/Core/Handlers/Items/iOS/ReorderableItemsViewController.cs | Mirrors the header refresh and grouped move validation/clamping for the legacy Items iOS handler. |
| // UICollectionView falls back to proposedIndexPath == originalIndexPath when the | ||
| // user drags over an area with no cells (e.g. an empty group's header region). | ||
| // In that case, redirect to the nearest empty group so the drop can succeed. | ||
| if (originalIndexPath.Equals(proposedIndexPath) && itemsView.CanMixGroups) | ||
| { | ||
| var emptyGroupTarget = FindNearestEmptyGroup(itemsSource, totalSections, originalIndexPath.Section); | ||
| if (emptyGroupTarget != null) | ||
| { | ||
| targetIndexPath = proposedIndexPath; | ||
| return emptyGroupTarget; | ||
| } | ||
| else | ||
| } |
There was a problem hiding this comment.
This change adds iOS/MacCatalyst-specific behavior for dropping into empty groups and for refreshing group headers after reorder. There doesn’t appear to be an existing UI test covering drag-and-drop reordering into an empty group on iOS/MacCatalyst, so regressions here may go unnoticed. Consider adding an Appium UI test (similar to other CollectionView reordering tests) that creates an empty group and verifies the item moves into it and the header count updates.
| // For empty groups, ensure we insert at index 0 | ||
| if (toList.Count == 0) | ||
| { | ||
| toItemIndex = 0; | ||
| } | ||
| else if (toItemIndex > toList.Count) | ||
| { | ||
| toItemIndex = toList.Count; | ||
| } | ||
| else if (toItemIndex < 0) | ||
| { | ||
| toItemIndex = 0; | ||
| } | ||
|
|
||
| var fromItem = fromList[fromItemIndex]; | ||
| SetObserveChanges(fromItemsSource, false); | ||
| SetObserveChanges(toItemsSource, false); | ||
| fromList.RemoveAt(fromItemIndex); | ||
| toList.Insert(toItemIndex, fromItem); |
There was a problem hiding this comment.
In grouped reorders, toItemIndex is clamped using toList.Count before fromList.RemoveAt(fromItemIndex). When reordering within the same group (i.e., fromList and toList are the same instance) and the destination is clamped to toList.Count (append), the subsequent RemoveAt reduces the count and toList.Insert(toItemIndex, ...) can become out-of-range. Consider handling the ReferenceEquals(fromList, toList) case by adjusting/clamping the destination index after removal (or decrementing when moving forward) to guarantee 0 <= toItemIndex <= toList.Count at insert time.
| // For empty groups, ensure we insert at index 0 | ||
| if (toList.Count == 0) | ||
| { | ||
| toItemIndex = 0; | ||
| } | ||
| else if (toItemIndex > toList.Count) | ||
| { | ||
| toItemIndex = toList.Count; | ||
| } | ||
| else if (toItemIndex < 0) | ||
| { | ||
| toItemIndex = 0; | ||
| } | ||
|
|
||
| var fromItem = fromList[fromItemIndex]; | ||
| SetObserveChanges(fromItemsSource, false); | ||
| SetObserveChanges(toItemsSource, false); | ||
| fromList.RemoveAt(fromItemIndex); | ||
| toList.Insert(toItemIndex, fromItem); |
There was a problem hiding this comment.
In grouped reorders, toItemIndex is clamped using toList.Count before fromList.RemoveAt(fromItemIndex). When reordering within the same group (i.e., fromList and toList are the same instance) and the destination is clamped to toList.Count (append), the subsequent RemoveAt reduces the count and toList.Insert(toItemIndex, ...) can become out-of-range. Consider handling the ReferenceEquals(fromList, toList) case by adjusting/clamping the destination index after removal (or decrementing when moving forward) to guarantee 0 <= toItemIndex <= toList.Count at insert time.
| // UICollectionView doesn't refresh supplementary views after interactive movement. | ||
| // Reload sections so group headers reflect updated data (e.g. item counts). | ||
| if (ItemsView?.IsGrouped == true) | ||
| { | ||
| var sectionCount = collectionView.NumberOfSections(); | ||
| if (sectionCount > 0) | ||
| { | ||
| UIView.PerformWithoutAnimation(() => | ||
| collectionView.ReloadSections(NSIndexSet.FromNSRange(new NSRange(0, sectionCount)))); | ||
| } |
There was a problem hiding this comment.
ReloadSections is being called for the entire grouped CollectionView after every interactive movement. For large numbers of groups this can be expensive and may cause unnecessary header re-creation. If the goal is to refresh item-count labels, consider tracking the source/destination sections during MoveItem and reloading only those sections (or otherwise limiting the reloaded section set).
| var sectionCount = collectionView.NumberOfSections(); | ||
| if (sectionCount > 0) | ||
| { | ||
| UIView.PerformWithoutAnimation(() => | ||
| collectionView.ReloadSections(NSIndexSet.FromNSRange(new NSRange(0, sectionCount)))); |
There was a problem hiding this comment.
ReloadSections is being called for the entire grouped CollectionView after every interactive movement. For large numbers of groups this can be expensive and may cause unnecessary header re-creation. If the goal is to refresh item-count labels, consider tracking the source/destination sections during MoveItem and reloading only those sections (or otherwise limiting the reloaded section set).
| var sectionCount = collectionView.NumberOfSections(); | |
| if (sectionCount > 0) | |
| { | |
| UIView.PerformWithoutAnimation(() => | |
| collectionView.ReloadSections(NSIndexSet.FromNSRange(new NSRange(0, sectionCount)))); | |
| var visibleIndexPaths = collectionView.IndexPathsForVisibleItems; | |
| if (visibleIndexPaths != null && visibleIndexPaths.Length > 0) | |
| { | |
| var sectionsToReload = new NSMutableIndexSet(); | |
| foreach (var indexPath in visibleIndexPaths) | |
| { | |
| sectionsToReload.Add((nuint)indexPath.Section); | |
| } | |
| if (sectionsToReload.Count > 0) | |
| { | |
| UIView.PerformWithoutAnimation(() => | |
| collectionView.ReloadSections(sectionsToReload)); | |
| } |
| // UICollectionView falls back to proposedIndexPath == originalIndexPath when the | ||
| // user drags over an area with no cells (e.g. an empty group's header region). | ||
| // In that case, redirect to the nearest empty group so the drop can succeed. | ||
| if (originalIndexPath.Equals(proposedIndexPath) && itemsView.CanMixGroups) | ||
| { | ||
| var emptyGroupTarget = FindNearestEmptyGroup(itemsSource, totalSections, originalIndexPath.Section); | ||
| if (emptyGroupTarget != null) | ||
| { | ||
| targetIndexPath = proposedIndexPath; | ||
| return emptyGroupTarget; | ||
| } |
There was a problem hiding this comment.
This change adds iOS/MacCatalyst-specific behavior for dropping into empty groups and for refreshing group headers after reorder. There doesn’t appear to be an existing UI test covering drag-and-drop reordering into an empty group on iOS/MacCatalyst, so regressions here may go unnoticed. Consider adding an Appium UI test (similar to other CollectionView reordering tests) that creates an empty group and verifies the item moves into it and the header count updates.
3 participants
Issue Description
When using a grouped CollectionView with CanReorderItems="True" and CanMixGroups="True", users could not drag items into empty groups on iOS/MacCatalyst. The item would snap back to its original position instead of being placed in the target empty group. Additionally, group header count labels did not update after a successful reorder.
Root Cause
Two issues in the iOS UICollectionView interactive movement API:
No drop target for empty groups: UICollectionView has no cells in empty sections, so it cannot compute a valid proposedIndexPath for the drop. It falls back to returning originalIndexPath (the item's starting position), effectively preventing the move. The delegator's GetTargetIndexPathForMove override was not handling this fallback case.
Stale group headers after reorder: UICollectionView.EndInteractiveMovement() updates cell positions but does not trigger a refresh of supplementary views (group headers). As a result, header-bound data like item counts remained stale after drag-and-drop.
Description of Change:
ReorderableItemsViewDelegator / ReorderableItemsViewDelegator2 — GetTargetIndexPathForMove: When UICollectionView can't resolve a drop target in an empty group area, it falls back to proposedIndexPath == originalIndexPath. The fix detects this condition (when CanMixGroups is enabled) and redirects the drop to the nearest empty group using FindNearestEmptyGroup, which searches outward from the current section for the closest match.
ReorderableItemsViewController / ReorderableItemsViewController2 — HandleLongPress: After EndInteractiveMovement(), UICollectionView does not refresh supplementary views (group headers). Added a ReloadSections call wrapped in UIView.PerformWithoutAnimation so group header data (e.g. item counts) updates immediately after a reorder completes.
Issues Fixed
Fixes #12008
Additional context:
The issue was also reproduced on Android and has been addressed separately in a dedicated PR #31867
Why no tests in this PR
The test cases for issue #12008 are already included in the companion Android fix PR #31867. Since the tests are shared and validated as part of the Android fix, they were not duplicated in this PR.
Tested the behaviour in the following platforms
Output Screenshot
BeforeiOS.mov
AfteriOS.mov