Fixed lost button foreground after pointer over

[eugenegff]

Fixes #5717

Button and RepeatButton foregrounds are lost after pointer over for buttons that use DefaultButtonStyle and DefaultRepeatButtonStyle, but not for the AccentButtonStyle.

    <StackPanel Spacing="8" HorizontalAlignment="Left">
        <Button Content="Btn1" Foreground="Red"/>
        <RepeatButton Content="Btn2" Foreground="Red"/>
        <Button Content="Btn3" Foreground="Red" Style="{StaticResource AccentButtonStyle}"/>
    </StackPanel>

Remarks related to the fix:

1. AccentButtonStyle has ContentPresenter.Foreground = "{TemplateBinding Foreground}" line of code and works properly - after resetting from PointerOver to Normal state property reverted from value set by visual state to local value.
2. DefaultButtonStyle and DefaultRepeatButtonStyle has no such ContentPresenter.Foreground = "{TemplateBinding Foreground}" line of code and seems to rely on Foreground property propagation, that is the reason buttons are initially red. But after returning from PointerOver to Normal state such property propagation does not occured. Bug could be fixed by adding aforementioned line of code, it is the current PR.
3. BUT ControlsResourcesVersion="Version1" does not exhibit the bug, despite missing ContentPresenter.Foreground = "{TemplateBinding Foreground}" line of code. I can not explain this behavior without sources for Button and VisualStateManager, it could be side effect of other differences between Version1 and Version2 versions of DefaultButtonStyle. Note, that WUX styles has no such line of code too.

P.S.
DropDownButton is affected too, probably many more controls too - not in current PR
P.P.S
ToggleButton is affected, SplitButton and ToggleSplitButton are unaffected

@jevansaks @StephenLPeters @tashatitova FYI

Win11 10.0.22000.258, WinUI 2.7

[zhuxb711]

Related to #5717

[eugenegff]

I determined what caused such problem in Version2 styles comparing to Version1 styles. ButtonForeground and ButtonForegroundPointerOver were different in Version1 and became the same in Version2. Tweaking Version2 ButtonForegroundPointerOver to be different than ButtonForeground fixes the issue too.

Seems that VisualStateManager somehow decides that it has no work to do if brushes are the same even when Foreground propagation is involved and work needs to be done.

Having this in mind, I think that "to add Foreground="{TemplateBinding Foreground}" to the button's ContentPresenter" is a good workaround, that should be immediately applied to the MUX, as underlying issue is in OS code, and MUX should support old OS versions too. Fix for Button and ToggleButton is in PR #6105, DropDownButton is affected too.

@StephenLPeters @ranjeshj @jevansaks @tashatitova FYI

Button_themeresources_v1.xaml

            <StaticResource x:Key="ButtonForeground" ResourceKey="SystemControlForegroundBaseHighBrush" />
            <StaticResource x:Key="ButtonForegroundPointerOver" ResourceKey="SystemControlHighlightBaseHighBrush" />

Button_themeresources.xaml

            <StaticResource x:Key="ButtonForeground" ResourceKey="TextFillColorPrimaryBrush" />
            <StaticResource x:Key="ButtonForegroundPointerOver" ResourceKey="TextFillColorPrimaryBrush" />

[eugenegff]

ToggleButton is affected too - added to current PR

[karkarl]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

Investigating root cause.

[StephenLPeters]

There is something very strange going on here. I changed ButtonForeground and ButtonForegroundPointerOver resources to point to a new resource named TextFillColorPrimaryBrush2 which points to the TextFillColorPrimary resource the same as TextFillColorPrimaryBrush and that resolved the issue. I haven't been able to find what is special about specifically the TextFillColorPrimaryBrush resource, but changing all instances of its use to any other brush I've tried fixes this issue.

Therefore I don't think this is the correct fix but simply masks something wrong with this resource...

[eugenegff]

@StephenLPeters If you create TextFillColorPrimaryBrushAlmostUnused near the TextFillColorPrimaryBrush and point DefaultTextForegroundThemeBrush to TextFillColorPrimaryBrushAlmostUnused - the bug is fixed!

So, brush referenced by DefaultTextForegroundThemeBrush has special handling.

<SolidColorBrush x:Key="TextFillColorPrimaryBrush" Color="{ThemeResource SystemColorWindowTextColor}" />
<SolidColorBrush x:Key="TextFillColorPrimaryBrushAlmostUnused" Color="{ThemeResource SystemColorWindowTextColor}" />

<StaticResource x:Key="DefaultTextForegroundThemeBrush" ResourceKey="TextFillColorPrimaryBrushAlmostUnused" />

Perhaps if Foreground is set by VisualState to DefaultTextForegroundThemeBrush then it is not considered 'modified' and is not reverted by VisualStateManager while transitioning from PointerOver to Normal state.

[StephenLPeters]

Yeah, the behavior isn't what I'd expect... Someone will have to debug and figure out what is actually happening..

[eugenegff]

Yeah, the behavior isn't what I'd expect... Someone will have to debug and figure out what is actually happening..

@StephenLPeters Given that this behavior is located inside the OS code and chances that this issue would be fixed in all OS versions supported by WinUI 2 are extremely small - what do you think about using TemplateBinding instead of relying on a Foreground inheritance (i.e. current PR) as a workaround for a problem?

[StephenLPeters]

I suspect this can be fixed from winui2 and want to understand the issue before committing to a fix.

[eugenegff]

@StephenLPeters BTW, from https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.controls.control.foreground

Typically, a ContentControl derived control uses a {TemplateBinding} markup extension to bind its Foreground value to the Foreground of a ContentPresenter within the control template.

I suspect that Foreground inheriting was intended for simpler scenarios like TextBlocks within StackPanels, DataTemplates and so one, not for the control templates. And documentation explicitly says that it is typical to use ContentPresenter.Foreground = "{TemplateBinding Foreground}" for ContentControls, and ButtonBase is a ContentControl.

I suspect this can be fixed from winui2 and want to understand the issue before committing to a fix.

You can make DefaultTextForegroundThemeBrush a separate brush rather than a reference to the TextFillColorPrimaryBrush, that will solve the problem. Note, that generic.xaml from OS does exactly this

OS 14393-22000:

<SolidColorBrush x:Key="DefaultTextForegroundThemeBrush" Color="{StaticResource SystemBaseHighColor}" />
<SolidColorBrush x:Key="SystemControlForegroundBaseHighBrush" Color="{StaticResource SystemBaseHighColor}" />
<StaticResource x:Key="ButtonForeground" ResourceKey="SystemControlForegroundBaseHighBrush" />

WinUI:

<SolidColorBrush x:Key="TextFillColorPrimaryBrush" Color="{StaticResource TextFillColorPrimary}" />
<StaticResource x:Key="DefaultTextForegroundThemeBrush" ResourceKey="TextFillColorPrimaryBrush" />
<StaticResource x:Key="ButtonForeground" ResourceKey="TextFillColorPrimaryBrush" />

fixed WinUI:

<SolidColorBrush x:Key="TextFillColorPrimaryBrush" Color="{StaticResource TextFillColorPrimary}" />
<SolidColorBrush x:Key="DefaultTextForegroundThemeBrush" Color="{StaticResource TextFillColorPrimary}" />
<StaticResource x:Key="ButtonForeground" ResourceKey="TextFillColorPrimaryBrush" />

The cost is memory for one SolidColorBrush - really cheap.

[tuggernuts]

Forgive my ignorance of the the bug squashing process for WinUI, but will this eventual fix be applicable to WinUI 3 as well? I would assume so, but everything here mentions 2.x. Thanks.

[eugenegff]

@StephenLPeters ?

[ojhad]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ojhad]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ojhad]

Forgive my ignorance of the the bug squashing process for WinUI, but will this eventual fix be applicable to WinUI 3 as well? I would assume so, but everything here mentions 2.x. Thanks.

This issue should already be fixed for button in WinUI3. I'll port over the fixes for ToggleButton and RepeatButton.