Ported Fluent Theme to ControlThemes.

[grokys]

What does the pull request do?

This PR ports the fluent theme to use ControlTheme with @Takoooooo's help.

Due to the way that control themes work, some of the themes required a bit of refactoring - in particular we decided not to allow nested /template/ selectors in control themes, so controls that used these had to have their sub-controls refactored out into separate sub-ControlThemes. This is a bit of a pain but should hopefully improve the stability of themes in the long-term as control themes are no longer relying on implementation details of other control themes.

The only place the lack of nested /template/ selectors turned out to really be a problem was in SplitButton as that control essentially adds additional states to Button (checked, flyout open). For the moment I've used a hack with setting the Tag property but we really need a proper solution for this - probably something like ClassSetter which would allow setting classes on controls from a style, or maybe something like the CSS :part selector (or both).

Depends on #7679
Depends on #8263

IMPORTANT

Currently targeting #7679 - change the PR target back to master before merging.

[grokys]

While refactoring, I noticed that this ContentPresenter doesn't seem to be used. Was it part of a feature that was cut? Can it be removed from the template?

[maxkatz6]

Yes, it's for Header property that UWP has but we don't. I.e., TextBox.Header, Slider.Header.
It works similarly how our Label works, just built in the template.

[grokys]

This is the "tag hack" I mentioned in the description. I'm not happy about it, but it seems to work for now.

[robloo]

Yes, this is very ugly... I hope the missing functionality is added so this doesn't become a permanent work-around.

[maxkatz6]

Just for case, this functionality wasn't removed. You still can use nested /template/ selectors outside of ControlTheme.

[robloo]

Well, as my luck goes, the places where it would be used (correctly) on my end are in ControlThemes. And I added a larger comment below but if Style still supports this it really seems like ControlTheme should as well.

[grokys]

You can still actually host <Style>s in the root element of your control theme template in order to use nested template selectors, however I'd try to avoid it if possible.

[grokys]

While testing I noticed that the indeterminate state for ToggleButton looks the same as the unchecked state. Is this intentional?

[timunie]

This is an issue in the toggle switch as well. I had a discussion about this in MahApps back then, The solution was to disallow null for IsChecked-property.

See also:

• https://docs.microsoft.com/en-us/windows/uwp/design/controls-and-patterns/toggles
• https://www.nngroup.com/articles/toggle-switch-guidelines/

[robloo]

Traditionally (in WPF/WinUI) CheckBox derives from ToggleButton and does make use of the three states. This is very common if a CheckBox is used at the top of a column for example representing each row selection.

Since other controls inherit from ToggleButton, and require three states, I don't think this can or should be removed. It is also just fine for ToggleButton to only consider two states in its style as it can only be toggled on/off by the user.

While testing I noticed that the indeterminate state for ToggleButton looks the same as the unchecked state. Is this intentional?

Yes, this is intentional and matches WinUI. It is only really useful in derived controls like CheckBox.

