Skip to content

[iOS, MacCatalyst] Fix CollectionView2 grouped ScrollTo MakeVisible producing inconsistent positions #35668

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.

Sign up for GitHub

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

Open
praveenkumarkarunanithi wants to merge 7 commits into
base: main
Choose a base branch
from
Open

[iOS, MacCatalyst] Fix CollectionView2 grouped ScrollTo MakeVisible producing inconsistent positions #35668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
49a337b
fix and test update
praveenkumarkarunanithi Apr 10, 2026
e9d3a26
Update ItemsViewController2.cs
praveenkumarkarunanithi Apr 21, 2026
6d77cb3
Merge branch 'main' into fix-34663
praveenkumarkarunanithi May 11, 2026
dd40287
Merge remote-tracking branch 'upstream/main' into fix-34663
praveenkumarkarunanithi May 29, 2026
f3026ae
updating fix logic to prevent memory leak as per AI concern
praveenkumarkarunanithi Jun 4, 2026
6b905cd
Update Issue34663.cs
praveenkumarkarunanithi Jun 19, 2026
3c207ac
Update Issue34663.cs
praveenkumarkarunanithi Jun 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Controls/src/Core/Handlers/Items/iOS/ObservableGroupedSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ internal class ObservableGroupedSource : IObservableItemsViewSource
bool _disposed;
List<ObservableItemsSource> _groups = new List<ObservableItemsSource>();

internal event NotifyCollectionChangedEventHandler CollectionViewUpdating;

public ObservableGroupedSource(IEnumerable groupSource, UICollectionViewController collectionViewController)
{
_collectionViewController = new(collectionViewController);
Expand Down Expand Up @@ -211,6 +213,9 @@ void Reload(bool collectionWasReset = false)

_groupCount = GroupsCount();

var args = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset);

