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

RadioButtons with the same GroupName will affect each other over windows if window is closed #2995

Closed
walterlv opened this issue May 14, 2020 · 17 comments
Assignees

Comments

@walterlv
Copy link
Contributor

walterlv commented May 14, 2020

  • .NET Core Version: 3.1.202
  • Windows version: 10.0.18363 / 10.0.19624
  • Does the bug reproduce also in WPF for .NET Framework 4.8?: Yes

Problem description:

If some RadioButtons are in the same group, they should check and uncheck each other. If we create two windows both contain the same XAML RadioButton groups, we'll find that the RadioButtons are separated into two windows. However, if one window is closed, the RadioButtons in the other window will affect those in the closed one.

This code below shows how to set the GroupName, but it's recommended to read the minimal reproduction at the end of this issue to know all the details.

<RadioButton GroupName="A" Content="Option 1" />
<RadioButton GroupName="A" Content="Option 2" />
<RadioButton GroupName="A" Content="Option 3" />

By running the minimal reproduction source code, you'll see the expected behavior and the broken behavior and I've posted the gifs below.

Expected behavior:

2020-05-14-radio-buttons-in-opened-windows

Actual behavior:

2020-05-14-radio-buttons-in-closed-windows

Minimal repro:

The GitHub repo:

The keys:

  1. Write some RadioButtons in a window XAML;
  2. We want to know whether the RadioButtons in different windows affect each other, so I bind the first RadioButton.IsChecked to a singleton. This means:
    • If the radio buttons affect only in a single window, the A window checks a radio button then the binding source viewmodel propertychanged, and then the B window checks a radio button due to the viewmodel new property value.
    • But if the radio buttons affect over windows, the A window checks a radio button then it change the binding source viewmodel and then checks the B window radio button. But the radio button in B window unchecks the radio button in A window, so the binding source viewmodel property is changed to false. At last, all values are false. As a result, no radio button can be checked.

If all the windows are open, all behave as expected:

image

If the previous windows are closed, then the radio buttons start to affect each other:

image

The XAML code:

<Window x:Class="Walterlv.Issues.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
        xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
        xmlns:local="clr-namespace:Walterlv.Issues"
        mc:Ignorable="d"
        Title="MainWindow" Height="450" Width="800">
    <StackPanel>
        <Border>
            <RadioButton GroupName="A" IsChecked="{Binding Bar, Source={x:Static local:Foo.Instance}}" Content="Option 1" />
        </Border>
        <Border>
            <RadioButton GroupName="A" Content="Option 2" />
        </Border>
        <Border>
            <RadioButton GroupName="A" Content="Option 3" />
        </Border>
    </StackPanel>
</Window>

The C# code:

using System.ComponentModel;
using System.Windows;

namespace Walterlv.Issues
{
    public partial class MainWindow : Window
    {
        public MainWindow()
        {
            InitializeComponent();
        }
    }

    public class Foo : INotifyPropertyChanged
    {
        public static Foo Instance { get; } = new Foo();

        public bool Bar
        {
            get => _bar;
            set
            {
                if (!Equals(_bar, value))
                {
                    _bar = value;
                    PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Bar)));
                }
            }
        }

        private bool _bar;
        public event PropertyChangedEventHandler PropertyChanged;
    }
}
@walterlv
Copy link
Contributor Author

From the open-source codes, we can find that the radio button auto checks the focus root as the group name scope:

KeyboardNavigation.GetVisualRoot(this)

@walterlv
Copy link
Contributor Author

Before the issue is fixed, we can try to solve it by adding a MarkupExtension to generate the GroupName with a random name. So different window instances have their own GroupNames and they will not affect each other over windows.

<StackPanel local:RadioButtonFix.IsScope="True">
    <Border>
        <RadioButton GroupName="{local:RadioButtonFix A}" Content="Option 1" />
    </Border>
    <Border>
        <RadioButton GroupName="{local:RadioButtonFix A}" Content="Option 2" />
    </Border>
    <Border>
        <RadioButton GroupName="{local:RadioButtonFix A}" Content="Option 3" />
    </Border>
</StackPanel>

@weltkante
Copy link

Something must be wrong with your setup, if I add RadioButtons with GroupName to a window and then create two instances of the same window (which obviously have the same group name since they have the same XAML) they don't affect each other. I'll have a look at your repro scenario.

@walterlv
Copy link
Contributor Author

@weltkante Yes, according to your description, they actually don't affect each other. In my scene, you should:

  1. Close one window (IMPORTANT);
  2. Bind the radio buttons to a singleton to check that that are affected each other.

@weltkante
Copy link

weltkante commented May 14, 2020

Ok I think I understood what you are reporting, your title is misleading. GroupName will not affect other windows.

  • You are sharing the binding across windows (intentionally I assume)
  • Closing the window does not disconnect the binding
  • RadioButtons connected via GroupName in a closed window don't seem to function properly
  • This leads to some kind of feedback loop through the closed window, preventing the open window of functioning correctly

I agree that there's a bug somewhere, but I'm not sure its about GroupName, it might also be about bindings.

Another option to work around is adding a GC.Collect(); somewhere (e.g. in your singleton property accessor, or with delay after window close) and observe the bug no longer reproducing. (Because the closed windows get collected and this disconnects the binding.)

@weltkante
Copy link

weltkante commented May 14, 2020

You are right the bug is in RadioButton.UpdateRadioButtonGroup. It doesn't need closed windows to trigger either, any window (or other control) currently not connected to a visual root will trigger the bug.

private void Button_Click(object sender, RoutedEventArgs e)
{
    new MainWindow().Show();
    new MainWindow();
    new MainWindow();
}

