Fix issue where icon on RadioMenuFlyout and ToggleMenuFlyout was not being displayed

[marcelwgn]

Description

Update templates to contain the icon viewbox and update visual states to update the state correctly.

Motivation and Context

Closes #4473

How Has This Been Tested?

Tested manually.

Screenshots (if appropriate):

[ranjeshj]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ranjeshj]

@chingucoding can you share pics of what the states look like now. Also, can you take a quick look at the original PR and see if we are missing any other template parts ? Thanks!

[marcelwgn]

@chingucoding can you share pics of what the states look like now. Also, can you take a quick look at the original PR and see if we are missing any other template parts ? Thanks!

Screenshots:

The only thing missing in the templates are the "KeyboardAcceleratorTextBlock", however I am not sure if this was a deliberate choice removing them or not. Maybe @tashatitova knows more about this?

[ranjeshj]

@chingucoding can you share pics of what the states look like now. Also, can you take a quick look at the original PR and see if we are missing any other template parts ? Thanks!

Screenshots:

The only thing missing in the templates are the "KeyboardAcceleratorTextBlock", however I am not sure if this was a deliberate choice removing them or not. Maybe @tashatitova knows more about this?

I don't think that is deliberate. We should add that back.

[marcelwgn]

Readded them now @ranjeshj .

[ranjeshj]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ranjeshj]

@chingucoding can you share pics once more with the possible combinations ? @YuliKl @marksfoster can you double check this is good ?

[marcelwgn]

Sure Ranjesh, here more elaborate screenshots:

[ranjeshj]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[teaP]

When I look at the generic.xaml version of the styles for MenuFlyoutItem and ToggleMenuFlyoutItem, they look much more up to date than what we currently have in here (e.g. they have Setters instead of these archaic Storyboards, the keyboard accelerator is in there, etc.). I'm not sure what the history is here, but it looks to me like both of these styles actually need to be updated to what generic.xaml has, minus reveal. Is that something you'd want to take on?

[marcelwgn]

#3958 deliberately changed the setters to objectanimations, that's why I thought that this would be the new way moving forward for these styles. So what is the correct way of doing this now? Were the changes of #3958 "incorrect" then @teaP ?

Edit: Looks like that there only was a movement of code, not replacement. None the less, what is the correct way here then?

[teaP]

#3958 deliberately changed the setters to objectanimations, that's why I thought that this would be the new way moving forward for these styles. So what is the correct way of doing this now? Were the changes of #3958 "incorrect" then @teaP ?

Edit: Looks like that there only was a movement of code, not replacement. None the less, what is the correct way here then?

We definitely should stick with setters anywhere there's no actual animation. I should have looked at #3958 more closely :P especially since it looks like we lost some things that you're adding back now.

[marcelwgn]

We definitely should stick with setters anywhere there's no actual animation. I should have looked at #3958 more closely :P especially since it looks like we lost some things that you're adding back now.

Well updated the menu flyout item styles now holistically to use setters where I was confident we can use a setter instead!

Out of curiosity, @teaP do you know why animations were used in the first place (as in why were those chosen few years ago instead of setters)?

[teaP]

We definitely should stick with setters anywhere there's no actual animation. I should have looked at #3958 more closely :P especially since it looks like we lost some things that you're adding back now.

Well updated the menu flyout item styles now holistically to use setters where I was confident we can use a setter instead!

Out of curiosity, @teaP do you know why animations were used in the first place (as in why were those chosen few years ago instead of setters)?

Thanks! Animations were historically used because VisualState.Setters didn't used to exist. :) It was added in TH1.

[marcelwgn]

Thanks! Animations were historically used because VisualState.Setters didn't used to exist. :) It was added in TH1.

OHhh I see, that makes sense, thank you @teaP !

[teaP]

Thanks for helping to clean this up!

[marcelwgn]

Problems should be resolved now.

[ranjeshj]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[teaP]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).