-
Notifications
You must be signed in to change notification settings - Fork 2k
[iOS] ScrollView: Clamp ScrollTo requests against AdjustedContentInset #37060
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 9 commits
e856c64
347ee65
0ddb79c
7f427d1
947997e
6894b31
ab129ab
b618395
2a26469
b89b634
e3f386a
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 |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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 | ||
|
|
@@ -45,13 +46,73 @@ 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(); | ||
| } | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| DispatchPendingScrollToRequest(); | ||
| } | ||
|
|
||
| void DispatchPendingScrollToRequest() | ||
| { | ||
| if (Handler is null || _pendingScrollToRequested is not { } pending) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (pending.Mode == ScrollToMode.Element) | ||
|
Collaborator
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.
[moderate] Regression Prevention and Test Coverage — This element-mode deferral lives in shared 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 |
||
| { | ||
| // 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 the request has to wait; OnSizeAllocated and ContentSizeChanged retry it. | ||
| // 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. | ||
| if (Width < 0 || Height < 0 || Content is { Width: < 0 } or { Height: < 0 }) | ||
|
Collaborator
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.
[moderate] Logic and Correctness — This guard was made strictly more restrictive and can now leave the caller's Previously The handler-removal path at line ~55 completes the task, so this is only unbounded while the handler stays attached and no layout ever runs — but that is a reachable state, and the failure mode is a silent permanent await rather than a wrong scroll position. Consider completing (or cancelling) the pending request on a bounded fallback, or dispatching with clamped-to-origin semantics after the first Related coverage gap: |
||
| { | ||
| 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); | ||
|
Collaborator
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.
[minor] Complexity Reduction — |
||
| return; | ||
| } | ||
|
|
||
| SendPendingScrollToRequest(); | ||
| } | ||
|
|
||
| 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 | ||
| ScrollToRequested?.Invoke(this, pending); | ||
|
Collaborator
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.
Concrete failure mechanism: The stated reason for bypassing
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. Partially adopted in e3f386a — dropping the replay raise entirely would break the compatibility |
||
| Handler.Invoke(nameof(IScrollView.RequestScrollTo), ConvertRequestMode(pending).ToRequest()); | ||
| } | ||
|
|
||
|
|
||
| /// <summary> | ||
| /// Gets the scroll position for the specified element. | ||
| /// </summary> | ||
|
|
@@ -62,39 +123,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> | ||
|
|
@@ -231,6 +315,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> | ||
|
|
@@ -426,6 +514,11 @@ void OnScrollToRequested(ScrollToRequestedEventArgs e) | |
| } | ||
| 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; | ||
|
Collaborator
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.
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. 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.
Collaborator
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.
Concrete failure mechanism: on iOS the handler is created when the view enters the visual tree, but the first arrange happens later. Suggest routing element-mode through the same gate: when
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. Adopted in e3f386a: |
||
|
|
||
| Handler.Invoke(nameof(IScrollView.RequestScrollTo), ConvertRequestMode(e).ToRequest()); | ||
| } | ||
| } | ||
|
|
@@ -470,6 +563,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); | ||
|
|
@@ -533,6 +634,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) | ||
|
|
||
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.
[moderate] Backward Compatibility — Completing the pending request when the handler goes away changes the previously supported "queue while detached, replay on attach" contract.
OnHandlerChangedCorealso runs withHandler is nullon transient detach (Shell tab switch, handler recreation on theme/MauiContextchange, 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.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.
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 (DisconnectHandler→ScrollFinished). 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.