-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[CarouselViewHandler2] Fir fox CurrentItem does not work when ItemSpacing is set #32135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…nts CurrentItem from updating correctly
|
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 |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
|
||
| var page = (offset.X + sectionMargin) / (env.Container.ContentSize.Width - sectionMargin * 2); | ||
| // Calculate page index accounting for ItemSpacing | ||
| var itemSpacing = itemsView.ItemsLayout is LinearItemsLayout linearLayout ? linearLayout.ItemSpacing : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you scroll to the very last item and verify CurrentItem updates correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsuarezruiz , I have verified this by scrolling to the last item, and the CurrentItem is updating correctly. For your reference, I’m attaching the video below.
ScrollingToLastItem.mov
|
|
||
| var page = (offset.X + sectionMargin) / (env.Container.ContentSize.Width - sectionMargin * 2); | ||
| // Calculate page index accounting for ItemSpacing | ||
| var itemSpacing = itemsView.ItemsLayout is LinearItemsLayout linearLayout ? linearLayout.ItemSpacing : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you verify also GridItemsLayout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsuarezruiz , CarouselView.ItemsLayout is explicitly typed as LinearItemsLayout (not the base ItemsLayout type), so it cannot be a GridItemsLayout.
Looking at the CarouselView class definition :
maui/src/Controls/src/Core/Items/CarouselView.cs
Lines 194 to 198 in 12b0495
| public LinearItemsLayout ItemsLayout | |
| { | |
| get => (LinearItemsLayout)GetValue(ItemsLayoutProperty); | |
| set => SetValue(ItemsLayoutProperty, value); | |
| } |
The property type system enforces that only LinearItemsLayout can be assigned.
| var itemSpacing = itemsView.ItemsLayout is LinearItemsLayout linearLayout ? linearLayout.ItemSpacing : 0; | ||
|
|
||
| var effectiveItemWidth = env.Container.ContentSize.Width - sectionMargin * 2 + itemSpacing; | ||
| double page = (offset.X + sectionMargin) / effectiveItemWidth; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double page = (offset.X + sectionMargin) / effectiveItemWidth;
Could effectiveItemWidth be zero? Potential Divide-by-Zero Exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsuarezruiz , Yes, there is a potential divide-by-zero exception that could occur when the container has no width or when the effective item width calculation results in zero or a negative value. To prevent this, I've added a guard to skip the scroll calculation in such cases, avoiding invalid position updates.
…ve item width is zero or negative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug in CarouselView2 where the CurrentItem property fails to update correctly when ItemSpacing is configured. The root cause was that the page index calculation only considered container size without accounting for the additional space consumed by ItemSpacing.
Key Changes:
- Updated page index calculation logic in iOS LayoutFactory2 to include ItemSpacing in the effective item width
- Added validation to prevent division by zero when effectiveItemWidth is non-positive
- Added comprehensive UI tests to validate the fix across platforms
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Controls/src/Core/Handlers/Items2/iOS/LayoutFactory2.cs | Updated page index calculation to account for ItemSpacing in CarouselView layout |
| src/Controls/tests/TestCases.HostApp/Issues/Issue32048.cs | Added HostApp UI test page demonstrating the CurrentItem update with ItemSpacing |
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue32048.cs | Added NUnit test to verify CurrentItem updates correctly when ItemSpacing is set |
| } | ||
|
|
||
| var page = (offset.X + sectionMargin) / (env.Container.ContentSize.Width - sectionMargin * 2); | ||
| // Calculate page index accounting for ItemSpacing |
Copilot
AI
Oct 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The comment on line 346 should clarify why itemSpacing is added to the effective width calculation. The current comment states 'accounting for ItemSpacing' but doesn't explain that the spacing is added because each item's scroll distance includes both its width and the spacing after it.
| // Calculate page index accounting for ItemSpacing | |
| // Calculate page index, including ItemSpacing because each item's scroll distance | |
| // consists of its width plus the spacing after it. This ensures paging accounts for | |
| // both the item width and the space between items. |
Copilot uses AI. Check for mistakes.
|
Azure Pipelines successfully started running 3 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
Issues Fixed
Fixes #32048
Validated the behaviour in the following platforms
Output
Before.mov
After.mov