@MauiBot MauiBot Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[minor] Performance-Critical Path — NotifyCollectionChangedEventArgs is allocated unconditionally on every Reload(), even though the only consumer of CollectionViewUpdating (CV2's ItemsViewController2) ignores everything except Action == Reset, and there is normally no subscriber at all for CV1. Guard the allocation on the delegate being non-null (if (CollectionViewUpdating is { } handler) handler(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));) — Reload() is called for every Reset on the bound collection.

CollectionViewUpdating?.Invoke(this, args);

_collectionView.ReloadData();
if (collectionWasReset)
{
Expand Down
54 changes: 54 additions & 0 deletions src/Controls/src/Core/Handlers/Items2/iOS/ItemsViewController2.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#nullable disable
using System;
using System.Collections.Generic;
using System.Collections.Specialized;
using System.ComponentModel;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
Expand Down Expand Up @@ -88,6 +89,7 @@ protected override void Dispose(bool disposing)

if (disposing)
{
UnsubscribeFromItemsSourceUpdating();
ItemsSource?.Dispose();

((IUIViewLifeCycleEvents)CollectionView).MovedToWindow -= MovedToWindow;
Expand Down Expand Up @@ -163,6 +165,7 @@ public override void ViewDidLoad()
base.ViewDidLoad();

ItemsSource = CreateItemsViewSource();
SubscribeToItemsSourceUpdating();

if (!(OperatingSystem.IsIOSVersionAtLeast(11) || OperatingSystem.IsMacCatalystVersionAtLeast(11)
#if TVOS
Expand Down Expand Up @@ -251,16 +254,65 @@ internal void ReloadData()
handler.SetCachedFirstItemSize(CoreGraphics.CGSize.Empty);
}

ResetEstimatedItemSize();
CollectionView.ReloadData();
}

internal void DisposeItemsSource()
{
UnsubscribeFromItemsSourceUpdating();
ItemsSource?.Dispose();
ItemsSource = new Items.EmptySource();
ReloadData();
}

void SubscribeToItemsSourceUpdating()
{
if (ItemsSource is Items.ObservableItemsSource observableSource)
{
observableSource.CollectionViewUpdating += OnItemsSourceCollectionViewUpdating;
}
else if (ItemsSource is Items.ObservableGroupedSource observableGroupedSource)
{
observableGroupedSource.CollectionViewUpdating += OnItemsSourceCollectionViewUpdating;
}
}

@MauiBot MauiBot Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Memory Leak Prevention — ObservableItemsSource deliberately holds its controller as WeakReference<UICollectionViewController> (see its constructor), because the source itself is rooted by the user's collection via ((INotifyCollectionChanged)itemSource).CollectionChanged += CollectionChanged. This += installs a strong managed reference from the source back to the controller, so for as long as the user's ObservableCollection is alive, the controller — and through it the UICollectionView, its cells, and the whole page/ViewModel graph — is kept alive. That is fine only if Dispose/DisposeItemsSource always runs; on iOS the controller can be released without Dispose running deterministically (e.g. Shell tab switch / handler disconnect ordering). Prefer a weak-event or holder-object subscription so the weak-controller design is preserved, and add a leak-detection device test covering a grouped CV whose source collection outlives the page.

void UnsubscribeFromItemsSourceUpdating()
{
if (ItemsSource is Items.ObservableItemsSource observableSource)
{

@MauiBot MauiBot Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] CollectionView iOS/MacCatalyst — Subscribing only to the top-level ObservableGroupedSource misses the per-group child ObservableItemsSource instances it creates in _groups. When an individual group's inner collection raises a Reset (or any change routed through ObservableItemsSource.Reload()/Update()), that child source calls collectionView.ReloadData() + InvalidateLayout() directly, the section provider re-runs, and it re-reads the now-stale global estimate — exactly the bypass path this PR is trying to close, but for grouped data, which is the reported repro shape. Subscribe to the child sources too (and unsubscribe when _groups is rebuilt), or hook the reset at a level both sources funnel through.

observableSource.CollectionViewUpdating -= OnItemsSourceCollectionViewUpdating;
}
else if (ItemsSource is Items.ObservableGroupedSource observableGroupedSource)
{
observableGroupedSource.CollectionViewUpdating -= OnItemsSourceCollectionViewUpdating;
}
}

void OnItemsSourceCollectionViewUpdating(object sender, NotifyCollectionChangedEventArgs e)
{
// ObservableItemsSource/ObservableGroupedSource Reload() calls
// collectionView.ReloadData() directly, bypassing ItemsViewController2.ReloadData().
// Reset the measured estimate here so the section provider uses fresh measurements.
if (e.Action == NotifyCollectionChangedAction.Reset)
{
ResetEstimatedItemSize();
}
}

/// <summary>
/// Resets the measured estimated item size so the section provider
/// falls back to the default estimate until a new measurement arrives.
/// </summary>
void ResetEstimatedItemSize()
{
if (CollectionView?.CollectionViewLayout is LayoutFactory2.CustomUICollectionViewCompositionalLayout compLayout)
{
compLayout.MeasuredEstimatedItemSize = null;
}
}

void EnsureLayoutInitialized()
{
if (_initialized)
Expand Down Expand Up @@ -290,8 +342,10 @@ protected virtual Items.IItemsViewSource CreateItemsViewSource()

public virtual void UpdateItemsSource()
{
UnsubscribeFromItemsSourceUpdating();
ItemsSource?.Dispose();
ItemsSource = CreateItemsViewSource();
SubscribeToItemsSourceUpdating();

ReloadData();
CollectionView.CollectionViewLayout.InvalidateLayout();
Expand Down
83 changes: 73 additions & 10 deletions src/Controls/src/Core/Handlers/Items2/iOS/LayoutFactory2.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,31 @@ static UICollectionViewLayout CreateListLayout(UICollectionViewScrollDirection s
//create global header and footer
layoutConfiguration.BoundarySupplementaryItems = CreateSupplementaryItems(null, layoutHeaderFooterInfo, scrollDirection, groupWidth, groupHeight);

var estimatedSizeHolder = new EstimatedItemSizeHolder();
var layout = new CustomUICollectionViewCompositionalLayout(snapInfo, groupingInfo, layoutHeaderFooterInfo, (sectionIndex, environment) =>
{
var effectiveItemHeight = itemHeight;
var effectiveGroupHeight = groupHeight;
var effectiveItemWidth = itemWidth;
var effectiveGroupWidth = groupWidth;

if (estimatedSizeHolder.Value is nfloat measuredSize)
{
var estimatedDimension = NSCollectionLayoutDimension.CreateEstimated(measuredSize);
if (scrollDirection == UICollectionViewScrollDirection.Vertical)
{
effectiveItemHeight = estimatedDimension;
effectiveGroupHeight = estimatedDimension;
}
else
{
effectiveItemWidth = estimatedDimension;
effectiveGroupWidth = estimatedDimension;
}
}

@MauiBot MauiBot Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] CollectionView iOS/MacCatalyst — The measured estimate is applied to items and groups, but the group header/footer supplementary items created a few lines below still use the original groupWidth/groupHeight (CreateSupplementaryItems(groupingInfo, null, scrollDirection, groupWidth, groupHeight)), i.e. they remain pinned at Estimated(30f). For the grouped CollectionView in issue #34663, section headers are typically the largest contributor to the cumulative offset error that ScrollTo(..., MakeVisible) accumulates, so the dominant source of drift is left unfixed while item estimates change. Either feed effectiveGroupWidth/effectiveGroupHeight (or a separately measured supplementary estimate) into CreateSupplementaryItems, or document why headers are exempt. The same gap exists in CreateGridLayout.

// Each item has a size
var itemSize = NSCollectionLayoutSize.Create(itemWidth, itemHeight);
var itemSize = NSCollectionLayoutSize.Create(effectiveItemWidth, effectiveItemHeight);
// Create the item itself from the size
var item = NSCollectionLayoutItem.Create(layoutSize: itemSize);

Expand All @@ -105,16 +126,16 @@ static UICollectionViewLayout CreateListLayout(UICollectionViewScrollDirection s
if (scrollDirection == UICollectionViewScrollDirection.Vertical)
{
var newGroupHeight = environment.Container.ContentSize.Height - peekAreaInsets.Top - peekAreaInsets.Bottom;
groupHeight = NSCollectionLayoutDimension.CreateAbsolute((nfloat)newGroupHeight);
effectiveGroupHeight = NSCollectionLayoutDimension.CreateAbsolute((nfloat)newGroupHeight);
}
else
{
var newGroupWidth = environment.Container.ContentSize.Width - peekAreaInsets.Left - peekAreaInsets.Right;
groupWidth = NSCollectionLayoutDimension.CreateAbsolute((nfloat)newGroupWidth);
effectiveGroupWidth = NSCollectionLayoutDimension.CreateAbsolute((nfloat)newGroupWidth);
}
}
// Each group of items (for grouped collections) has a size
var groupSize = NSCollectionLayoutSize.Create(groupWidth, groupHeight);
var groupSize = NSCollectionLayoutSize.Create(effectiveGroupWidth, effectiveGroupHeight);

// Create the group
// If vertical list, we want the group to layout horizontally (eg: grid columns go left to right)
Expand Down Expand Up @@ -143,7 +164,7 @@ static UICollectionViewLayout CreateListLayout(UICollectionViewScrollDirection s
groupHeight);

return section;
}, layoutConfiguration, itemsLayout);
}, layoutConfiguration, itemsLayout, estimatedSizeHolder);

return layout;
}
Expand All @@ -155,15 +176,36 @@ static UICollectionViewLayout CreateGridLayout(UICollectionViewScrollDirection s
var layoutConfiguration = new UICollectionViewCompositionalLayoutConfiguration();
layoutConfiguration.ScrollDirection = scrollDirection;

var estimatedSizeHolder = new EstimatedItemSizeHolder();
var layout = new CustomUICollectionViewCompositionalLayout(snapInfo, groupingInfo, headerFooterInfo, (sectionIndex, environment) =>
{
var effectiveItemHeight = itemHeight;
var effectiveGroupHeight = groupHeight;
var effectiveItemWidth = itemWidth;
var effectiveGroupWidth = groupWidth;

if (estimatedSizeHolder.Value is nfloat measuredSize)
{
var estimatedDimension = NSCollectionLayoutDimension.CreateEstimated(measuredSize);
if (scrollDirection == UICollectionViewScrollDirection.Vertical)
{
effectiveItemHeight = estimatedDimension;
effectiveGroupHeight = estimatedDimension;
}
else
{
effectiveItemWidth = estimatedDimension;
effectiveGroupWidth = estimatedDimension;
}
}

// Each item has a size
var itemSize = NSCollectionLayoutSize.Create(itemWidth, itemHeight);
var itemSize = NSCollectionLayoutSize.Create(effectiveItemWidth, effectiveItemHeight);
// Create the item itself from the size
var item = NSCollectionLayoutItem.Create(layoutSize: itemSize);

// Each group of items (for grouped collections) has a size
var groupSize = NSCollectionLayoutSize.Create(groupWidth, groupHeight);
var groupSize = NSCollectionLayoutSize.Create(effectiveGroupWidth, effectiveGroupHeight);

// Create the group
// If vertical list, we want the group to layout horizontally (eg: grid columns go left to right)
Expand Down Expand Up @@ -197,7 +239,7 @@ static UICollectionViewLayout CreateGridLayout(UICollectionViewScrollDirection s
groupHeight);

return section;
}, layoutConfiguration, itemsLayout);
}, layoutConfiguration, itemsLayout, estimatedSizeHolder);

return layout;
}
Expand Down Expand Up @@ -497,19 +539,40 @@ public static bool IsIndexPathValid(NSIndexPath indexPath, UICollectionView coll
return false;
}
}
class CustomUICollectionViewCompositionalLayout : UICollectionViewCompositionalLayout
/// <summary>
/// Holds the measured estimated item size shared between the layout and its
/// section provider closure, avoiding a layout → closure → layout retain cycle.
/// </summary>
internal sealed class EstimatedItemSizeHolder
{
internal nfloat? Value { get; set; }
}

internal class CustomUICollectionViewCompositionalLayout : UICollectionViewCompositionalLayout
{
LayoutSnapInfo _snapInfo;
ItemsLayout? _itemsLayout;
LayoutGroupingInfo? _groupingInfo;
LayoutHeaderFooterInfo? _headerFooterInfo;

public CustomUICollectionViewCompositionalLayout(LayoutSnapInfo snapInfo, LayoutGroupingInfo? groupingInfo, LayoutHeaderFooterInfo? headerFooterInfo, UICollectionViewCompositionalLayoutSectionProvider sectionProvider, UICollectionViewCompositionalLayoutConfiguration configuration, ItemsLayout? itemsLayout) : base(sectionProvider, configuration)
// Shared holder so the section provider closure can read the measured
// size without capturing a strong reference to this layout instance.
internal EstimatedItemSizeHolder EstimatedSizeHolder { get; }

// Convenience accessor — delegates to the shared holder.
internal nfloat? MeasuredEstimatedItemSize
{
get => EstimatedSizeHolder.Value;
set => EstimatedSizeHolder.Value = value;
}

public CustomUICollectionViewCompositionalLayout(LayoutSnapInfo snapInfo, LayoutGroupingInfo? groupingInfo, LayoutHeaderFooterInfo? headerFooterInfo, UICollectionViewCompositionalLayoutSectionProvider sectionProvider, UICollectionViewCompositionalLayoutConfiguration configuration, ItemsLayout? itemsLayout, EstimatedItemSizeHolder? estimatedSizeHolder = null) : base(sectionProvider, configuration)
{
_snapInfo = snapInfo;
_itemsLayout = itemsLayout;
_groupingInfo = groupingInfo;
_headerFooterInfo = headerFooterInfo;
EstimatedSizeHolder = estimatedSizeHolder ?? new EstimatedItemSizeHolder();
}

public override void FinalizeCollectionViewUpdates()
Expand Down
25 changes: 25 additions & 0 deletions src/Controls/src/Core/Handlers/Items2/iOS/TemplatedCell2.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ public override UICollectionViewLayoutAttributes PreferredLayoutAttributesFittin
// If this is the first item being measured, cache it for MeasureFirstItem strategy
SetCachedFirstItemSizeToHandler(_measuredSize.ToCGSize());
}

