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

Add missing HC resources #3927

Merged
merged 2 commits into from
Jan 12, 2021
Merged

Add missing HC resources #3927

merged 2 commits into from
Jan 12, 2021

Conversation

beervoley
Copy link
Contributor

@beervoley beervoley commented Jan 12, 2021

Description

These PR adds missing HighContrast Resources to Common themeresources.

Motivation and Context

When switching to HighContrast theme the app would crash because the resources that Controls reference are not defined for the HighContrast theme. With this PR it will be fixed.

How Has This Been Tested?

Manually switched to HC and made sure defined resources are working.

@beervoley beervoley added the team-Controls Issue for the Controls team label Jan 12, 2021
@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Jan 12, 2021
@beervoley
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@beervoley
Copy link
Contributor Author

<StaticResource x:Key="DefaultTextForegroundThemeBrush" ResourceKey="TextFillColorPrimaryBrush" />
<StaticResource x:Key="DefaultApplicationBackgroundThemeBrush" ResourceKey="SolidBackgroundFillColorTertiaryBrush" />
Do we still need these brushes?
As well as these:
<SolidColorBrush x:Key="SystemColorWindowTextColorBrush" Color="{ThemeResource SystemColorWindowTextColor}" />
<SolidColorBrush x:Key="SystemColorWindowColorBrush" Color="{ThemeResource SystemColorWindowColor}" />
These two were being used only in High Contrast theme and are not being used anymore anywhere in the code. Unless someone depends on it already?

@ranjeshj ranjeshj requested review from teaP and karkarl and removed request for YuliKl January 12, 2021 16:35
@ranjeshj
Copy link
Contributor

<StaticResource x:Key="DefaultTextForegroundThemeBrush" ResourceKey="TextFillColorPrimaryBrush" />
<StaticResource x:Key="DefaultApplicationBackgroundThemeBrush" ResourceKey="SolidBackgroundFillColorTertiaryBrush" />
Do we still need these brushes?
As well as these:
<SolidColorBrush x:Key="SystemColorWindowTextColorBrush" Color="{ThemeResource SystemColorWindowTextColor}" />
<SolidColorBrush x:Key="SystemColorWindowColorBrush" Color="{ThemeResource SystemColorWindowColor}" />
These two were being used only in High Contrast theme and are not being used anymore anywhere in the code. Unless someone depends on it already?

If someone is using it, we do not want to cause a crash. @YuliKl as FYI

@ranjeshj ranjeshj requested a review from YuliKl January 12, 2021 16:38
@karkarl
Copy link
Contributor

karkarl commented Jan 12, 2021

<StaticResource x:Key="DefaultTextForegroundThemeBrush" ResourceKey="TextFillColorPrimaryBrush" />
<StaticResource x:Key="DefaultApplicationBackgroundThemeBrush" ResourceKey="SolidBackgroundFillColorTertiaryBrush" />
Do we still need these brushes?
As well as these:
<SolidColorBrush x:Key="SystemColorWindowTextColorBrush" Color="{ThemeResource SystemColorWindowTextColor}" />
<SolidColorBrush x:Key="SystemColorWindowColorBrush" Color="{ThemeResource SystemColorWindowColor}" />
These two were being used only in High Contrast theme and are not being used anymore anywhere in the code. Unless someone depends on it already?

I initially added DefaultApplicationBackgroundThemeBrush to fix HC crashes in some test pages. It is now a duplicate of the background brush you added, so can be removed once you replace the current instances.

@ranjeshj ranjeshj merged commit 55dbe9f into master Jan 12, 2021
@ranjeshj ranjeshj deleted the vsiliush/hc_brush_update branch January 12, 2021 21:01
@ranjeshj
Copy link
Contributor

<StaticResource x:Key="DefaultTextForegroundThemeBrush" ResourceKey="TextFillColorPrimaryBrush" />
<StaticResource x:Key="DefaultApplicationBackgroundThemeBrush" ResourceKey="SolidBackgroundFillColorTertiaryBrush" />
Do we still need these brushes?
As well as these:
<SolidColorBrush x:Key="SystemColorWindowTextColorBrush" Color="{ThemeResource SystemColorWindowTextColor}" />
<SolidColorBrush x:Key="SystemColorWindowColorBrush" Color="{ThemeResource SystemColorWindowColor}" />
These two were being used only in High Contrast theme and are not being used anymore anywhere in the code. Unless someone depends on it already?

I initially added DefaultApplicationBackgroundThemeBrush to fix HC crashes in some test pages. It is now a duplicate of the background brush you added, so can be removed once you replace the current instances.

Lets do a separate change to remove this key..

@YuliKl
Copy link

YuliKl commented Jan 12, 2021

@beervoley I'm getting some feedback from design about these values, so we may need to change a few brush mapping before you complete the PR.

@YuliKl
Copy link

YuliKl commented Jan 12, 2021

Oops, didn't notice this was already merged.

@ranjeshj ranjeshj added area-Styling and removed needs-triage Issue needs to be triaged by the area owners labels Jan 13, 2021
<SolidColorBrush x:Key="CircleElevationBorderBrush" Color="{ThemeResource SystemColorWindowTextColor}" />
<SolidColorBrush x:Key="AccentControlElevationBorderBrush" Color="{ThemeResource SystemColorWindowTextColor}" />

<!-- Other -->

<SolidColorBrush x:Key="SystemColorWindowTextColorBrush" Color="{ThemeResource SystemColorWindowTextColor}" />
<SolidColorBrush x:Key="SystemColorWindowColorBrush" Color="{ThemeResource SystemColorWindowColor}" />
Copy link
Contributor

@marksfoster marksfoster Jan 14, 2021

Choose a reason for hiding this comment

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

Instead of creating all these solid color brushes (save on perf) why don't we add the rest of these brushes (SystemColorWindowTextColorBrush and SystemColorWindowColorBrush are already in this location and ready to go):

<SolidColorBrush x:Key="SystemColorWindowColorBrush" Color="{ThemeResource SystemColorWindowColor}" /> <SolidColorBrush x:Key="SystemColorWindowTextColorBrush" Color="{ThemeResource SystemColorWindowTextColor}" /> <SolidColorBrush x:Key="SystemColorHighlightColorBrush" Color="{ThemeResource SystemColorHighlightColor}" /> <SolidColorBrush x:Key="SystemColorHighlightTextColorBrush" Color="{ThemeResource SystemColorHighlightTextColor}" /> <SolidColorBrush x:Key="SystemColorButtonFaceColorBrush" Color="{ThemeResource SystemColorButtonFaceColor}" /> <SolidColorBrush x:Key="SystemColorButtonTextColorBrush" Color="{ThemeResource SystemColorButtonTextColor}" /> <SolidColorBrush x:Key="SystemColorHotlightColorBrush" Color="{ThemeResource SystemColorHotlightColor}" /> <SolidColorBrush x:Key="SystemColorGrayTextColorBrush" Color="{ThemeResource SystemColorGrayTextColor}" />

and convert all the brushes you are adding to static resources ( <StaticResource x:Key="TextFillColorPrimaryBrush" ResourceKey="SystemColorButtonTextColorBrush" />) ?

Kinnara added a commit to Kinnara/ModernWpf that referenced this pull request Jul 9, 2022
Kinnara added a commit to Kinnara/ModernWpf that referenced this pull request Jul 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Styling team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants