Update high contrast colors #4635
Conversation
ghost
added
the
ranjeshj
added
accessibility
|
Hi Marcel, Are the screenshots all "after" screenshots? Per the email thread, it looked like you added HighContrastAdjustment, can that be added for the button as well to remove the black backplate beneath the text on hover? I'm also curious about the button text color, if you're using HC1 theme, they look like they're using WindowText instead of ButtonText? Is the hover state using ButtonText for the base? I'm double checking with the designer of these, I just had a meeting with her and I want to make sure we're getting the right colors the first time... I'll update when I hear back from her. Thanks! |
|
All of the screenshots above were after the changes, here is a gif with the controls using HighContrastAdjustment="None":
I updated the code for button so that the foreground brush is now using SystemColorButtonTextColor in rest state. The hover uses the colors indicated by @YuliKl in #4584. Hope that clarifies @alejo4000 and thank you for helping with reviewing this PR! |
| @@ -62,16 +62,16 @@ | |||
| <StaticResource x:Key="AccentButtonBorderBrushPressed" ResourceKey="SystemControlHighlightTransparentBrush" /> | |||
| <StaticResource x:Key="AccentButtonBorderBrushDisabled" ResourceKey="SystemControlDisabledTransparentBrush" /> | |||
| <StaticResource x:Key="ButtonBackground" ResourceKey="SystemControlBackgroundBaseLowBrush" /> | |||
| <StaticResource x:Key="ButtonBackgroundPointerOver" ResourceKey="SystemControlBackgroundBaseLowBrush" /> | |||
| <StaticResource x:Key="ButtonBackgroundPressed" ResourceKey="SystemControlBackgroundBaseMediumLowBrush" /> | |||
| <StaticResource x:Key="ButtonBackgroundPointerOver" ResourceKey="SystemColorHighlightTextColor" /> | |||
There was a problem hiding this comment.
@ranjeshj @jevansaks I think I've asked this before, but to double-check: is it ok to use this kind of lookup from a SolidColorBrush to a Color directly?
There was a problem hiding this comment.
Good catch. I think it works in most cases that it's ok but I prefer to be explicit if we can. I feel like there is a corner case that I never remember where it causes us trouble.
There was a problem hiding this comment.
So do you want me to change all of the references to brushes then?
There was a problem hiding this comment.
Sounds like changing them to brushes would be safer.
Use these:
There was a problem hiding this comment.
@YuliKl Should we update the references to these colors holistically then just to be sure?
There was a problem hiding this comment.
Yes, all of these should point to the brush resource otherwise a SolidColorBrush is created for each one which will affect perf.
<StaticResource x:Key="ButtonBackgroundPointerOver" ResourceKey="SystemColorHighlightTextColorBrush" />
These are newly introduced brush resources to WinUI 2.6. They have the same names (with "Brush" added to the end) as the HC API colors making it much easier to understand (no more asking yourself "what HC color does SystemControlBackgroundBaseMediumLowBrush resolve to again?!"):
SystemColorWindowTextColorBrush
SystemColorWindowColorBrush
SystemColorButtonFaceColorBrush
SystemColorButtonTextColorBrush
SystemColorHighlightColorBrush
SystemColorHighlightTextColorBrush
SystemColorHotlightColorBrush
SystemColorGrayTextColorBrush
There was a problem hiding this comment.
Fixed now, @marksfoster @ranjeshj should I also update ALL other references?
There was a problem hiding this comment.
@chingucoding you mean update any other HC resources that point to the color instead of the brush?
There was a problem hiding this comment.
Yes, exactly @marksfoster .
There was a problem hiding this comment.
I think check this PR in and I'll talk to Yulia about this extra refactor work and whether we want to take it on.
Thanks! The specs from the bug you linked are indeed correct, I was mistaken about the base color. Also, thanks for the update to the foreground color! |
Co-authored-by: Yulia Klein <[email protected]>
Co-authored-by: Yulia Klein <[email protected]>
|
looks like there is a merge conflict. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@chingucoding can you merge in master ? There were some fixes that were made for these test failures. |
|
Merged now @ranjeshj |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description
Updated brushes as per #4584 and #4585
Motivation and Context
Closes #4584, Closes #4585
How Has This Been Tested?
Screenshots (if appropriate):