Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,20 @@ public void SetCustomContent(View content)
{
sv.Scrolled += ScrollViewScrolled;
removeScrolledEvent = () => sv.Scrolled -= ScrollViewScrolled;
void ScrollViewScrolled(object sender, ScrolledEventArgs e) =>
OnScrolled((nfloat)sv.ScrollY);
void ScrollViewScrolled(object sender, ScrolledEventArgs e)
{
// Use the event only for timing and read the offset from the native view.
// OnScrolled works in native offsets (which rest at -headerHeight, since
// SetHeaderContentInset carries the header in ContentInset), while the
// value ScrollY reports depends on which renderer is in play — the default
// handler publishes content coordinates, the compatibility renderer raw
// ContentOffset — so converting from it would be right for one and wrong
// for the other. With no native view there is no usable offset: in this
// convention 0 is not neutral but "scrolled past the header", so skip
// rather than collapse the header.
if (ScrollView is { } nativeScrollView)
OnScrolled(nativeScrollView.ContentOffset.Y);
}
}
#pragma warning disable CS0618 // Type or member is obsolete
else if (Content is CollectionView cv)
Expand Down
155 changes: 142 additions & 13 deletions src/Controls/src/Core/ScrollView/ScrollView.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Diagnostics;
using System.Threading.Tasks;
using Microsoft.Maui.Graphics;
using Microsoft.Maui.Handlers;
using Microsoft.Maui.Layouts;

namespace Microsoft.Maui.Controls
Expand All @@ -15,7 +16,7 @@ namespace Microsoft.Maui.Controls
[ContentProperty(nameof(Content))]
[DebuggerDisplay("{GetDebuggerDisplay(), nq}")]
#pragma warning disable CS0618 // Type or member is obsolete
public partial class ScrollView : Compatibility.Layout, ILayout, ILayoutController, IPaddingElement, IView, IVisualTreeElement, IInputTransparentContainerElement, IScrollViewController, IElementConfiguration<ScrollView>, IFlowDirectionController, IScrollView, IContentView, ISafeAreaElement, ISafeAreaView2
public partial class ScrollView : Compatibility.Layout, ILayout, ILayoutController, IPaddingElement, IView, IVisualTreeElement, IInputTransparentContainerElement, IScrollViewController, IElementConfiguration<ScrollView>, IFlowDirectionController, IScrollView, IScrollOffsetReceiver, IContentView, ISafeAreaElement, ISafeAreaView2
#pragma warning restore CS0618 // Type or member is obsolete
{
#region IScrollViewController
Expand All @@ -40,18 +41,92 @@ public Rect LayoutAreaOverride
public event EventHandler<ScrollToRequestedEventArgs> ScrollToRequested;

ScrollToRequestedEventArgs _pendingScrollToRequested;
bool _replayPendingScrollToRequestedEvent;

private protected override void OnHandlerChangedCore()
{
base.OnHandlerChangedCore();

if (Handler is not null && _pendingScrollToRequested is not null)
if (Handler is null)
{
OnScrollToRequested(_pendingScrollToRequested);
_pendingScrollToRequested = null;
// The handler went away with a request still queued, so nothing will ever
// dispatch it. Release the caller rather than leaving its task pending
// forever; Core does the same for its own pending request on disconnect.
if (_pendingScrollToRequested is not null)
{
_pendingScrollToRequested = null;
SendScrollFinished();

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] Backward Compatibility — Completing the pending request when the handler goes away changes the previously supported "queue while detached, replay on attach" contract. OnHandlerChangedCore also runs with Handler is null on transient detach (Shell tab switch, handler recreation on theme/MauiContext change, page re-parenting), so a request issued while detached — or one still queued when a detach happens — is now silently discarded and reported to the caller as a completed scroll that never occurred. Consider only draining on a terminal detach (e.g. when the element is also unparented), or distinguishing "finished" from "abandoned" so callers can tell the scroll did not happen.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not adopted — the "previously supported queue-while-detached, replay-on-attach contract" never actually worked for the caller: the old replay went through OnScrollToRequested, which reset _scrollCompletionSource, so the original caller's task was orphaned and never completed (that is one of the deferral bugs this PR fixes). There was no good contract to preserve; the choice was between completing the task on terminal detach or hanging it, and completing matches what the handler layer already does with its own pending request on disconnect (DisconnectHandlerScrollFinished). Keeping the request parked across detach would also resurrect ghost scrolls: a page re-pushed minutes later would suddenly execute a stale scroll from its previous life. Task<bool> has no "abandoned" channel today, and inventing one is a public API change beyond this fix's scope.

}

return;
}

DispatchPendingScrollToRequest();
}

void DispatchPendingScrollToRequest()
{
if (Handler is null || _pendingScrollToRequested is not { } pending)
{
return;
}

if (pending.Mode == ScrollToMode.Element)

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] Regression Prevention and Test Coverage — This element-mode deferral lives in shared Controls code, so it changes ScrollToAsync(element, …) timing on Android, Windows and Tizen too: previously the pending request was replayed synchronously from OnHandlerChangedCore, now it is held until Width/Height/ContentSize are valid and then pushed to the next dispatcher tick. Every test added in this PR (Issue36801, Issue36801DeferredElement) is wrapped in #if IOS || MACCATALYST, so the non-iOS timing change ships with zero coverage.

Per the shared-code rule ("Shared code changes are tested on all affected platforms"), please either add a platform-agnostic regression test for the deferred element request (a Controls.Core.UnitTests test over DispatchPendingScrollToRequest + a stub handler would be enough to pin the ordering), or scope the deferral so non-iOS platforms keep their previous behavior.

{
if (!IsElementTargetGeometryReady())
{
// The request has to wait; OnSizeAllocated and ContentSizeChanged retry it.
return;
}

// Those callbacks run while the pass that produced the sizes is still arranging
// children, so resolve on the next tick, once positions are final. Posting on
// every retry is deliberate: SendPendingScrollToRequest is a no-op once the
// request has been sent or superseded, so a dropped callback cannot wedge the
// request the way an "already queued" flag would.
Dispatcher.Dispatch(SendPendingScrollToRequest);

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] Complexity ReductionDispatchPendingScrollToRequest is invoked from both OnSizeAllocated and ContentSizeChanged, each of which can fire several times per layout pass, and every invocation queues another Dispatcher.Dispatch. The comment justifies this (idempotence beats an "already queued" flag, which is the right call), but the queued closures still accumulate for the lifetime of the deferral. Since SendPendingScrollToRequest already re-checks _pendingScrollToRequested, a cheap if (_pendingScrollToRequested is null) return; at the top of the dispatch site plus reusing a cached Action delegate field would keep the same no-wedge property without the per-callback allocation.

return;
}

SendPendingScrollToRequest();
}

// An element target is resolved against this ScrollView's geometry and the element's
// position inside the arranged content. Before the first layout pass Width/Height are
// still -1 (the never-arranged sentinel, for the content too), so a target computed
// then is garbage. The content check must be "not yet arranged" rather than "arranged
// to nothing": content can legitimately arrange to a zero size (a collapsed container),
// and that raises no further callbacks — gating on the size would hang the caller's
// task forever, while dispatching just clamps the target to the origin.
bool IsElementTargetGeometryReady() =>

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] Logic and CorrectnessIsElementTargetGeometryReady() validates only this and the immediate Content, but GetScrollPositionForElement resolves against the target element's arranged geometry (item.Width/item.Height for Center/End, and every ancestor's X/Y via GetCoordinate). A target that is a descendant of a container arranged later — or added to the tree after the content's first arrange (deferred/virtualized content, IsVisible toggled on) — passes this gate while its own Width/Height are still -1, yielding a target off by viewport ± 1. Consider extending the readiness check to the request's target element (and its ancestor chain up to Content).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not adopted, deliberately — gating on the target element's own geometry creates an unretryable park: the retry signals are the ScrollView's OnSizeAllocated/ContentSizeChanged, and neither fires when only a descendant arranges (a fixed-size container realizing its child a pass later changes no ScrollView-level geometry), so the request would hang with no callback left to drain it — strictly worse than a target that's off until re-requested. The realistic deep cases are already covered: a descendant inside a collapsed branch now dispatches immediately via the WillArrange() bail-out (154984e), and descendants arranged in the same pass as the content are final by dispatch time because resolution is posted to the next dispatcher tick. Note also that GetCoordinate walks X/Y, which have no -1 sentinel — an ancestor-chain readiness check on those is not actually expressible.

Width >= 0 && Height >= 0 && Content is not ({ Width: < 0 } or { Height: < 0 });

void SendPendingScrollToRequest()
{
if (Handler is null || _pendingScrollToRequested is not { } pending)
{
return;
}

_pendingScrollToRequested = null;

// Replay without going through OnScrollToRequested: that would reset the
// completion source and orphan the task the original caller is still awaiting.
// The event is re-raised only for requests parked before the handler attached:
// compatibility renderers subscribe to ScrollToRequested at attach and perform the
// scroll from it, so they would otherwise never see the request. A request parked
// with the handler present (waiting for element geometry) already notified its
// subscribers at request time, and re-raising would double-notify them.
if (_replayPendingScrollToRequestedEvent)
{
_replayPendingScrollToRequestedEvent = false;
ScrollToRequested?.Invoke(this, pending);
}

Handler.Invoke(nameof(IScrollView.RequestScrollTo), ConvertRequestMode(pending).ToRequest());
}


/// <summary>
/// Gets the scroll position for the specified element.
/// </summary>
Expand All @@ -62,39 +137,62 @@ public Point GetScrollPositionForElement(VisualElement item, ScrollToPosition po
double y = GetCoordinate(item, "Y", 0);
double x = GetCoordinate(item, "X", 0);

// The scrollable viewport can be smaller than this ScrollView's frame: on iOS, content
// insets (safe area, ContentInset) obscure part of the frame and (0,0) in scroll
// coordinates is the inset rest position. Compute element targets against the effective
// viewport so End/Center/MakeVisible land the element fully inside the visible region.
// Part of those insets can be baked into the content itself (the platform arranged the
// content inside safe-area-inset bounds): element coordinates already include that
// padding, so targets shift back by it — the platform-inset part is instead
// compensated when the request is translated to a native offset.
var viewportInsets = GetVisibleViewportInsets();
var contentInsets = GetContentCoordinateInsets();
double viewportWidth = Math.Max(0, Width - viewportInsets.HorizontalThickness);
double viewportHeight = Math.Max(0, Height - viewportInsets.VerticalThickness);

if (position == ScrollToPosition.MakeVisible)
{
var scrollBounds = new Rect(ScrollX, ScrollY, Width, Height);
// In content coordinates the visible window starts past the baked padding
var scrollBounds = new Rect(ScrollX + contentInsets.Left, ScrollY + contentInsets.Top, viewportWidth, viewportHeight);
var itemBounds = new Rect(x, y, item.Width, item.Height);
if (scrollBounds.Contains(itemBounds))
return new Point(ScrollX, ScrollY);
switch (Orientation)
{
case ScrollOrientation.Vertical:
position = y > ScrollY ? ScrollToPosition.End : ScrollToPosition.Start;
position = y > scrollBounds.Y ? ScrollToPosition.End : ScrollToPosition.Start;
break;
case ScrollOrientation.Horizontal:
position = x > ScrollX ? ScrollToPosition.End : ScrollToPosition.Start;
position = x > scrollBounds.X ? ScrollToPosition.End : ScrollToPosition.Start;
break;
case ScrollOrientation.Both:
position = x > ScrollX || y > ScrollY ? ScrollToPosition.End : ScrollToPosition.Start;
position = x > scrollBounds.X || y > scrollBounds.Y ? ScrollToPosition.End : ScrollToPosition.Start;
break;
}
}
switch (position)
{
case ScrollToPosition.Center:
y = y - Height / 2 + item.Height / 2;
x = x - Width / 2 + item.Width / 2;
y = y - viewportHeight / 2 + item.Height / 2;
x = x - viewportWidth / 2 + item.Width / 2;
break;
case ScrollToPosition.End:
y = y - Height + item.Height;
x = x - Width + item.Width;
y = y - viewportHeight + item.Height;
x = x - viewportWidth + item.Width;
break;
}
return new Point(x, y);
return new Point(x - contentInsets.Left, y - contentInsets.Top);
}

