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

Fixed theme variant properties #10310

Closed
wants to merge 1 commit into from
Closed

Conversation

jp2masa
Copy link
Contributor

@jp2masa jp2masa commented Feb 11, 2023

What does the pull request do?

Fixed theme variant properties: the names were wrong and the CLR properties have to be in the same class as the Avalonia property (also, changing the property owner doesn't seem to make any difference).

What is the current behavior?

It's impossible to bind to TopLevel.RequestedThemeVariant.

What is the updated/expected behavior with this PR?

It's possible to bind to TopLevel.RequestedThemeVariant. However, it's also possible to bind to any StyledElement.RequestedThemeVariant, so ThemeVariantScope seems to be useless (still testing things, not sure).

EDIT: Setting RequestedThemeVariant on controls works fine, so ThemeVariantScope actually doesn't seem to be needed.

How was the solution implemented (if it's not obvious)?

Moved the CLR property definition to StyledElement, where the Avalonia property is declared.

An alternative would be to remove the Avalonia property in StyledElement and make the one in TopLevel an AddOwner of the one in ThemeVariantScope.

Checklist

Fixed issues

Fixes #10309

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0029883-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@amwx
Copy link
Contributor

amwx commented Feb 11, 2023

From my understanding, ThemeVariantScope and the lack of RequestedThemeVariant on all controls is because Avalonia has inheriting properties where UWP doesn't which may cause issues in contexts where a control is using an inherited property (e.g. Foreground)

From theme dictionary PR:

Each control has two properties: RequestedThemeVariant and ActualThemeVariant. RequestedThemeVariant is only accessible for Application, TopLevel (including Window), and ThemeVariantScrope controls. Potentially it can be added to the Page control to improve mobile support. We need this restriction because Avalonia has inheritable properties, while UWP does not. This would cause problems with font and background colors inherited from the parent which has different theme variant. While ThemeVariantScrope override most common inheritable properties.

@jp2masa
Copy link
Contributor Author

jp2masa commented Feb 11, 2023

RequestedThemeVariant actually exists on all controls as an Avalonia property. The CLR properties in TopLevel and ThemeVariantScope were simply getters/setters for that Avalonia property which were broken, i.e. binding from XAML didn't work.

As I suggested, it can also be moved to ThemeVariantScope and TopLevel would AddOwner, removing the one in StyledElement (so it couldn't be set on all controls).

There is also the option to "fix" the XAML compiler if this is actually a bug, allowing to have CLR properties in inherited types, but then the Avalonia property in the base class doesn't make much sense I guess?

@maxkatz6
Copy link
Member

As I suggested, it can also be moved to ThemeVariantScope and TopLevel would AddOwner, removing the one in StyledElement (so it couldn't be set on all controls).

Yes, I did that in this PR #10149

@jp2masa
Copy link
Contributor Author

jp2masa commented Feb 12, 2023

Yes, I did that in this PR #10149

Thanks, I missed that, I'll close this then.

@jp2masa jp2masa closed this Feb 12, 2023
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.

Binding TopLevel.RequestedThemeVariant doesn't work?
4 participants