Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix interaction of RadioButton, shared bindings and hanging GC refs #10252

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

h3xds1nz
Copy link
Contributor

@h3xds1nz h3xds1nz commented Jan 10, 2025

Fixes #9035
Fixes #2995

Marking it as draft right now due to the changes from #10251 being included. Only the latest commit is relevant.

Description

This fixes actually two things, not just one and I'm fine with only fixing the #2995 / #9035 if desirable.

First fix

First fix resolves #2995 / #9035 report that was also attempted in #8202 but was reverted in #10105 due to obvious issues.
Original method compares the visual root that's found using the following:

DependencyObject rootScope = KeyboardNavigation.GetVisualRoot(this);
DependencyObject otherRoot = KeyboardNavigation.GetVisualRoot(radioButton);

The problem being that this search will result in null on Popups (hence why the original fix caused #10087), it will return null on radio buttons with closed Windows and depending on the focus situation it may return null on a fresh window.

Using InputElement.GetRootVisual instead we will retrieve the proper visual roots for Popups (PopupRoot), for open windows right after creation and for non-GCed RadioButton's but detached from their visual root it will retrieve the container.

Second fix/improvement

The original issue attempts to share a binding between two distincts visual roots using a singleton instance, which works until you have a hanging not-yet GCed RadioButton due to the reasons described in the first fix.

BindingExpression rootBindingSource = GetBindingExpression(IsCheckedProperty);
BindingExpression otherBindingSource = radioButton.GetBindingExpression(IsCheckedProperty);
if (rootBindingSource is not null && otherBindingSource is not null &&
    rootBindingSource.SourceItem == otherBindingSource.SourceItem &&
    rootBindingSource.SourcePropertyName == otherBindingSource.SourcePropertyName)
    continue;

Excuse the checking verbosity, however, this would allow for shared bindings (e.g. here - #2995 (comment)) to continue working properly even under the same visual root instead of just bound property being always set to false. All while retaining the usual RadioButton behavior and functionality. Can be simulated in updated repro with more stuff here - RadioButton.GroupName.Tests. You'll have to update the References though.

Customer Impact

Resolving long standing issues concerning RadioButton and bindings.

Regression

No.

Testing

Local build, extensive testing with multiple reproduction samples.

Risk

More testing is needed of course, but it would be a nice improvement to have this working. The second fix changes behavior, but changes it from non-working and appearing buggy to actually working nicely.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners January 10, 2025 02:23
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Jan 10, 2025
@h3xds1nz h3xds1nz marked this pull request as draft January 10, 2025 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution A label for all community Contributions draft PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
1 participant