[iOS] Fix EditorScrollingWhenEnclosedInBorder test failure on candidate branch - #35920
Conversation
|
/review -b feature/enhanced-reviewer -p android |
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@Vignesh-SF3580 — new AI review results are available based on this last commit:
2124644. To request a fresh review after new comments or commits, comment/review rerun.
Review Sessions — click to expand
Gate — Test Before & After Fix
Gate Result: ⚠️ SKIPPED
No tests were detected in this PR.
Recommendation: Add tests to verify the fix using the write-tests-agent.
UI Tests
Full UI test matrix will run (no specific categories detected from PR changes).
Pre-Flight — Context & Validation
Issue: #35920 - [iOS] Fix EditorScrollingWhenEnclosedInBorder test failure on candidate branch
PR: #35920 - [iOS] Fix EditorScrollingWhenEnclosedInBorder test failure on candidate branch
Platforms Affected: iOS/MacCatalyst implementation; user requested android for candidate testing
Files Changed: 1 implementation, 0 test
Key Findings
- PR modifies
src/Core/src/Handlers/Editor/EditorHandler.iOS.csinEditorHandler.GetDesiredSize. - Linked issue is the PR itself; no separate
Fixes/Closes/Resolves #...issue reference was found. - Gate was already skipped because no tests were detected; this phase did not rerun gate verification.
- Public PR comments only contained
/review -b feature/enhanced-reviewer -p android; public inline review comments were empty. - The PR fix skips the final height cap for caller-provided finite constraints, addressing the Border snapshot drift described in the PR.
Code Review Summary
Verdict: NEEDS_CHANGES
Confidence: low
Errors: 1 | Warnings: 0 | Suggestions: 0
Key code review findings:
- ❌
src/Core/src/Handlers/Editor/EditorHandler.iOS.cs:88-114— frame-derived cap can ignore explicit height increases after an overflowing non-auto-growing Editor has an old smaller native bounds height.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35920 | Skip final clamp when caller supplied finite constraints; still cap when infinite height is replaced by Bounds.Height. |
src/Core/src/Handlers/Editor/EditorHandler.iOS.cs |
Original PR; expert review found stale-bounds risk. |
Code Review — Deep Analysis
Code Review — PR #35920
Independent Assessment
What this changes: iOS/MacCatalyst EditorHandler.GetDesiredSize now skips the final height clamp when both incoming constraints are finite, preserving MAUI's normal measure contract for caller-provided finite constraints. The clamp still applies when an infinite height is substituted with PlatformView.Bounds.Height to preserve scrollability for non-auto-growing Editors.
Inferred motivation: Fix a 1 px vertical snapshot drift for EditorScrollingWhenEnclosedInBorder while preserving the previous rotation/scrollability fix for overflowing non-auto-growing Editors.
Reconciliation with PR Narrative
Author claims: The final cap should apply only when the handler internally substitutes a real frame bound, not when the caller provides a finite height constraint.
Agreement/disagreement: Agreement on the finite-constraint fix. The remaining concern is that the frame-derived bound can still be stale when MAUI is trying to satisfy a new explicit size request.
Prior Review Reconciliation
No prior ❌ Error findings found. Issue comments contained only /review -b feature/enhanced-reviewer -p android; inline review comments were empty via public API.
Blast Radius Assessment
- Runs for all instances: yes — all iOS/MacCatalyst Editors during measure.
- Startup impact: no.
- Static/shared state: no.
CI Status
- Required-check result: undetermined;
ghis unauthenticated in this environment. - Classification: undetermined.
- Action taken: confidence capped to low.
Findings
❌ Error — Explicit height increases can be ignored after content overflows
src/Core/src/Handlers/Editor/EditorHandler.iOS.cs:88-114
When heightConstraint is infinite and text content is taller than the existing native frame, the PR substitutes heightConstraint = PlatformView.Bounds.Height and later clamps the result back to that value. This preserves scrollability for the rotation case, but it can also override legitimate MAUI layout changes that should increase the Editor height. For example, a non-auto-growing overflowing Editor arranged at 100px can remain capped at 100px after HeightRequest changes to 200px because the old native frame is treated as the current real bound.
Failure-Mode Probing
- AutoSize=TextChanges after rotation: frame-height substitution is skipped when auto-growth is enabled.
- Caller-provided finite constraints: the PR's
elsebranch handles both-finite constraints and fixes the described Border snapshot drift. - Explicit resize after overflow: risk remains because old native bounds can become a hard cap.
- Handler lifecycle: no new subscriptions or static state.
Verdict: NEEDS_CHANGES
Confidence: low
Summary: The PR targets the finite-constraint drift correctly, but the old-frame clamp can block explicit height growth for overflowing Editors. Alternative fixes should preserve scrollability without treating stale native bounds as authoritative.
Report — Final Recommendation
Comparative Report - PR #35920
Inputs considered
pr: raw PR #35920 squashed commit.pr-plus-reviewer: raw PR plus the expert reviewer feedback applied in the sandbox candidate.try-fix-1: the only available STEP 5a individual candidate artifact underCustomAgentLogsTmp/PRState/35920/PRAgent/try-fix-1/.
The aggregate try-fix/content.md file was not present, so comparison used the individual candidate artifacts. The requested platform was Android. Gate verification was skipped before this phase because no tests were detected in the PR, and it was not rerun.
Candidate ranking
| Rank | Candidate | Regression / validation result | Assessment |
|---|---|---|---|
| 1 | pr-plus-reviewer |
No PR tests detected; sandbox patch only. | Best candidate. It preserves the raw PR's finite-constraint behavior while also honoring explicit VirtualView.Height before using stale native Bounds.Height, resolving the expert reviewer's major correctness concern. |
| 2 | pr |
Gate skipped; no tests detected. | Better than a failed validation candidate, but it leaves a major stale-bounds risk where an overflowing non-auto-growing Editor can ignore a later explicit height increase. |
| 3 | try-fix-1 |
Unit test command exited 0 with no matching tests; Android build exited 1 with NETSDK1005 because restored assets lacked net10.0-android. |
Functionally equivalent to the reviewer feedback patch, but candidates with failed regression/platform validation must rank lower than candidates that did not fail. The failure appears environment/restore-related, but it is still the only candidate with a failed requested-platform validation artifact. |
Analysis
The raw PR fix correctly identifies that the final clamp should not run for caller-provided finite constraints. That is the central fix for the Border snapshot drift and is a reasonable, localized iOS/MacCatalyst handler change.
The expert reviewer identified a real remaining failure mode: when the layout pass supplies infinite height and the native UITextView already has overflowing content, the raw PR can substitute the old Bounds.Height and then clamp the result back to that stale value. If the virtual Editor has an explicit height set or updated, the handler should prefer that explicit cross-platform height over the stale native frame. pr-plus-reviewer does that before falling back to the scrollability-preserving Bounds.Height path.
try-fix-1 applies the same code change as pr-plus-reviewer, but its saved Android validation failed before a successful platform result was produced. Per the ranking rule, a candidate with failed regression/platform validation must be ranked below candidates without that failure.
Winner
pr-plus-reviewer wins. It is the raw PR approach with the expert reviewer's actionable correctness fix applied, and it avoids selecting the STEP 5a candidate whose requested Android validation failed.
Future Action — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Issue Details
After PR #35309, the iOS snapshot for EditorScrollingWhenEnclosedInBorder differs from the baseline by approximately 1 px vertically. The Border outline, visible text lines, and scroll thumb endpoints are all shifted by one pixel, causing the exact-match snapshot comparison to fail.
Test name: EditorScrollingWhenEnclosedInBorder
Cause PR: #35309
Root Cause
PR #35309 added a final cap in EditorHandler.GetDesiredSize that clamps result.Height to heightConstraint. PR #35662 narrowed this behavior to skip the SizeThatFits substitution path, but the cap was still applied when the caller provided a finite heightConstraint (for example, when Border measures its child).
base.GetDesiredSize correctly returned the requested size, but the final cap reduced the calculated height to the Border's stroke-adjusted constraint, resulting in a 1 px vertical drift.
Description of Change
Added an else branch to the double.IsInfinity(...) check in GetDesiredSize (iOS) to mark caller-provided finite constraints using heightSubstitutedFromSizeThatFits = true.
The final height cap now applies only when the height value was substituted internally, restoring the standard MAUI measure behavior for caller-provided constraints while preserving the existing behavior.
Output