[Issue-Resolver] Fix #33264 - RadioButtonGroup not working with Collection View

[kubaflo]

Fix #33264 - RadioButtonGroup not working with CollectionView

Fixes #33264

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!

Summary

Fixed RadioButtonGroup.SelectedValue binding not working when RadioButtons are inside a CollectionView's ItemTemplate while RadioButtonGroup attached properties are set on a parent layout outside the CollectionView.

Quick verification:

• ✅ Tested on Android - Issue resolved, all tests passing
• ✅ Tested on iOS - Issue resolved (user verified)
• ✅ Edge cases tested
• ✅ UI tests added and passing

Root Cause

RadioButtonGroupController only registered RadioButtons through ChildAdded events. When RadioButtons were dynamically created inside a CollectionView's ItemTemplate, they never triggered ChildAdded events on the parent layout where RadioButtonGroup.GroupName and RadioButtonGroup.SelectedValue were set.

Without registration, when a RadioButton was checked, GetGroupController() returned null, so HandleRadioButtonGroupSelectionChanged() was never called and the SelectedValue binding was never updated.

Solution

Enhanced RadioButton to actively search up the visual tree for a matching RadioButtonGroup controller when checked, rather than relying solely on event-based registration:

Key Changes:

1. RadioButton.OnGroupSelectionChanged() - If no controller registered, searches ancestors for matching group and registers dynamically

2. RadioButton.FindControllerInAncestors() - Walks up visual tree to find layout with matching RadioButtonGroup.GroupName

3. RadioButtonGroupController.RegisterRadioButton() - Public method to register RadioButton with controller

4. RadioButtonGroup.GetController() - Exposes controller access for RadioButton

5. RadioButtonGroupController.SetSelectedValue() and SetGroupName() - Enhanced to proactively register discovered RadioButtons

Files Changed:

• src/Controls/src/Core/RadioButton/RadioButton.cs - Added ancestor search logic (+33 lines)
• src/Controls/src/Core/RadioButton/RadioButtonGroupController.cs - Added dynamic registration (+55 lines)
• src/Controls/src/Core/RadioButton/RadioButtonGroup.cs - Exposed controller access (+5 lines)

Testing

Before fix (Android):

• Initial SelectedValue: ''
• After tapping 'Choice 2', SelectedValue: '' ❌

After fix (Android):

• Initial SelectedValue: 'None'
• After tapping 'Choice 2', SelectedValue: 'Choice 2' ✅
• After tapping 'Choice 3', SelectedValue: 'Choice 3' ✅
• Test Run Successful. Total tests: 1 Passed: 1

After fix (iOS):

• User verified - working correctly ✅

Edge Cases Tested:

• ✅ RadioButtons in CollectionView DataTemplate - Primary scenario, now working
• ✅ Multiple radio button selections - Sequential taps update binding correctly
• ✅ Binding updates in both directions - Property changes verified

Platforms Tested:

• ✅ Android - All tests passing
• ✅ iOS - User verified working

Test Coverage

• ✅ Test page: src/Controls/tests/TestCases.HostApp/Issues/Issue33264.xaml
• ✅ Code-behind: src/Controls/tests/TestCases.HostApp/Issues/Issue33264.xaml.cs
• ✅ NUnit test: src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33264.cs
• ✅ Tests exact scenario from issue report

Breaking Changes

None - the fix adds proactive discovery without changing existing behavior for scenarios that already work.

[kubaflo]

https://github.com/user-attachments/files/24301852/chat.json

[Copilot]

Pull request overview

This PR fixes a bug where RadioButtonGroup.SelectedValue binding fails when RadioButtons are inside a CollectionView's ItemTemplate. The issue occurred because RadioButtons in templated contexts never triggered ChildAdded events on the parent layout where RadioButtonGroup properties were set, preventing controller registration.

Key Changes:

