Fix CarouselView Regression

[kubaflo]

Description

A proposed fix for a minor regression introduced in this pr #28225

Issues Fixed

Fixes #28930

Before	After

@albyrock87

[Copilot]

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• src/Controls/tests/TestCases.HostApp/Issues/Issue28930.xaml: Language not supported

[Copilot]

Consider adding an inline comment explaining the rationale for applying a custom measurement override for carousel view cells to aid future maintainers.

[PureWeen]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[Bamich]

Hi! I tried this, it seems ItemsLayout="HorizontalList" is not respected. All CarouselViews in my project become vertical (at least child Grid elements are arranged vertically)

[albyrock87]

@Bamich have you tried this after this merge cfe6e69 ?
If so, may you show a sample code, including whether you're using CV1 or CV2 handler?
Because all UITests pass here, including the one which fixes the issue.
Thanks

[Bamich]

@albyrock87 @kubaflo I tested it again. CV1 seems to work as expected. But CV2 doesn't - all child elements are arranged vertically.

This is also easily reproduced in the original sample:

1. Turn on CV2
2. Run sample
3. Most importantly - try to scroll the carousel horizontally, and then vertically, you will see that the scroll will go exactly vertically. In statics it is not obvious, you need to start scrolling :)

Direct ItemsLayout="HorizontalList" does not affect the behavior.

scr_rec.mov

You can also replace the MainPage.xml code in the original example with this simpler one, and then scroll the CarouselView in different directions:

<?xml version="1.0" encoding="utf-8" ?>
<ContentPage
	x:Class="IssueLabeliOS9060.MainPage"
	xmlns="http://schemas.microsoft.com/dotnet/2021/maui" xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
	xmlns:local="clr-namespace:IssueLabeliOS9060">
	<Grid>
		<Border Padding="10">
			<Grid VerticalOptions="Fill">
				<CarouselView HeightRequest="200" x:Name="MyCarousel" EmptyView="No data">
					<CarouselView.ItemsSource>
						<x:Array Type="{x:Type x:String}">
							<x:String>First</x:String>
							<x:String>Second</x:String>
							<x:String>Third</x:String>
						</x:Array>
					</CarouselView.ItemsSource>
					<CarouselView.ItemTemplate>
						<DataTemplate>
							<Grid
								Padding="20"
								HorizontalOptions="Fill" VerticalOptions="Fill">
								<Label
									Text="{Binding .}"
									HorizontalOptions="Fill" VerticalOptions="Fill"
									BackgroundColor="red" />
							</Grid>
						</DataTemplate>
					</CarouselView.ItemTemplate>
				</CarouselView>
			</Grid>
		</Border>
	</Grid>

</ContentPage>

[jsuarezruiz]

There are some CarouselView UITests failing on iOS/Catalyst:

The app was expected to be running still, investigate as possible crash

Could you review if are related with the changes?

[albyrock87]

@jsuarezruiz the fix is ready here kubaflo#4

I really don't understand why, but passing the scroll direction makes the Carousel2 handler crash the app without any message :/
Anyway we don't need that information in the new CarouselTemplatedCell2 🙂
I added a comment in the code and an additional test in Issue28930 to verify the carousel can scroll horizontally.

We can wait for the merge or you can cherry pick and run AZP immediately

[Bamich]

I have a proposal. It doesn't seem to be directly related to this PR, but since it's called "Improve iOS CarouselView performance", maybe we should fix this right away? If I set the scroll index for a CarouselView with CV2 on iOS "too early" I get a random exception "cannot access disposed object NSPathIndex". Perhaps we should move the projectedPosition calculation a little lower in CarouselViewController2.cs by changing this part:

<...>

// with Task.Delay(100) below it results in "cannot access disposed object NSPathIndex"
var projectedPosition = NSIndexPath.FromItemSection(position, _section);

if (LoopItemsSource.Loop)
{
	//We need to set the position to the correct position since we added 1 item at the beginning
	projectedPosition = GetScrollToIndexPath(position);
}

var uICollectionViewScrollPosition = IsHorizontal ? UICollectionViewScrollPosition.CenteredHorizontally :
	UICollectionViewScrollPosition.CenteredVertically;

await Task.Delay(100).ContinueWith((t) =>
{
	MainThread.BeginInvokeOnMainThread(() =>
	{
		if (!IsViewLoaded)
		{
			return;
		}
		InitialPositionSet = true;

		if (ItemsSource is null || ItemsSource.ItemCount == 0)
		{
			return;
		}

		CollectionView.ScrollToItem(projectedPosition, uICollectionViewScrollPosition, false);

		//Set the position on VirtualView to update the CurrentItem also
		SetPosition(position);

		UpdateVisualStates();
	});

});

<...>

to something like this?

<...>

var uICollectionViewScrollPosition = IsHorizontal ? UICollectionViewScrollPosition.CenteredHorizontally : 
	UICollectionViewScrollPosition.CenteredVertically;

// just wait..
await Task.Delay(100);

MainThread.BeginInvokeOnMainThread(() =>
{
	if (!IsViewLoaded)
	{
		return;
	}

	InitialPositionSet = true;

	if (ItemsSource is null || ItemsSource.ItemCount == 0)
	{
		return;
	}

	// Get projectedPosition immediately before use (no crash)
	NSIndexPath projectedPosition = LoopItemsSource.Loop
		? GetScrollToIndexPath(position)
		: NSIndexPath.FromItemSection(position, _section);

	CollectionView.ScrollToItem(projectedPosition, uICollectionViewScrollPosition, false);

		//Set the position on VirtualView to update the CurrentItem also
		SetPosition(position);

		UpdateVisualStates();
});

<...>

Thanks

[albyrock87]

@Bamich I think that's reasonable.
In the mean time, may you verify if the last commit here fixes the issue?
I believe so given I also added a check in the UITest, but if you can confirm I'd appreciate.
Thanks

[PureWeen]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[Bamich]

@albyrock87 Yes, I checked and confirm that the last commit fixes the scroll orientation issue

[jsuarezruiz]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).