Use global SystemParameters.FocusVisualStyleKey insead of per control FocusVisual style

[Yoooi0]

Is your feature request related to a problem? Please describe.
A bunch of control styles define a FocusVisual style that is used as a setter value for FocusVisualStyle property and there is no way to customize them other than creating custom styles for each control and setting FocusVisualStyle to your own style.

Most of them are identical:

MaterialDesignInXamlToolkit/MaterialDesignThemes.Wpf/Themes/MaterialDesignTheme.Button.xaml

Lines 17 to 30 in 2a38b2c

	<Style x:Key="FocusVisual">
	<Setter Property="Control.Template">
	<Setter.Value>
	<ControlTemplate>
	<Rectangle Margin="2"
	SnapsToDevicePixels="true"
	Stroke="{DynamicResource {x:Static SystemColors.ControlTextBrushKey}}"
	StrokeDashArray="1 2"
	StrokeThickness="1" />
	</ControlTemplate>
	</Setter.Value>
	</Setter>
	</Style>

Only OptionMarkFocusVisual seems to have different Margin:

MaterialDesignInXamlToolkit/MaterialDesignThemes.Wpf/Themes/MaterialDesignTheme.RadioButton.xaml

Lines 13 to 25 in 2a38b2c

	<Style x:Key="OptionMarkFocusVisual">
	<Setter Property="Control.Template">
	<Setter.Value>
	<ControlTemplate>
	<Rectangle Margin="0,0,0,0"
	SnapsToDevicePixels="true"
	Stroke="{DynamicResource {x:Static SystemColors.ControlTextBrushKey}}"
	StrokeDashArray="1 2"
	StrokeThickness="1" />
	</ControlTemplate>
	</Setter.Value>
	</Setter>
	</Style>

I think the FocusVisualStyle should be globally defined with SystemParameters.FocusVisualStyleKey in App.xaml:

<Style x:Key="{x:Static SystemParameters.FocusVisualStyleKey}" TargetType="Control">
    <Setter Property="Control.Template"> 
        ...
    </Setter>
</Style>

If Margin is all that needs to be changed for some controls then maybe a FocusVisualAssist.Margin could be added to allow those controls to have their own FocusVisualStyle:

<Style x:Key="OptionMarkFocusVisual" BasedOn="{x:Static SystemParameters.FocusVisualStyleKey}">
    <Setter Property="materialDesign:FocusVisualAssist.Margin" Value="0,0,0,0"/>
</Style>

[nicolaihenriksen]

@Yoooi0 I am not sure that approach would work though (without having tested it). When you define a Style, the TargetType (at least to my knowledge) needs to be an exact match, and as such, using Control as the TargetType will not match much of anything.

I believe you would still need to define a Style for the specific control types (and base it on the "global one"), and that seems to defeat the purpose at least a little bit? You would of course have only one place for the margin, coloring, etc., so there are some benefits.

Just my thoughts, and again I may be wrong...

[Yoooi0]

@nicolaihenriksen

The TargetType in this case is not necessary. But I've tested it and it applied my custom style on all controls except those that have their own FocusVisual style defined in mdix.

The KeyboardNavigation class in wpf just finds the style by key:

https://github.com/dotnet/wpf/blob/4dbc072e3f2c30930a7a9ff9dab9114660ae2b88/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Input/KeyboardNavigation.cs#L906

https://github.com/dotnet/wpf/blob/4dbc072e3f2c30930a7a9ff9dab9114660ae2b88/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Input/KeyboardNavigation.cs#L933

And the default wpf focus visual style is defined as:

https://github.com/dotnet/wpf/blob/89d172db0b7a192de720c6cfba5e28a1e7d46123/src/Microsoft.DotNet.Wpf/src/Themes/XAML/FocusVisual.xaml#L12-L23

In the same file you can also see the exact FocusVisual style found in mdix:

https://github.com/dotnet/wpf/blob/89d172db0b7a192de720c6cfba5e28a1e7d46123/src/Microsoft.DotNet.Wpf/src/Themes/XAML/FocusVisual.xaml#L68-L86

[nicolaihenriksen]

@Yoooi0 Then it makes more sense to me (since KeyboardNavigation does the lookup by key).

I think I understand your intention with the description now, thanks 😃

You state that

I think the FocusVisualStyle should be globally defined with SystemParameters.FocusVisualStyleKey in App.xaml

I think the problem with that is that the library itself does not ship with an App.xaml AFAIK; the demo app of course does, but that is not what you consume. Forcing that boiler-plate code onto consumers does not seem right to me. Another approach would be to put it in MaterialDesignX.Defaults.xaml.
Another point I think is important is that, I believe, going this route will now "force" the global focus visual style on ALL controls that don't explicitly override it; even custom controls that are not themed/styled by the MDIX library. This is somewhat of a (behavioral) breaking change.

I think I am probably in over my head here 🤣 so I will let @Keboo decide...

[Yoooi0]