Source code from UWP Generic.xaml

            <!-- Resources for Windows.UI.Xaml.Controls.Primitives.ToggleButton -->
            <StaticResource x:Key="ToggleButtonBackground" ResourceKey="SystemControlBackgroundBaseLowBrush" />
            <StaticResource x:Key="ToggleButtonBackgroundPointerOver" ResourceKey="SystemControlBackgroundBaseLowBrush" />
            <StaticResource x:Key="ToggleButtonBackgroundPressed" ResourceKey="SystemControlBackgroundBaseMediumLowBrush" />
            <StaticResource x:Key="ToggleButtonBackgroundDisabled" ResourceKey="SystemControlBackgroundBaseLowBrush" />
            <StaticResource x:Key="ToggleButtonBackgroundChecked" ResourceKey="SystemControlHighlightAccentBrush" />
            <StaticResource x:Key="ToggleButtonBackgroundCheckedPointerOver" ResourceKey="SystemControlHighlightAccentBrush" />
            <StaticResource x:Key="ToggleButtonBackgroundCheckedPressed" ResourceKey="SystemControlHighlightBaseMediumLowBrush" />
            <StaticResource x:Key="ToggleButtonBackgroundCheckedDisabled" ResourceKey="SystemControlBackgroundBaseLowBrush" />
            <StaticResource x:Key="ToggleButtonBackgroundIndeterminate" ResourceKey="SystemControlBackgroundBaseLowBrush" />
            <StaticResource x:Key="ToggleButtonBackgroundIndeterminatePointerOver" ResourceKey="SystemControlBackgroundBaseLowBrush" />
            <StaticResource x:Key="ToggleButtonBackgroundIndeterminatePressed" ResourceKey="SystemControlBackgroundBaseMediumLowBrush" />
            <StaticResource x:Key="ToggleButtonBackgroundIndeterminateDisabled" ResourceKey="SystemControlBackgroundBaseLowBrush" />
            <StaticResource x:Key="ToggleButtonForeground" ResourceKey="SystemControlForegroundBaseHighBrush" />
            <StaticResource x:Key="ToggleButtonForegroundPointerOver" ResourceKey="SystemControlHighlightBaseHighBrush" />
            <StaticResource x:Key="ToggleButtonForegroundPressed" ResourceKey="SystemControlHighlightBaseHighBrush" />
            <StaticResource x:Key="ToggleButtonForegroundDisabled" ResourceKey="SystemControlDisabledBaseMediumLowBrush" />
            <StaticResource x:Key="ToggleButtonForegroundChecked" ResourceKey="SystemControlHighlightAltChromeWhiteBrush" />
            <StaticResource x:Key="ToggleButtonForegroundCheckedPointerOver" ResourceKey="SystemControlHighlightAltChromeWhiteBrush" />
            <StaticResource x:Key="ToggleButtonForegroundCheckedPressed" ResourceKey="SystemControlHighlightAltChromeWhiteBrush" />
            <StaticResource x:Key="ToggleButtonForegroundCheckedDisabled" ResourceKey="SystemControlDisabledBaseMediumLowBrush" />
            <StaticResource x:Key="ToggleButtonForegroundIndeterminate" ResourceKey="SystemControlForegroundBaseHighBrush" />
            <StaticResource x:Key="ToggleButtonForegroundIndeterminatePointerOver" ResourceKey="SystemControlHighlightBaseHighBrush" />
            <StaticResource x:Key="ToggleButtonForegroundIndeterminatePressed" ResourceKey="SystemControlHighlightBaseHighBrush" />
            <StaticResource x:Key="ToggleButtonForegroundIndeterminateDisabled" ResourceKey="SystemControlDisabledBaseMediumLowBrush" />
            <StaticResource x:Key="ToggleButtonBorderBrush" ResourceKey="SystemControlForegroundTransparentBrush" />
            <StaticResource x:Key="ToggleButtonBorderBrushPointerOver" ResourceKey="SystemControlHighlightBaseMediumLowBrush" />
            <StaticResource x:Key="ToggleButtonBorderBrushPressed" ResourceKey="SystemControlHighlightTransparentBrush" />
            <StaticResource x:Key="ToggleButtonBorderBrushDisabled" ResourceKey="SystemControlDisabledTransparentBrush" />
            <StaticResource x:Key="ToggleButtonBorderBrushChecked" ResourceKey="SystemControlHighlightAltTransparentBrush" />
            <StaticResource x:Key="ToggleButtonBorderBrushCheckedPointerOver" ResourceKey="SystemControlHighlightBaseMediumLowBrush" />
            <StaticResource x:Key="ToggleButtonBorderBrushCheckedPressed" ResourceKey="SystemControlTransparentBrush" />
            <StaticResource x:Key="ToggleButtonBorderBrushCheckedDisabled" ResourceKey="SystemControlDisabledTransparentBrush" />
            <StaticResource x:Key="ToggleButtonBorderBrushIndeterminate" ResourceKey="SystemControlForegroundTransparentBrush" />
            <StaticResource x:Key="ToggleButtonBorderBrushIndeterminatePointerOver" ResourceKey="SystemControlHighlightBaseMediumLowBrush" />
            <StaticResource x:Key="ToggleButtonBorderBrushIndeterminatePressed" ResourceKey="SystemControlHighlightTransparentBrush" />
            <StaticResource x:Key="ToggleButtonBorderBrushIndeterminateDisabled" ResourceKey="SystemControlDisabledTransparentBrush" />

[grokys]

For the moment, the UserControl theme is being set by a style. We may want to override the effective theme for UserControl to always return {StaticResource typeof(UserControl)} when no theme is set explicitly?

