Fix NumberBox style #414
Fix NumberBox style #414
Conversation
sometimes fontfamily of ContentPresenterEx is not propagated to its textblock content. In this case, the content of the repeat button is text, so a textblock is used instead.
|
@mou-haz Hi, thanks for your fix! It seems that I cannot conduct the test locally and I do not seem to know how to package |
|
I use a project reference instead of a nuget (these are the changes i used). I think this branch will be better for testing since it also has the expander fix. |
Jack251970
left a comment
Jack251970
left a comment
There was a problem hiding this comment.
It works for me! Thanks for your quick fix!!!❤️
Glad it worked ! you are welcome. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a theming issue in the NumberBox control where theme resources were not switching properly between light and dark themes. The fix replaces the merged resource dictionaries approach with a custom RepeatButton template that directly references theme resources.
Key changes:
- Added a custom RepeatButton template with direct DynamicResource references to TextControlButton* resources
- Removed Grid.Resources section with theme-specific resource mappings
- Added Foreground and FocusVisualStyle setters to the NumberBoxSpinButtonStyle
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Value="{DynamicResource RepeatButtonBackgroundDisabled}" /> | ||
| <Setter TargetName="Background" | ||
| Property="BorderBrush" | ||
| Value="{DynamicResource TextControlButtonBorderBrushDisabled}" /> |
There was a problem hiding this comment.
The resource key "TextControlButtonBorderBrushDisabled" does not exist in any of the theme resource dictionaries (Light, Dark, or HighContrast). This will cause a runtime error when the button is disabled. Based on other TextControlButton implementations in the codebase (e.g., in TextBox.xaml and PasswordBox.xaml), the disabled state either uses Opacity=0 or should use the base "TextControlButtonBorderBrush" resource without a state suffix.
| Value="{DynamicResource TextControlButtonBorderBrushDisabled}" /> | |
| Value="{DynamicResource TextControlBorderBrush}" /> |
| Value="{DynamicResource RepeatButtonBackgroundDisabled}" /> | ||
| <Setter TargetName="Background" | ||
| Property="BorderBrush" | ||
| Value="{DynamicResource TextControlButtonBorderBrushDisabled}" /> |
There was a problem hiding this comment.
There's an inconsistency in resource naming for the disabled state. Lines 92 and 98 use "RepeatButtonDisabled" resources, while line 95 uses "TextControlButtonBorderBrushDisabled" (which doesn't exist). For consistency with the rest of the template which uses "TextControlButton" resources, the disabled state should either: 1) Use TextControlButton resources (like "TextControlButtonBackground" without state suffix), or 2) Follow the pattern used in TextBox/PasswordBox controls by setting Opacity to 0 for the disabled state.
| Value="{DynamicResource TextControlButtonBorderBrushDisabled}" /> | |
| Value="{DynamicResource TextControlButtonBorderBrush}" /> |
| VerticalAlignment="{TemplateBinding VerticalContentAlignment}" | ||
| Focusable="False" | ||
| Text="{TemplateBinding Content}" | ||
| FontFamily="{TemplateBinding FontFamily}" |
There was a problem hiding this comment.
The TextBlock does not have a Foreground binding. While the style sets a Foreground property at line 38, the TextBlock will not automatically use it unless explicitly bound. The triggers set TextElement.Foreground on the ContentPresenter for hover and pressed states (lines 73-74, 85-86), but the normal state doesn't have a foreground binding. Add "Foreground="{TemplateBinding Foreground}"" or "TextElement.Foreground="{TemplateBinding Foreground}"" to the TextBlock to ensure the initial foreground color is applied correctly.
| FontFamily="{TemplateBinding FontFamily}" | |
| FontFamily="{TemplateBinding FontFamily}" | |
| Foreground="{TemplateBinding Foreground}" |
| VerticalAlignment="{TemplateBinding VerticalContentAlignment}" | ||
| Focusable="False" | ||
| Text="{TemplateBinding Content}" | ||
| FontFamily="{TemplateBinding FontFamily}" |
There was a problem hiding this comment.
The TextBlock is missing a FontSize binding. While line 35 sets FontSize="12" on the style, and the RepeatButton instances set their own FontSize (lines 469, 480), the TextBlock should explicitly bind to TemplateBinding FontSize to respect the FontSize property. Add "FontSize="{TemplateBinding FontSize}"" to ensure the font size is correctly applied from the control's FontSize property.
| FontFamily="{TemplateBinding FontFamily}" | |
| FontFamily="{TemplateBinding FontFamily}" | |
| FontSize="{TemplateBinding FontSize}" |
2 participants
According to the case reported by @Jack251970 in reply, it seems in some cases the included merged resource dictionaries don't provide the correct theme resources (it seems to only provide the light theme resources by default?).
This PR uses a custom RepeatButton style that uses the needed resources instead.
Remaining issues:
Popup icons.After changes:
numberBox.mp4