RadioButtons: Fix SelectionChanged event issues

[Felix-Dev]

Description

This PR fixes two issues with the RadioButtons control's SelectionChanged event:

• The event is no longer raised two times instead of one time when selecting a radio button by clicking while another radio button is currently selected (selecting via API or keyboard already works correctly). The raised event now contains both the deselected item and the newly selected item (if any).
• No longer are 'null' items included in the SelectionChanged.AddedItems and SelectionChanged.RemovedItems collection. If. for example, selection is cleared, SelectionChanged.AddedItems no longer contains any item (currently, it would contain a single 'null' item in this case).

The root cause for the raised duplicated SelectionChanged events is that the RadioButton control has in-built logic which automatically maintains single-selection for grouped RadioButtons. Two RadioButton controls belong to the same group if they share the same parent. With the current implementation of the RadioButtons control this is indeed the case: Each RadioButton has the same parent (the ItemsRepeater). As such, selecting a new RadioButton automatically raises the Unchecked event for the currently selected RadioButton, in turn raising the SelectionChanged event. Following this, the Checked event of the newly selected RadioButton is raised, again raising the SelectionChanged event.

This PR solves this issue by wrapping each RadioButton of the RadioButtons control in a unqiue parent grid, this ensuring that each RadioButton has a different parent. As such, selecting a RadioButton no longer automatically deselects the currently selected RadioButton. Instead, selection and deselection is now handled entirely by the RadioButtons control and thus we have full control when and how often to raise the SelectionChanged event.

Note: Listening to the RadioButton::Click event instead of the Checked and Unchecked events is not an option here as that event does not handle all possible selection cases. For example, programmatic selection of a RadioButton would be no longer recognized then, thus breaking the RadioButtons control.

The added interaction test to to verify if the SelectionChanged event is raised only once per selection logs each raised SelectionChangd event and then counts the number of these created logs per selection. If there is a mismatch (so SelectionChanged event has been raised anything else than exactly one time), verification fails.

The behavior of not passing 'null' elements to the SelectionChanged.*Items collections is covered by the added API test. Since there was no existing RadioButtons' APITests project, this PR is creating one.

Motivation and Context

Fixes #3128 and fixes #3268.

How Has This Been Tested?

Added API and interaction tests.

[marcelwgn]

Strangely, this test does not show up in the Test Explorer on my machine, even after rebuilding. If it shows up on your machine, this might be just some random issue with VS and we can just accept that.

[Felix-Dev]

Generally, I am having troubles getting newly created APITests projects to work. I always dread having to create new ones here as I will almost every time be unable to actually execute the test project (VS indicated the USING_TAEF flag is set and this breaks the whole APITests project for me). It then takes multiple VS closes, re-builds, running API tests from other, already existing APITests projects etc...until finally I can properly test my added APITests project properly. (It really is more like me trying out things rather randomly in the hope that at some point I end up "lucky" and suddenly I can build and run the new APITests project.)

Rest assured that I ran this test in VS locally here once I finally managed to get it up successfully and verified that it fails without the supplied fix and and runs fine with the fix supplied.

[StephenLPeters]

Yeah, I don't understand why its so hard/nebulus to get this set up properly.. You can run the tests using Taef instead though which ends up being easier than MSTest via VS a lot of the time

[Felix-Dev]

@chingucoding I noticed that if the APITests do not show up initially, I can run the APITest "LocalizationTests" which seems to always show up. While it will probably fail, after running that test, the API tests for the control in question do show up.

If you haven't done that yet, you could see if you get the same result.

[marcelwgn]

That worked for me, awesome! Thanks for sharing that, that makes it definitely easier to get new API test projects running.

[StephenLPeters]

weird.. @kmahone

[ranjeshj]

Is this an issue with Visual studio's test explorer not identifying the correct set of tests ? I'm curious if our build is not touching the correct files for VS to pick it up. Do you see it for all builds or just incremental builds. Luke fixed an issue with incremental builds recently.

[marcelwgn]

I'm inclined to say it is more about test detection. Even after rebuilding a project (deleting BuildOutput folder), the new tests don't show up all the time.

[StephenLPeters]

@Felix-Dev can you merge master to pick up the CI build fix that just went in?

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

@Felix-Dev The pipeline fix just went in, could you please merge master into this PR?

[Felix-Dev]

@StephenLPeters Since this PR is currently is waiting on PR #3194 I would like to wait for that PR to to be merged with master before merging with master here (this PR in its current form doesn't need to be run in the test pipeline anyway as its far from complete). Does that work for you?

[StephenLPeters]

yup makes sense

[Felix-Dev]

@StephenLPeters Merged with master, added grid wrapping logic, improved tests.

[StephenLPeters]

In general I think it is good to cast your object to they widest type that fulfills your needs, for instance cast to toggle button instead of RadioButton when you are calling Check(). Or casting to Control instead of RadioButton when you want to call focus. In general I think this strategy tends to enable more retemplating scenarios. What if a consumer want to use RadioButtons with their CustomSuperCoolRadioButton which derives from toggle button.

[Felix-Dev]

What if a consumer want to use RadioButtons with their CustomSuperCoolRadioButton which derives from toggle button.

Given the current implementation, we would wrap that toggle button, which is not a RadioButton, in a RadioButton then:

microsoft-ui-xaml/dev/RadioButtons/RadioButtonsElementFactory.cpp

Line 39 in dc08d9b

if (auto const radioButton = newContent.try_as<winrt::RadioButton>())

So when our RepeaterElementFactory instance is used here, we know that every item is actually a RadioButton here (ignoring its wrapping Grid element for now).

A developer could, however, pass a custom ItemTeplate to the ItemsRepeater used by a RadioButtons control which would use ToggleButtons instead of RadioButtons. Given that, we should probably allow the widest type possible here (which still makes sense) which would be a ToogleButton control.

[StephenLPeters]

Yeah, I think it would likely be fine to leave as is for radio buttons, but I don't think I see a downside of changing this and do see a minor upside.

[Felix-Dev]

@StephenLPeters

In general I think it is good to cast your object to they widest type that fulfills your needs, for instance cast to toggle button instead of RadioButton when you are calling Check(). Or casting to Control instead of RadioButton when you want to call focus.

See this case:

if (auto const radioButton = TryGetRadioButtonFromRepeaterElementIndex(focusedIndex, repeater))
{
    if (radioButton.Focus(winrt::FocusState::Programmatic))
    {
        return true;
    }
}

but there is also

if (auto const radioButton = TryGetRadioButtonFromRepeaterElementIndex(index, repeater))
{
    radioButton.IsChecked(containerIsChecked);
}

I guess I could change the return value TryGetRadioButtonFromRepeaterElementIndex() from type ToggleButton to, say, UIElement and then cast it in the above cases to either a Control or a ToggleButton but then I perhaps should change the method name (since it wouldn't return a "RadioButton" any longer but an "UIElement"). And I kinda prefer it named this way because it helps making it clear that we are actually working with RadioButtons here (wrapped inside a Grid). What do you think?

[StephenLPeters]

Maybe TryGetRadioButtonAsUIElement, You probably don't need the FromRepeaterElementIndex part because its implied by the function parameters?

[Felix-Dev]

That could work, yeah.

[Felix-Dev]

@StephenLPeters I updated the implementation to make use of the widest types as possible (it now resembles the implementation before this PR). Please take a look.

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

@Felix-Dev looks like a real test failure.

[Felix-Dev]

@StephenLPeters Failing tests all run fine for me:

Looking at the Test pipeline output, for all failing tests the logged error is the same:

[Error]: System.Runtime.InteropServices.COMException (0x80040201): An event was unable to invoke any of the subscribers (Exception from HRESULT: 0x80040201)    at UIAutomationClient.IUIAutomationTreeWalker.GetFirstChildElementBuildCache(IUIAutomationElement element, IUIAutomationCacheRequest cacheRequest)

I fired up Narrator just to see if I notice anything but all seems fine to me. Any thoughts here?

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[marcelwgn]

@Felix-Dev Looks like there are some merge conflicts.

[Felix-Dev]

@chingucoding If I merge with master now, I won't be able to run interaction tests in my local branch for this PR any longer since PR #3359 hasn't yet been merged. I would like to wait until that PR has been merged so that in the meantime, if required, I can look into the failing interaction tests (in the test pipeline) again and won't be blocked here.

Thanks for the ping though.

[Felix-Dev]

Merged with master.

[ranjeshj]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[gabbybilka]

Thank you for submitting this PR, the current focus of the team is to assist our users in moving from WinUI 2 to WinUI 3. We are closing this PR as it does not address resolving a critical issue for WinUI 2 or WinUI 3.