• Added dynamic controller discovery when RadioButtons are checked without registration
• Enhanced RadioButtonGroupController to proactively register RadioButtons found via tree traversal
• Exposed controller access via RadioButtonGroup.GetController() for RadioButton to use

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
Issue33264.cs	NUnit UI test verifying RadioButtonGroup binding works in CollectionView
Issue33264.xaml.cs	Test page code-behind with ViewModel and state logging
Issue33264.xaml	Test page XAML demonstrating RadioButtons in CollectionView DataTemplate
RadioButtonGroupController.cs	Added RegisterRadioButton(), dynamic discovery in SetSelectedValue()/SetGroupName()
RadioButtonGroup.cs	Exposed GetController() for RadioButton access
RadioButton.cs	Added FindControllerInAncestors() and dynamic registration in OnGroupSelectionChanged()

Comments suppressed due to low confidence (1)

src/Controls/src/Core/RadioButton/RadioButtonGroupController.cs:220

• Calling Descendants() on the layout in SetSelectedValue() can be expensive, especially when called frequently during binding updates. Since this method is called every time SelectedValue changes, repeatedly traversing the entire visual tree to discover and register RadioButtons could impact performance. Consider caching discovered RadioButtons or using a more targeted approach when the CollectionView's items change, rather than scanning the entire tree on every value change.

			foreach (var child in _layout.Descendants())
			{
				if (child is RadioButton radioButton && radioButton.GroupName == _groupName)
				{
					// Register the RadioButton if not already registered
					if (!groupControllers.TryGetValue(radioButton, out _))
					{
						groupControllers.Add(radioButton, this);
					}

					if (radioButtonValue is not null)
					{
						if (radioButton.Value is not null && radioButton.Value.Equals(radioButtonValue))
						{
							radioButton.SetValue(RadioButton.IsCheckedProperty, true, specificity: SetterSpecificity.FromHandler);
						}
					}
					else
					{
						// Setting null - uncheck the selected radio button in the group
						if (radioButton.IsChecked)
						{
							radioButton.SetValue(RadioButton.IsCheckedProperty, false, specificity: SetterSpecificity.FromHandler);
						}
					}
				}
			}

[Copilot]

Calling Descendants() in SetGroupName() after every group name change is expensive and can impact performance. This method traverses the entire visual tree to discover RadioButtons. Consider whether this discovery needs to happen on every group name change, or if it could be optimized by only scanning when necessary (e.g., when the group name is initially set or when known structural changes occur).

[Copilot]

Using Task.Delay().Wait() with a hardcoded delay is not a reliable approach for UI testing. This pattern is discouraged in the MAUI test infrastructure. Instead, rely on App.WaitForElement() which has built-in retry logic. If you need additional time for rendering, App.WaitForElement() already handles this with its timeout mechanism.

[Copilot]

Using Task.Delay().Wait() for hardcoded wait times is unreliable for UI tests. App.WaitForElement() already includes appropriate timeout logic. Remove this line and rely on the WaitForElement calls to handle timing.

[Copilot]

Using Task.Delay().Wait() for hardcoded wait times is unreliable for UI tests. App.WaitForElement() already includes appropriate timeout logic. Remove this line and rely on the WaitForElement calls to handle timing.

[PureWeen]

Thoughts?

Review Feedback: PR #33268 - RadioButtonGroup not working with CollectionView

🤖 Copilot Session Summary

This review was conducted through an interactive deep-dive session with the following prompts:

1. "I want to do a deep review of PR 33268 can you please 1) analyze the fix 2) see if you can think of alternative fixes? 3) test those fixes against the UITests that were added"

   • Analyzed the PR's code changes and root cause
   • Identified the regression from MessagingCenter removal in .NET 10 Preview 5
   • Proposed two alternative fixes with different trade-offs

