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

Fixing Button resources #10442

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dipeshmsft
Copy link
Member

@dipeshmsft dipeshmsft commented Feb 11, 2025

Fixes # #9886

Description

Button styles were using the common / Fluent brushes directly rather than Button's control brushes like ButtonBorderBrush, ButtonBorderBrushPointerOver, etc.

In this PR, I have fixed the Button styles to use the Button control brushes correctly.

Customer Impact

Developers will be able to modify the Button control brushes without affecting other controls.

Regression

No

Testing

Local testing

Risk

No

Microsoft Reviewers: Open in CodeFlow

@dipeshmsft dipeshmsft requested review from a team as code owners February 11, 2025 06:32
@dotnet-policy-service dotnet-policy-service bot added the PR metadata: Label to tag PRs, to facilitate with triage label Feb 11, 2025
@dipeshmsft dipeshmsft added this to the .NET 10 milestone Feb 11, 2025
@@ -329,8 +336,18 @@
<SolidColorBrush x:Key="ButtonForegroundPointerOver" Color="{StaticResource TextFillColorPrimary}" />
<SolidColorBrush x:Key="ButtonForegroundPressed" Color="{StaticResource TextFillColorSecondary}" />
<SolidColorBrush x:Key="ButtonForegroundDisabled" Color="{StaticResource TextFillColorDisabled}" />
<!--<SolidColorBrush x:Key="ButtonBorderBrush" Color="{StaticResource ControlElevationBorder}" />-->
<!--<SolidColorBrush x:Key="ButtonBorderBrushPointerOver" Color="{StaticResource ControlElevationBorder}" />-->
<LinearGradientBrush x:Key="ButtonBorderBrush" MappingMode="Absolute" StartPoint="0,0" EndPoint="0,3">
Copy link

@robert-abeo robert-abeo Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should just re-use ControlElevationBorderBrush. I don't think there is any reason to duplicate this complicated Brush. Otherwise, it is duplicate code and any updates would then have to be done multiple places -- bad practice.

WinUI does the same: https://github.com/microsoft/microsoft-ui-xaml/blob/589ced40542b9b4de2fddf0e58a1e3eecd164e2f/src/controls/dev/CommonStyles/Button_themeresources.xaml#L26-L27

I would ALWAYS review WinUI when making these types of changes.


Edit: I believe the below syntax works in WPF. I spend more time with more modern XAML frameworks so this needs to be tested.

    <StaticResource
        x:Key="ButtonBorderBrush"
        ResourceKey="ControlElevationBorderBrush" />
    <StaticResource
        x:Key="ButtonBorderBrushPointerOver"
        ResourceKey="ControlElevationBorderBrush" />

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that works, but let me give it a try.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

I get the following error when I do the above. However, I have seen this syntax in other places, but the StaticExtension there is a subclass of StaticResourceExtension for that : https://github.com/Kinnara/ModernWpf/blob/master/ModernWpf/Markup/StaticResourceExtension.cs

After that they can use the above syntax.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not actually sure what is going on here. The existing StaticResourceExtensions has this constructor as well:

public StaticResourceExtension(
object resourceKey)
{
ArgumentNullException.ThrowIfNull(resourceKey);
_resourceKey = resourceKey;

Either way, this is useful functionality. Perhaps it is a good idea to support it in Vanilla WPF as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking further, the error seems to be thrown by a Debug.Assert statement here :

Debug.Assert(!InStaticResourceSection, "We do not support x:Key within a StaticResource Section");

I don't know why this is here. I will open up an issue for this and try to find out what's the reasoning behind this.

By the way, would you know how this works ? Does it create a copy of the actual resource or is just a reference to the original resource ?

<!--<SolidColorBrush x:Key="AccentButtonBorderBrush" Color="{DynamicResource AccentControlElevationBorder}" />-->
<!--<SolidColorBrush x:Key="AccentButtonBorderBrushPointerOver" Color="{DynamicResource AccentControlElevationBorder}" />-->
<SolidColorBrush x:Key="AccentButtonForegroundDisabled" Color="{StaticResource TextOnAccentFillColorDisabled}" />
<LinearGradientBrush x:Key="AccentButtonBorderBrush" MappingMode="RelativeToBoundingBox" StartPoint="0,1" EndPoint="0,0">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as for ButtonBorderBrush, ButtonBorderBrushPointerOver -- re-use the pre-existing brushes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR metadata: Label to tag PRs, to facilitate with triage Win 11 Theming
Projects
Status: 🥅 Todo
Development

Successfully merging this pull request may close these issues.

3 participants