// The scrollable viewport can be smaller than the frame: on iOS the adjusted content
// insets obscure part of it. The handler owns that coordinate convention and reports
// it here; handlers whose viewport always equals the frame don't implement the contract.
Thickness GetVisibleViewportInsets() =>
(Handler as IScrollViewportProvider)?.ViewportInsets ?? default;

Thickness GetContentCoordinateInsets() =>
(Handler as IScrollViewportProvider)?.ContentCoordinateInsets ?? default;

/// <summary>
/// Sends the scroll finished notification.
/// </summary>
Expand Down Expand Up @@ -231,6 +329,10 @@ void ContentSizeChanged(object sender, EventArgs e)
// The ContentSize includes the margins for the content
ContentSize = new Size(frameSize.Width + margin.HorizontalThickness,
frameSize.Height + margin.VerticalThickness);

// The content has been arranged, so an element target can now be resolved: its
// position is read from the content tree, which is only meaningful once laid out
DispatchPendingScrollToRequest();
}

/// <summary>
Expand Down Expand Up @@ -423,9 +525,24 @@ void OnScrollToRequested(ScrollToRequestedEventArgs e)
if (Handler is null)
{
_pendingScrollToRequested = e;
_replayPendingScrollToRequestedEvent = true;
}
else if (e.Mode == ScrollToMode.Element && !IsElementTargetGeometryReady())

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] Regression Prevention — Parking an element-mode request until IsElementTargetGeometryReady() introduces a path where the request is never dispatched at all: retries only come from OnSizeAllocated and ContentSizeChanged, so a ScrollView that is never arranged (IsVisible="False" / Visibility.Collapsed is skipped by LayoutManager, so Width/Height stay at the -1 sentinel) leaves _pendingScrollToRequested set forever and the caller's ScrollToAsync(element, …) task never completes. Before this change the request was handed straight to the handler, which clamped and called SendScrollFinished(), so the task always completed. The new unit tests cover handler-removal (DeferredElementScrollCompletesWhenTheHandlerGoesAway) and zero-size content (DeferredElementScrollCompletesWhenContentArrangesToZero) but not the never-arranged ScrollView itself — please add that negative case and a terminal path (complete or cancel) for it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adopted in 154984e: both park sites now check WillArrange() (the IsVisible chain up to the root) and dispatch immediately when the view sits in a collapsed branch — the target clamps and the caller's task completes, matching the other platforms. Two new regression tests cover the collapsed ScrollView itself (ElementRequestOnCollapsedScrollViewCompletesInsteadOfHanging) and a collapsed ancestor at handler-attach time (DeferredElementRequestOnCollapsedAncestorDispatchesOnAttach).

{
// The handler exists but layout has not run yet (e.g. ScrollToAsync from
// OnAppearing): resolving the element target now would compute against the -1
// never-arranged sentinels. Park it for the layout callbacks instead — the
// subscribers were already notified above, so the replay must not re-raise.
_pendingScrollToRequested = e;
_replayPendingScrollToRequestedEvent = false;
}
else
{
// This request supersedes anything still queued: a deferred element request
// whose dispatch is already scheduled must not run afterwards and restore the
// older target (latest request wins).
_pendingScrollToRequested = null;

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)

