-
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
Changes from all commits
b208402
5dab21d
6a473e3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -343,7 +343,17 @@ public static UICollectionViewLayout CreateCarouselLayout( | |
| return; | ||
| } | ||
|
|
||
| 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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
jsuarezruiz marked this conversation as resolved.
|
||
|
|
||
| var effectiveItemWidth = env.Container.ContentSize.Width - sectionMargin * 2 + itemSpacing; | ||
|
|
||
| if (effectiveItemWidth <= 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| double page = (offset.X + sectionMargin) / effectiveItemWidth; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Could effectiveItemWidth be zero? Potential Divide-by-Zero Exception?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
|
||
| if (Math.Abs(page % 1) > (double.Epsilon * 100) || cv2Controller.ItemsSource.ItemCount <= 0) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| namespace Maui.Controls.Sample.Issues; | ||
|
|
||
| [Issue(IssueTracker.Github, 32048, "CurrentItem does not update when ItemSpacing is set", PlatformAffected.iOS)] | ||
| public class Issue32048 : ContentPage | ||
| { | ||
| CarouselView2 carouselView; | ||
| Label currentItemLabel; | ||
| string firstItem = "Baboon"; | ||
|
|
||
| public Issue32048() | ||
| { | ||
| carouselView = new CarouselView2 | ||
| { | ||
| AutomationId = "CarouselViewWithItemSpacing", | ||
| HeightRequest = 400, | ||
| BackgroundColor = Colors.LightGray, | ||
| ItemsLayout = new LinearItemsLayout(ItemsLayoutOrientation.Horizontal) | ||
| { | ||
| ItemSpacing = 10, | ||
| SnapPointsType = SnapPointsType.MandatorySingle, | ||
| }, | ||
| ItemTemplate = new DataTemplate(() => | ||
| { | ||
| Label label = new Label | ||
| { | ||
| HorizontalOptions = LayoutOptions.Center, | ||
| VerticalOptions = LayoutOptions.Center | ||
| }; | ||
| label.SetBinding(Label.TextProperty, "."); | ||
|
|
||
| return new Grid | ||
| { | ||
| Children = { label } | ||
| }; | ||
| }), | ||
| ItemsSource = new string[] | ||
| { | ||
| "Baboon", | ||
| "Capuchin Monkey", | ||
| "Blue Monkey", | ||
| "Squirrel Monkey", | ||
| "Golden Lion Tamarin" | ||
| } | ||
| }; | ||
|
|
||
| carouselView.CurrentItemChanged += OnCurrentItemChanged; | ||
|
|
||
| currentItemLabel = new Label | ||
| { | ||
| AutomationId = "Issue32048StatusLabel", | ||
| Text = "Failure" | ||
| }; | ||
|
|
||
| Grid grid = new Grid | ||
| { | ||
| Padding = 25, | ||
| RowSpacing = 10, | ||
| RowDefinitions = | ||
| { | ||
| new RowDefinition(GridLength.Auto), | ||
| new RowDefinition(GridLength.Auto) | ||
| } | ||
| }; | ||
|
|
||
| grid.Add(carouselView); | ||
| grid.Add(currentItemLabel, row: 1); | ||
|
|
||
| Content = grid; | ||
| } | ||
|
|
||
| void OnCurrentItemChanged(object sender, CurrentItemChangedEventArgs e) | ||
| { | ||
| if (e.CurrentItem is not null && e.CurrentItem.ToString() != firstItem) | ||
| { | ||
| currentItemLabel.Text = "Success"; | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| #if TEST_FAILS_ON_WINDOWS // Issue Link - https://github.com/dotnet/maui/issues/31670 | ||
| using NUnit.Framework; | ||
| using UITest.Appium; | ||
| using UITest.Core; | ||
|
|
||
| namespace Microsoft.Maui.TestCases.Tests.Issues; | ||
|
|
||
| public class Issue32048 : _IssuesUITest | ||
| { | ||
| public Issue32048(TestDevice device) : base(device) | ||
| { | ||
| } | ||
|
|
||
| public override string Issue => "CurrentItem does not update when ItemSpacing is set"; | ||
|
|
||
| [Test] | ||
| [Category(UITestCategories.CarouselView)] | ||
| public void VerifyCurrentItemUpdatesWithItemSpacing() | ||
| { | ||
| App.WaitForElement("CarouselViewWithItemSpacing"); | ||
| App.ScrollRight("CarouselViewWithItemSpacing"); | ||
|
|
||
| #if MACCATALYST | ||
| Thread.Sleep(1000); | ||
| #endif | ||
| var resultLabel = App.WaitForElement("Issue32048StatusLabel").GetText(); | ||
| Assert.That(resultLabel, Is.EqualTo("Success")); | ||
| } | ||
| } | ||
| #endif |
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.