[Windows] Fixed ItemSpacing doesn't work in Carousel View#30014
[Windows] Fixed ItemSpacing doesn't work in Carousel View#30014kubaflo merged 11 commits intodotnet:inflight/currentfrom
Conversation
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR enables ItemSpacing for CarouselView on Windows by applying an ItemContainerStyle and adjusting item size calculations, and adds a UI test to verify the fix.
- Added a host app page and shared UI test for Issue 29772
- Updated
CarouselViewHandler.Windowsto set container padding and account forItemSpacingwhen measuring items
Reviewed Changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue29772.cs | Added a UI test for ItemSpacing but gated to iOS/Catalyst only |
| src/Controls/tests/TestCases.HostApp/Issues/Issue29772.cs | Host app page with a CarouselView setup, binding, and automation ID for testing ItemSpacing |
| src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs | Applied ItemContainerStyle and updated GetItemWidth/GetItemHeight to subtract spacing |
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #30014 | Add ItemContainerStyle with half-spacing padding on ListViewItem; unconditionally subtract ItemSpacing from item dimensions |
✅ PASSED (Gate) | CarouselViewHandler.Windows.cs |
P1 (gate) issue from prior review now fixed; dynamic update and UWP label remain |
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix (claude-opus-4.6) | Margin-based spacing: FrameworkElement.MarginProperty on ListViewItem with BasedOn style inheritance; unconditional ItemSpacing subtraction in GetItemWidth/GetItemHeight |
✅ Pass | 1 file | Margin creates gaps outside container; better semantic match for spacing between items |
| 2 | try-fix (claude-sonnet-4.6) | ItemContentControl.Margin via PropertyChangedCallback on existing ItemSpacingProperty; symmetric GetItemSpacing(); unconditional subtraction in GetItemWidth/GetItemHeight |
✅ Pass | 2 files | Activates existing data-binding infrastructure; no ItemContainerStyle at all |
| 3 | try-fix (gpt-5.3-codex) | ContainerContentChanging event handler to apply asymmetric ListViewItem.Padding per container |
❌ Fail | 1 file | 5.22% visual diff — asymmetric padding produced inconsistent rendering |
| 4 | try-fix (gpt-5.4) | Runtime UpdateItemSpacing() method subscribed to LinearItemsLayout.ItemSpacing changes; reapplies Control.PaddingProperty style + invalidates item sizes dynamically |
✅ Pass | 1 file | Adds dynamic update support missing from PR; most complete solution |
| PR | PR #30014 | Control.PaddingProperty on ListViewItem via ItemContainerStyle; unconditional ItemSpacing subtraction |
✅ PASSED (Gate) | 1 file | P1 gate issue from prior review addressed |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | No | ItemsStackPanel has no Spacing property; panel-level approach not viable |
| gpt-5.4 | 2 | No | Agrees viable approaches exhausted |
Exhausted: Yes
Selected Fix: PR's fix — Gate ✅ PASSED; P1 issue (PeekAreaInsets gate) addressed; approach is valid. Attempt 4 shows a dynamic update path is feasible, but that is a P2 enhancement. Recommend PR approval with minor required changes.
📋 Report — Final Recommendation
✅ Final Recommendation: APPROVE
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Issue #29772 — Windows CarouselView ItemSpacing ignored |
| Gate | ✅ PASSED | windows — tests FAIL without fix, PASS with fix |
| Try-Fix | ✅ COMPLETE | 4 attempts: 3 passing, 1 failing; cross-pollination exhausted |
| Report | ✅ COMPLETE |
Summary
The PR fixes ItemSpacing not being applied on Windows CarouselView. The root cause is correctly identified and the fix approach is valid. The primary concern from the prior agent review (spacing subtraction gated on PeekAreaInsets > 0) has been addressed in the updated PR — the subtraction is now unconditional for matching orientation. All try-fix alternatives passed confirm this is a viable fix space; the PR's approach is the simplest and most focused.
Root Cause
CarouselViewHandler.CreateCarouselListLayout() never applied any padding or margin to ListViewItem containers, so ItemSpacing had no visual effect on Windows. The existing GetItemSpacing() method was unused for container styling.
Fix Quality
What the PR does right:
GetItemContainerStyle()correctly usesControl.PaddingPropertyonListViewItemwith half-spacing per side, creating the visual gapGetItemWidth()/GetItemHeight()now unconditionally subtractItemSpacingwhen orientation matches — the P1 gate issue from the prior review is fixedItemContainerStyleapplied at list creation for both horizontal and vertical orientations- UI test with Windows snapshot present; gate confirms it validates the fix
Minor issues (non-blocking):
GetItemContainerStyle()only called at list creation (P2): IfItemSpacingchanges at runtime, theItemContainerStylewon't update. Try-Fix attempt 4 showed a dynamic subscription approach is feasible. This is an acceptable known limitation for this PR scope.PlatformAffected.UWPin HostApp (P3 — cosmetic):[Issue(IssueTracker.Github, 29772, "...", PlatformAffected.UWP)]— modern MAUI usesPlatformAffected.Allfor Windows. Cosmetic only.CarouselViewShouldRenderCorrectly.pngmodified (P2): An existing unrelated Windows snapshot changed; PR description doesn't explain why. Should be verified to confirm the fix didn't regress the existing test.
Try-Fix Comparison
| Approach | Files | Result | Notes |
|---|---|---|---|
| PR's fix | 1 | ✅ Gate | Control.PaddingProperty + unconditional subtraction |
| Margin-based (attempt 1) | 1 | ✅ Pass | FrameworkElement.MarginProperty — semantically different |
| ItemContentControl callback (attempt 2) | 2 | ✅ Pass | Activates existing binding chain |
| Asymmetric padding (attempt 3) | 1 | ❌ Fail | 5.22% visual diff |
| Dynamic update (attempt 4) | 1 | ✅ Pass | Adds runtime reactivity |
Selected Fix: PR's fix — simplest single-file change, gate validated, P1 correctness issue resolved.
kubaflo
left a comment
There was a problem hiding this comment.
Could you please review the AI's summary?
b69529d to
b0d0d25
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://github.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 30014Or
iex "& { $(irm https://github.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 30014" |
kubaflo
left a comment
There was a problem hiding this comment.
The test is passing without a fix
|
@kubaflo The gate-without-fix check passed incorrectly because old baseline snapshots used a red background, masking the ItemSpacing difference. Now the baselines have been updated to green background. With corrected snapshots, the test will now fail without the fix |
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kubaflo
left a comment
There was a problem hiding this comment.
Could you please try:
Selected Fix: Attempt 4 approach — keep PR's Control.PaddingProperty ItemContainerStyle, remove PeekAreaInsets gate from GetItemWidth/GetItemHeight. Simplest, least invasive change that is provably correct.
|
@kubaflo I have implemented the suggested approach |
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
<!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Root Cause: CarouselView does not utilize the ItemSpacing property during layout styling ### Description of Change Added an item container style based on the horizontal or vertical orientation of the CarouselView, and applied a corresponding style to the ListViewBase <!-- Enter description of the fix in this section --> ### Issues Fixed <!-- Please make sure that there is a bug logged for the issue being fixed. The bug should describe the problem and how to reproduce it. --> Fixes #29772 ### Tested the behaviour in the following platforms - [x] Windows - [x] Android - [x] iOS - [x] Mac ### Screenshot | Before Issue Fix | After Issue Fix | |----------|----------| | <img src="https://github.com/user-attachments/assets/4af88696-c5a6-4683-bceb-1933781368f3"> | <img src="https://github.com/user-attachments/assets/f55c2b45-22c3-4ab2-aae5-c668adaf33e4"> | <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
<!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Root Cause: CarouselView does not utilize the ItemSpacing property during layout styling ### Description of Change Added an item container style based on the horizontal or vertical orientation of the CarouselView, and applied a corresponding style to the ListViewBase <!-- Enter description of the fix in this section --> ### Issues Fixed <!-- Please make sure that there is a bug logged for the issue being fixed. The bug should describe the problem and how to reproduce it. --> Fixes dotnet#29772 ### Tested the behaviour in the following platforms - [x] Windows - [x] Android - [x] iOS - [x] Mac ### Screenshot | Before Issue Fix | After Issue Fix | |----------|----------| | <img src="https://github.com/user-attachments/assets/4af88696-c5a6-4683-bceb-1933781368f3"> | <img src="https://github.com/user-attachments/assets/f55c2b45-22c3-4ab2-aae5-c668adaf33e4"> | <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
<!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Root Cause: CarouselView does not utilize the ItemSpacing property during layout styling ### Description of Change Added an item container style based on the horizontal or vertical orientation of the CarouselView, and applied a corresponding style to the ListViewBase <!-- Enter description of the fix in this section --> ### Issues Fixed <!-- Please make sure that there is a bug logged for the issue being fixed. The bug should describe the problem and how to reproduce it. --> Fixes #29772 ### Tested the behaviour in the following platforms - [x] Windows - [x] Android - [x] iOS - [x] Mac ### Screenshot | Before Issue Fix | After Issue Fix | |----------|----------| | <img src="https://github.com/user-attachments/assets/4af88696-c5a6-4683-bceb-1933781368f3"> | <img src="https://github.com/user-attachments/assets/f55c2b45-22c3-4ab2-aae5-c668adaf33e4"> | <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
<!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Root Cause: CarouselView does not utilize the ItemSpacing property during layout styling ### Description of Change Added an item container style based on the horizontal or vertical orientation of the CarouselView, and applied a corresponding style to the ListViewBase <!-- Enter description of the fix in this section --> ### Issues Fixed <!-- Please make sure that there is a bug logged for the issue being fixed. The bug should describe the problem and how to reproduce it. --> Fixes #29772 ### Tested the behaviour in the following platforms - [x] Windows - [x] Android - [x] iOS - [x] Mac ### Screenshot | Before Issue Fix | After Issue Fix | |----------|----------| | <img src="https://github.com/user-attachments/assets/4af88696-c5a6-4683-bceb-1933781368f3"> | <img src="https://github.com/user-attachments/assets/f55c2b45-22c3-4ab2-aae5-c668adaf33e4"> | <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
<!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Root Cause: CarouselView does not utilize the ItemSpacing property during layout styling ### Description of Change Added an item container style based on the horizontal or vertical orientation of the CarouselView, and applied a corresponding style to the ListViewBase <!-- Enter description of the fix in this section --> ### Issues Fixed <!-- Please make sure that there is a bug logged for the issue being fixed. The bug should describe the problem and how to reproduce it. --> Fixes #29772 ### Tested the behaviour in the following platforms - [x] Windows - [x] Android - [x] iOS - [x] Mac ### Screenshot | Before Issue Fix | After Issue Fix | |----------|----------| | <img src="https://github.com/user-attachments/assets/4af88696-c5a6-4683-bceb-1933781368f3"> | <img src="https://github.com/user-attachments/assets/f55c2b45-22c3-4ab2-aae5-c668adaf33e4"> | <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
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:
CarouselView does not utilize the ItemSpacing property during layout styling
Description of Change
Added an item container style based on the horizontal or vertical orientation of the CarouselView, and applied a corresponding style to the ListViewBase
Issues Fixed
Fixes #29772
Tested the behaviour in the following platforms
Screenshot