[maxkatz6]

Overriding makes sense to me. Isn't it the same about Window?

[grokys]

Window is slightly different in that it overrides IStyleable.StyleKey => typeof(Window); whereas UserControl doesn't do that due to #2280. It might make sense to use the same technique for both though now.

[grokys]

I removed this line as it was causing the ComboBox popup border to be truncated at the top and bottom. I've noticed this for a long time but never investigated it. @danwalmsley you seem to have added this line, do you remember why and it is ok to remove it?

[robloo]

That is a copy-paste from Fluent v1 in WinUI

https://github.com/microsoft/microsoft-ui-xaml/blob/7ed0bcf32a0e02b673a3d29555bd604d1f9ac0b0/dev/ComboBox/ComboBox_themeresources_v1.xaml#L721

That Margin (and any others like it) can safely be removed. It was just a work-around for WinUI/UWP specific issues.

[grokys]

Rebased to credit @Takoooooo for the work he did on this.

[grokys]

@danwalmsley I changed these to be buttons, because previously as panels you lost the ability to press and then cancel the press by releasing outside the bounds of the control.

[robloo]

In particular we decided not to allow nested /template/ selectors in control themes, so controls that used these had to have their sub-controls refactored out into separate sub-ControlThemes

I've found other places where directly accessing the template of a control within a template is very useful. Not trying to start another big discussion but if I had a vote I would not want to remove this functionality. It is totally find for one control default style to depend on another control's default style within the same theme (translating to Avalonia's "new terminology" default style = ControlTheme, Theme = FluentTheme). That is a well established convention in XAML styling from the very beginning.

Additionally, you are adding code duplication of styles so it would be easy for a button in one control style to be out-of-sync or outdated with the main button's default style. That is an undetectable error -- however, referring to a template part that no longer exists can be detected. So... well, I would not have done this for

1. Code duplication
2. Sure there is separation now so some changes don't break dependent control styles; however, now you have undetectable places where code can become out-of-sync design-wise
3. Deviation from existing XAML convention which has worked fine for years. A single theme can have inter-dependencies.

[grokys]

I've found other places where directly accessing the template of a control within a template is very useful. Not trying to start another big discussion but if I had a vote I would not want to remove this functionality.

Yeah we had a pretty intense discussion about this internally as well, but the general consensus was:

• The /template/ selector was generally speaking a mistake. It came from the CSS /deep/ selector which was removed and for good reason - it breaks encapsulation and is essentially the equivalent of changing private fields in a class using reflection
• A /template/ selector which can poke into any template was needed previously because of the way our styles worked - there was no scoping of styles
• The CSS /deep/ selector was replaced by the :part selector which is much better because it defines an API contract

So the plan is to:

• Add a :part selector - the details of this need to be fleshed out, but we already have concept of "parts" and we already have the [TemplatePart] attribute, so conceptually it's a good fit for Avalonia
• Add a ClassSetter which can be used to add extra state to other controls from a style
• Long term, eventually do a gentle deprecation of the /template/ selector outside control themes

To give some context: we're currently basically unable to change our templates between major releases because the template contents essentially become a public API due to /template/. So we have to balance the fact that 1) /template is a problem and 2) removing /template/ would break a lot of code if we did it now.

Given the above we felt that preventing meddling with implementation details of other controls in a new feature is a good start, but removing the feature as it already exists is probably a step too far right now.

We are actually open to removing the limitation on nested /template/ selectors in control themes if a problem arises that literally cannot be solved by any other means, but as you know it's far easier to add features than remove them! So for the moment I'm hoping that by disallowing nested template selectors we can understand what exactly they're needed for and try to come up with better solutions for these use-cases.

Does that make sense?

[grokys]

A single theme can have inter-dependencies.

The problem with this is, how do you define a theme? Are we talking about the fluent theme as defined in Avalonia.Themes.Fluent? Or does it include Avalonia.Controls.DataGrid/ColorPicker fluent themes? If it also includes those then it probably should include 3rd party fluent themes, right? And so you're back to having the contents of our themes be a public API.

[grokys]

Started a discussion to discuss the usages of /template/ selectors: #8490

[avaloniaui-team]

You can test this PR using the following package version. 0.10.999-cibuild0022401-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]