I think the problem with that is that the library itself does not ship with an App.xaml AFAIK; the demo app of course does, but that is not what you consume. Forcing that boiler-plate code onto consumers does not seem right to me. Another approach would be to put it in MaterialDesignX.Defaults.xaml.

Yes that would be a breaking change (in time for 5.0.0 I guess 😛).
Users would have to add a resource dictionary that defines the mdix focus style:

<ResourceDictionary Source=".../Themes/MaterialDesignTheme.FocusVisual.xaml" />

Or have to define their own x:Key="{x:Static SystemParameters.FocusVisualStyleKey}" style.
Or maybe mdix could just use the default wpf focus style for everything (so delete all custom FocusVisual styles) which are identical except the Margins.

Another point I think is important is that, I believe, going this route will now "force" the global focus visual style on ALL controls that don't explicitly override it; even custom controls that are not themed/styled by the MDIX library.

I think this is already the case? I was getting the focus visual style (the wpf default one) on many unwanted controls because I didnt specify Focusable/IsTabStop/FocusVisualStyle.

[nicolaihenriksen]

I think this is already the case? I was getting the focus visual style (the wpf default one) on many unwanted controls because I didnt specify Focusable/IsTabStop/FocusVisualStyle.

I would be surprised that you got any of the ones defined in MDIX since the SystemParameters.FocusVisualStyleKey is not used anywhere in the library. But since they are "the same" as the default one, I am pretty confident that you just got the default style (and not an MDIX defined one).

But looking at the codebase, I agree that some cleanup could possibly be done. I am not sure I agree that using SystemParameters.FocusVisualStyleKey as the key for the Style is a good idea as that would most likely have the (behavioral) breaking change I describe above.

But centralizing the 2 different styles into a common place could be an option. I think the challenge with that is that we want those styles to be "statically reachable" via {StaticResource XyzStyle}, and that would mean we need to include a merged dictionary with the common stuff into all of the existing MaterialDesignTheme.<Control>.xaml files. But that is slightly better than having duplicated styles lying around I guess... @Keboo Thoughts?

[Yoooi0]

I would be surprised that you got any of the ones defined in MDIX since the SystemParameters.FocusVisualStyleKey is not used anywhere in the library. But since they are "the same" as the default one, I am pretty confident that you just got the default style (and not an MDIX defined one).

Yes I meant the default wpf style not mdix style.
It was also applying on those controls when I specified a custom style with SystemParameters.FocusVisualStyleKey.

I am not sure I agree that using SystemParameters.FocusVisualStyleKey as the key for the Style is a good idea as that would most likely have the (behavioral) breaking change I describe above.

When FocusVisualStyle on a control is unset KeyboardNavigation defaults to SystemParameters.FocusVisualStyleKey, so mdix would not have to directly use SystemParameters.FocusVisualStyleKey, just don't specify custom FocusVisualStyle at all unless a control needs a totally custom one, in that case it should be BasedOn SystemParameters.FocusVisualStyleKey.

It seems like a wpf built-in solution for focus styles.
I think its the only solution that can be used to globally override focus style.

We will wait for @Keboo input as im not an expert in wpf.

[nicolaihenriksen]

@Yoooi0 Our conversation here triggered a need in me to dig deeper and do some testing on how things work. I think I got wiser, but whether it will be of much use for potentially solving the issue at hand is a different question. What I concluded was:

1. If FocusVisualStyle is not explictly set on a Control, KeyboardNavigation will fall back to the WPF built-in style defined by the SystemParameters.FocusVisualStyleKey. Just like you mentioned in your comment - thanks for teaching me new stuff 😃

2. If I attempt to "override" the style defined by the SystemParameters.FocusVisualStyleKey with my own style, but still leave FocusVisualStyle not explicitly set on a Control, KeyboardNavigation will still fall back to the WPF built-in style defined by the SystemParameters.FocusVisualStyleKey, not the one I tried to override it with. This surprised me a bit 😮 The details of why that happens is hidden in the way KeyboardNavigation actually looks up the key; it is not just a standard Control.FindResource() call which I would expect would have found my overridden style.

3. If I override the style defined by the SystemParameters.FocusVisualStyleKey like in bullet 2, but also set FocusVisualStyle="{StaticResource {x:Static SystemParameters.FocusVisualStyleKey}}" on a Control, the control now uses my overridden style.

So, in order to define a "global" focus visual style somewhere in MDIX, we could use the SystemParameters.FocusVisualStyleKey as the key for it, but we would still need to explicitly set it on all controls for it to have any effect. This would of course avoid the need for the merged dictionary in all the XAML files as I commented above.

Perhaps this is in fact what the issue description is all about?! But as my bullet 3 above shows, I don't think it is sufficient just to define the style under the right key, I believe we need to explicitly set it in all of the styles in order for it to have any effect.

UPDATE:
After browsing a bit around, I stumbled upon this issue in the dotnet/wpf repo which is exactly what I see in bullet 2 above. Judging from that, and the associated PR, it would seem the issue is fixed. However, it seems it has not yet made it into the framework, since I can still reproduce the issue in MDIX, even after the latest update to .NET7 by @Keboo.