2. "Feel free to test all your alternatives to see if they fix the issue in a better way. It's fine to assume the fix on the PR is wrong and you need to start from your own solution. Exhaust your alternatives."

   • Reverted PR fix and validated UI tests fail correctly without fix
   • Implemented and tested Alternative 1 (Minimal Ancestor Search - 44 lines)
   • Implemented and tested Alternative 2 (DescendantAdded event - 14 lines)
   • Both alternatives passed the same UI tests as the PR's fix

3. "Please review the UITests on this PR and see if you think they provide adequate coverage. Please add any additional UITests. Please revert the changes on the PR that fix the problem and validate that the UITests correctly fail with those changes reverted."

   • Reviewed existing UI test coverage
   • Attempted to add edge case tests (discovered issue number conflict causing app crash)
   • Validated tests correctly fail without fix and pass with fix

4. "So all of the UITests added on this PR pass with your suggested changes?"

   • Confirmed Alternative 2 (DescendantAdded) passes PR's UI tests with only 14 lines of changes

Recommendation

⚠️ Request Changes

Required changes:

1. Consider using the simpler DescendantAdded approach (14 lines) instead of the current ancestor-search approach (93 lines)

Recommended changes:

1. Remove redundant registration code from SetSelectedValue() and SetGroupName() if keeping current approach
2. Add null check for parent as BindableObject in FindControllerInAncestors() to prevent potential null reference

---

📋 Full PR Review Details

Summary

PR #33268 fixes a regression where RadioButtonGroup.SelectedValue binding doesn't work when RadioButtons are inside a CollectionView's ItemTemplate. The fix is functionally correct but over-engineered. I identified two simpler alternatives that achieve the same result with significantly less code.

Root Cause Analysis

The regression was introduced in commit 6a96e92d50 when MessagingCenter was removed from RadioButtonGroup in .NET 10 Preview 5.

Why it worked before (MessagingCenter approach):

• RadioButtons broadcast selection changes via MessagingCenter
• Any layout with matching GroupName could receive and process the message
• Decoupled from parent-child hierarchy

Why it broke (ConditionalWeakTable approach):

• RadioButtons are registered via ChildAdded events on the parent layout
• CollectionView creates RadioButtons inside virtualized containers
• These RadioButtons are NOT direct children of the layout where RadioButtonGroup.GroupName is set
• ChildAdded only fires when CollectionView is added, not when items inside it are created

Fix Comparison

Approach	Lines Changed	Complexity	Pros	Cons
PR's Fix	93 lines	High	Comprehensive	Redundant code, DRY violation
Alternative 1 (Ancestor Search)	44 lines	Medium	Lazy registration	Runs on every selection
Alternative 2 (DescendantAdded)	14 lines	Low	Proactive, minimal	Event overhead

PR's Approach (93 lines)

Adds ancestor search in OnGroupSelectionChanged() + redundant registration in 4 other places:

• RegisterRadioButton()
• HandleRadioButtonGroupSelectionChanged()
• SetSelectedValue()
• SetGroupName()

Alternative 1: Minimal Ancestor Search (44 lines)

Only adds ancestor search in OnGroupSelectionChanged(). Registration happens lazily when RadioButton is checked.

// In RadioButton.cs
RadioButtonGroupController FindAndRegisterWithAncestorController()
{
    var parent = this.Parent;
    while (parent != null)
    {
        if (parent is BindableObject bindable)
        {
            var parentGroupName = RadioButtonGroup.GetGroupName(bindable);
            if (parentGroupName == this.GroupName)
            {
                var controller = RadioButtonGroup.GetController(bindable);
                if (controller != null)
                {
                    RadioButtonGroupController.RegisterRadioButton(this, controller);
                    return controller;
                }
            }
        }
        parent = parent.Parent;
    }
    return null;
}

Alternative 2: DescendantAdded Event (14 lines) - RECOMMENDED

Subscribe to DescendantAdded event which fires for ALL descendants at any depth:

// In RadioButtonGroupController constructor
_layout.DescendantAdded += DescendantAdded;