UpdateLayoutEstimatedItemSize();

@MauiBot MauiBot Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Performance-Critical Path — UpdateLayoutEstimatedItemSize() runs on every item-cell (re)measure, not just the first one, and the cheap short-circuit (MeasuredEstimatedItemSize is null) is evaluated last. Each call first walks CollectionViewHandler (which itself re-walks PlatformHandler.VirtualView → view.Parent → itemsView.Handler with three type checks — note the getter is already invoked twice on this path via GetCachedFirstItemSizeFromHandler/SetCachedFirstItemSizeToHandler) and then crosses into Obj-C for Controller.CollectionView.CollectionViewLayout. During fast scrolling of a large CV this is repeated per cell per pass for no benefit. Hoist a cheap managed guard (e.g. a bool _estimateReported on the controller/handler, reset alongside ResetEstimatedItemSize) and return early before the handler walk and the Obj-C property access.

}
else
{
Expand Down Expand Up @@ -176,6 +178,29 @@ private void SetCachedFirstItemSizeToHandler(CGSize size)
CollectionViewHandler?.SetCachedFirstItemSize(size);
}

/// <summary>
/// Updates the layout's estimated item size with the actual measured size.
/// Called once after the first non-supplementary cell is measured, so that
/// subsequent section provider calls use a realistic estimate instead of
/// the hardcoded 30px default.
/// </summary>
private void UpdateLayoutEstimatedItemSize()
{
if (CollectionViewHandler?.Controller?.CollectionView?.CollectionViewLayout
is LayoutFactory2.CustomUICollectionViewCompositionalLayout compLayout
&& compLayout.MeasuredEstimatedItemSize is null)

@MauiBot MauiBot Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] CollectionView iOS/MacCatalyst — MeasuredEstimatedItemSize is null makes this a write-once, first-cell-wins global estimate that is shared by every item in every section. Concrete failure scenarios: (1) DataTemplateSelector or heterogeneous grouped data — whichever cell happens to be measured first (which is not necessarily index 0, and for a scrolled-to position is whatever cell UIKit prepares first) poisons the estimate for all other templates; (2) rotation / dynamic-type / container resize — items remeasure to a materially different height but the estimate is pinned to the pre-rotation value forever, because the only reset paths are ReloadData() and a source Reset. Given the bug being fixed is precisely offset drift caused by a wrong estimate, a stale-but-wrong estimate reintroduces the same class of drift. Consider tracking the estimate per section/template, or updating it whenever the measured dimension diverges from the current estimate beyond a tolerance.