⚠️ Deferral only covers the Handler is null window, not the "not yet arranged" window. DispatchPendingScrollToRequest documents that an element target is meaningless until Width/Height leave the -1 never-arranged sentinel, but a request only becomes pending when Handler is null. Once the handler is attached and layout has not yet run (a very common ordering on iOS — e.g. ScrollToAsync(element, ScrollToPosition.End) from OnAppearing, or a page in a not-yet-laid-out Shell tab), this else branch dispatches immediately and ConvertRequestModeGetScrollPositionForElement runs with Width == Height == -1. With this PR that is now silently absorbed: Math.Max(0, Width - insets) yields a 0 viewport, so Center/End degrade to Start instead of being deferred, and no later callback re-resolves the target. Consider gating on geometry rather than on the handler, i.e. in the else branch set _pendingScrollToRequested = e and call DispatchPendingScrollToRequest() so OnSizeAllocated/ContentSizeChanged retry it, instead of dispatching unconditionally.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in e3f386a — element requests now gate on arranged geometry regardless of when the handler attached; details on the newer thread from the follow-up review.

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)

⚠️ Warning — the new "not yet arranged" guard is bypassed on the most common pre-layout path.

DispatchPendingScrollToRequest correctly refuses to resolve an element target while Width < 0 || Height < 0 || Content is { Width: < 0 } or { Height: < 0 }, but that guard is only reachable for requests that were queued because Handler was null. Here, in the else branch, an element-mode request is converted immediately via ConvertRequestMode(e) with no arrange check.

Concrete failure mechanism: on iOS the handler is created when the view enters the visual tree, but the first arrange happens later. ScrollToAsync(element, ScrollToPosition.Center, ...) invoked from Page.OnAppearing/Loaded/a PropertyChanged handler therefore hits this branch with Handler != null and Width == Height == -1 (and the element's own Width/Height still -1). GetScrollPositionForElement then computes against viewportWidth = Math.Max(0, -1 - insets)0 and item.Width == -1, so Center/End produce targets like y + (-1)/2 off garbage geometry; GetTargetContentOffset clamps that to the inset rest position and the caller's Task completes having scrolled nowhere. Because this PR now does handle the same situation correctly on the handler-null path, the behaviour is inconsistent depending on whether the handler happened to be attached yet.

Suggest routing element-mode through the same gate: when pending.Mode == ScrollToMode.Element and the arrange sentinels are still -1, store into _pendingScrollToRequested and let OnSizeAllocated/ContentSizeChanged retry, instead of dispatching immediately.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adopted in e3f386a: OnScrollToRequested now parks an element-mode request behind the same arranged-geometry gate (IsElementTargetGeometryReady) when the handler is attached but layout hasn't run; OnSizeAllocated/ContentSizeChanged retry it, and the geometry-ready path stays synchronous. Unit test ElementRequestWithHandlerAttachedWaitsForArrange covers the OnAppearing-style ordering.


Handler.Invoke(nameof(IScrollView.RequestScrollTo), ConvertRequestMode(e).ToRequest());
}
}
Expand Down Expand Up @@ -470,6 +587,14 @@ double IScrollView.VerticalOffset
}
}

void IScrollOffsetReceiver.UpdateScrollOffsets(double horizontalOffset, double verticalOffset)
{
// The reported offsets moved because the platform insets did, not because anything
// scrolled: keep ScrollX/ScrollY (and their bindings) current without raising Scrolled
ScrollX = horizontalOffset;
ScrollY = verticalOffset;
}

void IScrollView.RequestScrollTo(double horizontalOffset, double verticalOffset, bool instant)
{
var request = new ScrollToRequest(horizontalOffset, verticalOffset, instant);
Expand Down Expand Up @@ -533,6 +658,10 @@ protected override Size ArrangeOverride(Rect bounds)
protected override void OnSizeAllocated(double width, double height)
{
base.OnSizeAllocated(width, height);

// Geometry is now known, so an element-mode request held back at handler-attach
// can be resolved
DispatchPendingScrollToRequest();
}

Size ICrossPlatformLayout.CrossPlatformArrange(Rect bounds)
Expand Down
Loading
Loading