Skip to content

Control Themes#8263

Merged
grokys merged 37 commits intomasterfrom
feature/7120-control-themes
Jul 22, 2022
Merged

Control Themes#8263
grokys merged 37 commits intomasterfrom
feature/7120-control-themes

Conversation

@grokys
Copy link
Copy Markdown
Member

@grokys grokys commented Jun 3, 2022 •
edited
Loading

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
Closed
@avaloniaui-team
Copy link
Copy Markdown
Contributor

avaloniaui-team commented Jun 3, 2022

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
Copy Markdown
Contributor

avaloniaui-team commented Jun 4, 2022

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
Copy Markdown
Contributor

avaloniaui-team commented Jun 4, 2022

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

maxkatz6 commented Jun 6, 2022 •
edited
Loading

@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
Copy Markdown
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?

@grokys
Added support for implicit themes.
d21e634 
If no `Theme` property is provided, try to look up a resource keyed with the control's `StyleKey`.
@avaloniaui-team
Copy link
Copy Markdown
Contributor

avaloniaui-team commented Jun 8, 2022

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
Copy Markdown
Contributor

avaloniaui-team commented Jun 10, 2022

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 mentioned this pull request Jun 19, 2022
Closed
@maxkatz6 maxkatz6 added this to the 11.0 milestone Jun 19, 2022
@avaloniaui-team
Copy link
Copy Markdown
Contributor

avaloniaui-team commented Jul 3, 2022

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

@avaloniaui-team
Copy link
Copy Markdown
Contributor

avaloniaui-team commented Jul 4, 2022

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]

@grokys grokys mentioned this pull request Jul 4, 2022
Merged
@grokys
Don't run an unnecessary batch update.
4d23058 
If there are no styles to detach, there's no reason to run a batch update.
@avaloniaui-team
Copy link
Copy Markdown
Contributor

avaloniaui-team commented Jul 4, 2022

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
@grokys
Revert "Make MovePreviousOrParent internal as well."
030c956 
This reverts commit 5e1f28f.
@grokys
Revert "Fix parent selectors with /template/ at end."
a341c33 
This reverts commit e50b416 as it was not functioning correctly.
@grokys
Skip these failing tests for now.
3e17bd0 
I will work on a fix.
@avaloniaui-team
Copy link
Copy Markdown
Contributor

avaloniaui-team commented Jul 6, 2022

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
Copy Markdown
Contributor

avaloniaui-team commented Jul 7, 2022

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]

@grokys
Disallow selectors with trailing /template/.
0023770 
It's an invalid selector: what does `Button /template/` select?
@avaloniaui-team
Copy link
Copy Markdown
Contributor

avaloniaui-team commented Jul 7, 2022

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]

@grokys grokys mentioned this pull request Jul 7, 2022
Merged
@grokys
Added ItemContainerTheme property.
05c1f58 
Otherwise there's no easy way to apply themes to item containers.
@grokys grokys mentioned this pull request Jul 11, 2022
Merged
@avaloniaui-team
Copy link
Copy Markdown
Contributor

avaloniaui-team commented Jul 19, 2022

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 reviewed Jul 21, 2022
View reviewed changes
Comment thread src/Avalonia.Base/StyledElement.cs
@avaloniaui-team
Copy link
Copy Markdown
Contributor

avaloniaui-team commented Jul 21, 2022

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
@grokys grokys mentioned this pull request Jul 29, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maxkatz6 maxkatz6 maxkatz6 approved these changes

+1 more reviewer

@danwalmsley danwalmsley danwalmsley left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@maxkatz6 maxkatz6

Labels

None yet

Projects

None yet

Milestone

11.0

Development

Successfully merging this pull request may close these issues.

6 participants

@grokys @avaloniaui-team @robloo @maxkatz6 @amwx @danwalmsley