Skip to content

Commit

Permalink
[windows] Fixed CarouselView items rendering (#16842)
Browse files Browse the repository at this point in the history
* [windows] Added ArrangeOverride override and changed MeasureOverride in ItemContentControl to not call base when the ItemHeight and ItemWidth are defined

When the height and width of the items are already defined, it's likely that the base.MeasureOverride (WinUI) call reduces the original size, causing issues like the items in a CarouselView not fitting the container, which causes more items to be displayed at once.
This fix makes sure that we always return the max height and width between the measured override and the existing items height and width.
Also, adding ArrangeOverride from WinUI base class to control the layout correctly and completely on our side.

Fixes Bug: #12567

* Added Appium test for CarouselView to validate fix for bug #12567

* Fix RS0016 + added inheritdoc

* Fixed return statement on ArrangeOverride according to PR feedback

* Fixed CarouselViewHandler resizing logic according to PR feedback

* Attempt to fix test errors

---------

Co-authored-by: Juan Diego Herrera <[email protected]>
  • Loading branch information
mauroa and jknaudt21 authored Aug 23, 2023
1 parent 6aaf343 commit 862fa07
Show file tree
Hide file tree
Showing 5 changed files with 243 additions and 53 deletions.
135 changes: 135 additions & 0 deletions src/Controls/samples/Controls.Sample.UITests/Issues/Issue12567.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
using System.Collections.ObjectModel;
using Microsoft.Maui;
using Microsoft.Maui.Controls;
using Microsoft.Maui.Graphics;

namespace Maui.Controls.Sample.Issues
{
[Issue(IssueTracker.Github, 12567, "Carousel View does not behave properly in Windows", PlatformAffected.UWP)]
public class Issue12567 : TestContentPage
{
protected override void Init()
{
var carouselItemsCount = 10;
var viewModel = new CarouselViewModel();

viewModel.GenerateItems(carouselItemsCount);

BindingContext = viewModel;

var itemsLayout = new LinearItemsLayout(ItemsLayoutOrientation.Horizontal)
{
SnapPointsType = SnapPointsType.MandatorySingle,
SnapPointsAlignment = SnapPointsAlignment.Center
};
var itemTemplate = GetCarouselItemTemplate();
var carouselView = new CarouselView
{
ItemsLayout = itemsLayout,
ItemTemplate = itemTemplate,
Margin = new Thickness(0, 10, 0, 10),
BackgroundColor = Colors.Grey,
AutomationId = "TestCarouselView",
PeekAreaInsets = new Thickness(30, 0, 30, 0)
};
var button = new Button
{
Text = "Get Last Item",
AutomationId = "GetLast",
Margin = new Thickness(0, 10, 0, 10)
};
var carouselContent = new Grid();

carouselContent.Children.Add(carouselView);

var layout = new StackLayout
{
Orientation = StackOrientation.Vertical,
VerticalOptions = LayoutOptions.Fill,
};

button.Clicked += (sender, args) =>
{
carouselView.ScrollTo(carouselItemsCount - 1, position: ScrollToPosition.End, animate: true);
};

carouselView.SetBinding(ItemsView.ItemsSourceProperty, "Items");
layout.Children.Add(button);
layout.Children.Add(carouselContent);

Content = layout;
}

DataTemplate GetCarouselItemTemplate()
{
return new DataTemplate(() =>
{
var layout = new StackLayout
{
Orientation = StackOrientation.Vertical,
VerticalOptions = LayoutOptions.Fill
};
var nameLabel = new Label
{
HorizontalOptions = LayoutOptions.Center,
Margin = new Thickness(10),
AutomationId = "NameLabel"
};
var numberLabel = new Label
{
HorizontalOptions = LayoutOptions.Center,
Margin = new Thickness(10),
AutomationId = "NumberLabel"
};

nameLabel.SetBinding(Label.TextProperty, new Binding("Name"));
numberLabel.SetBinding(Label.TextProperty, new Binding("Number"));

layout.Children.Add(nameLabel);
layout.Children.Add(numberLabel);

return new Frame
{
Content = layout,
HasShadow = false
};
});
}
}

class CarouselData
{
public string Name { get; set; }

public int Number { get; set; }
}

class CarouselViewModel : BindableObject
{
ObservableCollection<CarouselData> items;

public ObservableCollection<CarouselData> Items
{
get => items;
set
{
items = value;
OnPropertyChanged();
}
}

public void GenerateItems(int itemsCount)
{
Items = new ObservableCollection<CarouselData>();

for (var i = 1; i <= itemsCount; i++)
{
Items.Add(new CarouselData
{
Name = $"Test Item {i}",
Number = i
});
}
}
}
}
41 changes: 27 additions & 14 deletions src/Controls/src/Core/Handlers/Items/CarouselViewHandler.Windows.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using WScrollMode = Microsoft.UI.Xaml.Controls.ScrollMode;
using WSnapPointsAlignment = Microsoft.UI.Xaml.Controls.Primitives.SnapPointsAlignment;
using WSnapPointsType = Microsoft.UI.Xaml.Controls.SnapPointsType;
using Windows.Foundation;

namespace Microsoft.Maui.Controls.Handlers.Items
{
Expand All @@ -21,6 +22,7 @@ public partial class CarouselViewHandler : ItemsViewHandler<CarouselView>
ScrollViewer _scrollViewer;
WScrollBarVisibility? _horizontalScrollBarVisibilityWithoutLoop;
WScrollBarVisibility? _verticalScrollBarVisibilityWithoutLoop;
Size _currentSize;

protected override IItemsLayout Layout { get; }

Expand All @@ -30,7 +32,7 @@ public partial class CarouselViewHandler : ItemsViewHandler<CarouselView>
protected override void ConnectHandler(ListViewBase platformView)
{
ItemsView.Scrolled += CarouselScrolled;
ListViewBase.SizeChanged += InitialSetup;
ListViewBase.SizeChanged += OnListViewSizeChanged;

UpdateScrollBarVisibilityForLoop();

Expand All @@ -44,7 +46,7 @@ protected override void DisconnectHandler(ListViewBase platformView)

if (ListViewBase != null)
{
ListViewBase.SizeChanged -= InitialSetup;
ListViewBase.SizeChanged -= OnListViewSizeChanged;

if (CollectionViewSource?.Source is ObservableItemTemplateCollection observableItemsSource)
observableItemsSource.CollectionChanged -= OnCollectionItemsSourceChanged;
Expand All @@ -54,20 +56,20 @@ protected override void DisconnectHandler(ListViewBase platformView)
{
_scrollViewer.ViewChanging -= OnScrollViewChanging;
_scrollViewer.ViewChanged -= OnScrollViewChanged;
_scrollViewer.SizeChanged -= InitialSetup;
_scrollViewer.SizeChanged -= OnScrollViewSizeChanged;
}

base.DisconnectHandler(platformView);
}

protected override void UpdateItemsSource()
{
var itemsSource = ItemsView.ItemsSource;
var itemsSource = ItemsView?.ItemsSource;

if (itemsSource == null)
return;

var itemTemplate = ItemsView.ItemTemplate;
var itemTemplate = ItemsView?.ItemTemplate;

if (itemTemplate == null)
return;
Expand All @@ -90,7 +92,7 @@ protected override void OnScrollViewerFound(ScrollViewer scrollViewer)
_scrollViewer = scrollViewer;
_scrollViewer.ViewChanging += OnScrollViewChanging;
_scrollViewer.ViewChanged += OnScrollViewChanged;
_scrollViewer.SizeChanged += InitialSetup;
_scrollViewer.SizeChanged += OnScrollViewSizeChanged;

UpdateScrollBarVisibilityForLoop();
}
Expand Down Expand Up @@ -331,6 +333,11 @@ int GetItemPositionInCarousel(object item)

void UpdateCarouselViewInitialPosition()
{
if (ListViewBase == null)
{
return;
}

if (ListViewBase.Items.Count > 0)
{
if (Element.Loop)
Expand Down Expand Up @@ -406,7 +413,7 @@ WSnapPointsAlignment GetWindowsSnapPointsAlignment(SnapPointsAlignment snapPoint

void UpdateSnapPointsType()
{
if (_scrollViewer == null)
if (_scrollViewer == null || CarouselItemsLayout == null)
return;

if (CarouselItemsLayout.Orientation == ItemsLayoutOrientation.Horizontal)
Expand All @@ -418,7 +425,7 @@ void UpdateSnapPointsType()

void UpdateSnapPointsAlignment()
{
if (_scrollViewer == null)
if (_scrollViewer == null || CarouselItemsLayout == null)
return;

if (CarouselItemsLayout.Orientation == ItemsLayoutOrientation.Horizontal)
Expand Down Expand Up @@ -521,14 +528,20 @@ void OnCollectionItemsSourceChanged(object sender, NotifyCollectionChangedEventA
SetCarouselViewPosition(carouselPosition);
}

void InitialSetup(object sender, SizeChangedEventArgs e)
void OnListViewSizeChanged(object sender, SizeChangedEventArgs e) => Resize(e.NewSize);

void OnScrollViewSizeChanged(object sender, SizeChangedEventArgs e)
{
if (e.NewSize.Width > 0 && e.NewSize.Height > 0)
{
ListViewBase.SizeChanged -= InitialSetup;
//If there's a scroll viewer, it's enough to resize based on its size changed event, so we can avoid two event handlers doing the same
ListViewBase.SizeChanged -= OnListViewSizeChanged;
Resize(e.NewSize);
}

if (_scrollViewer != null)
_scrollViewer.SizeChanged -= InitialSetup;
void Resize(Size newSize)
{
if (newSize != _currentSize && newSize.Width > 0 && newSize.Height > 0)
{
_currentSize = newSize;

UpdateItemsSource();
UpdateSnapPointsType();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,58 +263,48 @@ void UpdateSemanticProperties(IView view)
this.SetAutomationPropertiesAccessibilityView(_visualElement, defaultAccessibilityView);
}

/// <inheritdoc/>
protected override WSize ArrangeOverride(WSize finalSize)
{
if (_renderer == null)
{
return base.ArrangeOverride(finalSize);
}

var width = ItemWidth == default ? finalSize.Width : ItemWidth;
var height = ItemHeight == default ? finalSize.Height : ItemHeight;
var size = new WSize(width, height);

return base.ArrangeOverride(size);
}

/// <inheritdoc/>
protected override WSize MeasureOverride(WSize availableSize)
{
if (_renderer == null)
{
return base.MeasureOverride(availableSize);
}

var width = ItemWidth == default ? availableSize.Width : ItemWidth;
var height = ItemHeight == default ? availableSize.Height : ItemHeight;
var measuredSize = base.MeasureOverride(new WSize(width, height));
var finalWidth = Max(measuredSize.Width, ItemWidth);
var finalHeight = Max(measuredSize.Height, ItemHeight);
var finalSize = new WSize(finalWidth, finalHeight);
var frameworkElement = Content as FrameworkElement;
var formsElement = _renderer.VirtualView as VisualElement;
var visualElement = _renderer.VirtualView as VisualElement;
var margin = _renderer.VirtualView.Margin;

if (ItemHeight != default || ItemWidth != default)
{
formsElement.Layout(new Rect(0, 0, ItemWidth, ItemHeight));

var wsize = new WSize(ItemWidth, ItemHeight);

frameworkElement.Margin =
WinUIHelpers.CreateThickness(
margin.Left + ItemSpacing.Left,
margin.Top + ItemSpacing.Top,
margin.Right + ItemSpacing.Right,
margin.Bottom + ItemSpacing.Bottom);
frameworkElement.Margin = WinUIHelpers.CreateThickness(margin.Left, margin.Top, margin.Right, margin.Bottom);
visualElement.Layout(new Rect(0, 0, finalWidth, finalHeight));

if (CanMeasureContent(frameworkElement))
frameworkElement.Measure(wsize);

return base.MeasureOverride(wsize);
}
else
if (CanMeasureContent(frameworkElement))
{
var (width, height) = formsElement.Measure(availableSize.Width, availableSize.Height,
MeasureFlags.IncludeMargins).Request;

width = Max(width, availableSize.Width);
height = Max(height, availableSize.Height);

formsElement.Layout(new Rect(0, 0, width, height));

var wsize = new WSize(width, height);

frameworkElement.Margin = WinUIHelpers.CreateThickness(
margin.Left,
margin.Top,
margin.Right,
margin.Bottom);

if (CanMeasureContent(frameworkElement))
frameworkElement.Measure(wsize);

return base.MeasureOverride(wsize);
frameworkElement.Measure(finalSize);
}

return finalSize;
}

double Max(double requested, double available)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Microsoft.Maui.Controls.PointerGestureRecognizer.PointerReleasedCommand.get -> S
Microsoft.Maui.Controls.PointerGestureRecognizer.PointerReleasedCommand.set -> void
Microsoft.Maui.Controls.PointerGestureRecognizer.PointerReleasedCommandParameter.get -> object!
Microsoft.Maui.Controls.PointerGestureRecognizer.PointerReleasedCommandParameter.set -> void
override Microsoft.Maui.Controls.Platform.ItemContentControl.ArrangeOverride(Windows.Foundation.Size finalSize) -> Windows.Foundation.Size
static readonly Microsoft.Maui.Controls.PointerGestureRecognizer.PointerPressedCommandParameterProperty -> Microsoft.Maui.Controls.BindableProperty!
static readonly Microsoft.Maui.Controls.PointerGestureRecognizer.PointerPressedCommandProperty -> Microsoft.Maui.Controls.BindableProperty!
static readonly Microsoft.Maui.Controls.PointerGestureRecognizer.PointerReleasedCommandParameterProperty -> Microsoft.Maui.Controls.BindableProperty!
Expand Down
Loading

0 comments on commit 862fa07

Please sign in to comment.