As such GC.Collect(); doesn't always help as someone may hold disconnected UI elements in a cache. If such disconnected UI elements contain grouped radio buttons they'll also trigger the bug, it doesn't need a Window, its just convenient for reproduction.

I'd summarize it as this: All disconnected UI elements share the same radio button group scope, which leads to bugs when disconnected UI is still connected via bindings. Closed windows act as disconnected UI, but they are not the only case.

@walterlv
Copy link
Contributor Author

@weltkante Thanks for your efforts to simplify this issue and to hit the critical codes.

@xinyuehtx
Copy link

xinyuehtx commented May 14, 2020

临时解决方案

public static class GroupNameProvider
    {
        public static readonly DependencyProperty BuildScopeProperty = DependencyProperty.RegisterAttached(
            "BuildScope", typeof(bool), typeof(GroupNameProvider), new PropertyMetadata(PropertyChangedCallback));

        private static void PropertyChangedCallback(DependencyObject d, DependencyPropertyChangedEventArgs e)
        {
            if (e.NewValue is bool value && value == true)
            {
                SetGroupNameA(d, Guid.NewGuid().ToString("N"));
            }
        }

        public static void SetBuildScope(DependencyObject element, bool value)
        {
            element.SetValue(BuildScopeProperty, value);
        }

        public static bool GetBuildScope(DependencyObject element)
        {
            return (bool) element.GetValue(BuildScopeProperty);
        }

        public static readonly DependencyProperty GroupNameAProperty = DependencyProperty.RegisterAttached(
            "GroupNameA", typeof(string), typeof(GroupNameProvider), new PropertyMetadata(default(string)));

        private static void SetGroupNameA(DependencyObject element, string value)
        {
            element.SetValue(GroupNameAProperty, value);
        }

        public static string GetGroupNameA(DependencyObject element)
        {
            return (string) element.GetValue(GroupNameAProperty);
        }
    }
<Grid x:Name="Panel" local:GroupNameProvider.BuildScope="True">
    <RadioButton IsChecked="{Binding Foo}"
      GroupName="{Binding ElementName=Panel,Path=(local:GroupNameProvider.GroupNameA)}"/>
</Grid>

blog

@NaBian
Copy link
Contributor

NaBian commented May 14, 2020

simplified your demo:

<StackPanel>
    <Border>
        <RadioButton GroupName="A" IsChecked="{Binding Bar, Source={x:Static local:Foo.Instance}}" Content="Option 1" />
    </Border>
    <Border>
        <RadioButton GroupName="A" Content="Option 2" />
    </Border>
    <Border>
        <RadioButton GroupName="A" Content="Option 3" />
    </Border>
</StackPanel>
<StackPanel>
    <Border>
        <RadioButton GroupName="A" IsChecked="{Binding Bar, Source={x:Static local:Foo.Instance}}" Content="Option 1" />
    </Border>
    <Border>
        <RadioButton GroupName="A" Content="Option 2" />
    </Border>
    <Border>
        <RadioButton GroupName="A" Content="Option 3" />
    </Border>
</StackPanel>

just open window one time

@weltkante
Copy link

weltkante commented May 14, 2020

Yes this is what happens, but when you do it intentionally its not a bug.

The point of the bug report is that everything not attached to a visual root will start sharing their radiobutton groups. So while you have two windows open they can use the same radiobutton groupnames and they will be distinct, but if you close/hide them they start to share their groups. If the radiobuttons were databound this will lead to a feedback loop.

The important part of the repro is, that it initially works (groups are distinct), and only starts breaking if you start closing windows, because then they change into the broken state.

@walterlv
Copy link
Contributor Author

@NaBian Your simpler code really describes the reason for this issue, but not describes the pain for this issue. Also @weltkante .

If radio buttons with the same group names affect each other even across different windows, then we have to give up the embedded group name feature because our visible radio buttons may be influenced by the ones which should have been destroyed.

This means we have to use the method from @xinyuehtx for nearly every radio button with group names if our programs have to open different window instances of one window class.

@RevitArkitek
Copy link

I have this same issue and it took me days to figure out what was wrong. In my case I have a UserControl that uses GroupName to control RadioButton grouping in a Grid (I can't add them to StackPanel or GroupBox for graphical reasons). When I duplicate an object, I remove the UserControl from my window and create a new one to show the duplicate. I then modify the radiobuttons in the duplicate. If I then go to the original object, I see my radiobuttons update to match the properties. But when I go back to the duplicate, the radiobuttons have changed to match the original object.

@harshit7962 harshit7962 self-assigned this Aug 31, 2023
@harshit7962
Copy link
Member

harshit7962 commented Sep 5, 2023

Analysis so far - While registering the radio buttons, the reference to them in hashtable is added as a weak reference which should allow the GC to collect it when the window is closed. However, it looks like there is some other pointer strongly referencing the radiobutton even after the window is closed (which is why it is still not null in this check).

Continuing to investigate where is the reference being held for the radio button after the window is closed.

@xinyuehtx
Copy link

xinyuehtx commented Sep 5, 2023 via email

@harshit7962
Copy link
Member

@walterlv We have merged the above PR which fixes the issue. Can you verify if it solves the issue.

@harshit7962 harshit7962 added 📭 waiting-author-feedback To request more information from author. and removed Bug Product bug (most likely) labels Oct 10, 2023
@lindexi
Copy link
Member

lindexi commented Oct 10, 2023

Awesome @harshit7962

@walterlv
Copy link
Contributor Author

@harshit7962 I've tested it on .NET 8 rc2 and find that it is fixed. Thank you!

@ghost ghost removed the 📭 waiting-author-feedback To request more information from author. label Oct 11, 2023
@ghost ghost removed this from the Future milestone Oct 11, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants