Open dipeshmsft opened 1 week ago
LGTM.
I think it may be possible to reduce the number of properties that we are exposing here, if the operations to calculate the variations of AccentColor are simpler.
This was widely discussed a few years ago in the WinUI and CommunityToolkit repositories. Accent color calculations are not a simple algorithm and it's hidden by Microsoft within Windows. Your design team considers this algorithm proprietary and you won't release it publicly. This means everyone has to read AccentLight1-3, AccentDark1-3, etc. directly from Windows which does the calculation.
The calculation is fairly complex too and it making sure the Accent colors 1-3 are calculated from the base color in a way that ensures contrast and good visibility. It isn't simply adjusting the luminence, value or brightness of colors. It also adjusts Hue in some cases.
In earlier versions of Windows, visual styles were defined using system colors and brushes, and corresponding to that we had SystemColors static class that contained getters to all the system colors, brushes and corresponding resource keys for using the colors and brushes in XAML.
Yes, but keep in mind the structure of Fluent theme is as follows. I simplified by removing light/dark/contrast variations and non-color resources like FontSize, Padding, etc.
0 System provided colors (accent)
1 Base Fluent Colors & Brushes
2 Fluent Control-Specific Brushes
3 Fluent Control Theme Styles
Mapping the Fluent theme to SystemColors essentially means you need to provide levels 0 and 1. Level zero is the accent colors. But level 1 is:
We need access to level 2 resources to be able to re-template but that is part of another discussion.
@robert-abeo , thanks for the insight on the history of accent colors. It pretty much means that we would have to access all of them from WinUI's UISetting type then.
Yeah, while working with Fluent I have understood the layering in the resources and styles and I will try to keep that in order. But I have included the brush keys here because that has been the general use in theme styles. So basically it will support either scenario.
cc @ThomasGoulet73 @lindexi @batzen @Symbai @pomianowski @rladuca @vatsan-madhavan
Looks great, what about the change while the application is running, maybe some 'AccentColorsChanged' delegate?
These would be updated, so if you use DynamicResource with the keys, the color will change when it changes in the system. There is also UserPreferenceChanged event generated when the theme changes.
LGTM for SystemColors.
If the community is okay with the shape of this API and there are no objections, I will go ahead and mark it as ready for review tomorrow.
Based on latest strategy for a "hybrid" registration of styles I think it does make sense to only extend SystemColors with the accent colors.
Summarizing the longer comment above, Fluent2 defines system colors in XAML resources directly (a small subset below). If we were to loose access to these by string key the next best option would have been to add as many as possible to SystemColors. That will no longer be needed. The hope is in .NET 10+ WPF will stop mangling theme style resource keys as well so this can all be simplified further in the future again. Either way SystemColors won't need this information now.
Note on the keys: You've dropped the System
prefix which will require changing the references for all of us with existing Fluent2 theme styles. I think that makes sense though if you are placing them in SystemColors
.
If you went with the separate AccentColorManager
or even a FluentColors
class, I would expect the keys to match 1-and-1 with WinUI though.
Side-note, you might find something useful here:
LGTM except a small question: Is the OS call used to retrieve the values of these properties available/working on all supported versions of Windows ? And if not, what should be the behavior in this case.
@ThomasGoulet73, no we used a WinRT type UISettings to retreive the values, this is present from Windows 10. In the case where we don't have this type to provide a value, we will provide a fallback for the same.
Background and Motivation
Since Windows 10 onwards, Accent color has been one of the key compoenents of visual styles in Windows controls. However, there is no easy way to get the system's accent color. Accent colors can be used in custom styles to align the styling of controls with what the OS is showing.
In earlier versions of Windows, visual styles were defined using system colors and brushes, and corresponding to that we had
SystemColors
static class that contained getters to all the system colors, brushes and corresponding resource keys for using the colors and brushes in XAML.Since, Accent color and it's variations are also system wide defined colors, we propose to expose them using the
SystemColors
class.API Proposal
I think it may be possible to reduce the number of properties that we are exposing here, if the operations to calculate the variations of AccentColor are simpler. We don't know that yet so, I would like to keep these APIs experimental.
Alternative Design
Rather than having the accent colors defined here, we can introduce a new class, say
AccentColorManager
which will store the system accent and opens up possibility for theme related customization.