There is also a dotnet-campus repo which is a fork of the WPF repo. In there, there is still an open PR which references it.

[Yoooi0]

After browsing a bit around, I stumbled upon dotnet/wpf#1164 in the dotnet/wpf repo which is exactly what I see in bullet 2 above. Judging from that, and the associated dotnet/wpf#1165, it would seem this issue is fixed. However, it seems it has not yet made it into the framework since I can still reproduce the issue in MDIX even after the latest update to .NET7 by @Keboo.

Yes was about to mention that issue 😅
But thats surprising because I made sure to test it before making the issue, and I 100% managed to override the default style.

I just tested mdix demo app from master branch with this at the bottom of App.xaml:

      <Style x:Key="{x:Static SystemParameters.FocusVisualStyleKey}" TargetType="Control">
        <Setter Property="Control.Template">
          <Setter.Value>
            <ControlTemplate>
              <Rectangle StrokeThickness="1" Stroke="red" SnapsToDevicePixels="true" />
            </ControlTemplate>
          </Setter.Value>
        </Setter>
      </Style>

And you are correct, by default it seems to not work.
I had to set TargetFrameworks in demo csproj to just net7.0-windows (removed net472;netcoreapp3.1;net6.0-windows;) and it started to work fine.

[nicolaihenriksen]

@Yoooi0 I just noticed my error, I had my solution setup to run the net472 version of the output, not the .NET7. Changing that allows me to override it correctly.
UPDATE: Haha you found the same issue with the demo app 🤣

However since MDIX supports older frameworks, I still think we would need to apply the style key explicitly to have the same behavior across the board. I doubt that a change like this will be backported to older frameworks.
And even if doing so, there would still be the difference that, when a consumer runs under .NET7, MDIX would apply the custom focus visual style for all controls, but for older frameworks it would only apply it on the controls defined in MDIX.

[Yoooi0]

I had my solution setup to run the net472 version of the output, not the .NET7. Changing that allows me to override it correctly.

Doh, didnt even think to check, assumed visual studio will choose latest by default.

However since MDIX supports older frameworks, I still think we would need to apply the style key explicitly to have the same behavior across the board. I doubt that a change like this will be backported to older frameworks.

I think the simplest solution would be to just not specify any custom focus styles in mdix, which means all controls will use the default built-in wpf style, and since they are exactly the same anyways it wont be that big of an issue. You just wont be able to override it on anything below .net 7, but at least we would be able to override it in .net 7.

[nicolaihenriksen]

I think the simplest solution would be to just not specify any custom focus styles in mdix, which means all controls will use the default built-in wpf style, and since they are exactly the same anyways it wont be that big of an issue. You just wont be able to override it on anything below .net 7, but at least we would be able to override it in .net 7.

That is definitely an option, let's see what @Keboo thinks. And Kevin, sorry for the loooong conversation with you being mentioned multiple times 😬

[Keboo]

Wow lots of great discussion here (and no, I don't mind the tags 😃). First a thank you to both of you for digging into this.

First a quick comment on versioning and backwards compatibility. Though this library tries to follow SemVer it is a little difficult, as SemVer intentionally does not define "backwards compatible". In C# there are two forms of backwards compatibility (binary and source). With .NET Core, and the global assembly cache (GAC) not mattering as much, I tend to not care about binary backwards compatibility (it is somewhat rare for a newer version of the library to get used without the consuming app recompiling). Source compatibility is typically how I attempt to version this library (with the exception of the PackIcon as these are auto generated nightly), but with XAML there is one more use case, visual backwards compatibility. This is where things get weird because to maintain visual backwards compatibility you can make very few changes to any XAML without changing something visually. As a compromise, I have decided to simply flag (with a label) items that are "major" visual changes (where "major" is a completely subjective determination). So, with all of that said, I think some of this would fall under that third case of a visual breaking change.

So, after looking it over, I think I am fine with simply not setting any custom focus style in the library and letting controls fall back to the default WPF. This area of the UI is one that Material Design doesn't really address and leaves a lot up to platforms to decide what is best. Typically, in these situations, I favor falling back to default WPF behavior as that will likely be what is expected by consumers. It also doesn't seem to be that bad given that with the current release, it is not really possible for people to customize it with our current setup anyway.

I do also really like standardizing all of the focus visual styles into a single style keyed on SystemParameters.FocusVisualStyleKey. We don't need to have so much duplication. Technically deleting those styles would be a "breaking change" if someone were referencing them (a bit unlikely, but people do copy the styles into their own projects and tweak them. I would create the new style, remove the usages of the old styles, and then add them to the list of breaking changes in #2255, and we can delete them as part of 5.0.0.

Sound reasonable?

One final note, if you want an easy way to debug different frameworks you can select which one VS will use when you press F5.

[nicolaihenriksen]

One final note, if you want an easy way to debug different frameworks you can select which one VS will use when you press F5.

This was exactly what I was hit by. It was set to net472 which is why there was no effect of overriding the SystemParameters.FocusVisualStyleKey. Changing to net7.0-windows allowed the override.