// New handler
void DescendantAdded(object sender, ElementEventArgs e)
{
    if (string.IsNullOrEmpty(_groupName) || _layout == null)
        return;

    if (e.Element is RadioButton radioButton)
        AddRadioButton(radioButton);
}

Why Alternative 2 is best:

• Minimal code change (14 lines in 1 file)
• Uses existing infrastructure (DescendantAdded event)
• Proactive registration (RadioButtons registered as soon as added)
• No ancestor walking needed
• No redundant registration code

Test Coverage

The PR includes a UI test that properly validates the fix:

• ✅ Tests selection change updates binding
• ✅ Tests multiple selection changes
• ✅ Test correctly fails without fix, passes with fix

Testing Results

Test	Without Fix	With PR Fix	Alt 1	Alt 2
RadioButtonGroupBindingWorksInsideCollectionView	❌ FAIL	✅ PASS	✅ PASS	✅ PASS

Platform: iOS (iPhone Xs, iOS 18.5)

Issues Found

Must Fix

1. Redundant registration code - The same registration logic appears in 5 places, violating DRY principle

Should Fix

1. Potential NullReferenceException in FindControllerInAncestors():

   var parentGroupName = RadioButtonGroup.GetGroupName(parent as BindableObject);

   If parent is not a BindableObject, this passes null to GetGroupName(), which may throw.

2. Missing DescendantRemoved handler - If using DescendantAdded, should also handle DescendantRemoved for cleanup (though not strictly necessary due to ConditionalWeakTable)

Approval Checklist

• Code solves the stated problem
• Minimal, focused changes (over-engineered)
• Appropriate test coverage
• No security concerns
• Follows .NET MAUI conventions

Recommendation

Replace the current implementation with Alternative 2 (DescendantAdded). It:

• Reduces code from 93 lines to 14 lines
• Eliminates redundant registration code
• Uses existing MAUI infrastructure
• Is easier to maintain and understand

Review Metadata

• Reviewer: PR Review Agent
• Date: 2025-12-26
• PR: #33268
• Issue: #33264
• Platforms Tested: iOS

[PureWeen]

/rebase

[PureWeen]

could this be a unit test vs a UITest?

[kubaflo]

Why a Unit Test Would Fail

A unit test like this would not work:

 [Test]
 public void RadioButtonGroupWithCollectionView_UnitTest()
 {
     var stackLayout = new StackLayout();
     RadioButtonGroup.SetGroupName(stackLayout, "choices");
     
     var collectionView = new CollectionView();
     stackLayout.Children.Add(collectionView);
     
     // ❌ This would fail - no template rendering happens in unit tests
     collectionView.ItemsSource = new[] { "Choice 1", "Choice 2" };
     
     // ❌ RadioButtons are never created - ItemTemplate doesn't render
     // ❌ DescendantAdded events never fire
 }

The current UI test is correctly implemented and necessary because it
validates the full rendering pipeline and event system that the fix depends
on.

[PureWeen]

Why a Unit Test Would Fail

A unit test like this would not work:

 [Test]
 public void RadioButtonGroupWithCollectionView_UnitTest()
 {
     var stackLayout = new StackLayout();
     RadioButtonGroup.SetGroupName(stackLayout, "choices");
     
     var collectionView = new CollectionView();
     stackLayout.Children.Add(collectionView);
     
     // ❌ This would fail - no template rendering happens in unit tests
     collectionView.ItemsSource = new[] { "Choice 1", "Choice 2" };
     
     // ❌ RadioButtons are never created - ItemTemplate doesn't render
     // ❌ DescendantAdded events never fire
 }

The current UI test is correctly implemented and necessary because it validates the full rendering pipeline and event system that the fix depends on.

yea I think we could still fake the templates getting created or possibly use a different view with the same characteristics?

[kubaflo]

Reopening

[kubaflo]

I can't reopen so I've created another one #33343