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

Don't use LocalValue priority for values in ControlTemplate #7679

Merged
merged 13 commits into from
Jul 22, 2022

Conversation

grokys
Copy link
Member

@grokys grokys commented Feb 23, 2022

What does the pull request do?

We have an ongoing source of confusion in Avalonia in that values set in a control template are set using LocalValue priority meaning that they can't be overridden by style setters (to be fair, WPF/UWP have this too but it's less noticeable due to their styling system).

This PR changes the XAML compiler to set literal values, StaticResources and DynamicResources with TemplatedParent priority, meaning that the values can be overridden in styles which have a trigger selector.

Questions

  • This change is specific to the XAML compiler. We can add a way to explicitly use TemplatedParent priority to indexer bindings (we already have [~Foo.Bar] syntax) but how would e.g. Avalonia.Markup.Declarative do this?
  • What about {Binding}s - should they change their priority in ControlTemplates? What if they have an explicit Priority already set?
  • Is TemplatedParent priority going to be OK here? I think it is, but I may have missed something. Do we need a separate priority?
  • Should we rename TemplatedParent priority to simply Template?

Breaking changes

Styles which had non-working overrides of control template values will now come alive and potentially mess things up (you can see such a problem in the HamburgerMenu control in ControlCatalog here).

Fixed issues

Fixes #2789

@avaloniaui-team
Copy link
Contributor

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

@pr8x
Copy link
Contributor

pr8x commented Feb 23, 2022

we already have [~Foo.Bar] syntax

What does it do?

@amwx
Copy link
Contributor

amwx commented Apr 10, 2022

What's the status on this PR?

Couple comments to the questions:

Questions

* What about `{Binding}`s - should they change their priority in `ControlTemplate`s? What if they have an explicit `Priority` already set?

I would expect anything within a ControlTemplate to have a Template related priority that can then be overridden in a separate style selector. See the WinUI SplitView template file for an example of Bindings working:
https://github.com/microsoft/microsoft-ui-xaml/blob/main/dev/SplitView/SplitView_themeresources.xaml

Can you set an explicit priority in Xaml, or is this another Markup.Declarative issue? If so, no offense to them, but getting around these issues in C# is a lot easier than Xaml and I would say make everything template priority.

* Should we rename `TemplatedParent` priority to simply `Template`?

This would probably make sense to do IMO

@grokys
Copy link
Member Author

grokys commented Apr 14, 2022

What's the status on this PR?

We've found that overring controls themes with this change, but without something like #2769 / #7378 is just too difficult so I'm afraid that this change has had to be shelved before we can implement something like that first.

@amwx
Copy link
Contributor

amwx commented Apr 14, 2022

What's the status on this PR?

We've found that overring controls themes with this change, but without something like #2769 / #7378 is just too difficult so I'm afraid that this change has had to be shelved before we can implement something like that first.

Understandable, but not great. I hope this and the styling system upgrades are still in the queue for 11.0. Probably reciting the obvious here, but the longer these upgrades wait, the harder it becomes to make the change.

@grokys grokys mentioned this pull request Jun 10, 2022
2 tasks
@grokys grokys changed the base branch from master to feature/7120-control-themes July 7, 2022 08:47
@grokys
Copy link
Member Author

grokys commented Jul 7, 2022

Updated this branch with changes from #8263 as this PR is not much use without that one. Temporarily updated the target to the branch for #8263 as well as to not include the changes from that PR in the diff.

Base automatically changed from feature/7120-control-themes to master July 22, 2022 12:00
@avaloniaui-team
Copy link
Contributor

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

@grokys grokys marked this pull request as ready for review July 22, 2022 15:22
@grokys
Copy link
Member Author

grokys commented Jul 22, 2022

Now that #8479 has been merged, this PR can be merged too.

We will probably want to think about changing the TemplatedParent priority to rename it and lower its priority, but lets get this merged as-is and think about that in a subsequent PR.

@maxkatz6 maxkatz6 enabled auto-merge July 22, 2022 18:01
@maxkatz6 maxkatz6 merged commit 8f7ed7d into master Jul 22, 2022
@maxkatz6 maxkatz6 deleted the refactor/controltemplate-binding-priority branch July 22, 2022 18:19
@avaloniaui-team
Copy link
Contributor

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

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.

Set Template-provided properties with a separate BindingPriority
6 participants