Conversation
|
Hey there @kubaflo! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
btw: Windows image looks very different than others. Just to be clear, is that a bug or not? |
Hmm I’m not sure. Could you maybe have a look at this please? |
|
I can see:
and without your first commit (2a43d95) it looks like this:
|
I think without my commit it looks very wrong. On iOS and Android, everything looks as expected, I think Windows might need some adjustment, but I don't have a Windows machine to work on it. Thanks for sending screenshots! I think only this part doesn't work on Windows, but maybe it is how it is supposed to be. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 25036Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 25036" |
There was a problem hiding this comment.
Pull request overview
This PR fixes nested FlexLayout rendering, which was completely broken — nested flex layouts had zero size and their children (including images, image buttons, and background colors) were not rendered. The root cause was that AddFlexItem() tried to reuse a child FlexLayout's internal _root Flex.Item as the item in the parent flex tree, which caused the child's Layout() method to short-circuit (because _root.Parent != null indicated it was a non-root node and skipped layout entirely). The fix always creates a fresh Flex.Item for each child—including nested FlexLayout children—and assigns a SelfSizing callback to measure through the standard child.Measure() path for all children.
Changes:
- Core fix in
FlexLayout.cs: Remove the special-case path that reused a childFlexLayout's_rootas the flex item; instead always create a newFlex.Itemwith aSelfSizingcallback for all children - New UI test host page (
Issue20461.xaml/.xaml.cs): Reproduces the nested flex layout scenario with images, image buttons, and multiple nesting levels - New NUnit test and snapshot baselines for Android and Windows (iOS snapshot also present)
Reviewed changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/Controls/src/Core/Layout/FlexLayout.cs |
Core fix: removes reuse of child FlexLayout's _root, always creates a new Flex.Item for all children |
src/Controls/tests/TestCases.HostApp/Issues/Issue20461.xaml |
UI test page reproducing nested FlexLayouts with images and image buttons |
src/Controls/tests/TestCases.HostApp/Issues/Issue20461.xaml.cs |
Code-behind for the test page |
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue20461.cs |
NUnit test that waits for nested elements and verifies the screenshot |
src/Controls/tests/TestCases.Android.Tests/snapshots/android/NestedFlexLayoutShouldRenderCorrectly.png |
Android baseline snapshot |
src/Controls/tests/TestCases.WinUI.Tests/snapshots/windows/NestedFlexLayoutShouldRenderCorrectly.png |
Windows baseline snapshot |
The Copilot reviewer agent on PR #25036 (Nested Flex Layouts) ran 5 try-fix attempts across models and hit the 180-minute timeout during Round 2 cross-pollination. Doubling to 360 minutes to accommodate complex PRs that need multiple fix attempts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #25036 | Stop reusing a nested FlexLayout's internal _root; create a dedicated Flex.Item for each child and attach self-sizing to that item so nested layouts measure/render independently. |
⏳ PENDING (Gate) | src/Controls/src/Core/Layout/FlexLayout.cs |
Original PR; linked UI test uses Issue20461 |
🚦 Gate — Test Verification
Gate Result: ✅ PASSED
Platform: android
Mode: Full Verification
- Tests FAIL without fix: ✅
- Tests PASS with fix: ✅
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Lazily rebuild nested FlexLayout roots during layout and install a sizing proxy on the captured parent item. |
❌ FAIL | 1 file | Rendered content but screenshot still differed by 6.90% on first-pass geometry. |
| 2 | try-fix | Repair nested FlexLayout parent-child item associations in PopulateLayout / OnParentSet so nested layouts get a proxy parent item with SelfSizing, without changing AddFlexItem. |
✅ PASS | 1 file | Passed Android Issue20461; substantially larger change (+226/-73). |
| 3 | try-fix | Keep nested root reuse but always attach SelfSizing to the inserted item. |
❌ FAIL | 1 file | Smaller idea, but insufficient; Android Issue20461 still failed. |
| 4 | try-fix | Child-managed FlexItemProxy for nested FlexLayout instances. |
1 file | Partial artifacts only; temp-clone/app-launch contamination prevented trustworthy validation. | |
| 5 | try-fix | Parent-local child→Flex.Item ownership map so each parent tracks its inserted wrapper item explicitly. |
✅ PASS | 1 file | Passed Android Issue20461; near-PR size (+34/-30) but adds ownership-map complexity. |
| PR | PR #25036 | Create a dedicated Flex.Item for each child in AddFlexItem and attach self-sizing there. |
✅ PASSED (Gate) | 1 file | Original PR; Android gate passed; smallest passing implementation (+26/-29). |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 1 | Yes | Lazy root rebuild/proxy sizing attempt executed. |
| claude-sonnet-4.6 | 1 | Yes | PopulateLayout/OnParentSet repair path passed. |
| gpt-5.3-codex | 1 | Yes | SelfSizing-on-reused-root attempt executed and failed. |
| gemini-3-pro-preview | 1 | Yes | Child-managed proxy concept proposed, but execution was blocked by temp clone contamination. |
| claude-opus-4.6 | 2 | No | NO NEW IDEAS |
| claude-sonnet-4.6 | 2 | No | NO NEW IDEAS |
| gpt-5.3-codex | 2 | Yes | NEW IDEA: parent-local child→item ownership map |
| gemini-3-pro-preview | 2 | No | NO NEW IDEAS |
| claude-opus-4.6 | 3 | No | NO NEW IDEAS |
| claude-sonnet-4.6 | 3 | No | NO NEW IDEAS |
| gpt-5.3-codex | 3 | No | NO NEW IDEAS |
| gemini-3-pro-preview | 3 | No | NO NEW IDEAS |
Passing Candidate Comparison
| Candidate | Result | Approx. Size | Strengths | Tradeoffs |
|---|---|---|---|---|
| PR | ✅ PASS | +26/-29 | Smallest passing change; fixes root cause at item creation point; easy to reason about | None observed on Android; Windows concern remains only in PR discussion, not this test run |
| #2 | ✅ PASS | +226/-73 | Explicitly handles multiple initialization paths | Much larger and more complex than needed |
| #5 | ✅ PASS | +34/-30 | Also small; explicit parent ownership model | Adds state-tracking complexity beyond the PR need |
Exhausted: Yes
Selected Fix: PR #25036 — All passing solutions converge on the same invariant: nested FlexLayout must participate as a measured leaf in the parent tree via a separate parent-owned item. The PR implements that invariant with the smallest, clearest passing change.
📋 Report — Final Recommendation
✅ Final Recommendation: APPROVE
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Linked issues and PR discussion reviewed; Windows concern noted from comments. |
| Gate | ✅ PASSED | Android full verification: tests fail without fix and pass with fix. |
| Try-Fix | ✅ COMPLETE | 5 attempts total, 2 alternative passes, design space exhausted. |
| Report | ✅ COMPLETE |
Summary
PR #25036 fixes the nested FlexLayout rendering bug on Android with valid UI coverage. The gate verified the added Issue20461 test actually fails on the broken baseline and passes with the PR fix. I then ran the mandatory try-fix exploration across four model families plus follow-up rounds; while two independent alternatives also passed, the PR’s implementation was the smallest and clearest passing solution.
Root Cause
The bug comes from nested FlexLayout ownership of Flex.Item state. Reusing a child FlexLayout's internal _root as the parent-owned flex item lets the parent and child trees interfere with each other. Passing fixes all separate those responsibilities so the nested layout is measured as a leaf in the parent tree while keeping its own _root internal.
Fix Quality
The PR addresses the root cause directly in AddFlexItem by ensuring the parent uses a dedicated wrapper Flex.Item with SelfSizing. Compared with the passing alternatives, it achieves the same correct invariant with less code and less state-management complexity. Based on Android verification, this is the best fix among the candidates explored.
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run maui-pr-devicetests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description of Change
I'm considering whether this change might have any unforeseen implications. At present, it seems safe because flex layouts nested inside other flex layouts aren't functioning as expected at all and this change only affects this situation
Issues Fixed
Fixes: #24653
Fixes: #20461
Fixes #26013