[iOS] CarouselView2: Update internal scroll indicators for compositional layout#33639
[iOS] CarouselView2: Update internal scroll indicators for compositional layout#33639SubhikshaSf4851 wants to merge 4 commits intodotnet:inflight/currentfrom
Conversation
dotnet-policy-service
bot
added
community ✨
Tamilarasan-Paranthaman
added
platform/ios
area-controls-collectionview
SubhikshaSf4851
changed the title
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Fixes iOS (and Mac Catalyst) CarouselView2 scroll indicator visibility by propagating ScrollBarVisibility updates to the internal UIScrollView used by UICollectionViewCompositionalLayout.
Changes:
- Update iOS
UICollectionViewscrollbar visibility extensions to also update the internalUIScrollViewwhen usingUICollectionViewCompositionalLayout. - Add a device test to validate CarouselView2 scrollbar visibility updates on iOS.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Core/src/Platform/iOS/CollectionViewExtensions.cs | Propagates scroll indicator visibility to the internal UIScrollView used by compositional layouts (CV2). |
| src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs | Adds an iOS device test intended to validate the CarouselView2 scroll indicator behavior. |
src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs
Outdated
Show resolved
Hide resolved
src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs
Outdated
Show resolved
Hide resolved
src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs
Outdated
Show resolved
Hide resolved
rmarinho
added
s/agent-changes-requested
I’ve updated the changes based on the review. Specifically:
|
rmarinho
added
s/agent-approved
kubaflo
added
s/agent-fix-implemented
kubaflo
added
s/agent-approved
|
/azp run maui-pr-uitests , maui-pr-devicetests |
|
Azure Pipelines successfully started running 2 pipeline(s). |
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Could you please review the Ai's summary comment?
kubaflo
added
s/agent-fix-win
Updated the PR description to clarify that the macOS snapshot changes for CarouselViewShouldScrollToRightPosition.png and IndicatorViewWithTemplatedIcon.png are intentional. These baselines were regenerated to reflect the current correct rendering after this fix.
BeginInvokeOnMainThread is intentional here. LayoutIfNeeded() forces an expensive synchronous layout pass and doesn't guarantee the internal scroll view is created by UIKit at that point. Deferring to the next run loop iteration is the safer and more efficient approach. no changes are needed
Polling isn't necessary here. The condition depends only on a BeginInvokeOnMainThread callback — which runs on the same thread as the test. await Task.Delay(100) yields back to the run loop, which drains the callback in microseconds. 100ms is more than sufficient and no changes are needed.
Avoided VerifyScreenshot() intentionally — scroll indicators on iOS are translucent and only visible while scrolling, so a static screenshot would pass even when the bug is present. Asserting the property value directly is more reliable and not susceptible to visual flakiness. |
PureWeen
force-pushed
the
inflight/current
branch
from
0bba31b to
627b389
Compare
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #33639 | Walk UICollectionView.Subviews to find internal UIScrollView; mirror indicator visibility; retry via BeginInvokeOnMainThread if not yet created |
⏳ PENDING (Gate) | CollectionViewExtensions.cs |
Original PR fix |
Edge Cases Noted
- Internal
UIScrollViewmay not exist immediately (deferred layout) — addressed with single retry logic - Fix scoped only to
UICollectionViewCompositionalLayout(CV2), not affecting CV1 or other layouts TryApplyToInternalScrollViewreturns the first non-selfUIScrollViewsubview — could theoretically match wrong view if Apple adds other scroll views internally (low risk in practice, documented in code)- macOS snapshots changed — indicates fix has visual impact on macOS too (expected, same code path via MacCatalyst)
- Test uses
Task.Delay(100)for timing — potentially fragile but acceptable in device test context
Issue: #29390 - [iOS] Horizontal Scroll Bar Not Visible on CarouselView (CV2)
PR: #33639 - [iOS] Fixed Scrollbar should render correctly in Carousel View2
Author: SubhikshaSf4851 (community / partner: Syncfusion)
Platforms Affected: iOS, macOS (CV2 only — UICollectionViewCompositionalLayout)
Files Changed: 1 implementation, 1 device test, 2 Mac snapshots
Key Findings
- Root cause:
UICollectionViewCompositionalLayout(used by CV2/CarouselView2) manages scroll indicators via an internalUIScrollViewsubview. SettingShowsHorizontalScrollIndicator/ShowsVerticalScrollIndicatoronly on the outerUICollectionViewhas no visual effect in CV2. - Fix: Extended
CollectionViewExtensions.csto walk theUICollectionView.Subviewsand update the first non-selfUIScrollViewfound. A single deferred retry viaBeginInvokeOnMainThreadhandles the case where the internal scroll view doesn't exist yet at the time the property is set. - Test type: Device test (
CollectionViewTests.iOS.cs) — NOT a UI test in TestCases.HostApp/Shared.Tests. - Prior agent review: 3 inline review comments from Copilot reviewer (all resolved by PR author): null dereference, timing race, missing
internalScrollViewhorizontal assertion. - Snapshot changes: Two Mac test snapshots modified — likely due to minor visual changes from the fix.
- Risk note:
TryApplyToInternalScrollViewrelies on internal Apple implementation details (UIScrollView subview of UICollectionViewCompositionalLayout). Documented in code comment.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #33639 | Walk UICollectionView.Subviews to find internal UIScrollView; propagate indicator visibility; one deferred retry via BeginInvokeOnMainThread |
⏳ PENDING (Gate) | CollectionViewExtensions.cs |
Original PR |
Result: ✅ COMPLETE
Issue: #29390 - [iOS] Horizontal Scroll Bar Not Visible on CarouselView (CV2)
PR: #33639 - [iOS] Fixed Scrollbar should render correctly in Carousel View2
Platforms Affected: iOS, MacCatalyst
Files Changed: 1 implementation, 1 test, 2 snapshot artifacts
Key Findings
- The linked issue reports that
CarouselViewusing CV2 does not show the horizontal scrollbar on iOS/macOS even whenHorizontalScrollBarVisibilityisAlwaysorDefault. - The PR updates
src/Core/src/Platform/iOS/CollectionViewExtensions.csso compositional-layout-backedUICollectionViewinstances also propagate indicator visibility to an internalUIScrollView, with one deferred retry if that subview is not yet available. - Test coverage added in
src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.csis an iOS device test, not a HostApp/UI test; the PR also includes two Mac snapshot updates that appear unrelated to the bug fix itself. - Prior automated review comments focused on the test becoming timing-dependent and not fully asserting the internal horizontal indicator path; the current version addresses those concerns by polling for the internal scroll view and asserting both native and internal indicator state in the
AlwaysandNevercases.
Edge Cases Mentioned
- The issue was verified on both iOS and MacCatalyst, while the new automated test only exercises the iOS device-test surface.
- The production fix depends on
UICollectionViewCompositionalLayoutcontinuing to expose a separate internalUIScrollViewfor indicators; if UIKit changes that implementation, the helper silently falls back to updating only the outer collection view.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #33639 | Mirror horizontal/vertical scroll indicator visibility from compositional-layout UICollectionView to its internal UIScrollView, retrying once on the main thread if the inner scroll view is not yet PENDING (Gate) |
src/Core/src/Platform/iOS/CollectionViewExtensions.cs, src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs |
Original PR | created. |
Issue: #29390 - [iOS] Horizontal Scroll Bar Not Visible on CarouselView (CV2)
PR: #33639 - [iOS] Fixed Scrollbar should render correctly in Carousel View2
Platforms Affected: iOS, MacCatalyst/macOS
Files Changed: 1 implementation, 1 test, 2 snapshot updates
Key Findings
- The linked issue reports that
CarouselViewusing CV2 does not show the horizontal scrollbar on iOS/macOS even whenHorizontalScrollBarVisibilityisAlwaysorDefault. - The PR updates
src/Core/src/Platform/iOS/CollectionViewExtensions.csso compositional-layout-backed collection views also propagate scrollbar visibility to the internalUIScrollView, with one deferred retry if that internal scroll view is not ready yet. - The PR adds an iOS device test in
src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs, so the relevant test surface is DeviceTest. - Review discussion focused on making the new test deterministic: wait for the internal scroll view to exist, account for deferred updates, and assert the internal horizontal indicator in the
Nevercase. - The PR also updates two macOS snapshot baselines (
CarouselViewShouldScrollToRightPosition.png,IndicatorViewWithTemplatedIcon.png); these are collateral test artifacts, not the primary fix surface.
Edge Cases Mentioned
- Verified repro comment confirms the issue reproduces on both iOS and MacCatalyst.
- The PR description explicitly notes the fix depends on
UICollectionViewCompositionalLayoutusing an internalUIScrollView, which is the key platform-specific behavior under test.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #33639 | Propagate scrollbar visibility from the outer UICollectionView to the internal compositional-layout UIScrollView, with a deferred retry, and validate it with an iOS device PENDING (Gate) |
src/Core/src/Platform/iOS/CollectionViewExtensions.cs, src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs |
Original PR | test. |
🚦 Gate — Test Verification
Gate Result: ⚠️ SKIPPED
Platform: iOS
Mode: Skipped — no UI tests in TestCases.HostApp or TestCases.Shared.Tests
Reason: PR #33639 includes only a Device Test in src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs. The Gate phase requires tests in TestCases.HostApp or TestCases.Shared.Tests to be verified via BuildAndRunHostApp.ps1. Device tests use a different test infrastructure (XHarness/Helix).
- Tests FAIL without fix: ⏳ NOT VERIFIED (device test infrastructure, not UI test)
- Tests PASS with fix: ⏳ NOT VERIFIED
Note: The PR includes a device test CheckCarouselViewScrollBarVisibilityUpdates that exercises the fix. Try-Fix phase will use Run-DeviceTests.ps1 to validate fix candidates.
Gate Result: ⚠️ SKIPPED
Platform: iOS
Mode: Full Verification (attempted)
Reason: No tests found in TestCases.HostApp or TestCases.Shared.Tests. The PR adds a device test (src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs) — a different test type (xUnit device tests) that runs on a real/simulated device via the run-device-tests skill, not the verify-tests-fail-without-fix skill (which targets UI tests).
Test that exists:
CollectionViewTests.iOS.cs→CheckCarouselViewScrollBarVisibilityUpdates()device test
Impact: Cannot run the automated FAIL/PASS verification cycle via verify-tests-fail-without-fix. Proceeding to Try-Fix phase.
- Tests FAIL without fix:
⚠️ UNVERIFIED (no UI tests to run) - Tests PASS with fix:
⚠️ UNVERIFIED (no UI tests to run)
Gate SKIPPEDResult:
Platform: ios
Mode: Full Verification
- Tests FAIL without fix:
- Tests PASS with fix:
Notes
- PR [iOS] CarouselView2: Update internal scroll indicators for compositional layout #33639 does not add a
TestCases.HostApp/TestCases.Shared.TestsUI test that theverify-tests-fail-without-fixskill can run. - The added coverage is an iOS device test in
src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs, so the standard Gate workflow is not applicable here. - Proceeding to mandatory Try-Fix exploration with this limitation documented.
Gate SKIPPEDResult:
Platform: ios
Test Type: DeviceTest (Controls)
Mode: Full Verification
- Tests FAIL without Unable to verify due to iOS simulator deployment timeout (XHarness exit code 83 / app launch failure).fix:
- Tests PASS with Not run because the without-fix phase did not complete.fix:
Detected Runner: pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Controls -Platform ios -TestFilter "<filter>"
Working Try-Fix Command: pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Controls -Platform ios -TestFilter "FullyQualifiedName~CheckCarouselViewScrollBarVisibilityUpdates"
Notes: The verification skill auto-detected this PR as an iOS Controls device-test change. Full gate verification was blocked by the local simulator environment timing out during app launch/deployment, so Phase 3 continues under the environment-blocker rule.
🔧 Fix — Analysis & Comparison
Gate Result: ⚠️ SKIPPED
Platform: iOS
Mode: Skipped — no UI tests in TestCases.HostApp or TestCases.Shared.Tests
Reason: PR #33639 includes only a Device Test in src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs. The Gate phase requires tests in TestCases.HostApp or TestCases.Shared.Tests to be verified via BuildAndRunHostApp.ps1. Device tests use a different test infrastructure (XHarness/Helix).
- Tests FAIL without fix: ⏳ NOT VERIFIED (device test infrastructure, not UI test)
- Tests PASS with fix: ⏳ NOT VERIFIED
Note: The PR includes a device test CheckCarouselViewScrollBarVisibilityUpdates that exercises the fix. Try-Fix phase will use Run-DeviceTests.ps1 to validate fix candidates.
Gate Result: ⚠️ SKIPPED
Platform: iOS
Mode: Full Verification (attempted)
Reason: No tests found in TestCases.HostApp or TestCases.Shared.Tests. The PR adds a device test (src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs) — a different test type (xUnit device tests) that runs on a real/simulated device via the run-device-tests skill, not the verify-tests-fail-without-fix skill (which targets UI tests).
Test that exists:
CollectionViewTests.iOS.cs→CheckCarouselViewScrollBarVisibilityUpdates()device test
Impact: Cannot run the automated FAIL/PASS verification cycle via verify-tests-fail-without-fix. Proceeding to Try-Fix phase.
- Tests FAIL without fix:
⚠️ UNVERIFIED (no UI tests to run) - Tests PASS with fix:
⚠️ UNVERIFIED (no UI tests to run)
Gate SKIPPEDResult:
Platform: ios
Mode: Full Verification
- Tests FAIL without fix:
- Tests PASS with fix:
Notes
- PR [iOS] CarouselView2: Update internal scroll indicators for compositional layout #33639 does not add a
TestCases.HostApp/TestCases.Shared.TestsUI test that theverify-tests-fail-without-fixskill can run. - The added coverage is an iOS device test in
src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs, so the standard Gate workflow is not applicable here. - Proceeding to mandatory Try-Fix exploration with this limitation documented.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #33639 | Propagate scrollbar visibility from the outer UICollectionView to the internal compositional-layout UIScrollView, with a deferred retry, and validate it with an iOS device Gate blocked |
src/Core/src/Platform/iOS/CollectionViewExtensions.cs, src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs |
Original PR; gate blocked by local iOS simulator timeout | test. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|
Exhausted: No
Selected Fix: awaiting try-fix attempts.Pending
📋 Report — Final Recommendation
Gate Result: ⚠️ SKIPPED
Platform: iOS
Mode: Skipped — no UI tests in TestCases.HostApp or TestCases.Shared.Tests
Reason: PR #33639 includes only a Device Test in src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs. The Gate phase requires tests in TestCases.HostApp or TestCases.Shared.Tests to be verified via BuildAndRunHostApp.ps1. Device tests use a different test infrastructure (XHarness/Helix).
- Tests FAIL without fix: ⏳ NOT VERIFIED (device test infrastructure, not UI test)
- Tests PASS with fix: ⏳ NOT VERIFIED
Note: The PR includes a device test CheckCarouselViewScrollBarVisibilityUpdates that exercises the fix. Try-Fix phase will use Run-DeviceTests.ps1 to validate fix candidates.
Gate Result: ⚠️ SKIPPED
Platform: iOS
Mode: Full Verification (attempted)
Reason: No tests found in TestCases.HostApp or TestCases.Shared.Tests. The PR adds a device test (src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs) — a different test type (xUnit device tests) that runs on a real/simulated device via the run-device-tests skill, not the verify-tests-fail-without-fix skill (which targets UI tests).
Test that exists:
CollectionViewTests.iOS.cs→CheckCarouselViewScrollBarVisibilityUpdates()device test
Impact: Cannot run the automated FAIL/PASS verification cycle via verify-tests-fail-without-fix. Proceeding to Try-Fix phase.
- Tests FAIL without fix:
⚠️ UNVERIFIED (no UI tests to run) - Tests PASS with fix:
⚠️ UNVERIFIED (no UI tests to run)
Gate SKIPPEDResult:
Platform: ios
Mode: Full Verification
- Tests FAIL without fix:
- Tests PASS with fix:
Notes
- PR [iOS] CarouselView2: Update internal scroll indicators for compositional layout #33639 does not add a
TestCases.HostApp/TestCases.Shared.TestsUI test that theverify-tests-fail-without-fixskill can run. - The added coverage is an iOS device test in
src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.iOS.cs, so the standard Gate workflow is not applicable here. - Proceeding to mandatory Try-Fix exploration with this limitation documented.
📋 Expand PR Finalization Review
PR #33639 Finalization Review
Title: Needs Update
Current: [iOS] Fixed Scrollbar should render correctly in Carousel View2
Recommended: [iOS] CarouselView2: Update internal scroll indicators for compositional layout
Why: The current title is grammatically awkward and underspecifies the actual fix. The implementation is specifically about propagating ScrollBarVisibility to the internal UIScrollView used by UICollectionViewCompositionalLayout in CV2, so the title should say that directly.
Description: Good foundation, but tighten accuracy
Quality assessment:
- Structure: Good. It has the required NOTE block and key sections.
- Technical depth: Good. The root cause and fix approach are described.
- Accuracy: Mostly good, but not fully aligned with the final PR contents.
- Completeness: Needs platform/test cleanup and artifact clarification.
Keep:
- The NOTE block.
- The root cause section describing CV2/compositional layout using an internal
UIScrollView. - The description of updating both
UICollectionViewand the internal scroll view.
Update:
- The
Tested the behavior in the following platformschecklist currently marks Windows and Android as tested, but this PR only changes iOS/CV2 code and an iOS device test. That section overclaims relative to the actual diff. - Use MacCatalyst instead of generic Mac if that is the platform that was actually exercised.
- Remove the empty before/after comparison table, or populate it.
- If the Mac snapshot changes remain in the PR, explain why they are part of this fix.
Suggested description adjustment:
- Keep the current root-cause and fix sections.
- Correct the tested-platform list to only the platforms actually validated.
- Add one sentence noting that the fix depends on CV2's compositional-layout internal scroll view.
Code Review Findings
Important Issues
Unrelated Mac snapshot updates should be removed or justified
- Files:
src/Controls/tests/TestCases.Mac.Tests/snapshots/mac/CarouselViewShouldScrollToRightPosition.pngsrc/Controls/tests/TestCases.Mac.Tests/snapshots/mac/IndicatorViewWithTemplatedIcon.png
- Problem: These files are unrelated to the iOS CarouselView2 scrollbar fix described by the issue, the title, and the code changes. Leaving them in the PR adds review noise and makes it harder to tell whether the PR contains unintended visual-regression artifacts.
- Recommendation: Drop the snapshot-only commit unless these images are intentionally required for this fix. If they are intentional, explain the causal link in the PR description.
Suggestions
- The implementation relies on UIKit's internal compositional-layout structure (
UICollectionViewhosting a separate internalUIScrollView) and applies only a single deferred retry. That is acceptable for this fix, but it is worth calling out in the description because the behavior depends on UIKit internals and may need revalidation on future iOS updates. - The new device test is much stronger after the follow-up commits because it now waits for the internal scroll view and asserts both native and internal indicators for both
AlwaysandNever. If this test becomes flaky later, the fixedTask.Delay(100)calls would be the first thing to revisit.
Looks Good
- The root cause in the description matches the actual code change: CV2/compositional layout uses an internal
UIScrollView, so updating only the outerUICollectionViewwas insufficient. - The final test coverage is materially better than the initial version shown in the commit history; it now validates the key internal-scroll-view behavior the bug depends on.
- The code change is scoped to the iOS/CV2 path and does not introduce obvious cross-platform churn in the handler logic itself.
Final Verdict
Not ready to merge as-is.
The core fix looks reasonable, but before merge I would:
- Update the PR title to describe the actual implementation.
- Correct the PR description's tested-platform claims.
- Remove or explicitly justify the unrelated Mac snapshot updates.
MauiBot
added
s/agent-review-incomplete
SubhikshaSf4851
changed the title
PureWeen
force-pushed
the
inflight/current
branch
from
6e563dc to
00535e4
Compare
MauiBot
added
s/agent-approved
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!
Root Cause :
When using CV2,
UICollectionViewCompositionalLayoutrelies on an internalUIScrollView. Scrollbar visibility were applied only to theUICollectionView, but for the scrollbars to render correctly, the visibility must also be updated on the internal scroll view.Description of Change
CollectionViewExtensions.csto also update the scroll indicator visibility on the internalUIScrollViewwhen aUICollectionViewCompositionalLayoutis used.Snapshot Updates — macOS (Intentional)
The following macOS snapshots were updated as part of this PR:
CarouselViewShouldScrollToRightPosition.png(Issue [Windows] [Scenario Day] IndicatorView using templated icons not working #7144)IndicatorViewWithTemplatedIcon.png(Issue [Android] CarouselView doesn't scroll to the right Position after changing the ItemSource with Loop enabled #17283)These changes are intentional because, after this PR, the scrollbars are now rendered on macOS as expected. Previously, they were not visible due to the underlying issue. With the fix in place, the visual output has been corrected, and the regenerated snapshots reflect this expected behavior rather than a regression.
Issues Fixed
Fixes #29390
Tested the behavior in the following platforms
BeforeFix29390.mov
AfterFix29390.mov