diff --git a/.github/agent-pr-session/pr-33134.md b/.github/agent-pr-session/pr-33134.md new file mode 100644 index 000000000000..19b6325c46de --- /dev/null +++ b/.github/agent-pr-session/pr-33134.md @@ -0,0 +1,203 @@ +# PR Review: #33134 - [Android] EmptyView doesn't display when CollectionView is placed inside a VerticalStackLayout + +**Date:** 2026-01-09 | **Issue:** [#32932](https://github.com/dotnet/maui/issues/32932) | **PR:** [#33134](https://github.com/dotnet/maui/pull/33134) + +## βœ… Status: COMPLETE + +| Phase | Status | +|-------|--------| +| Pre-Flight | βœ… COMPLETE | +| πŸ§ͺ Tests | βœ… COMPLETE | +| 🚦 Gate | βœ… COMPLETE | +| πŸ”§ Fix | βœ… COMPLETE | +| πŸ“‹ Report | βœ… COMPLETE | + +--- + +
+πŸ“‹ Issue Summary + +CollectionView has an EmptyView property for rendering a view when the ItemsSource is empty. This does not work when the CollectionView is placed inside a VerticalStackLayout. + +**Steps to Reproduce:** +1. Place a CollectionView inside a VerticalStackLayout +2. Set an empty ItemsSource +3. Set an EmptyView or EmptyViewTemplate +4. Run on Android - EmptyView does not display + +**Platforms Affected:** +- [ ] iOS +- [x] Android +- [ ] Windows +- [ ] MacCatalyst + +**Reproduction repo:** https://github.com/rrbabbb/EmptyViewNotWorkingRepro + +
+ +
+πŸ“ Files Changed + +| File | Type | Changes | +|------|------|---------| +| `src/Controls/src/Core/Handlers/Items/Android/SizedItemContentView.cs` | Fix | +2 lines | +| `src/Controls/tests/TestCases.HostApp/Issues/Issue32932.cs` | Test | New file | +| `src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue32932.cs` | Test | New file | + +
+ +
+πŸ’¬ PR Discussion Summary + +**Key Comments:** +- No significant reviewer feedback at time of review + +**Author:** NanthiniMahalingam (Syncfusion partner) + +**Labels:** platform/android, area-controls-collectionview, community ✨, partner/syncfusion + +
+ +
+πŸ§ͺ Tests + +**Status**: βœ… COMPLETE + +- [x] PR includes UI tests +- [x] Tests reproduce the issue +- [x] Tests follow naming convention (`IssueXXXXX`) + +**Test Files:** +- HostApp: `src/Controls/tests/TestCases.HostApp/Issues/Issue32932.cs` +- NUnit: `src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue32932.cs` + +**Test Description:** Places a CollectionView with an empty ItemsSource inside a VerticalStackLayout and verifies that the EmptyView (a Label with AutomationId="EmptyView") is visible. + +**Verification:** +- Tests compile successfully +- Follow proper naming conventions (Issue32932) +- Use `CollectionView2` handler for Android +- Have correct `[Issue()]` attributes and categories + +
+ +
+🚦 Gate - Test Verification + +**Status**: βœ… COMPLETE + +- [x] Tests FAIL without fix (bug reproduced) +- [x] Tests PASS with fix (bug fixed) + +**Result:** VERIFIED βœ… + +**Test Execution Results:** + +| State | Result | Details | +|-------|--------|---------| +| **WITH fix** | βœ… PASS | EmptyView element found immediately (1 Appium query) | +| **WITHOUT fix** | ❌ FAIL (Expected) | EmptyView never appears - 25+ retries before timeout | + +**Verification Method:** +1. Ran test with PR fix applied β†’ PASS +2. Reverted `SizedItemContentView.cs` to pre-fix version (before commit `b498b13ef6`) +3. Ran test without fix β†’ FAIL (TimeoutException: `Timed out waiting for element...`) +4. Restored fix + +**Log Evidence:** +- WITH fix: `method: 'id', selector: 'com.microsoft.maui.uitests:id/EmptyView'` β†’ found immediately +- WITHOUT fix: 25+ consecutive searches for `EmptyView` element, never found, test times out + +
+ +
+πŸ”§ Fix Candidates + +**Status**: βœ… COMPLETE + +| # | Source | Approach | Test Result | Files Changed | Notes | +|---|--------|----------|-------------|---------------|-------| +| 1 | try-fix | Fix at source in `EmptyViewAdapter.GetHeight()/GetWidth()` - return `double`, check `IsInfinity()` before casting | βœ… PASS | `EmptyViewAdapter.cs` | Fixes bug at the source but changes method signatures | +| 2 | try-fix | Fix at source in `EmptyViewAdapter` - use `double` throughout + `Math.Abs` + infinity check | βœ… PASS | `EmptyViewAdapter.cs` | Cleaner version of #1 but still changes method signatures | +| 3 | try-fix | **Avoid int.MaxValue entirely:** keep `GetHeight/GetWidth` as `int`, but change the lambdas passed to `SizedItemContentView`/`SimpleViewHolder` to return `double.PositiveInfinity` when `RecyclerViewHeight/Width` is infinite | βœ… PASS | `EmptyViewAdapter.cs` (+4/-3) | Preserves infinity without signature changes and without magic-number checks; not defensive for other callers | +| 4 | try-fix | Check for infinity BEFORE casting in `GetHeight()`/`GetWidth()`, return 0 to trigger fallback | ❌ FAIL | `EmptyViewAdapter.cs` | **Why failed:** Fallback to `parent.MeasuredHeight` returns 0 during initial layout pass - doesn't provide valid dimensions | +| 5 | try-fix | Skip setting `RecyclerViewHeight/Width` when infinite | ❌ FAIL | `ItemsViewHandler.Android.cs` | **Why failed:** Same issue as #4 - fallback mechanism doesn't work when parent not yet measured | +| 6 | try-fix | Add `GetHeightAsDouble()`/`GetWidthAsDouble()` methods that return infinity when appropriate, use in `CreateEmptyViewHolder` lambdas | βœ… PASS | `EmptyViewAdapter.cs` (+18) | Cleaner version of #3 with dedicated helper methods; fixes at source, no heuristic, no signature changes | +| PR | PR #33134 | Add `NormalizeDimension()` helper in `SizedItemContentView` that converts `int.MaxValue` β†’ `double.PositiveInfinity` | βœ… VERIFIED | `SizedItemContentView.cs` | **SELECTED** - Defensive/low-risk downstream patch | + +### Root Cause Analysis + +When a CollectionView is inside a VerticalStackLayout, the height constraint passed to the CollectionView is `double.PositiveInfinity` (unconstrained). This value propagates to `EmptyViewAdapter.RecyclerViewHeight`. + +In `EmptyViewAdapter.GetHeight()`, the code casts this to `int`: `int height = (int)RecyclerViewHeight;` + +In C#, `(int)double.PositiveInfinity` produces `int.MaxValue` (2147483647). This value is then passed via lambda to `SizedItemContentView.OnMeasure()`, where the code checks `double.IsInfinity(targetHeight)` - but `int.MaxValue` is not infinity, so the check fails and layout calculations break. + +### Key Insight from Failed Attempts + +Attempts #4 and #5 revealed that: +- `(int)double.PositiveInfinity` in C# actually produces `int.MaxValue` (verified empirically) +- The fallback to `parent.MeasuredHeight` doesn't work during initial layout when parent hasn't been measured yet +- The solution must **preserve infinity semantics** rather than trying to fall back to measured values + +### Comparison of Passing Fixes + +| Criteria | PR's Fix | Source-Level (#3/#6) | +|----------|----------|---------------------| +| **Correctness** | βœ… Handles `int.MaxValue` β†’ `∞` | βœ… Preserves `∞` directly | +| **Defensive** | βœ… Protects ALL callers of `SizedItemContentView` | ❌ Only fixes EmptyView path | +| **Risk** | βœ… Minimal - 1 line addition | ⚠️ Adds new methods to adapter | +| **Heuristic concern** | ⚠️ Assumes `int.MaxValue` means infinity | βœ… No heuristic | + +**Why PR's heuristic is valid:** `(int)double.PositiveInfinity` produces `int.MaxValue` in C#, so the heuristic is actually semantically correct - `int.MaxValue` really does mean "this was infinity before the cast." + +**Exhausted:** Yes (6 try-fix attempts completed) +**Selected Fix:** PR #33134's fix (NormalizeDimension helper in SizedItemContentView) + +**Rationale:** The PR's fix is the best choice because: +1. **Defensive:** Protects ALL callers of `SizedItemContentView.OnMeasure()`, not just EmptyView +2. **Low-risk:** Minimal code change (1 helper method, 2 call sites) +3. **Semantically correct:** The heuristic `int.MaxValue β†’ PositiveInfinity` is valid because `(int)double.PositiveInfinity` produces `int.MaxValue` in C# +4. **No signature changes:** Doesn't require changing method signatures in `EmptyViewAdapter` + +
+ +--- + +## πŸ“‹ Final Report + +### Recommendation: βœ… APPROVE + +**Summary:** PR #33134 correctly fixes the Android EmptyView visibility bug when CollectionView is inside a VerticalStackLayout. + +### Technical Analysis + +**Root Cause:** +- CollectionView inside VerticalStackLayout receives `double.PositiveInfinity` as height constraint +- `EmptyViewAdapter.GetHeight()` casts this to `int`: `(int)double.PositiveInfinity` β†’ `int.MaxValue` +- `SizedItemContentView.OnMeasure()` checks `double.IsInfinity(targetHeight)` but `int.MaxValue` is not infinity +- Layout calculations fail, EmptyView doesn't display + +**Fix:** +- Adds `NormalizeDimension()` helper that converts `int.MaxValue` back to `double.PositiveInfinity` +- Applied at the point of use in `OnMeasure()`, making it defensive for all callers + +### Quality Assessment + +| Aspect | Rating | Notes | +|--------|--------|-------| +| **Correctness** | βœ… Excellent | Fix addresses root cause, tests verify behavior | +| **Test Coverage** | βœ… Good | UI test properly reproduces the bug | +| **Risk** | βœ… Low | Minimal, surgical change | +| **Code Quality** | βœ… Good | Clean helper method, proper naming | +| **Alternative Comparison** | βœ… Done | 6 alternatives explored, PR's approach is best | + +### Test Verification + +- βœ… Test PASSES with fix (EmptyView visible) +- βœ… Test FAILS without fix (TimeoutException - EmptyView never appears) + +### Minor Suggestions (Non-blocking) + +1. **Consider XML doc comment** for `NormalizeDimension()` explaining why the conversion is needed +2. **Consider adding regression test for Grid container** as reported in issue comments diff --git a/src/Controls/src/Core/Handlers/Items/Android/SizedItemContentView.cs b/src/Controls/src/Core/Handlers/Items/Android/SizedItemContentView.cs index 196d1725900f..6f9278355833 100644 --- a/src/Controls/src/Core/Handlers/Items/Android/SizedItemContentView.cs +++ b/src/Controls/src/Core/Handlers/Items/Android/SizedItemContentView.cs @@ -25,8 +25,8 @@ protected override void OnMeasure(int widthMeasureSpec, int heightMeasureSpec) return; } - double targetWidth = _width(); - double targetHeight = _height(); + double targetWidth = NormalizeDimension(_width()); + double targetHeight = NormalizeDimension(_height()); if (!double.IsInfinity(targetWidth)) targetWidth = Context.FromPixels(targetWidth); @@ -48,5 +48,7 @@ protected override void OnMeasure(int widthMeasureSpec, int heightMeasureSpec) SetMeasuredDimension((int)size.Width, (int)size.Height); } } + + static double NormalizeDimension(double value) => value == int.MaxValue ? double.PositiveInfinity : value; } } diff --git a/src/Controls/tests/TestCases.HostApp/Issues/Issue32932.cs b/src/Controls/tests/TestCases.HostApp/Issues/Issue32932.cs new file mode 100644 index 000000000000..7342521b1e4d --- /dev/null +++ b/src/Controls/tests/TestCases.HostApp/Issues/Issue32932.cs @@ -0,0 +1,61 @@ +namespace Maui.Controls.Sample.Issues; + +[Issue(IssueTracker.Github, 32932, "[Android] EmptyView doesn’t display when CollectionView is placed inside a VerticalStackLayout", PlatformAffected.Android)] + +public class Issue32932 : TestShell +{ + protected override void Init() + { + var shellContent = new ShellContent + { + Title = "Home", + Route = "MainPage", + Content = new Issue32932CollectionViewPage() { Title = "Home" } + }; + + Items.Add(shellContent); + } + + + class Issue32932CollectionViewPage : ContentPage + { + public Issue32932CollectionViewPage() + { + Title = "Issue 32932 CollectionView Page"; + + var collectionView = new CollectionView2 + { + AutomationId = "EmptyView", + ItemTemplate = new DataTemplate(() => + { + var label = new Label(); + label.SetBinding(Label.TextProperty, new Binding(".")); // bind to the string item + return label; + }), + + // EmptyView: ContentView -> VerticalStackLayout -> Label "No values found..." + EmptyView = new ContentView + { + Content = new VerticalStackLayout + { + Children = + { + new Label { Text = "No values found..." , AutomationId= "EmptyViewLabel"} + } + } + } + }; + + collectionView.ItemsSource = null; // Null collection to trigger EmptyView + + // inner stack that contains the title and the CollectionView + var innerStack = new VerticalStackLayout + { + Children = { collectionView } + }; + + Content = innerStack; + } + } +} + diff --git a/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue32932.cs b/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue32932.cs new file mode 100644 index 000000000000..6ed0e60060f5 --- /dev/null +++ b/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue32932.cs @@ -0,0 +1,27 @@ +using NUnit.Framework; +using UITest.Appium; +using UITest.Core; + +namespace Microsoft.Maui.TestCases.Tests.Issues; + +public class Issue32932 : _IssuesUITest +{ + public Issue32932(TestDevice device) : base(device) + { + } + + public override string Issue => "[Android] EmptyView doesn’t display when CollectionView is placed inside a VerticalStackLayout"; + + [Test] + [Category(UITestCategories.CollectionView)] + public void EmptyViewShouldDisplayWhenCollectionViewIsInsideVerticalStackLayout() + { + App.WaitForElement("EmptyView"); + +#if WINDOWS // EmptyView(null ItemsSource) elements Not Accessible via Automation on Windows Platform Issue Link: https://github.com/dotnet/maui/issues/28022 + VerifyScreenshot(); +#else + App.WaitForElement("EmptyViewLabel"); +#endif + } +} \ No newline at end of file