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

Control Themes #8263

Merged
merged 37 commits into from
Jul 22, 2022
Merged

Control Themes #8263

merged 37 commits into from
Jul 22, 2022

Conversation

grokys
Copy link
Member

@grokys grokys commented Jun 3, 2022

Background

The current biggest problem with our styling system is that there is no way to override an existing style without previously applied styles "leaking" through. This is especially important in the context of theming controls: if one has a globally-applied theme, then to re-theme a particular control or set of controls, one must reset every value set in the globally-applied theme. This can be impossible to do if you don't know what the globally-applied theme is and also makes #7679 infeasible as the only way to reset these values is to use local values. This subject has been discussed in various threads:

The two main proposals are contained in #7120 and #7378. This PR implements something more similar to #7120: I will go into the discussion we had over the pros and cons of each of these approaches, and why we chose this solution, in a separate comment.

This PR

This PR implements a system similar to the WPF/UWP styling system, in addition to our existing CSS-like styling system:

  • A Theme property was added to the Control class which accepts an instance of a ControlTheme object. This property is analogous to the WPF/UWP FrameworkElement.Style property.
  • ControlTheme is a type of style that is placed in a ResourceDictionary instead of a Styles collection, and has a TargetType property instead of a Selector. This class is analogous to the WPF/UWP Style class.
    • It also has a BasedOn property
  • If Control.Theme is not set, then the framework will attempt to find a ControlTheme resource with a key matching the control's style key
  • Control.Theme can be specified inline, e.g.
<Button>
  <Button.Theme>
    <ControlTheme>
      <!-- Setters and nested styles here -->
    </ControlTheme>
  </Button.Theme>
</Button>
  • Control.Theme can be set via a StaticResource, e.g.
<Button Theme="{StaticResource MyButtonTheme}"/>
  • A ControlTheme can contain nested styles which are permitted to select on the control that has the theme and its direct templated children
  • Control themes are applied at a lower priority than CSS-like styles

Naming

This system unfortunately diverges from WPF/UWP's naming:

  • The FrameworkElement.Style property becomes Control.Theme
  • The Style class becomes ControlTheme

Ideally we would have used the same naming as WPF/UWP, but the problem is that we already have a Style class, and it has a Selector. A Selector is not valid on a ControlTheme, and similarly ControlTheme has TargetType and BasedOn properties which are not valid on styles.

Our choices here were:

  • Find some other name for our existing Style class. This would be a huge API breakage.
  • Use a single Style class with Selector, TargetType and BasedOn properties, and choose which ones are relevant depending on where the Style appears (in a Styles collection or a resource dictionary). This would be confusing.
  • Find a new naming for the new properties/classes. This is what we chose.

Todo

  • Validate selectors in control themes
  • BasedOn

For subsequent PRs:

  • Default control themes (i.e. Generic.xaml equivalent)
  • Port fluent and simple theme

@grokys grokys mentioned this pull request Jun 3, 2022
@avaloniaui-team
Copy link
Contributor

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

@avaloniaui-team
Copy link
Contributor

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

@avaloniaui-team
Copy link
Contributor

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

@robloo
Copy link
Contributor

robloo commented Jun 4, 2022

Overall, I guess I'm not a fan of introducing another class here. It would have been better for Style to do all of this IMO.

@robloo
Copy link
Contributor

robloo commented Jun 4, 2022

Also, I wonder why there is aversion to using Key like other XAML frameworks? It makes sense to be able to reference a ControlTemplate by Key in both BasedOn and directly in controls to choose which ControlTheme they want to apply overriding anything else.

@maxkatz6
Copy link
Member

maxkatz6 commented Jun 6, 2022

@robloo

Also, I wonder why there is aversion to using Key like other XAML frameworks

It isn't? ControlTheme needs to be stored somewhere, and it usually would be in ResourceDictionaries, and accessed by the key. Same as in WPF, where FrameworkElement.Style is not a key, but an actual style object.

It would have been better for Style to do all of this IMO

Yeah, but what to do with Selector property? It's confusing to have TargetType and Selector property, where Selector wouldn't work. Also, BasedOn won't work on old styles.
Since old avalonia styles (CssStyle) and control themes (WpfStyle) are quite difference by the way how they used, isn't it better to distingue them by name? It would be possible to rename old Style to StyleSheet (or something) and add new Style in this PR, but that's a way bigger breaking change that we would want to ever have.

@maxkatz6
Copy link
Member

maxkatz6 commented Jun 6, 2022

We had two options for naming as well: ControlTheme (and Control.Theme prop) or ControlStyle (and Control.Style prop).
Second one is closer to WPF API, but we once again have confusion with old Styles. Technically any old style is already a control's style, and some developers might be confused how old styles are related to Control.Style (and they aren't).

So, in the end new ControlTheme class is the least confusing one. Even though steps away from WPF naming.
Also, it would work better with Control.ThemeVariant property (alternative of FrameworkElement.RequestedTheme), as it basically is a color variant of control theme.

Any other options possibly?

If no `Theme` property is provided, try to look up a resource keyed with the control's `StyleKey`.
@avaloniaui-team
Copy link
Contributor

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

@avaloniaui-team
Copy link
Contributor

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

@maxkatz6 maxkatz6 added this to the 11.0 milestone Jun 19, 2022
@avaloniaui-team
Copy link
Contributor

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

If there are no styles to detach, there's no reason to run a batch update.
@avaloniaui-team
Copy link
Contributor

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

grokys added 3 commits July 6, 2022 10:02
This reverts commit e50b416 as it was not functioning correctly.
I will work on a fix.
@avaloniaui-team
Copy link
Contributor

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

@avaloniaui-team
Copy link
Contributor

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

It's an invalid selector: what does `Button /template/` select?
@avaloniaui-team
Copy link
Contributor

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

Otherwise there's no easy way to apply themes to item containers.
@avaloniaui-team
Copy link
Contributor

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

@danwalmsley danwalmsley marked this pull request as ready for review July 21, 2022 19:24
@avaloniaui-team
Copy link
Contributor

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

@grokys grokys changed the title WIP: Control Themes Control Themes Jul 21, 2022
@grokys grokys merged commit b6b2ee1 into master Jul 22, 2022
@grokys grokys deleted the feature/7120-control-themes branch July 22, 2022 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants