Fix Path Rendering Issue Inside StackLayout When Margin Is Set#28071
Fix Path Rendering Issue Inside StackLayout When Margin Is Set#28071Shalini-Ashokan wants to merge 10 commits intodotnet:mainfrom
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| result.Height += StrokeThickness; | ||
| result.Width += StrokeThickness; | ||
|
|
||
| if (this is Line or Path or Polyline) |
There was a problem hiding this comment.
Why is required for Line, PolyLine and Path and not for example for Polyline?
There was a problem hiding this comment.
@jsuarezruiz, In non-closed shapes such as Line, Polyline, and Path, the margin is not considered when the shape is inside a StackLayout and no explicit size is defined. The base size value is returned before measuring the path size because the measured result includes the margin, causing the shape to be invisible.
When the height or width is not explicitly set for these shapes, the shape size is reset using boundsByFlattening and does not account for the margin. Hence, the margin is now added at the end of non-closed shapes
|
/rebase |
f1cec6f to
31e0980
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/rebase |
a8d00d3 to
d25ca12
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/rebase |
d25ca12 to
d16f590
Compare
|
/rebase |
d16f590 to
103e2e1
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/rebase |
2443cb7 to
54a3927
Compare
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/rebase |
54a3927 to
b19f12a
Compare
| <StackLayout VerticalOptions="Start"> | ||
| <Label AutomationId="PathLabel" | ||
| Text="Path with Margin"/> | ||
| <Path Stroke="Red" |
There was a problem hiding this comment.
Could update the sample to add a Line and a Polyline?
There was a problem hiding this comment.
@jsuarezruiz, Added Line and Polyline shapes to the sample as per your suggestion.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
62b58de to
308d356
Compare
308d356 to
d05f63f
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 28071Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 28071" |
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #28071 | Adjust Shape.MeasureOverride to avoid early-returning when measured size only comes from margin, then add margin back for Line/Path/Polyline after path-bound measurement; validate with screenshot UITest |
⏳ PENDING (Gate) | src/Controls/src/Core/Shapes/Shape.cs, src/Controls/tests/TestCases.HostApp/Issues/Issue13801.xaml, src/Controls/tests/TestCases.HostApp/Issues/Issue13801.xaml.cs, src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue13801.cs |
Snapshot assets added for Android/iOS/Mac/Windows |
🚦 Gate — Test Verification
Gate Result: ❌ FAILED
Platform: android
Mode: Full Verification
- Tests FAIL without fix: ✅
- Tests PASS with fix: ❌
Notes
- Verification was run via isolated task agent using the
verify-tests-fail-without-fixskill forIssue13801on Android. - The test reproduced the bug without the fix, which means the test coverage is meaningful.
- The same test still failed with the fix applied, timing out while navigating to / validating the
Issue13801page with the same rendering issue still present. - Final status from gate verification: the PR fix did not make the Android verification pass.
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix (claude-opus-4.6) |
Explicit-size guard with universal margin inclusion | ✅ PASS | 1 file | Passing summary captured from task output; later Gemini tooling reused attempt-1, so Opus artifacts were not preserved on disk |
| 2 | try-fix (claude-sonnet-4.6) |
Compute pathBounds first for geometry-bearing shapes and bypass base.MeasureOverride; only fall back to base for zero-geometry shapes |
✅ PASS | 1 file | Cross-platform shape-level fix; preserved in attempt-2 |
| 3 | try-fix (gpt-5.3-codex) |
Clamp negative margin thickness to zero before the early-return comparison | ✅ PASS | 1 file | Smallest textual diff among shape-level fixes, but mainly addresses negative-margin behavior added in the repro page |
| 4 | try-fix (gemini-3-pro-preview) |
Force re-measure / explicitly ensure geometry+stroke+margin for geometry shapes | ✅ PASS | 1 file | Passed on android, but analysis warns a naive version may bypass stretch behavior; preserved as attempt-4 |
| 5 | cross-pollinated try-fix | Android handler-level intrinsic size: remove GetDesiredSize zeroing and revert PR margin patching |
❌ FAIL | 2 files | Rendered but snapshot differed by 4.79%, so not a viable replacement |
| 6 | cross-pollinated try-fix | Android intrinsic geometry cache + RequestLayout/InvalidateMeasure so handler reports real auto size |
✅ PASS | 3 files | Fixes Android without changing Shape.MeasureOverride; more invasive and platform-specific |
| 7 | cross-pollinated try-fix | Android OnMeasure zero-signal + on-demand intrinsic geometry in GetDesiredSize |
✅ PASS | 3 files | Also passes, but requires Android-only handler/public API changes |
| 8 | cross-pollinated try-fix | Android handler manual geometry + margin desired size | ❌ FAIL | 1 file | Double-counted margin and produced 5.57% screenshot diff |
| PR | PR #28071 | Margin-aware early return threshold plus late margin add-back only for Line/Path/Polyline |
❌ FAILED (Gate) | 4 source files (+ snapshots) | Android gate still failed with PR fix applied |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 1 | Yes | Android handler/cache invalidation or ordering fix before first measure |
| claude-sonnet-4.6 | 1 | Yes | Pre-inflate Android measure specs by margins in OnMeasure |
| gpt-5.3-codex | 1 | No | NO NEW IDEAS |
| gemini-3-pro-preview | 1 | Yes | Fix Android ShapeViewHandler.GetDesiredSize instead of margin math in Shape.MeasureOverride |
| claude-opus-4.6 | 2 | Yes | Handler desired size variant that manually incorporates geometry and margin |
| claude-sonnet-4.6 | 2 | No | NO NEW IDEAS |
| gpt-5.3-codex | 2 | No | NO NEW IDEAS |
| gemini-3-pro-preview | 2 | No | NO NEW IDEAS |
| claude-opus-4.6 | 3 | Yes | Arrange/render-time Android offset or clipping idea (not run due max 3 rounds) |
| claude-sonnet-4.6 | 3 | Yes | Map MAUI margin to native padding idea (not run due max 3 rounds) |
| gpt-5.3-codex | 3 | No | NO NEW IDEAS |
| gemini-3-pro-preview | 3 | No | NO NEW IDEAS |
Exhausted: Yes
Selected Fix: Candidate #2 — best balance of correctness and maintainability. It addresses the underlying issue at the shared Shape.MeasureOverride layer, keeps the fix cross-platform, avoids Android-only API/handler churn, and is more robust than the PR's current margin-threshold patch.
📋 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Linked issue #13801, PR discussion, changed files, and test coverage classified |
| Gate | ❌ FAILED | Android full verification reproduced the bug without the fix, but the same test still failed with the PR fix applied |
| Try-Fix | ✅ COMPLETE | 8 candidate fixes reviewed/tested, 6 passing on android |
| Report | ✅ COMPLETE |
Summary
The PR adds a meaningful UITest for Issue13801, but the submitted implementation in Shape.MeasureOverride did not pass Android gate verification: the test failed without the fix (good) and also failed with the fix applied (bad). Try-fix exploration found multiple independent alternatives that do pass the Android screenshot test, which means the current PR fix is not the best available solution.
The strongest alternative is a shared Shape.MeasureOverride change that computes pathBounds first for geometry-bearing shapes, bypasses base.MeasureOverride only for those shapes, and then adds stroke and margin once at the end. That approach passed the Android test, directly addresses the underlying measurement problem, and avoids the Android-specific handler churn required by the platform-only alternatives.
Root Cause
For auto-sized shapes with intrinsic geometry, the current pipeline can treat a margin-only desired size as a legitimate measured result. On Android, that produced a non-zero measurement that still did not represent actual content size, so the early-return path in Shape.MeasureOverride short-circuited before geometry-based sizing could correct the result. The PR's current fix tries to patch this by comparing the base-measured size to margin thickness and later adding margin back for selected shape types, but that still failed the Android gate.
Fix Quality
The PR fix is directionally related to the issue, but it is too narrow and did not satisfy the Android verification requested for this review. In contrast, multiple alternative fixes passed; among them, Candidate #2 is the best tradeoff:
- It is cross-platform, unlike the Android handler alternatives.
- It is root-cause oriented, because it avoids relying on
base.MeasureOverridefor shapes that already have intrinsic path geometry. - It is maintainable, because it changes one shared method instead of adding Android-specific intrinsic-size caches or new
OnMeasurebehavior. - It is more robust than the minimal negative-margin tweak (Candidate Third #3), which appears tailored to the added negative-margin repro cases rather than the original issue itself.
The Issue13801VerifyPathMargin test passes locally on Android with the current fix. Candidate #2 is not valid because it skips base.MeasureOverride() for any shape that has geometry. Every shape (Rectangle, Ellipse, Polygon, etc.) has geometry, so Candidate #2 would change measurement behavior for all shapes — not just the broken ones — causing potential regressions. The current fix is scoped to only Line, Path, and Polyline because only these shapes hit the fall through measurement path where margin gets lost. All other shapes already measure correctly through the handler and return early, so they are unaffected. |
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
When a Margin value is set for the Path, it renders correctly. However, when the Path is placed inside a StackLayout, it does not render.
Root Cause
While measuring the Path, the size calculation does not account for the Margin value, which leads to incorrect path rendering.
Description of Change
To include the Margin value in size calculations and ensure proper Path rendering.
Validated the behavior in the following platforms
Issues Fixed
Fixes #13801
Output ScreenShot