{
var measuredDimension = ScrollDirection == UICollectionViewScrollDirection.Vertical
? (nfloat)_measuredSize.Height
: (nfloat)_measuredSize.Width;

if (measuredDimension > 0 && !nfloat.IsNaN(measuredDimension) && !nfloat.IsInfinity(measuredDimension))
{
compLayout.MeasuredEstimatedItemSize = measuredDimension;

@MauiBot MauiBot Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Logic and Correctness — The measured estimate is written into the layout, but nothing invalidates the layout afterwards. UICollectionViewCompositionalLayout only re-invokes its section provider closure when the layout is invalidated, and this write happens inside PreferredLayoutAttributesFittingAttributes (i.e. during a layout pass that has already built its sections from the old Estimated(30) dimensions). Concrete scenario: grouped CV, first ScrollTo(..., MakeVisible) — the first cell measures at e.g. 44pt and sets MeasuredEstimatedItemSize = 44, but the section provider for the current and already-built sections is never re-run, so the offsets that ScrollTo computes are still derived from the 30pt estimate. The new estimate is only picked up if some unrelated code path later calls InvalidateLayout(), which makes the fix non-deterministic. This is consistent with the Gate result (both UI tests pass identically with and without the production change). Either invalidate explicitly (deferred to the next runloop turn to avoid re-entrant invalidation from within a layout pass) or explain why the estimate is guaranteed to be consumed.

}
}
}

public override void LayoutSubviews()
{
base.LayoutSubviews();
Expand Down
Loading
Loading