[iOS] ScrollView: Clamp ScrollTo requests against AdjustedContentInset - #37060
Conversation
MapRequestScrollTo clamped programmatic scrolls to [0, ContentSize - Frame], ignoring the UIScrollView's AdjustedContentInset. On inset scroll views (the standard .NET 10 edge-to-edge configuration) scroll-to-end stopped short by the adjusted-inset sum and scroll-to-zero could not restore the natural rest position (-adjustedInset.Top). Translate cross-platform content coordinates into native offset space, clamp against the inset-aware range, and apply the reverse translation when reporting ScrollX/ScrollY so requests round-trip. With zero insets the math degenerates to the previous behavior. Fixes dotnet#36801 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 37060Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 37060" |
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hey there @@albyrock87! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
Hey there @albyrock87! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 7 findings
See inline comments for details.
| var maxScrollVertical = Math.Max(minScrollVertical, uiScrollView.ContentSize.Height + adjustedInset.Bottom - bounds.Height); | ||
|
|
||
| var targetHorizontal = Math.Clamp(request.HorizontalOffset - (double)adjustedInset.Left, minScrollHorizontal, maxScrollHorizontal); | ||
| var targetVertical = Math.Clamp(request.VerticalOffset - (double)adjustedInset.Top, minScrollVertical, maxScrollVertical); |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
❌ Error — Logic and Correctness / Regression Prevention: subtracting adjustedInset.Top here regresses ScrollToAsync(element, ScrollToPosition.End | Center | MakeVisible-downwards).
Controls/ScrollView.ConvertRequestMode → GetScrollPositionForElement (src/Controls/src/Core/ScrollView/ScrollView.cs:86-94) computes the End target as y = elementY - Height + element.Height, where Height is VisualElement.Height — the full frame height, not the inset-reduced visible viewport. That value then arrives here as request.VerticalOffset.
With ContentInsetAdjustmentBehavior.Automatic (the default for SafeAreaEdges.Default, see MauiScrollView.cs:249) and adjTop = 96, adjBottom = 34, bounds.Height = H:
- native target =
elementY + elemH - H - 96 - visible bottom edge in content coords =
target + H - adjBottom=elementY + elemH - 96 - 34 - → the element bottom sits 130pt below the visible bottom edge (was 34pt before this PR).
For a typical 30-44pt row that means the element the caller asked to scroll to is now entirely outside the visible region, whereas before it was only clipped by the home-indicator inset. ScrollToPosition.Start genuinely improves, so the origin shift is right — but the viewport length used by GetScrollPositionForElement must be corrected in the same change (or the End/Center/MakeVisible path compensated here) and covered by a test. As written the PR trades one broken direction for another with no coverage for ScrollToAsync(element, position) at all.
There was a problem hiding this comment.
Fixed in 347ee65. The regression was real: with the new coordinate mapping, GetScrollPositionForElement computed End/Center targets against the full frame height while the effective viewport is smaller by the insets. It now computes against the effective viewport (Height/Width minus AdjustedContentInset on iOS/Catalyst, unchanged elsewhere), so ScrollToAsync(element, End) lands the element's bottom edge exactly at the bottom of the unobscured viewport — for all insets, not just the pre-PR adjustedInset.Bottom clipping. Covered by the new ScrollToElementEndLandsInsideVisibleViewport UI test, whose oracle is geometric (probe frame vs. viewport bottom in window coordinates), verified passing on the iOS simulator.
| var minScrollHorizontal = -(double)adjustedInset.Left; | ||
| var minScrollVertical = -(double)adjustedInset.Top; | ||
| var maxScrollHorizontal = Math.Max(minScrollHorizontal, uiScrollView.ContentSize.Width + adjustedInset.Right - bounds.Width); | ||
| var maxScrollVertical = Math.Max(minScrollVertical, uiScrollView.ContentSize.Height + adjustedInset.Bottom - bounds.Height); |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
adjustedInset.Bottom double-counts the bottom inset when SafeAreaEdges is Container / All / SoftInput.
For those regions MauiScrollView.UpdateContentInsetAdjustmentBehavior selects Always/Never (MauiScrollView.cs:251-254), and MauiScrollView.CrossPlatformArrange then already pads the reported content size:
else if (ContentInsetAdjustmentBehavior != UIScrollViewContentInsetAdjustmentBehavior.Automatic)
{
width += _safeArea.HorizontalThickness; // MauiScrollView.cs:500-504
height += _safeArea.VerticalThickness;
}For Container (→ Always) _safeArea is SystemAdjustedContentInset (MauiScrollView.cs:390-393) and AdjustedContentInset is non-zero, so with content of arranged height C:
ContentSize.Height = C + safeTop + safeBottom- new max =
C + safeTop + 2*safeBottom - bounds.Height, while the offset that puts the last content pixel flush with the visible bottom isC + safeBottom - bounds.Height.
So ScrollToAsync(0, ContentSize.Height, ...) on a SafeAreaEdges.Container ScrollView now stops with safeTop + safeBottom of blank space under the last item instead of safeTop. Please verify this mode on device and add a device/UI test for SafeAreaEdges.Container — the added test only covers the Automatic configuration, where the new formula is correct.
There was a problem hiding this comment.
Fixed in 347ee65. When ContentInsetAdjustmentBehavior is Always, the clamp now excludes the system-contributed inset (AdjustedContentInset - ContentInset, i.e. exactly the amount CrossPlatformArrange also baked into ContentSize) from the reachable maximum, so scroll-to-end stops at the content instead of safeTop + safeBottom into the padding. Computed from public UIScrollView properties — no MauiScrollView changes. Never mode is unaffected (its padding intentionally substitutes for the inset), and Automatic keeps the plain inset-aware formula, which the UI tests verify on-simulator against the content platform view's actual frame.
| // is (-adjustedInset.Left, -adjustedInset.Top) and the native maximum extends past | ||
| // ContentSize - Bounds by the trailing insets. Translate the request into native offset | ||
| // space and clamp against the inset-aware range (issue #36801). | ||
| var adjustedInset = uiScrollView.AdjustedContentInset; |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
AdjustedContentInset can still be stale on the deferred PendingScrollToRequest path, so the fix may not apply to the scenario fixed by #35395.
The guard above only defers on ContentSize == CGSize.Empty. The drain happens in MauiScrollView.LayoutSubviews → ProcessPendingScrollRequest() (MauiScrollView.cs:347-350), which runs immediately after ContentSize = contentSize (line 337) and before base.LayoutSubviews() (line 353). Under ContentInsetAdjustmentBehavior.Automatic, UIKit derives adjustedContentInset from whether the view is scrollable (ContentSize vs Bounds) — this repo documents exactly that at MauiScrollView.cs:384-388 and 469-482, and the class overrides AdjustedContentInsetDidChange() precisely because the update arrives as a separate callback.
Net effect: ScrollView.ScrollToAsync(...) invoked from Page.OnAppearing (the path added by #35395) can compute with adjustedInset == UIEdgeInsets.Zero and land on the pre-fix offset, with nothing re-running the request afterwards. Please confirm on device and add coverage for the OnAppearing/first-layout path — the added UI test only taps a button long after layout has settled.
There was a problem hiding this comment.
Addressed in 347ee65, with an interesting root cause: the deferred path was broken before ever reaching the inset math. OnHandlerChangedCore replayed the pending request through OnScrollToRequested, which calls CheckTaskCompletionSource() — resetting the completion source and orphaning the task the original caller was awaiting, so a pre-layout ScrollToAsync never completed at all. The replay now invokes the handler directly. Coverage added: the HostApp page issues a ScrollToAsync from the constructor (before handler/layout exist, the #35395 shape) and the UI test asserts it settles on the inset-aware maximum; the page re-evaluates on a converge loop, so a late AdjustedContentInset settle would surface as a persistent failure. Verified passing on the iOS simulator — at drain time (LayoutSubviews, after ContentSize assignment) the insets were already final, so no re-clamp machinery was added.
| // rest offset is (-adjustedInset.Left, -adjustedInset.Top), which maps to (0,0) | ||
| // cross-platform so ScrollToAsync(ScrollX, ScrollY, ...) round-trips (issue #36801). | ||
| var adjustedInset = platformView.AdjustedContentInset; | ||
| VirtualView.HorizontalOffset = platformView.ContentOffset.X + adjustedInset.Left; |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
ScrollView.ScrollX/ScrollY bindable properties and ScrolledEventArgs on iOS, but the translation is applied only inside the Scrolled callback and nothing added here is covered by a test.
- Staleness:
UIScrollView.Scrolledfires oncontentOffsetchanges. WhenAdjustedContentInsetchanges while the view is scrolled (not at rest) UIKit leavescontentOffsetalone, so no event fires and the publishedScrollX/ScrollYstay wrong by the inset delta until the user scrolls again. The new HostApp page itself performs exactly such a mutation (Issue36801.cs:77setsContentInsetfromLoaded). Consider republishing fromMauiScrollView.AdjustedContentInsetDidChange()(MauiScrollView.cs:147). - No coverage:
EvaluateOffsetin the new HostApp page asserts nativeContentOffset.Yonly — it never reads_scrollView.ScrollY. Nothing in this PR (or inScrollViewTests.iOS.cs/ScrollViewHandlerTests.iOS.cs) asserts thatScrollY == 0at rest with a non-zeroAdjustedContentInset, which is the actual contract being introduced. A Controls device test would pin this cheaply.
There was a problem hiding this comment.
Partially addressed in 347ee65. (2) is covered: the HostApp oracle now also asserts the public contract — after each scroll (top, end, and the deferred pre-layout one) ScrollView.ScrollY must equal the native offset shifted by AdjustedContentInset.Top (0 at rest) — verified passing on the iOS simulator. (1) — republishing from AdjustedContentInsetDidChange when insets change while scrolled and at rest — is consciously deferred to keep this fix minimal: it needs a platform-view→handler hook, the staleness self-corrects on the next scroll event, and it predates this PR in spirit (main published raw native offsets that were wrong by the inset at all times, not just after an inset change). Happy to follow up separately if maintainers want it.
| } | ||
|
|
||
| double expected = expectEnd | ||
| ? nativeScrollView.ContentSize.Height + adjustedInset.Bottom - nativeScrollView.Bounds.Height |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
expected here is character-for-character maxScrollVertical from ScrollViewHandler.iOS.cs:135 (ContentSize.Height + adjustedInset.Bottom - Bounds.Height), and line 106 mirrors minScrollVertical (-adjustedInset.Top). The test therefore asserts the production code against itself: it goes green for any formula the handler happens to use, including the SafeAreaEdges.Container over-scroll noted on ScrollViewHandler.iOS.cs:135.
There is already an independent, user-visible oracle in the page that is never used — ProbeLabel ("BOTTOM PROBE", line 45-50) is created with an AutomationId but no assertion in TestCases.Shared.Tests/Tests/Issues/Issue36801.cs ever queries it. Asserting App.WaitForElement("ProbeLabel") after scroll-to-end (and that it is not displayed before) would prove the content is actually reachable rather than that the arithmetic matches itself.
There was a problem hiding this comment.
Fixed in 347ee65. The oracle no longer mirrors the implementation: the end expectation is measured from the content platform view's actual frame (contentView.Frame.Bottom + adjustedInset.Bottom - Bounds.Height), the element-End expectation from the probe's frame in window coordinates, and ScrollY is asserted against the cross-platform contract. The previously-unused ProbeLabel is now exercised too: after scroll-to-end and element-End, the Appium side asserts the probe rect is fully inside the page rect — pre-fix, the undershoot left the probe entirely below the fold, so this discriminates independently of any arithmetic.
| { | ||
| _endResultLabel.Text = "EndPending"; | ||
| await _scrollView.ScrollToAsync(0, _scrollView.ContentSize.Height, animated: false); | ||
| await Task.Delay(100); |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
💡 Suggestion — Regression Prevention and Test Coverage: the fixed Task.Delay(100) turns the 5s retry in the UI test into dead weight.
WaitForTextToBePresentInElement("EndResultLabel", "Success", timeout: 5s) (HelperExtensions.cs:1052) re-polls the label, but the label is written exactly once, 100ms after the tap. If the offset has not settled at that instant the page records a permanent Fail (end): ... and the remaining ~4.9s of polling can never recover — the test fails deterministically on a slow/loaded CI device rather than retrying.
ScrollToAsync(..., animated: false) maps to Instant == true, so ScrollFinished() is already raised synchronously after SetContentOffset and the await alone guarantees the offset was applied. Either drop the delay, or re-evaluate on a timer/loop so the label converges to Success and the wait helper's retry actually does something.
There was a problem hiding this comment.
Fixed in 347ee65 — went with the converge option: the fixed Task.Delay(100) is gone, and each result label is now written by a loop that re-evaluates every 250ms (up to 5s) until the oracle reports success. The WaitForTextToBePresentInElement retry window is therefore meaningful again: a slow CI device converges to Success instead of freezing a permanently-failed first sample.
| @@ -0,0 +1,35 @@ | |||
| #if IOS // Validates iOS-specific UIScrollView AdjustedContentInset math with iOS-only instrumentation in the HostApp page | |||
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
💡 Suggestion — Platform-Specific Code Scoping: #if IOS leaves MacCatalyst — which ships the changed code — untested, and the HostApp fallback would report a false pass if the guard were widened.
ScrollViewHandler.iOS.cs compiles for both iOS and MacCatalyst, but this project defines MACCATALYST (not IOS) for the MacCatalyst head (Controls.TestCases.Shared.Tests.csproj:27-32), so the fix ships to MacCatalyst with zero coverage. Compounding it, the HostApp's #else branch returns "Success: not applicable on this platform" (TestCases.HostApp/Issues/Issue36801.cs:114), and WaitForTextToBePresentInElement does a case-insensitive Contains("Success") — so if anyone later relaxes this guard to #if IOS || MACCATALYST without also widening the HostApp guard, the test passes green while asserting nothing. Please either extend both guards to MacCatalyst, or make the non-iOS fallback return a string that cannot match "Success".
There was a problem hiding this comment.
Fixed in 347ee65. Both guards are now #if IOS || MACCATALYST (HostApp instrumentation and the UI test), so MacCatalyst — which ships this handler code — runs the tests. The false-pass trap is closed too: the non-applicable fallback now returns Skipped (...), which cannot match the case-insensitive Contains("Success").
This comment has been minimized.
This comment has been minimized.
…urface Review follow-up on dotnet#37060, keeping the change minimal relative to main: - ScrollView.GetScrollPositionForElement now computes End/Center/MakeVisible targets against the effective viewport (frame minus AdjustedContentInset on iOS/Catalyst) instead of the full frame. With the new coordinate mapping the old math landed the element adjustedInset.Top+Bottom past the visible region for downward scrolls (review error finding). - With ContentInsetAdjustmentBehavior.Always, MauiScrollView bakes the safe area into ContentSize while UIKit also applies it via AdjustedContentInset; MapRequestScrollTo now excludes the duplicated (system-contributed) amount from the reachable range, computed from public UIScrollView properties. - OnHandlerChangedCore replayed a pending scroll request through OnScrollToRequested, which resets the completion source and orphans the task the original caller is awaiting — a pre-layout ScrollToAsync never completed. The replay now invokes the handler directly. - Issue36801 HostApp page reworked per review: geometric oracles (content and probe platform-view frames, plus the ScrollX/ScrollY cross-platform contract) instead of mirroring the implementation formula, a converge loop instead of a fixed Task.Delay, MacCatalyst included in all guards, and the non-applicable fallback no longer matches "Success". New coverage: deferred pre-layout scroll, ScrollToAsync(element, End), and Appium-side probe visibility assertions. All Issue36801 UI tests verified passing on the iOS simulator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Refactor only, no behavior change: MapRequestScrollTo shrinks back to defer/compute/apply, and the coordinate translation and inset-aware clamping move into a focused static helper. The Always-mode compensation now adjusts an effective content extent once instead of threading a duplicated-padding UIEdgeInsets through the max formulas, and the already-at-target check is a single CGPoint comparison. UI tests re-verified passing on the iOS simulator after the refactor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 3 findings
See inline comments for details.
|
|
||
| // 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); |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
ScrollToRequested raise for every deferred request.
OnScrollToRequested (line ~509) raises ScrollToRequested?.Invoke(this, e) unconditionally, before it branches on Handler is null. So a request that gets parked in _pendingScrollToRequested has already notified every subscriber of IScrollViewController.ScrollToRequested. Re-raising it here means the event fires twice for one logical ScrollToAsync call.
Concrete failure mechanism: ScrollToRequested is public API (IScrollViewController) and is consumed by the compatibility ScrollViewRenderers and by third-party/legacy renderers, which perform the scroll directly from that event. For a ScrollView whose handler attaches after the request (the exact scenario this PR adds — ScrollToAsync before handler attach, or the element-mode deferral that re-enters through Dispatcher.Dispatch(SendPendingScrollToRequest)), such a subscriber will execute the scroll once on the original raise and again on the replay, producing a double scroll / duplicate animation, and any subscriber that counts requests (test harnesses included) sees 2 for 1 call.
The stated reason for bypassing OnScrollToRequested — not resetting _scrollCompletionSource — is correct and only requires the Handler.Invoke line below. Dropping this ScrollToRequested?.Invoke keeps the completion-source semantics intact while preserving one-raise-per-request.
There was a problem hiding this comment.
Partially adopted in e3f386a — dropping the replay raise entirely would break the compatibility ScrollViewRenderers: they subscribe to ScrollToRequested only when they attach (exactly when a pre-handler request replays) and perform the scroll from that event, so without the replay a deferred ScrollToAsync would never scroll under a compat renderer. The double raise for early subscribers also predates this PR (OnHandlerChangedCore used to replay through OnScrollToRequested). What this PR newly introduced is the geometry-parked path, where subscribers were already notified at request time — that replay no longer re-raises (_replayPendingScrollToRequestedEvent). Net result: exactly one raise for any request made with the handler present, and the pre-handler park keeps the raise-at-replay contract the compatibility renderers rely on. DeferredRequestReplaysEventForSubscribersAttachedWithTheHandler locks the compat contract in.
| // 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; |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
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.
There was a problem hiding this comment.
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.
|
|
||
| // Content has not been arranged through CrossPlatformArrange (e.g. ContentSize | ||
| // was mapped directly), so the content size is the only extent available | ||
| if (arranged == CGRect.Empty) |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
💡 Suggestion — CGRect.Empty conflates "never arranged" with "arranged to an empty rect at the origin".
_arrangedContentRect is assigned on every CrossPlatformArrange as new CGRect(contentOrigin, contentSize.ToCGSize()). In the branch where UIKit owns the inset (SystemAdjustedContentInset != Zero and behaviour != Never), contentOrigin is CGPoint.Empty, so content that legitimately arranges to a zero size — no Content, a collapsed/IsVisible=false container, or a template that measured to Size.Zero — records exactly CGRect.Empty and this sentinel check silently reports "not arranged".
Concrete failure mechanism: the fallback then returns the raw ContentSize, which in that same arrange pass was deliberately padded (height += _safeArea.VerticalThickness, or inflated to Bounds.Height + 1 under Automatic) — i.e. exactly the non-authoritative value the doc-comment above says must not be used. GetTargetContentOffset clamps against that inflated extent, so ScrollToAsync(0, y, ...) over empty content can park at a non-zero offset showing blank space instead of clamping to the rest position, and the alreadyAtTarget short-circuit misfires with it.
A bool _hasArrangedContent flag (set alongside _arrangedContentRect) removes the ambiguity without changing any of the intended behaviour.
There was a problem hiding this comment.
Adopted in e3f386a: _arrangedContentRect is now CGRect?, so "never arranged" is distinct from "arranged to nothing" and an empty ScrollView clamps to the rest position instead of the padded/inflated ContentSize.
kubaflo
left a comment
There was a problem hiding this comment.
Looks good but, could you please resolve conflicts?
Conflicts in MauiScrollView.CrossPlatformArrange: the landscape-notch fix reworked the arrange branches with an arrangeX that can be non-zero even when UIKit owns the insets (Automatic + vertical scroll). The arranged- rect capture now records (arrangeX, 0) in the UIKit-owned branch and the inset bounds origin in the MAUI-owned branch, so the measured scrollable extent picks up the new origin semantics automatically. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…quest Three changes from the latest review round, plus fixture updates for the merged landscape-notch rework: Element-mode requests now park behind the arranged-geometry gate even when the handler is already attached (ScrollToAsync(element, ...) from OnAppearing used to resolve against the -1 never-arranged sentinels and scroll nowhere); OnSizeAllocated/ContentSizeChanged retry them and the geometry-ready path stays synchronous. The ScrollToRequested replay raise is now limited to requests parked before the handler attached: compatibility renderers subscribe at attach and perform the scroll from the event, so that replay is load-bearing — but a request parked with the handler present (waiting for geometry) already notified its subscribers, and re-raising would double-notify. MauiScrollView._arrangedContentRect is nullable so "never arranged" is distinct from "arranged to nothing": an empty ScrollView now clamps to the rest position instead of the padded/inflated ContentSize. The Issue36801 fixture expected SafeAreaEdges.Default to resolve to Automatic; since the landscape-notch fix (dotnet#35533) Default on a vertical scroll view resolves to Never (Automatic remains only for horizontal scroll views), so the resolved-mode oracle and test parameters follow. Verified on the merged base: ScrollViewUnitTests 23/23, Issue36801 + Issue36801DeferredElement + ShellFlyoutHeaderScrollViewContent iOS UI suites 12/12 on the simulator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@kubaflo conflicts resolved — merged The three findings from the latest AI review round are also addressed in e3f386a: element-mode requests now wait for arranged geometry even when the handler is already attached, Verified locally on the merged base: |
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 6 findings
See inline comments for details.
| if (!alreadyAtTarget) | ||
| { | ||
| uiScrollView.SetContentOffset(new CGPoint(minScrollHorizontal, minScrollVertical), !request.Instant); | ||
| uiScrollView.SetContentOffset(target, !request.Instant); |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[major] Logic and Correctness — The alreadyAtTarget guard compares against a target clamped with MAUI's own ScrollableContentSize/AdjustedContentInset math, not against the range UIKit itself will accept. When the two disagree by more than one device pixel (any mode where the reconstructed _arrangedContentRect + baked.Bottom extent exceeds the live UIScrollView.ContentSize, e.g. content whose arrange result is stale relative to the last LayoutSubviews ContentSize write), alreadyAtTarget is false but SetContentOffset(target, animated: true) resolves to the same offset UIKit is already resting at. UIKit performs no animation in that case, so ScrollAnimationEnded never fires and neither branch below calls SendScrollFinished() — the caller's ScrollToAsync task stays pending forever, exactly the hazard the comment above cites but only guarded for the sub-pixel case. Suggest clamping target a second time against UIKit's live range (ContentSize/AdjustedContentInset/Bounds) before the comparison, or arming a fallback completion so the awaited task cannot wedge.
There was a problem hiding this comment.
Not adopted — the described hang cannot occur, and the suggested cap would reintroduce a bug this PR fixes. Two facts: (1) UIScrollView.SetContentOffset(target, animated: true) does not clamp programmatic offsets — whatever target we compute, UIKit animates to it and ScrollAnimationEnded fires, so a range disagreement between our extent and ContentSize cannot produce a no-op animation. The only no-op case is a zero-distance set, which is exactly what the existing device-pixel alreadyAtTarget guard short-circuits (completing synchronously). (2) Capping the target against UIKit's ContentSize-based range would resurrect the Automatic-mode bug from the previous review round: in the transient state where ContentSize omits the safe-area origin, the correct target exceeds that range — clamping to it would stop the scroll short by the origin again. The measured extent is authoritative precisely because ContentSize is not.
| _pendingScrollToRequested = e; | ||
| _replayPendingScrollToRequestedEvent = true; | ||
| } | ||
| else if (e.Mode == ScrollToMode.Element && !IsElementTargetGeometryReady()) |
There was a problem hiding this comment.
🔍 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.
There was a problem hiding this comment.
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).
| if (_pendingScrollToRequested is not null) | ||
| { | ||
| _pendingScrollToRequested = null; | ||
| SendScrollFinished(); |
There was a problem hiding this comment.
🔍 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.
There was a problem hiding this comment.
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.
| (virtualView.HorizontalOffset, virtualView.VerticalOffset) = GetContentCoordinateOffsets(platformView); | ||
| } | ||
|
|
||
| static (double HorizontalOffset, double VerticalOffset) GetContentCoordinateOffsets(UIScrollView platformView) |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[moderate] Backward Compatibility / Cross-Platform Consistency — This redefines the observable value of ScrollView.ScrollX/ScrollY and of ScrolledEventArgs on iOS/MacCatalyst: previously the raw native ContentOffset (negative at rest whenever an adjusted inset exists), now inset-relative content coordinates. Collapsing-header/parallax app code that compensated for the negative rest offset — the single most common consumer of Scrolled on iOS — will now double-compensate, and the same pattern still reports raw offsets under the compatibility renderer (see the new ShellFlyoutLayoutManager comment). The in-repo consumer is updated, but this is a user-visible break that needs a breaking-change note in the PR description/release notes so it is not discovered by apps at runtime.
There was a problem hiding this comment.
The PR description already carries this — it has a dedicated behavioral-change block measuring the consequences on both platforms (including the ScrollToAsync(0, ContentSize.Height - Height) shortfall and the inset-refresh semantics). I've now promoted it from [!NOTE] to a [!WARNING] titled Breaking behavioral change (iOS/MacCatalyst) and marked it as release-notes material so it can't be missed when the release notes are compiled.
| /// arranged rect <see cref="ScrollableContentSize"/> measures naturally excludes it (issue #36801). | ||
| /// </remarks> | ||
| internal SafeAreaPadding SafeAreaBakedIntoContent => | ||
| _appliesSafeAreaAdjustments && |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[moderate] Logic and Correctness — SafeAreaBakedIntoContent restates the arrange-branch predicate (SystemAdjustedContentInset == UIEdgeInsets.Zero || ContentInsetAdjustmentBehavior == …Never) instead of sharing it with the branch at line 633 that actually decides whether the safe area is baked in. The XML doc says it "mirrors the arrange-side branch exactly", which is precisely the fragility: any future edit to one predicate silently desynchronizes the coordinate conversion in GetTargetContentOffset/ViewportInsets, producing offsets wrong by the whole safe-area thickness with no compile-time signal. Extract the condition into a single private property (e.g. UIKitCompensatesForSafeArea) and use it from both sites.
There was a problem hiding this comment.
Adopted in 154984e: the arrange branch and SafeAreaBakedIntoContent now share a single UIKitCompensatesForSafeArea predicate, so they cannot desynchronize. The doc also spells out the one deliberate asymmetry post-#35410: the horizontal safe-area origin kept for vertical Automatic is not reported as baked, because that axis cannot scroll (extent clamped to the frame) and the arranged-rect origin already carries it for ScrollableContentSize.
| // 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() => |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[moderate] Logic and Correctness — IsElementTargetGeometryReady() 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).
There was a problem hiding this comment.
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.
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@albyrock87 — new AI review results are available based on commit
e3f386a.
🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix
Gate Result: ✅ PASSED
Platform: IOS · Base: inflight/current · Merge base: 72e93c3b
✅ Verified (new API / feature) — this PR adds new API and a test that references it in the same project, so reverting the fix un-compiles the test: there is no valid "fails without the fix" baseline to establish (a compile-coupled baseline). The gate instead verified the fix by a clean build + pass with the fix, so this is a real PASS rather than a non-committal INCONCLUSIVE.
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
🧪 ScrollViewUnitTests ScrollViewUnitTests |
🛠️ BUILD ERROR | ✅ PASS — 25s |
🖥️ Issue36801 Issue36801 |
✅ FAIL — 506s | ✅ PASS — 171s |
🖥️ Issue36801DeferredElement Issue36801DeferredElement |
✅ FAIL — 150s | ✅ PASS — 128s |
🔴 Without fix — 🧪 ScrollViewUnitTests: 🛠️ BUILD ERROR · 40s
Error-relevant lines (filtered from the build log):
/Users/cloudtest/vss/_work/1/s/src/Controls/tests/Core.UnitTests/ScrollViewUnitTests.cs(537,77): error CS0234: The type or namespace name 'IScrollViewportProvider' does not exist in the namespace 'Microsoft.Maui.Handlers' (are you missing an assembly reference?) [/Users/cloudtest/vss/_work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
🟢 With fix — 🧪 ScrollViewUnitTests: PASS ✅ · 25s
(no coded error found; showing last 1200 chars)
RequestWithWrappingContent(orientation: Horizontal) [63 ms]
Passed GetsCorrectSizeRequestWithWrappingContent(orientation: Both) [< 1 ms]
Passed TestScrollTo [< 1 ms]
Passed TestScrollToElement [1 ms]
Passed TestChildChanged [< 1 ms]
Passed InsetRefreshUpdatesOffsetsWithoutRaisingScrolled [< 1 ms]
Passed DeferredRequestReplaysEventForSubscribersAttachedWithTheHandler [< 1 ms]
Passed TestScrollToElementNotAnimated [< 1 ms]
Passed DeferredElementScrollDispatchesOnceContentIsArranged [< 1 ms]
Passed SetScrollPosition [< 1 ms]
Passed TestConstructor [1 ms]
Passed TestBackToBackBiDirectionalScroll [< 1 ms]
Passed DeferredElementScrollCompletesWhenTheHandlerGoesAway [4 ms]
Passed DeferredElementScrollCompletesWhenContentArrangesToZero [< 1 ms]
[xUnit.net 00:00:00.83] Finished: Microsoft.Maui.Controls.Core.UnitTests
Passed TestScrollWasNotFiredOnNeither [< 1 ms]
Passed DirectRequestSupersedesDeferredElementRequest [< 1 ms]
Passed TestScrollToNotAnimated [< 1 ms]
Passed TestChildDoubleSet [< 1 ms]
Passed ElementTargetsAccountForViewportAndContentCoordinateInsets [< 1 ms]
Test Run Successful.
Total tests: 23
Passed: 23
Total time: 1.1366 Seconds
🔴 Without fix — 🖥️ Issue36801: FAIL ✅ · 506s
Error-relevant lines (filtered from the build log):
at Microsoft.Maui.TestCases.Tests.Issues.Issue36801.ScrollToElementEndInEachInsetMode(String modeButton, String expectedMode) in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue36801.cs:line 120
at InvokeStub_Issue36801.ScrollToElementEndInEachInsetMode(Object, Span`1)
at Microsoft.Maui.TestCases.Tests.Issues.Issue36801.ScrollToExtremesInEachInsetMode(String modeButton, String expectedMode) in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue36801.cs:line 90
at InvokeStub_Issue36801.ScrollToExtremesInEachInsetMode(Object, Span`1)
at Microsoft.Maui.TestCases.Tests.Issues.Issue36801.ScrollToAsyncReachesInsetAwareExtremes() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue36801.cs:line 24
at Microsoft.Maui.TestCases.Tests.Issues.Issue36801.ScrollToElementEndLandsInsideVisibleViewport() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue36801.cs:line 61
at Microsoft.Maui.TestCases.Tests.Issues.Issue36801DeferredElement.DeferredElementScrollLandsInsideVisibleViewport() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue36801DeferredElement.cs:line 24
🟢 With fix — 🖥️ Issue36801: PASS ✅ · 171s
(no coded error found; showing last 1200 chars)
chInsetMode Start
>>>>> 8/9/2026 3:14:56 PM ScrollToExtremesInEachInsetMode Stop
Passed ScrollToExtremesInEachInsetMode("ModeContainerButton","Always") [3 s]
>>>>> 8/9/2026 3:14:56 PM ScrollToAsyncReachesInsetAwareExtremes Start
>>>>> 8/9/2026 3:15:00 PM ScrollToAsyncReachesInsetAwareExtremes Stop
Passed ScrollToAsyncReachesInsetAwareExtremes [3 s]
>>>>> 8/9/2026 3:15:00 PM ScrollToElementEndLandsInsideVisibleViewport Start
>>>>> 8/9/2026 3:15:02 PM ScrollToElementEndLandsInsideVisibleViewport Stop
Passed ScrollToElementEndLandsInsideVisibleViewport [1 s]
>>>>> 8/9/2026 3:15:07 PM FixtureSetup for Issue36801DeferredElement(iOS)
>>>>> 8/9/2026 3:15:11 PM DeferredElementScrollLandsInsideVisibleViewport Start
>>>>> 8/9/2026 3:15:11 PM DeferredElementScrollLandsInsideVisibleViewport Stop
Passed DeferredElementScrollLandsInsideVisibleViewport [479 ms]
NUnit Adapter 4.5.0.0: Test execution complete
Results File: /Users/cloudtest/vss/_work/1/s/CustomAgentLogsTmp/UITests/TestResults/Issue36801.trx
Test Run Successful.
Total tests: 11
Passed: 11
Total time: 58.7392 Seconds
>>> TRX_RESULT_FILE: /Users/cloudtest/vss/_work/1/s/CustomAgentLogsTmp/UITests/TestResults/Issue36801.trx
🔴 Without fix — 🖥️ Issue36801DeferredElement: FAIL ✅ · 150s
Error-relevant lines (filtered from the build log):
at Microsoft.Maui.TestCases.Tests.Issues.Issue36801DeferredElement.DeferredElementScrollLandsInsideVisibleViewport() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue36801DeferredElement.cs:line 24
🟢 With fix — 🖥️ Issue36801DeferredElement: PASS ✅ · 128s
(no coded error found; showing last 1200 chars)
specified pattern.
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.06] Discovering: Controls.TestCases.iOS.Tests
[xUnit.net 00:00:00.20] Discovered: Controls.TestCases.iOS.Tests
NUnit Adapter 4.5.0.0: Test execution started
Running selected tests in /Users/cloudtest/vss/_work/1/s/artifacts/bin/Controls.TestCases.iOS.Tests/Debug/net10.0/Controls.TestCases.iOS.Tests.dll
NUnit3TestExecutor discovered 1 of 1 NUnit test cases using Current Discovery mode, Non-Explicit run
>>>>> 8/9/2026 3:17:15 PM FixtureSetup for Issue36801DeferredElement(iOS)
>>>>> 8/9/2026 3:17:19 PM DeferredElementScrollLandsInsideVisibleViewport Start
>>>>> 8/9/2026 3:17:20 PM DeferredElementScrollLandsInsideVisibleViewport Stop
Passed DeferredElementScrollLandsInsideVisibleViewport [543 ms]
NUnit Adapter 4.5.0.0: Test execution complete
Results File: /Users/cloudtest/vss/_work/1/s/CustomAgentLogsTmp/UITests/TestResults/Issue36801DeferredElement.trx
Test Run Successful.
Total tests: 1
Passed: 1
Total time: 20.3771 Seconds
>>> TRX_RESULT_FILE: /Users/cloudtest/vss/_work/1/s/CustomAgentLogsTmp/UITests/TestResults/Issue36801DeferredElement.trx
⚠️ Failure Details
- 🛠️ ScrollViewUnitTests without fix: build failed before tests could run
/Users/cloudtest/vss/_work/1/s/src/Controls/tests/Core.UnitTests/ScrollViewUnitTests.cs(537,77): error CS0234: The type or namespace name 'IScrollViewportProvider' does not exist in the namespace 'Mic...
📁 Fix files reverted (5 files)
src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellFlyoutLayoutManager.cssrc/Controls/src/Core/ScrollView/ScrollView.cssrc/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cssrc/Core/src/Platform/iOS/CoreGraphicsExtensions.cssrc/Core/src/Platform/iOS/MauiScrollView.cs
New files (not reverted):
src/Core/src/Core/IScrollOffsetReceiver.cssrc/Core/src/Handlers/ScrollView/IScrollViewportProvider.cs
📋 Pre-Flight — Context & Validation
PR #37060 Pre-Flight
Context
- Title:
[iOS] ScrollView: Clamp ScrollTo requests against AdjustedContentInset - Issue: Fixes #36801.
- Base / head:
inflight/currentat72e93c3b71f3b4dfc959e33f791c0ba034069eaa/ PR heade3f386a81145ac1ee6eb23be7b0980269c860172. - Review worktree: merge commit
1d05db659a7627c7af6a0c367f6bba0e10739164. - Platform: iOS.
- Gate: PASSED previously. Do not rerun the fail-without-fix gate and do not modify
gate/content.md.
Problem
The existing iOS ScrollView path treats native ContentOffset as the MAUI offset and clamps programmatic requests as though the full frame were visible. With non-zero AdjustedContentInset, the valid native range begins at the negative leading inset and ends after accounting for both trailing inset and the effective viewport. Consequently, top/end requests and element-aligned requests land incorrectly, reported offsets do not round-trip in content coordinates, inset-only changes can leave reported offsets stale, and element requests made before usable arranged geometry can resolve too early.
Existing PR Approach
The inspected 14-file diff:
- Defines MAUI offsets as content coordinates and translates to/from iOS native offset coordinates in
ScrollViewHandler.iOS.cs. - Adds internal
IScrollViewportProviderandIScrollOffsetReceivercontracts for effective viewport data and silent inset-derived offset refreshes. - Derives a scrollable content extent in
MauiScrollView.csfrom arranged content geometry rather than trusting UIKitContentSize. - Changes
ScrollView.csrequest ownership so direct requests supersede pending requests, deferred element requests wait for arranged geometry, completion sources remain attached to the original caller, and element alignment uses effective viewport/content-coordinate insets. - Reads the native offset directly in
ShellFlyoutLayoutManager.csbecause its header-collapse math is in UIKit coordinates. - Adds focused Core unit tests and iOS UI fixtures for inset modes, top/end and element targets, deferred requests, silent inset refresh, and Shell flyout behavior.
Production files changed:
src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellFlyoutLayoutManager.cssrc/Controls/src/Core/ScrollView/ScrollView.cssrc/Core/src/Core/IScrollOffsetReceiver.cssrc/Core/src/Handlers/ScrollView/IScrollViewportProvider.cssrc/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cssrc/Core/src/Platform/iOS/CoreGraphicsExtensions.cssrc/Core/src/Platform/iOS/MauiScrollView.cs
Bounded Test Surface
Run only these gate-detected tests; never run a full suite:
dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName~ScrollViewUnitTests"pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform ios -TestFilter "Issue36801"pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform ios -TestFilter "Issue36801DeferredElement"
The prior gate recorded 23/23 Core tests, 11/11 Issue36801 UI tests, and 1/1 deferred-element UI test passing with the PR fix. The gate also proved the UI tests fail on the pre-fix production tree.
Attempt Constraints
- Each candidate must use a different root-cause strategy from the PR and from earlier candidates.
- One implementation/test pass is allowed, followed by at most one focused correction and retest of the failing test(s).
- Preserve all unrelated pre-existing worktree changes. Candidate diffs and reports must contain only the attempted fix.
- Follow the iOS threading, safe-area, handler, public-API, and UI-test repository instructions applicable to touched files.
- Perform the
try-fixskill's inline expert self-review; do not launch a reviewer agent.
🔬 Code Review — Deep Analysis
Expert Code Review — PR #37060
Title: [iOS] ScrollView: Clamp ScrollTo requests against AdjustedContentInset
Issue: Fixes #36801
Base / head: inflight/current @ 72e93c3b71 → PR head e3f386a811 (reviewed in merge worktree 1d05db659a)
Scope reviewed: raw submitted PR HEAD only — 14 files, +1392 / −32. No candidate patches considered.
Gate (supplied, not rerun): PASSED — tests fail without fix, pass with fix.
Regression cross-reference: regression-check/risks.json → CLEAN (0 overlaps, 0 REVERTs).
1. Independent Assessment (formed before reading any narrative)
What the change actually does
The PR redefines the iOS/MacCatalyst ScrollView offset coordinate system and rebuilds the programmatic-scroll pipeline on top of it. Five mechanically distinct changes:
- Coordinate-system redefinition (
ScrollViewHandler.iOS.cs). Cross-platform offsets become content coordinates: reporting addsAdjustedContentInset.Left/ToptoContentOffset(GetContentCoordinateOffsets, line 366); requests subtract it (GetTargetContentOffset, line 194).(0,0)cross-platform now means "inset rest position", soScrollToAsync(ScrollX, ScrollY)round-trips. - Inset-aware clamping. The old clamp
[0, ContentSize − Frame]is replaced by[−adjustedInset.Leading, contentExtent + adjustedInset.Trailing − Bounds], wherecontentExtentcomes from a newMauiScrollView.ScrollableContentSize. - Authoritative extent (
MauiScrollView.cs)._arrangedContentRectrecords whatCrossPlatformArrangeactually placed;ScrollableContentSizederives the clamp extent from it plusSafeAreaBakedIntoContent, deliberately not fromUIScrollView.ContentSize(which is padded/inflated by three different arrange branches). - Two new internal contracts.
IScrollViewportProvider(handler → Controls:ViewportInsets,ContentCoordinateInsets,NotifyInsetsChanged) andIScrollOffsetReceiver(handler → Controls: updateScrollX/ScrollYwithout raisingScrolled). Bothinternal, both consumed viaas-pattern so non-implementing handlers degrade to the old behavior. - Request-ownership rework (
ScrollView.cs). Element-mode requests are parked until arranged geometry exists; a later direct request supersedes a parked one; replay avoids re-enteringOnScrollToRequestedso the original caller'sTaskCompletionSourceis not orphaned;ScrollToRequestedis re-raised only for requests parked before handler attach (compat-renderer subscribers).
Design quality
The architecture is sound and correctly layered. Specifically:
- The inset convention is owned by the handler and published through an interface rather than re-derived in Controls — this is the right side of the Core/Controls boundary (Dimension 4).
GetVisibleViewportInsets()/GetContentCoordinateInsets()inScrollView.cscontain no iOS knowledge. - Both new interfaces are
internal, so there is no public API surface change — noPublicAPI.Unshipped.txtentry needed, no interface-addition breaking change (Dimension 7 clean).ScrollViewHandler's generic parameters are unchanged (ViewHandler<IScrollView, UIScrollView>), avoiding the known binary-break trap. - The
IScrollOffsetReceiversplit is the correct fix for the real root cause of the stale-offset half of #36801:IScrollView.HorizontalOffset/VerticalOffsetsetters route throughSetScrolledPosition, which raisesScrolled. An inset-only change must not manufacture a scroll notification. Adding a separate receiver rather than adding a flag to the existing setter keeps the "setter means scrolled" invariant intact. - I independently verified the coordinate math is internally consistent across the three arrange modes. In the "baked" mode the content is arranged at origin
(safeArea.Left, safeArea.Top), so element positions carry the padding;GetScrollPositionForElementsubtractscontentInsetsat return (line 172) and adds it intoscrollBounds(line 156), whileGetContentCoordinateOffsetsadds onlyAdjustedContentInset(which excludes the baked part by construction). Worked example — verticalScrollView, safe-area baked, element at contenty = safeArea.Top: target= y − baked.Top = 0; native= 0 − 0 = 0; reportedScrollY = 0. Round-trips. TheMakeVisibleearly-outreturn new Point(ScrollX, ScrollY)(line 159) is in the same space as the computed branches, so it does not leak a mixed coordinate. IsCloseTowith1 / Screen.Scaletolerance is the right instrument and matches the repo's existingEqualsAtPixelLevelconvention (Dimension 1/15). The stated motivation — an animatedSetContentOffsetfor a sub-pixel delta may never raiseScrollAnimationEnded, wedging the awaited task — is a genuine iOS behavior and was previously an unguarded hang.NotifyInsetsChanged()is raised fromAdjustedContentInsetDidChangeafterValidateSafeArea()has refreshed_safeArea(line 158), soSafeAreaBakedIntoContentis not read stale. I checked this explicitly because the ordering is the obvious place to get it wrong; the PR gets it right.ShellFlyoutLayoutManageris correctly identified as the one in-repo consumer whose math is in native offsets and is switched to readScrollView.ContentOffset.Ydirectly. Theif (ScrollView is { } nativeScrollView)guard with the "0 is not neutral, it means scrolled past the header" reasoning is correct — collapsing on a null native view would have been a visible bug.
Cross-cutting checks that came back clean
| Dimension | Result |
|---|---|
| 7 Public API Surface | Clean — both interfaces internal, no PublicAPI.*.txt churn, no handler generic-parameter change |
| 8 Async/Threading | Clean — Dispatcher.Dispatch is main-thread; no new fire-and-forget; SendPendingScrollToRequest is idempotent by design so a dropped post cannot wedge |
| 11 Memory Leak | Clean — no new subscriptions in ConnectHandler; the flyout closure captures this/ScrollView exactly as before, and removeScrolledEvent unsubscription is unchanged |
| 13 Platform Scoping | Clean — .iOS.cs / Platform/iOS/ only; IScrollViewportProvider lives in src/Core so ScrollView.cs can reference it without a platform dependency |
| 18 Trimming/AOT | Neutral — no new reflection (the pre-existing GetProperty in GetCoordinate is untouched) |
| 2 Performance | Clean — no LINQ/allocation added to the scroll path; GetContentCoordinateOffsets returns a tuple and reads AdjustedContentInset once |
| 6 Regression cross-ref | CLEAN — no lines removed that a prior labeled bug-fix PR added |
Test evidence in the PR
7 new Core unit tests (ScrollViewUnitTests.cs, +249) plus 3 UI test fixtures (Issue36801, Issue36801DeferredElement, ShellFlyoutHeaderScrollViewContent, +710 host-app/+182 test). Coverage is genuinely scenario-driven, not generic: handler-goes-away completion, dispatch-once-arranged, direct-supersedes-deferred, zero-size-content completion, viewport/content-inset element targets with explicit expected values (440/310/375/310), event-replay-exactly-once (eventCount == 1), and inset refresh not raising Scrolled (scrolledCount == 0 then == 1). The negative cases are present for the two hang paths the author anticipated. Gate (supplied) recorded 23/23 Core, 11/11 Issue36801, 1/1 deferred UI.
2. Actionable Findings
Six findings recorded to inline-findings.json. Severity and exact evidence below.
F1 — [major] Animated scroll can still wedge the awaited task
src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs:180
alreadyAtTarget (line 176) compares ContentOffset against a target clamped by MAUI's reconstructed range (ScrollableContentSize + AdjustedContentInset + Bounds, lines 194–212). UIKit clamps SetContentOffset(_:animated:) against its own ContentSize. When the two ranges disagree by more than one device pixel — which is exactly the situation ScrollableContentSize's own XML doc says exists, since ContentSize is "not authoritative" and is padded/inflated/origin-shifted per mode — the guard reports "not at target", the animated call resolves to the offset UIKit is already at, no animation runs, ScrollAnimationEnded never fires, and neither branch at line 183 calls SendScrollFinished(). The caller's ScrollToAsync task stays pending forever.
This is the same failure mode the comment at lines 170–174 was written to prevent; the guard just does not cover the case where the disagreement is larger than a pixel. Suggested fix: clamp target a second time against UIKit's live range immediately before the comparison, or arm a fallback completion.
F2 — [major] Element-mode request can be parked forever on a never-arranged ScrollView
src/Controls/src/Core/ScrollView/ScrollView.cs:530
OnScrollToRequested now parks element-mode requests when !IsElementTargetGeometryReady(). Retries come only from OnSizeAllocated (line 661) and ContentSizeChanged (line 334). A ScrollView with IsVisible="False" is skipped by LayoutManager in both measure and arrange, so Width/Height stay at the -1 never-arranged sentinel and neither callback ever fires — _pendingScrollToRequested is never drained and the caller's task never completes.
Before this PR the request was handed straight to the handler, which clamped the garbage target to 0 and called SendScrollFinished(); the task always completed. The author reasoned carefully about two hang paths (handler-detach at line 55, zero-size content in the IsElementTargetGeometryReady comment) and wrote a test for each; this third path is the one that was missed, and it has no test. Per Dimension 6, a new gate needs a test for the input that must not trip it.
F3 — [moderate] Detach now discards a queued request instead of replaying it
src/Controls/src/Core/ScrollView/ScrollView.cs:58
OnHandlerChangedCore runs with Handler is null on transient detach — Shell tab switch, handler recreation, page re-parenting — not only on terminal teardown. The new drain clears _pendingScrollToRequested and calls SendScrollFinished(), so a request issued while detached (or still queued when a detach happens) is silently dropped and reported to the caller as a completed scroll that never occurred. This directly contradicts the repo's Shell guidance (Dimension 3: "the view might be removed and re-added — do not null state eagerly in disconnect"). The intent (don't leave the task pending) is right; the trigger is too broad.
F4 — [moderate] Undocumented observable break in iOS ScrollX/ScrollY/Scrolled
src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs:366
ScrollView.ScrollX/ScrollY and ScrolledEventArgs on iOS/MacCatalyst change from raw native ContentOffset (negative at rest whenever an adjusted inset exists) to inset-relative content coordinates. Collapsing-header/parallax app code that compensated for the negative rest offset — the single most common consumer of Scrolled on iOS — will now double-compensate. The PR's own ShellFlyoutLayoutManager comment concedes the value "depends on which renderer is in play", so the compatibility renderer still reports raw offsets: the same app code is now correct under one renderer and wrong under the other.
The change is defensible (it is what makes the round-trip work and what the issue asks for), and the one in-repo consumer is updated. What is missing is the breaking-change note in the PR description / release notes, per Dimension 12.
F5 — [moderate] Duplicated safe-area predicate creates silent drift risk
src/Core/src/Platform/iOS/MauiScrollView.cs:530
SafeAreaBakedIntoContent restates the arrange-branch predicate at line 633 rather than sharing it. Its own XML doc says it "mirrors the arrange-side branch exactly" — which is precisely the fragility. Any future edit to one predicate desynchronizes ViewportInsets, ContentCoordinateInsets, and GetTargetContentOffset from what arrange actually did, producing offsets wrong by the entire safe-area thickness with no compile-time signal and no unit-test coverage (the baked/non-baked split is only exercised through iOS UI tests). Extract a single private UIKitCompensatesForSafeArea property used by both sites.
F6 — [moderate] Readiness gate does not cover the target element
src/Controls/src/Core/ScrollView/ScrollView.cs:101
IsElementTargetGeometryReady() checks this and the immediate Content, but GetScrollPositionForElement resolves against the target element's geometry: item.Width/item.Height for Center/End (lines 165–172) and every ancestor's X/Y via GetCoordinate. A target inside a container arranged later, or added after the content's first arrange (virtualized/deferred content, IsVisible toggled on), passes the gate with its own Width/Height still -1, producing a target off by viewport ± 1. The one-tick Dispatcher.Dispatch hides this in the common case, which makes it an intermittent rather than deterministic wrong-offset.
3. Failure-Mode Probing
Hypotheses tested against the code; recording the ones that came back negative so the verdict is not read as unexamined.
| Probe | Result |
|---|---|
Does NotifyInsetsChanged fire before _safeArea is refreshed, publishing a new-inset/old-safe-area frame? |
No. It is placed after the ValidateSafeArea() block (line 158). |
Does the MakeVisible early-out return a coordinate in a different space than the computed branches? |
No. ScrollX/ScrollY are content coordinates that already exclude the baked part, matching x − contentInsets.Left. |
Does IScrollOffsetReceiver.UpdateScrollOffsets bypass binding notification by writing the field directly? |
No. It writes the ScrollX/ScrollY properties, so the read-only BindableProperty still notifies; only Scrolled is suppressed, which is the point. |
Can the re-raised ScrollToRequested double-notify compat-renderer subscribers? |
No. _replayPendingScrollToRequestedEvent is set true only on the handler-null path (line 528) and false on the geometry-wait path (line 537); the test asserts eventCount == 1 for both. |
| Can a superseding direct request be overwritten by an already-scheduled deferred dispatch? | No. The else branch clears _pendingScrollToRequested (line 544) and SendPendingScrollToRequest no-ops on null. Covered by DirectRequestSupersedesDeferredElementRequest. |
Does Dispatcher.Dispatch on every retry queue unbounded work? |
Bounded in effect — the callback is a no-op once the request is sent or superseded; the author's rationale (a dropped callback cannot wedge the request the way an "already queued" flag would) is correct. |
Does _arrangedContentRect go stale when content is removed? |
Self-healing — CrossPlatformArrange runs every layout pass and rewrites it; the nullable is only for the never-arranged / direct-ContentSize-mapped case, which correctly falls back to ContentSize. |
Does the flyout change break when ScrollView (native) is assigned after subscription? |
No. The closure reads the field at invocation time, and assignment happens in SetContentView before scrolling is possible. |
Any other in-repo consumer of Controls ScrollView.ScrollY needing the same treatment as the flyout? |
One adjacent — SwipeView.cs:396 uses e.ScrollY deltas only, so a constant coordinate shift cancels out. No change needed. |
4. Blast Radius
Platforms: iOS + MacCatalyst behavior changes. Android/Windows are untouched at the platform layer, but ScrollView.cs (shared) changes GetScrollPositionForElement and the whole request-dispatch lifecycle for all platforms — the inset math degrades to default Thickness there (no handler implements IScrollViewportProvider), so the element-target math is arithmetically unchanged. The timing change (F2/F3/F6) is cross-platform and is the widest-reaching part of this PR.
Controls affected: ScrollView directly. Indirectly, anything hosting a ScrollView — Shell flyout header (explicitly handled), SwipeView (delta-only, safe), and any app Scrolled subscriber on iOS (F4).
Frequently-regressed component overlap: iOS safe area + ScrollView ContentSize is a known oscillation-prone area (MauiScrollView layout loops, #33595). This PR reads the safe-area state rather than mutating it and adds no new ContentSize write, so it does not add a new oscillation source. ScrollableContentSize correctly mirrors the orientation clamp LayoutSubviews applies, which was the trap here.
Risk concentration: the highest-risk surface is not the iOS math (which is consistent and UI-tested across inset modes) but the shared request-lifecycle state machine — three interacting flags/fields (_pendingScrollToRequested, _replayPendingScrollToRequestedEvent, _scrollCompletionSource) across five entry points. F2, F3, and F6 all live there.
5. Test / CI Evidence
- Gate (supplied, not rerun): PASSED — 23/23
ScrollViewUnitTests, 11/11Issue36801iOS UI, 1/1Issue36801DeferredElement; UI tests proven to fail on the pre-fix tree. - Regression cross-reference:
CLEAN— no reverted prior fixes. - Coverage gaps identified: never-arranged
ScrollView(F2), transient-detach replay (F3), baked-vs-non-baked predicate divergence (F5, no unit-level coverage), late-arranged target element (F6). - UI tests are iOS-only. That is defensible here — the platform math is iOS-specific — but the shared lifecycle changes in
ScrollView.cs(deferral, supersede, drain-on-detach) apply to Android and Windows and are covered only by platform-agnostic Core unit tests. That is acceptable coverage for those paths.
6. Verdict
NEEDS_CHANGES — Confidence: high on the design being right and on F1/F2 being real; medium on F6's practical frequency.
This is high-quality, well-reasoned work. The coordinate-system redefinition is the correct root-cause fix for #36801 rather than a symptom patch, the layering is right, the new contracts are internal (no API risk), and the commentary explains why at every non-obvious decision. The regression cross-reference is clean and the test suite is scenario-specific rather than generic.
It does not merge as-is because of two [major] hang paths in the awaited-task lifecycle:
- F1 — the
alreadyAtTargetguard protects against a sub-pixel wedge but not against MAUI's clamp range disagreeing with UIKit's, which is a condition the PR's own documentation says exists. - F2 — a never-arranged
ScrollViewparks an element request forever, a terminal-completion regression from the previous behavior, with no test for that negative case.
Both are narrow to fix (a second clamp against UIKit's live range; a terminal path plus a test for the never-arranged case). F4 needs only a breaking-change note, not a code change. F3, F5, and F6 are worth addressing but would not block on their own.
🛠️ Try-Fix — Analysis & Comparison
Alternative Fix Candidates for PR #37060
Candidate 1 — Live Placed-Content Geometry
Model: claude-opus-5
Result: PASS — one implementation/test pass; no correction round.
Approach: Treat the clamp's source of extent as the root cause. Instead of recording an arranged-content rectangle and reconstructing a shadow ScrollableContentSize, read the live placed content subview frame from GetContentView().Frame, capped by UIKit ContentSize. Extract the arrange branch condition into UIKitCompensatesForSafeArea so arrange and coordinate conversion share one decision, and refresh inset-derived offsets from UIKit's AdjustedContentInsetDidChange.
Difference from the PR: The PR persists _arrangedContentRect, reconstructs a separate scrollable extent, and listens through SafeAreaInsetsDidChange. This candidate records no arrange geometry, asks the placed content view where its trailing edge is, centralizes the safe-area compensation predicate, and observes the native adjusted-inset callback. The test-pinned content-coordinate and deferred-request contracts remain necessarily equivalent.
Files changed:
src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cssrc/Controls/src/Core/ScrollView/ScrollView.cssrc/Core/src/Handlers/ScrollView/IScrollViewportProvider.cssrc/Core/src/Platform/iOS/MauiScrollView.cssrc/Core/src/Core/IScrollOffsetReceiver.cssrc/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellFlyoutLayoutManager.cs
Tests:
| Command | Result |
|---|---|
dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName~ScrollViewUnitTests" |
PASS — 23/23 |
pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform ios -TestFilter "Issue36801" |
PASS — 11/11 |
pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform ios -TestFilter "Issue36801DeferredElement" |
PASS — 1/1 |
Self-review: 2 moderate findings, 0 critical/major. Both are documented in the attempt artifacts.
Baseline/restore note: EstablishBrokenBaseline.ps1 refused to establish the baseline because 55 unrelated pre-existing worktree changes were present. The candidate preserved them and used a target-file-scoped non-destructive baseline fallback, then ran EstablishBrokenBaseline.ps1 -Restore. The target source tree was restored and the same 55 pre-existing changes remained. This is a deviation from the skill's mandated baseline mechanism, although the candidate tests themselves completed successfully.
Artifacts:
- Narrative and candidate-only diff:
../try-fix-1/content.md - Standard attempt artifacts:
attempt-1/ - Full diff:
attempt-1/fix.diff - Test output:
attempt-1/test-output.log - Self-review:
attempt-1/reviewer-findings.json
Candidate 2
Model: gpt-5.6-sol
Result: BLOCKED — no implementation or tests.
Approach selected: Treat MAUI's manual terminal-range calculation as the root cause. Translate MAUI content-coordinate requests into viewport-sized target rectangles and delegate resolution/clamping to UIKit's UIScrollView.ScrollRectToVisible, leaving UIKit to use its authoritative ContentSize, AdjustedContentInset, and effective viewport. Offset reporting would still translate coordinates, but MAUI would neither cache nor calculate a scrollable extent.
Difference from the PR: The PR persists _arrangedContentRect and reconstructs ScrollableContentSize; this strategy would create no shadow extent and delegate clamping to UIKit.
Difference from candidate 1: Candidate 1 reads the live content subview frame capped by ContentSize; this strategy would read no content frame and perform no MAUI-side extent calculation.
Failure analysis: The mandatory pwsh .github/scripts/EstablishBrokenBaseline.ps1 command exited 1 before any code mutation because the worktree contains 55 unrelated pre-existing dirty entries. Per the execution constraints, candidate 2 did not use a manual fallback. Its candidate-only diff is empty, and none of the three bounded tests ran. EstablishBrokenBaseline.ps1 -Restore exited 0 with no baseline state to restore; all seven target production paths remained clean and the unrelated changes were preserved.
Files changed: None.
Self-review: 0 findings against the empty diff.
Artifacts:
- Narrative:
../try-fix-2/content.md - Standard attempt artifacts:
attempt-2/ - Baseline failure log:
attempt-2/baseline.log - Empty diff:
attempt-2/fix.diff - Self-review:
attempt-2/reviewer-findings.json
Aggregate Result
One validated alternative and one unvalidated native-primitive design were produced within the two-candidate bound:
| Candidate | Strategy | Result |
|---|---|---|
| 1 | Read live placed-content geometry; centralize UIKit safe-area compensation; observe adjusted-inset changes | PASS — 23 Core + 11 primary UI + 1 deferred UI |
| 2 | Delegate target clamping to UIScrollView.ScrollRectToVisible |
BLOCKED before implementation |
Candidate 1 is the only empirically validated alternative. Its baseline establishment required a target-file-scoped fallback because the mandated script rejected the dirty workspace; candidate 2 deliberately did not repeat that deviation.
🏁 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Winner: pr-plus-reviewer
The submitted PR has the right root-cause design and passed the trusted Gate, but the expert review identified two concrete awaited-task hang paths. The single consolidated reviewer patch closes those blockers while preserving the PR's inset-aware coordinate model, and all three required targeted validations pass. Because the winning changes are not in the submitted PR HEAD, the recommendation is request changes.
Comparative Ranking
| Rank | Candidate | Evidence | Assessment |
|---|---|---|---|
| 1 | pr-plus-reviewer |
PASS — 24/24 Core, 11/11 Issue36801, 1/1 deferred-element UI |
Best balance of correctness and evidence. It retains the PR architecture, caps targets to UIKit's live scroll range, prevents invisible-tree requests from waiting forever, centralizes safe-area ownership, and adds focused regression coverage. |
| 2 | try-fix-1 |
PASS — 23/23 Core, 11/11 Issue36801, 1/1 deferred-element UI |
Validated alternative that caps live placed-content geometry by ContentSize and centralizes safe-area ownership. It still reproduces the shared never-arranged element-request lifecycle that the expert review identified, and its self-review records two moderate risks. Its baseline procedure also required a nonstandard fallback. |
| 3 | pr |
Trusted Gate PASS — 23/23 Core, 11/11 Issue36801, 1/1 deferred-element UI |
Correct overall coordinate-system design, internal contracts, Shell conversion, and strong scenario coverage. Ranked below passing refined candidates because expert review found two major completion hazards: a target outside UIKit's live range can suppress the animation callback, and an invisible never-arranged element request can remain parked forever. |
| 4 | try-fix-2 |
BLOCKED — no implementation or tests | The ScrollRectToVisible strategy is only a design proposal. With no candidate diff or regression evidence, it cannot outrank implemented passing candidates. |
No candidate failed a regression test. try-fix-2 is ranked last because validation was blocked, not treated as a pass.
Why pr-plus-reviewer Wins
- UIKit-range completion safety: the candidate clamps the PR's reconstructed extent by live
ContentSize, soSetContentOffsetis not asked to animate to an offset UIKit will silently collapse to its current position. - Never-arranged completion safety: requests targeting an invisible visual tree now complete without being parked for layout callbacks that cannot occur. The new focused Core test raises the bounded count from 23 to 24.
- Single safe-area predicate:
SafeAreaBakedIntoContentandCrossPlatformArrangenow shareUIKitCompensatesForSafeArea, removing the drift identified by expert review. - Regression evidence: the full bounded surface passed once in the mandated sandbox, and all recorded output paths identify
/Users/cloudtest/vss/_work/_temp/pr-37060-pr-plus-reviewer.
Remaining Uncertainty
The refinement deliberately stops after one patch and one validation pass. The expert review's moderate concerns about transient handler detach dropping a queued request and target descendants whose geometry becomes ready later remain unresolved. The submitted description already calls out the iOS/MacCatalyst offset-coordinate behavioral change, so no metadata rewrite is needed for that point.
Required Change
Apply pr-plus-reviewer/reviewer.patch to the submitted PR. The complete resulting source/test candidate is preserved in pr-plus-reviewer/candidate.patch.
📱 UI Tests — ScrollView,Shell,ViewBaseTests
Detected UI test categories: ScrollView,Shell,ViewBaseTests
❌ Deep UI tests — 605 passed, 8 failed, 30 skipped across 3 categories on platform-pool agent (replaces in-process counts above).
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
ScrollView |
173/175 (1 ❌, 1 skipped) | — |
Shell |
320/356 (7 ❌, 29 skipped) | 5 diff PNGs |
ViewBaseTests |
112/112 ✓ | — |
🔍 AI analysis of failures — PR-related vs unrelated
🔍 AI-generated triage (GitHub Copilot CLI) — a heuristic judgement of whether each deep UI test failure is connected to this PR's changes. Verify before relying on it.
Likely PR-related: one or more failures appear connected to this PR's changes.
- ✗ PR-related — ScrollView behavior (~1 test):
ScrollViewInitiallyNotEnabledThenEnabledexercises the shared ScrollView implementation and fails functionally on iOS, where this PR substantially changes ScrollView lifecycle, offset, and handler behavior. - ● Unrelated — Shell SearchHandler tests (~6 tests): four failures are snapshot-baseline mismatches and two are generic Appium element timeouts, while the PR changes neither SearchHandler nor its visual baselines.
- ● Unrelated — Transparent Shell navigation bar (~1 test): the small snapshot-only difference concerns navigation-bar rendering, not the modified Shell flyout ScrollView layout path.
Strongest signal: the sole functional failure directly overlaps the PR's ScrollView changes; the remaining failures have baseline or timeout signatures outside the changed behavior.
📸 Snapshot differences — baseline vs actual vs diff (all 5)
For each failing
VerifyScreenshotsnapshot: the committed baseline, the actual render on this CI agent, and the computed diff. Ordered by likely PR-relevance — snapshots whose baseline/test file this PR changed are shown first. A large, uniform diff across many snapshots is usually a cross-machine baseline/environment mismatch (e.g. the macOS TitleBar / window chrome), not a code regression — compare against baseline history before concluding.
❌ ScrollView — 1 failed test
ScrollViewInitiallyNotEnabledThenEnabled
Assert.That("Success", Is.EqualTo(success))
Expected string length 12 but was 7. Strings differ at index 0.
Expected: "Initial Text"
But was: "Success"
-----------^
at Microsoft.Maui.TestCases.Tests.Issues.ScrollViewIsEnabledTests.ScrollViewInitiallyNotEnabledThenEnabled() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/ScrollViewIsEnabled.cs:line 113
1) at Microsoft.Maui.TestCases.Tests.Issues.ScrollViewIsEnabledTests.ScrollViewInitiallyNotEnabledThenEnabled() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/ScrollViewIsEnabled.cs:line 113
❌ Shell — 7 failed tests
TransparentShellNavBarShouldRemainTransparentAfterKeyboardDismiss
VisualTestUtils.VisualTestFailedException :
Snapshot different than baseline: TransparentShellNavBarShouldRemainTransparentAfterKeyboardDismiss.png (0.67% difference)
If the correct baseline has changed (this isn't a a bug), then update the baseline image.
See test attachment or download the build artifacts to get the new snapshot file.
More info: https://aka.ms/visual-test-workflow
at VisualTestUtils.VisualRegressionTester.Fail(String message) in /_/src/TestUtils/src/VisualTestUtils/VisualRegressionTester.cs:line 162
at VisualTestUtils.VisualRegressionTester.VerifyMatchesSnapshot(String name, ImageSnapshot actualImage, String environmentName, ITestContext testContext) in /_/src/TestUtils/src/VisualTestUtils/VisualRegressionTester.cs:line 123
at Microsoft.Maui.TestCases.Tests.UITest.<VerifyScreenshot>g__Verify|13_0(String name, <>c__DisplayClass13_0&) in /_/src/Controls/tests/TestCases.Shared.Tests/UITest.cs:line 477
at Microsoft.Maui.TestCases.Tests.UITest.VerifyScreenshot(
...
VerifyShellSearch_CancelButtonColor
System.TimeoutException : Timed out waiting for element...
at UITest.Appium.HelperExtensions.Wait(Func`1 query, Func`2 satisfactory, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2790
at UITest.Appium.HelperExtensions.WaitForAtLeastOne(Func`1 query, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2817
at UITest.Appium.HelperExtensions.WaitForElement(IApp app, String marked, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency, Nullable`1 postTimeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 826
at Microsoft.Maui.TestCases.Tests.ShellSearchHandlerFeatureTests.VerifyShellSearch_CancelButtonColor() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/ShellSearchHandlerFeatureTests.cs:line 623
at System.RuntimeMethodHandle.InvokeMethod(ObjectHandleOnStack t
...
SearchHandlerClearIconUpdatesAtRuntime
VisualTestUtils.VisualTestFailedException :
Snapshot different than baseline: SearchHandlerClearIconUpdatesAtRuntime.png (4.60% difference)
If the correct baseline has changed (this isn't a a bug), then update the baseline image.
See test attachment or download the build artifacts to get the new snapshot file.
More info: https://aka.ms/visual-test-workflow
at VisualTestUtils.VisualRegressionTester.Fail(String message) in /_/src/TestUtils/src/VisualTestUtils/VisualRegressionTester.cs:line 162
at VisualTestUtils.VisualRegressionTester.VerifyMatchesSnapshot(String name, ImageSnapshot actualImage, String environmentName, ITestContext testContext) in /_/src/TestUtils/src/VisualTestUtils/VisualRegressionTester.cs:line 123
at Microsoft.Maui.TestCases.Tests.UITest.<VerifyScreenshot>g__Verify|13_0(String name, <>c__DisplayClass13_0&) in /_/src/Controls/tests/TestCases.Shared.Tests/UITest.cs:line 477
at Microsoft.Maui.TestCases.Tests.UITest.VerifyScreenshot(String name, Nullable`1 ret
...
SearchHandlerClearPlaceholderIconUpdatesAtRuntime
VisualTestUtils.VisualTestFailedException :
Snapshot different than baseline: SearchHandlerClearPlaceholderIconUpdatesAtRuntime.png (4.61% difference)
If the correct baseline has changed (this isn't a a bug), then update the baseline image.
See test attachment or download the build artifacts to get the new snapshot file.
More info: https://aka.ms/visual-test-workflow
at VisualTestUtils.VisualRegressionTester.Fail(String message) in /_/src/TestUtils/src/VisualTestUtils/VisualRegressionTester.cs:line 162
at VisualTestUtils.VisualRegressionTester.VerifyMatchesSnapshot(String name, ImageSnapshot actualImage, String environmentName, ITestContext testContext) in /_/src/TestUtils/src/VisualTestUtils/VisualRegressionTester.cs:line 123
at Microsoft.Maui.TestCases.Tests.UITest.<VerifyScreenshot>g__Verify|13_0(String name, <>c__DisplayClass13_0&) in /_/src/Controls/tests/TestCases.Shared.Tests/UITest.cs:line 477
at Microsoft.Maui.TestCases.Tests.UITest.VerifyScreenshot(String name, Nul
...
SearchHandlerQueryIconUpdatesAtRuntime
VisualTestUtils.VisualTestFailedException :
Snapshot different than baseline: SearchHandlerQueryIconUpdatesAtRuntime.png (4.66% difference)
If the correct baseline has changed (this isn't a a bug), then update the baseline image.
See test attachment or download the build artifacts to get the new snapshot file.
More info: https://aka.ms/visual-test-workflow
at VisualTestUtils.VisualRegressionTester.Fail(String message) in /_/src/TestUtils/src/VisualTestUtils/VisualRegressionTester.cs:line 162
at VisualTestUtils.VisualRegressionTester.VerifyMatchesSnapshot(String name, ImageSnapshot actualImage, String environmentName, ITestContext testContext) in /_/src/TestUtils/src/VisualTestUtils/VisualRegressionTester.cs:line 123
at Microsoft.Maui.TestCases.Tests.UITest.<VerifyScreenshot>g__Verify|13_0(String name, <>c__DisplayClass13_0&) in /_/src/Controls/tests/TestCases.Shared.Tests/UITest.cs:line 477
at Microsoft.Maui.TestCases.Tests.UITest.VerifyScreenshot(String name, Nullable`1 ret
...
VerifyShellSearch_TextColor
System.TimeoutException : Timed out waiting for element...
at UITest.Appium.HelperExtensions.Wait(Func`1 query, Func`2 satisfactory, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2790
at UITest.Appium.HelperExtensions.WaitForAtLeastOne(Func`1 query, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2817
at UITest.Appium.HelperExtensions.WaitForElement(IApp app, String marked, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency, Nullable`1 postTimeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 826
at Microsoft.Maui.TestCases.Tests.ShellSearchHandlerFeatureTests.VerifyShellSearch_TextColor() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/ShellSearchHandlerFeatureTests.cs:line 600
at System.RuntimeMethodHandle.InvokeMethod(ObjectHandleOnStack target, V
...
SearchHandlerResetAllRestoresDefaultIcons
VisualTestUtils.VisualTestFailedException :
Snapshot different than baseline: SearchHandlerResetAllRestoresDefaultIcons.png (4.49% difference)
If the correct baseline has changed (this isn't a a bug), then update the baseline image.
See test attachment or download the build artifacts to get the new snapshot file.
More info: https://aka.ms/visual-test-workflow
at VisualTestUtils.VisualRegressionTester.Fail(String message) in /_/src/TestUtils/src/VisualTestUtils/VisualRegressionTester.cs:line 162
at VisualTestUtils.VisualRegressionTester.VerifyMatchesSnapshot(String name, ImageSnapshot actualImage, String environmentName, ITestContext testContext) in /_/src/TestUtils/src/VisualTestUtils/VisualRegressionTester.cs:line 123
at Microsoft.Maui.TestCases.Tests.UITest.<VerifyScreenshot>g__Verify|13_0(String name, <>c__DisplayClass13_0&) in /_/src/Controls/tests/TestCases.Shared.Tests/UITest.cs:line 477
at Microsoft.Maui.TestCases.Tests.UITest.VerifyScreenshot(String name, Nullable`1
...
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs)
🧭 Next Steps — reviewer patch required (pr-plus-reviewer)
The reviewer-enhanced candidate won, so the submitted PR still needs those changes.
Why: The refined PR candidate preserves the submitted root-cause design while closing the expert review's two concrete task-completion hazards and centralizing safe-area ownership. It passed all bounded regressions: 24 Core tests, 11 primary iOS UI tests, and 1 deferred-element iOS UI test.
Apply PRAgent/pr-plus-reviewer/reviewer.patch from the CopilotLogs
artifact (or follow the report's Required submitted-PR change), push the update, and run
the review again.
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
|
Latest AI review round fully dispositioned in 154984e — three findings adopted, three rebutted with mechanisms in the inline replies: Adopted
Not adopted (inline replies have the details)
Verified locally: @kubaflo the new commit will need a fresh |
#37060) On iOS, `MapRequestScrollTo` clamped programmatic scrolls to `[0, ContentSize - Frame]`, ignoring the `UIScrollView`'s `AdjustedContentInset`. On inset scroll views (the standard .NET 10 edge-to-edge configuration where the scroll view consumes the safe area, or any scroll view with content insets) the valid native offset range is `[-adjustedInset.Top, contentExtent + adjustedInset.Bottom - Bounds.Height]`, so: - `ScrollToAsync` to the end stopped **short by `adjustedInset.Top + adjustedInset.Bottom`**, leaving the last element under the home indicator while a finger fling lands it correctly above. - `ScrollToAsync(0, 0)` could not restore the natural rest position, which is `-adjustedInset.Top`, not `0`. This PR establishes one convention: **MAUI scroll offsets are content coordinates, where `(0,0)` places the top-left of the content at the top-left of the *visible* viewport.** That is what Android already does, and what `CollectionView` already reports on iOS (`ItemsViewDelegator` publishes `ContentOffset + ContentInset`) — `ScrollView` was the outlier reporting raw native offsets. 1. **Requests are translated into native offset space and clamped against the inset-aware range** (`request - adjustedInset`, clamped to `[-adjustedInset.Top, contentExtent + adjustedInset.Bottom - Bounds.Height]`). With zero insets this degenerates to the previous math. `MauiScrollView.ScrollableContentSize` supplies the extent by **measuring the rect its own arrange placed the content into** (recorded origin + arranged size, plus any trailing safe-area padding baked into the content coordinates), because `ContentSize` is not authoritative: depending on the inset mode the arrange pass pads it with a safe area UIKit is *also* applying through `AdjustedContentInset`, inflates it to keep UIKit in scrollable mode, or omits a non-zero safe-area origin the content was arranged at. Measuring covers every `ContentInsetAdjustmentBehavior` mode — including `Automatic` with a zero system inset — with no mode branching. 2. **Reported offsets get the reverse translation**, so `ScrollX`/`ScrollY` are content coordinates and `ScrollToAsync(ScrollX, ScrollY, …)` round-trips. When the insets change without the offset moving (rotation, an auto-hiding bar), the values are refreshed through an internal silent path (`IScrollOffsetReceiver`): `ScrollX`/`ScrollY` and their bindings stay current, but no `Scrolled` event is manufactured for a change no scroll produced. 3. **Element targets are computed against the effective viewport**, not the full frame — otherwise `ScrollToAsync(element, End)` lands the element `adjustedInset.Top + Bottom` past the visible region. The viewport comes from an internal Core contract (`IScrollViewportProvider`) implemented by the iOS handler, so the coordinate convention has a single owner and cross-platform code holds no platform inset knowledge. 4. **Deferred requests are fixed on two counts.** Replaying through `OnScrollToRequested` reset the completion source and orphaned the task the caller was awaiting, so a pre-handler `ScrollToAsync` never completed. And element-mode requests are no longer resolved at handler-attach, when `Width`/`Height` are `-1` and every content coordinate is `0`; they wait for geometry and a non-empty `ContentSize`, then resolve on the next dispatcher tick once the layout pass has finished arranging children. 5. **`ShellFlyoutLayoutManager` converts back to native coordinates.** Its header math works in native offsets (which rest at `-headerHeight`, since the header is carried in `ContentInset`), and its `ScrollView` branch consumed `ScrollY` verbatim — the adjacent `CollectionView` branch already does the equivalent conversion. No new public API; the new contracts and members are internal. > [!NOTE] > Behavioral change: on iOS/MacCatalyst `ScrollView.ScrollX`/`ScrollY` and `ScrolledEventArgs` now report content coordinates instead of raw `ContentOffset` — at rest they read `0` rather than `-adjustedInset.Top`. This aligns iOS with Android and with `CollectionView`'s existing iOS behavior. One consequence measured on both platforms: `ScrollToAsync(0, ContentSize.Height - Height)` does not reach the end when the scroll view consumes the safe area, because the reachable maximum is `contentHeight - visibleViewport` while that expression uses the full frame height. An Android probe confirms the identical shortfall there (short by exactly `paddingTop + paddingBottom`), so this is cross-platform-consistent rather than iOS-specific; `ScrollToAsync(0, ContentSize.Height)` lands exactly at the end on both, and clamps safely. > > A second consequence: because the reported offset now includes `AdjustedContentInset.Top`, it changes when the **top** (or left) inset changes even though the user did not scroll — a nav bar auto-hiding, a rotation, an iPad split-view resize, or `SetHeaderContentInset` installing a Shell flyout header inset. With `adjTop` going 96 → 0 and `ContentOffset` untouched by UIKit, `ScrollY` moves 296 → 200, which is accurate: 96 points that were behind the bar are now visible, so only 200 remain hidden above. That refresh flows through the internal `IScrollOffsetReceiver` path, which updates `ScrollX`/`ScrollY` (and bindings) **without raising `Scrolled`** — scroll-driven consumers doing raw delta math (`SwipeView.OnParentScrolled`, hide-on-scroll logic) never observe a scroll that didn't happen, and an inset change caused *by* a hiding bar cannot feed back into that same logic. A real scroll still reports normally. > > Android reaches the same end state by the opposite route, measured on an emulator: changing the scroll view's padding 156 → 0 leaves `ScrollY` at 200 and raises no event, but re-lays-out the content 156px higher (`CONTENT screenY -444 -> -600`). Because iOS keeps `contentInset` outside the content, nothing moves there and the number must change instead. Both platforms preserve "content hidden above the visible top", and neither raises `Scrolled` for an inset/padding-only change. The keyboard is **not** a trigger for this phantom shift: it changes the bottom inset, which does not enter `ScrollY = ContentOffset.Y + AdjustedContentInset.Top`. (It can still move `ScrollY` indirectly when the view sits near the bottom and UIKit re-pins `ContentOffset` into the shortened range — but there the content really moves, so that is an ordinary scroll notification.) Fixes #36801 `Issue36801` (iOS + MacCatalyst): a tall ScrollView given explicit content insets so the clamping math is exercised deterministically on any simulator. Oracles are geometric rather than restatements of the implementation — the end position is measured from the content platform view's actual frame, the element position from the probe's frame in window coordinates — and each result converges on a re-evaluation loop instead of a fixed delay. - **Scroll to end / to top** — the content's last pixel must rest at the bottom of the unobscured viewport; the top must land on the natural rest position; `ScrollY` must match the content-coordinate contract. - **All three inset modes** — `Automatic`, `Never` and `Always` bake the safe area into the content differently, so each is run. `SafeAreaEdges` is set on the `ScrollView` (it does not propagate from the page) and the fixture asserts the *resolved* `ContentInsetAdjustmentBehavior`, so a drifting mode fails loudly instead of quietly testing another branch. - **`ScrollToAsync(element, End)`** — the probe's bottom edge must sit exactly at the visible viewport bottom. - **Deferred offset and deferred element requests** — both issued before the handler exists. - **Appium-side probe assertions** — the probe rect must be fully inside the page rect, which discriminates independently of any in-app arithmetic. `ShellFlyoutHeaderScrollViewContent` (iOS + MacCatalyst): a Shell with `FlyoutHeaderBehavior.Scroll` over a `ScrollView` flyout content — the path the existing `ShellFlyoutHeaderBehavior` test cannot reach, since the default flyout uses `ShellTableViewController` and raw native offsets. Unit tests (`ScrollViewUnitTests`) cover the deferred-request lifecycle, element targets under viewport/content-coordinate insets, and that an inset-only refresh updates `ScrollX`/`ScrollY` without raising `Scrolled` while a real scroll still does. Verified on the iOS simulator: **12/12 pass** (the full `Issue36801` inset-mode matrix, both deferred fixtures, and the Shell flyout header test). Each fix was also checked to fail without it — the Shell test reports `headerY=-106` on the unfixed code, and the deferred element test reported `actual=2523 expected=738` before the arrange-ordering fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
Heads-up for anyone following the review threads here: the final round's commit ( |



Description of Change
On iOS,
MapRequestScrollToclamped programmatic scrolls to[0, ContentSize - Frame], ignoring theUIScrollView'sAdjustedContentInset. On inset scroll views (the standard .NET 10 edge-to-edge configuration where the scroll view consumes the safe area, or any scroll view with content insets) the valid native offset range is[-adjustedInset.Top, contentExtent + adjustedInset.Bottom - Bounds.Height], so:ScrollToAsyncto the end stopped short byadjustedInset.Top + adjustedInset.Bottom, leaving the last element under the home indicator while a finger fling lands it correctly above.ScrollToAsync(0, 0)could not restore the natural rest position, which is-adjustedInset.Top, not0.This PR establishes one convention: MAUI scroll offsets are content coordinates, where
(0,0)places the top-left of the content at the top-left of the visible viewport. That is what Android already does, and whatCollectionViewalready reports on iOS (ItemsViewDelegatorpublishesContentOffset + ContentInset) —ScrollViewwas the outlier reporting raw native offsets.request - adjustedInset, clamped to[-adjustedInset.Top, contentExtent + adjustedInset.Bottom - Bounds.Height]). With zero insets this degenerates to the previous math.MauiScrollView.ScrollableContentSizesupplies the extent by measuring the rect its own arrange placed the content into (recorded origin + arranged size, plus any trailing safe-area padding baked into the content coordinates), becauseContentSizeis not authoritative: depending on the inset mode the arrange pass pads it with a safe area UIKit is also applying throughAdjustedContentInset, inflates it to keep UIKit in scrollable mode, or omits a non-zero safe-area origin the content was arranged at. Measuring covers everyContentInsetAdjustmentBehaviormode — includingAutomaticwith a zero system inset — with no mode branching.ScrollX/ScrollYare content coordinates andScrollToAsync(ScrollX, ScrollY, …)round-trips. When the insets change without the offset moving (rotation, an auto-hiding bar), the values are refreshed through an internal silent path (IScrollOffsetReceiver):ScrollX/ScrollYand their bindings stay current, but noScrolledevent is manufactured for a change no scroll produced.ScrollToAsync(element, End)lands the elementadjustedInset.Top + Bottompast the visible region. The viewport comes from an internal Core contract (IScrollViewportProvider) implemented by the iOS handler, so the coordinate convention has a single owner and cross-platform code holds no platform inset knowledge.OnScrollToRequestedreset the completion source and orphaned the task the caller was awaiting, so a pre-handlerScrollToAsyncnever completed. And element-mode requests are no longer resolved at handler-attach, whenWidth/Heightare-1and every content coordinate is0; they wait for geometry and a non-emptyContentSize, then resolve on the next dispatcher tick once the layout pass has finished arranging children.ShellFlyoutLayoutManagerconverts back to native coordinates. Its header math works in native offsets (which rest at-headerHeight, since the header is carried inContentInset), and itsScrollViewbranch consumedScrollYverbatim — the adjacentCollectionViewbranch already does the equivalent conversion.No new public API; the new contracts and members are internal.
Warning
Breaking behavioral change (iOS/MacCatalyst) — release-notes material: on iOS/MacCatalyst
ScrollView.ScrollX/ScrollYandScrolledEventArgsnow report content coordinates instead of rawContentOffset— at rest they read0rather than-adjustedInset.Top. This aligns iOS with Android and withCollectionView's existing iOS behavior. One consequence measured on both platforms:ScrollToAsync(0, ContentSize.Height - Height)does not reach the end when the scroll view consumes the safe area, because the reachable maximum iscontentHeight - visibleViewportwhile that expression uses the full frame height. An Android probe confirms the identical shortfall there (short by exactlypaddingTop + paddingBottom), so this is cross-platform-consistent rather than iOS-specific;ScrollToAsync(0, ContentSize.Height)lands exactly at the end on both, and clamps safely.A second consequence: because the reported offset now includes
AdjustedContentInset.Top, it changes when the top (or left) inset changes even though the user did not scroll — a nav bar auto-hiding, a rotation, an iPad split-view resize, orSetHeaderContentInsetinstalling a Shell flyout header inset. WithadjTopgoing 96 → 0 andContentOffsetuntouched by UIKit,ScrollYmoves 296 → 200, which is accurate: 96 points that were behind the bar are now visible, so only 200 remain hidden above. That refresh flows through the internalIScrollOffsetReceiverpath, which updatesScrollX/ScrollY(and bindings) without raisingScrolled— scroll-driven consumers doing raw delta math (SwipeView.OnParentScrolled, hide-on-scroll logic) never observe a scroll that didn't happen, and an inset change caused by a hiding bar cannot feed back into that same logic. A real scroll still reports normally.Android reaches the same end state by the opposite route, measured on an emulator: changing the scroll view's padding 156 → 0 leaves
ScrollYat 200 and raises no event, but re-lays-out the content 156px higher (CONTENT screenY -444 -> -600). Because iOS keepscontentInsetoutside the content, nothing moves there and the number must change instead. Both platforms preserve "content hidden above the visible top", and neither raisesScrolledfor an inset/padding-only change. The keyboard is not a trigger for this phantom shift: it changes the bottom inset, which does not enterScrollY = ContentOffset.Y + AdjustedContentInset.Top. (It can still moveScrollYindirectly when the view sits near the bottom and UIKit re-pinsContentOffsetinto the shortened range — but there the content really moves, so that is an ordinary scroll notification.)Issues Fixed
Fixes #36801
Tests
Issue36801(iOS + MacCatalyst): a tall ScrollView given explicit content insets so the clamping math is exercised deterministically on any simulator. Oracles are geometric rather than restatements of the implementation — the end position is measured from the content platform view's actual frame, the element position from the probe's frame in window coordinates — and each result converges on a re-evaluation loop instead of a fixed delay.ScrollYmust match the content-coordinate contract.Automatic,NeverandAlwaysbake the safe area into the content differently, so each is run.SafeAreaEdgesis set on theScrollView(it does not propagate from the page) and the fixture asserts the resolvedContentInsetAdjustmentBehavior, so a drifting mode fails loudly instead of quietly testing another branch.ScrollToAsync(element, End)— the probe's bottom edge must sit exactly at the visible viewport bottom.ShellFlyoutHeaderScrollViewContent(iOS + MacCatalyst): a Shell withFlyoutHeaderBehavior.Scrollover aScrollViewflyout content — the path the existingShellFlyoutHeaderBehaviortest cannot reach, since the default flyout usesShellTableViewControllerand raw native offsets.Unit tests (
ScrollViewUnitTests) cover the deferred-request lifecycle, element targets under viewport/content-coordinate insets, and that an inset-only refresh updatesScrollX/ScrollYwithout raisingScrolledwhile a real scroll still does.Verified on the iOS simulator: 12/12 pass (the full
Issue36801inset-mode matrix, both deferred fixtures, and the Shell flyout header test). Each fix was also checked to fail without it — the Shell test reportsheaderY=-106on the unfixed code, and the deferred element test reportedactual=2523 expected=738before the arrange-ordering fix.🤖 Generated with Claude Code