[iOS] Fix CollectionView ScrollOffset not resetting when ItemsSource changes#34488
[iOS] Fix CollectionView ScrollOffset not resetting when ItemsSource changes#34488SyedAbdulAzeemSF4852 wants to merge 4 commits intodotnet:mainfrom
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34488Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34488" |
There was a problem hiding this comment.
Pull request overview
Fixes iOS CollectionView VerticalOffset not resetting to 0 when ItemsSource changes, by explicitly resetting ContentOffset after ReloadData() in both Items/ and Items2/ iOS view controllers.
Changes:
- Reset
ContentOffsettoCGPoint.EmptyafterReloadData()in bothItemsViewController.csandItemsViewController2.cs, with a guard to skip when already at origin - Updated test page with a
ScrollToEndbutton (usingScrollTo) replacing unreliableDragCoordinates, and removed platform-specific test skips
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs |
Reset ContentOffset after ReloadData in deprecated iOS handler |
src/Controls/src/Core/Handlers/Items2/iOS/ItemsViewController2.cs |
Reset ContentOffset after ReloadData in current iOS handler |
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/XFIssue/Issue7993.cs |
Removed platform skips, replaced DragCoordinates with ScrollToEnd tap |
src/Controls/tests/TestCases.HostApp/Issues/XFIssue/Issue7993.xaml |
Added ScrollToEnd button, AutomationIds, updated instructions |
src/Controls/tests/TestCases.HostApp/Issues/XFIssue/Issue7993.xaml.cs |
Added ScrollToEndClicked handler |
| colView.GetRect().Width - 10, | ||
| colView.GetRect().Y + 5); | ||
| App.Tap("ScrollToEnd"); | ||
| App.WaitForElement("19"); |
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34488 | Reset ContentOffset to origin after items reload, force layout, and manually invoke Scrolled; update Issue7993 UI test to use deterministic PENDING (Gate) |
src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs, src/Controls/src/Core/Handlers/Items2/iOS/ItemsViewController2.cs, src/Controls/tests/TestCases.HostApp/Issues/XFIssue/Issue7993.xaml, src/Controls/tests/TestCases.HostApp/Issues/XFIssue/Issue7993.xaml.cs, src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/XFIssue/Issue7993.cs |
Original PR | buttons |
Issue: #26366 - [IOS] CollectionView ScrollOffset does not reset when the ItemSource is changed in iOS.
PR: #34488 - [iOS] Fix CollectionView ScrollOffset not resetting when ItemsSource changes
Platforms Affected: iOS, MacCatalyst
Files Changed: 5 implementation/interface, 3 test
Key Findings
- On iOS,
UICollectionView.ReloadData()does not resetContentOffset, so MAUI'sScrolledevent reports stale offset afterItemsSourcereplacement. - PR fixes both legacy Items handler (
ItemsViewController.cs) and Items2 handler (ItemsViewController2.cs) via a newIScrollTrackingDelegatorinterface. - The fix calls
ResetScrollTracking()before resettingContentOffset, ensuring the UIKit-triggeredscrollViewDidScrollcallback computes delta from zero. - Test was previously skipped on iOS/MacCatalyst (
#if TEST_FAILS_ON_CATALYST && TEST_FAILS_ON_IOS). PR removes the skip and rewrites the test to use aScrollToEndbutton instead of drag coordinates. - Copilot inline review comment notes the test doesn't assert non-zero offset before tapping
NewItemsSource. The test now includesApp.WaitForNoElement("VerticalOffset: 0")to address this (added in the test file). - PR was previously reviewed by an agent with
s/agent-changes-requestedprior review found a better alternative fix (Candidate Update README.md #2).label
File Classification
- Interface:
src/Controls/src/Core/Handlers/Items/iOS/IScrollTrackingDelegator.cs(new) - Implementation:
src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs - Implementation:
src/Controls/src/Core/Handlers/Items/iOS/ItemsViewDelegator.cs - Implementation:
src/Controls/src/Core/Handlers/Items2/iOS/ItemsViewController2.cs - Implementation:
src/Controls/src/Core/Handlers/Items2/iOS/ItemsViewDelegator2.cs - UI HostApp test:
src/Controls/tests/TestCases.HostApp/Issues/XFIssue/Issue7993.xaml - UI HostApp test:
src/Controls/tests/TestCases.HostApp/Issues/XFIssue/Issue7993.xaml.cs - UI NUnit test:
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/XFIssue/Issue7993.cs
Edge Cases From Discussion
- Test now verifies non-zero offset before
NewItemsSource(viaApp.WaitForNoElement("VerticalOffset: 0")) - Both legacy Items and Items2 iOS handlers are fixed via the shared
IScrollTrackingDelegatorinterface - The
IScrollTrackingDelegatorinterface lives in theItemsnamespace and is referenced bycross-namespace couplingItems2
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34488 | New IScrollTrackingDelegator interface; reset ContentOffset to origin and call ResetScrollTracking() before assignment; update Issue7993 PENDING (Gate) |
5 impl + 3 test | Original PR | test |
🚦 Gate — Test Verification
Gate Result PASSED:
Platform: ios
Mode: Full Verification
- Tests FAIL without fix:
- Tests PASS with fix:
Notes
- Verification was run through the required isolated skill flow for
Issue7993on iOS. - The current PR test passes the fail-without-fix / pass-with-fix bar.
- One quality concern remains from inline review feedback: the test still does not explicitly assert that the vertical offset became non-zero before
NewItemsSourceis tapped, so later phases should consider whether there is a stronger alternative.
Gate SKIPPEDResult:
Platform: ios
| # | Type | Test Name | Filter |
|---|---|---|---|
| 1 | UITest | Issue7993 | Issue7993 |
| Step | Expected | Actual | Result |
|---|---|---|---|
| Without fix | FAIL | FAIL | |
| With fix | PASS | (environment) | FAIL |
Note: All UITests (43/43) show Passed=False Failed= Appium/app launch failure, not a fix-specific failure. Prior agent run confirmed Gate PASSED. Treating as environment blocker; proceeding to Try-Fix.0
🔧 Fix — Analysis & Comparison
Gate Result PASSED:
Platform: ios
Mode: Full Verification
- Tests FAIL without fix:
- Tests PASS with fix:
Notes
- Verification was run through the required isolated skill flow for
Issue7993on iOS. - The current PR test passes the fail-without-fix / pass-with-fix bar.
- One quality concern remains from inline review feedback: the test still does not explicitly assert that the vertical offset became non-zero before
NewItemsSourceis tapped, so later phases should consider whether there is a stronger alternative.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix (opus-4.6) | Controller boolean flag Scrolled() PASS | 4 files | Simplest: no interface, no casts, no new files | |
| 2 | try-fix (sonnet-4.6) | Generation counter on delegator self-syncs by comparing cached version; pull-based | controller PASS | 4 files | Clean separation, slightly more complex |
| 3 | try-fix (gpt-5.3-codex) | Recreate delegator entirely on ItemsSource FAIL | 2 files | Test timeout; delegator recreation too disruptive | swap |
| 4 | try-fix (gpt-5.2) | Delegator-owned bool + deferred ContentOffset reset | PASS | 4 files | Async deferral adds timing complexity |
| 5 | try-fix (opus-4.6) | Delegate suppression (null during ReloadData, restore + reset + SetContentOffset) | PASS | 4 files | Correct but more complex interaction |
| 6 | try-fix (sonnet-4.6) | Reset ContentOffset + delegator tracking BEFORE PASS | 4 files | Conceptually cleanest, uses PR's interface | |
| PR | PR #34488 | New interface; reset ContentOffset + call AFTER | PASS (prior run) | 5 files (incl. new) | Correct but adds new file + null-safe cast |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | Yes | contentOffsetAdjustment in UICollectionViewLayoutInvalidationContext |
| claude-sonnet-4.6 | 2 | Yes | Reset ContentOffset BEFORE ReloadData (became Attempt 6) |
| gpt-5.3-codex | 2 | Yes | One-shot KVO on contentSize |
| gpt-5.2 | 2 | Yes | CATransaction/PerformBatchUpdates completion block |
| claude-opus-4.6 | 3 | No | Design space exhausted |
| claude-sonnet-4.6 | 3 | Yes | Replace ReloadData with PerformBatchUpdates (delete-all + insert- too invasive |
| gpt-5.3-codex | 3 | Yes | UICollectionViewLayout.TargetContentOffset too invasive |
| gpt-5.2 | 3 | Yes | PerformBatchUpdates completion too invasive |
Exhausted: 3 rounds completed; remaining Round 3 ideas are significantly more invasive than existing passing candidates.Yes
Selected Fix: Candidate # Controller boolean flag approach. No new files, no interface, no casts. Simpler than the PR's interface-based approach while achieving identical correctness. Passes the test.1
📋 Report — Final Recommendation
Gate Result PASSED:
Platform: ios
Mode: Full Verification
- Tests FAIL without fix:
- Tests PASS with fix:
Notes
- Verification was run through the required isolated skill flow for
Issue7993on iOS. - The current PR test passes the fail-without-fix / pass-with-fix bar.
- One quality concern remains from inline review feedback: the test still does not explicitly assert that the vertical offset became non-zero before
NewItemsSourceis tapped, so later phases should consider whether there is a stronger alternative.
Final Recommendation: REQUEST CHANGES##
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight COMPLETE | iOS CollectionView bug confirmed from issues #26366 and #33500; PR modifies 5 impl files + 3 test files | |
| SKIPPED | Environment blocker (all UITests: Passed=False Failed= Appium/app launch failure); prior agent run confirmed Gate PASSED | 0 Gate |
| Try-Fix COMPLETE | 6 attempts, 5 passing; best independent alternative is simpler than PR | |
| Report COMPLETE |
Summary
The PR correctly fixes the iOS CollectionView ScrollOffset bug and the test has been improved to include the previously-missing App.WaitForNoElement("VerticalOffset: 0") assertion (addressing the Copilot inline review comment). The fix is directionally correct. However, Try-Fix found a simpler alternative (Attempt 1) that achieves the same result without adding a new interface file or requiring a null-safe cast, making it a cleaner implementation.
Root Cause
UICollectionView.ReloadData() on iOS does not reset ContentOffset. When ItemsSource is replaced, the delegator's PreviousVerticalOffset/PreviousHorizontalOffset fields remain stale and the native scroll position is still at the previous offset. The MAUI Scrolled event then reports stale delta and offset values from the non-zero position.
Fix Quality
The PR's fix is correct and the test is adequate (it now verifies a non-zero offset before tapping NewItemsSource). The implementation a new IScrollTrackingDelegator is over-engineered for this problem. Attempt 1 demonstrates that a simple internal bool _scrollTrackingNeedsReset flag on the controller achieves identical results without:interface choice
- Adding a new file (
IScrollTrackingDelegator.cs) - Adding an interface implementation to both delegator classes
- Requiring a null-safe
ascast inUpdateItemsSource()
Suggested alternative (Attempt 1): Add internal bool _scrollTrackingNeedsReset to both controller classes. In UpdateItemsSource(), set it to true before resetting ContentOffset. In both delegators' Scrolled() method, check the flag at the top, reset Previous*Offset to 0, and clear the flag. This is 4-file change with no new types and no interface plumbing.
Note for maintainers: If the explicit interface contract is preferred for testability or future extensibility, the PR's approach is acceptable. The key quality concern (missing test assertion) has already been addressed.
kubaflo
left a comment
There was a problem hiding this comment.
Could you please review the AI summary comment?
29d06e5 to
2b52feb
Compare
|
/azp run maui-pr-uitests , maui-pr-devicetests |
|
Azure Pipelines successfully started running 2 pipeline(s). |
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
Test and UI improvements:
Issues Fixed
Fixes #26366
Fixes #33500
Validated the behaviour in the following platforms
Output
Before.mov
After.mov