microsoft / microsoft-ui-xaml

Windows UI Library: the latest Windows 10 native controls and Fluent styles for your applications
MIT License
6.34k stars 677 forks source link

TreeView does not respect SystemAccentColor override #1189

Open dpaulino opened 5 years ago

dpaulino commented 5 years ago

Describe the bug The TreeView will only respect the OS system colour. As a developer, if I override the SystemAccentColor in my App.xaml file, the entire app should respect my new override. However, the TreeView does not respect it.

Steps to reproduce the bug

Steps to reproduce the behavior:

  1. Clone this repo and check out the themeBug branch: https://github.com/dpaulino/treeviewtest/tree/themeBug
  2. Change your OS system colour to anything except for #00CC6A (light green).
  3. Launch the app from Visual Studio 2019 by pressing F5.
  4. Click on any tree item.
  5. You will find the tree item will be highlighted with the OS system colour, while one of the buttons in the sample app will be using the override colour as listed in App.xaml.

Expected behavior The tree should respect the SystemAccentColor override.

Screenshots

image

Version Info

NuGet package version: 2.1.190606001

Windows 10 version Saw the problem?
Insider Build (xxxxx)
May 2019 Update (18362) yes
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)
Device form factor Saw the problem?
Desktop Yes
Mobile
Xbox
Surface Hub
IoT

Additional context

jevansaks commented 5 years ago

Sounds like an issue with the resource lookup/override stuff.

dpaulino commented 5 years ago

hi all, any updates on this? My customers are wanting the ability to change the app's colour manually without changing their OS colours. This issue with the TreeView is blocking me.

jevansaks commented 5 years ago

Have you tried putting the color override in a different place in the app.xaml? It might need to be "earlier" than the inclusion of XamlControlsResources in order to take effect. But it's more likely that accent colors is just not a set of resources that's possible to override in the way you're doing it.

That said, in Windows 1803 and greater there is the ColorPaletteResources class which lets you override accent color more reliably and you could try that out.

dpaulino commented 5 years ago

So you're saying to change this

    <Application.Resources>
        <ResourceDictionary>

            <ResourceDictionary.MergedDictionaries>
                <XamlControlsResources xmlns="using:Microsoft.UI.Xaml.Controls"/>
            </ResourceDictionary.MergedDictionaries>

            <Color x:Key="SystemAccentColor">#00CC6A</Color>

        </ResourceDictionary>
    </Application.Resources>

to this?

    <Application.Resources>
        <ResourceDictionary>

            <Color x:Key="SystemAccentColor">#00CC6A</Color>

            <ResourceDictionary.MergedDictionaries>
                <XamlControlsResources xmlns="using:Microsoft.UI.Xaml.Controls"/>
            </ResourceDictionary.MergedDictionaries>

        </ResourceDictionary>
    </Application.Resources>
jevansaks commented 5 years ago

Not quite, I think you'd have to order them in the MergedDictionaries list like:

    <Application.Resources>
        <ResourceDictionary>

            <ResourceDictionary.MergedDictionaries>
                <ResourceDictionary>
                    <Color x:Key="SystemAccentColor">#00CC6A</Color>
                </ResourceDictionary>
                <XamlControlsResources xmlns="using:Microsoft.UI.Xaml.Controls"/>
            </ResourceDictionary.MergedDictionaries>

        </ResourceDictionary>
    </Application.Resources>
dpaulino commented 5 years ago

Just tried your suggestion, @jevansaks image Didn't work 😞

jevansaks commented 5 years ago

And did you try ColorPaletteResources yet?

dpaulino commented 5 years ago

Is there a way of using that in older versions of Windows 10? I get an error when I try to use ColorPaletteResources: image

jevansaks commented 5 years ago

You would need to use conditional XAML to use it only 17134+.

dpaulino commented 5 years ago

Still not working 😞

image

dpaulino commented 5 years ago

Hi all, any updates on this issue? My customers are asking to be able to customize my app's colours, but I cannot implement this feature due to this treeview issue

dpaulino commented 5 years ago

Friendly ping. Any thoughts on whether this will be addressed or not?

dpaulino commented 5 years ago

Hello again, I'm wondering if there's an estimate of the work required for this bug to be fixed?

chrisglein commented 4 years ago

Cloned the branch and tried the repro. Button (set to AccentButtonStyle) picks up the value from ColorPaletteResources, TreeView's selected state does not. I don't know if this is a TreeView issue or a general styling issue similar to #430.

TreeViewItem's selected state maps to TreeViewItemBackgroundSelected which in turn maps to SystemControlHighlightAccent3RevealBackgroundBrush. You'd think this would be hit by the accent color.

Needs further investigation.

dpaulino commented 4 years ago

Anything I can do to help with the investigation? If it's a simple fix, I'd be glad to try to fix this myself. (I'd have to learn how to set up the whole library repo but I'd still be happy to try)

dpaulino commented 4 years ago

Friendly ping. Anyone have ideas?

Felix-Dev commented 4 years ago

@dpaulino As a workaround for now, you could use the accent brushes used by the ListView:

<c:TreeView ItemsSource="{x:Bind ViewModel.Nodes}"
            DragItemsCompleted="TreeView_DragItemsCompleted"
            DragItemsStarting="TreeView_DragItemsStarting">
    <c:TreeView.Resources>
        <StaticResource x:Key="TreeViewItemBackgroundSelected" ResourceKey="ListViewItemBackgroundSelected" />
        <StaticResource x:Key="TreeViewItemBackgroundSelectedPointerOver" ResourceKey="ListViewItemBackgroundSelectedPointerOver" />
        <StaticResource x:Key="TreeViewItemBackgroundSelectedPressed" ResourceKey="ListViewItemBackgroundSelectedPressed" />
    </c:TreeView.Resources>
    <c:TreeView.ItemTemplate>
        <DataTemplate x:DataType="local:ExplorerItem">
            <c:TreeViewItem ItemsSource="{x:Bind Children}" Content="{x:Bind Name}"/>
        </DataTemplate>
    </c:TreeView.ItemTemplate>
</c:TreeView>

treeviewitem-accent-color

The issue you are seeing here is that the brushes used for the TreeViewItem are not picking up the overriden SystemAccentColor. The ListViewItem brushes do, however, because they are directly consuming the overriden SystemAccentColor:

<StaticResource x:Key="ListViewItemBackgroundSelected" ResourceKey="SystemControlHighlightListAccentLowBrush" />
<SolidColorBrush x:Key="SystemControlHighlightListAccentLowBrush" Color="{ThemeResource SystemAccentColor}" Opacity="0.4" />

<StaticResource x:Key="TreeViewItemBackgroundSelected" ResourceKey="SystemControlHighlightAccent3RevealBackgroundBrush" />
<RevealBackgroundBrush x:Key="SystemControlHighlightAccent3RevealBackgroundBrush" TargetTheme="Light" Color="{ThemeResource SystemAccentColorLight3}" FallbackColor="{ThemeResource SystemAccentColorLight3}" />

SystemAccentColorLight3 is not updated (or its update is not propagated down to the app) when SystemAccentColor is overriden. See for example the following XAML (with <Color x:Key="SystemAccentColor">#00CC6A</Color> overriden on the app level):

<StackPanel Orientation="Horizontal" Margin="0,40,0,40" HorizontalAlignment="Center">
    <Button Content="TreeViewItemSelectedBrush" Margin="0,0,10,0" Background="{ThemeResource SystemControlHighlightAccent3RevealBackgroundBrush}"/>
    <Button Content="ListViewItemSelectedBrush" Background="{ThemeResource SystemControlHighlightListAccentLowBrush}"/>
</StackPanel>

with the result: image

This particular bug for the SystemControlHighlightAccent3RevealBackgroundBrush can only be fixed with WinUI 3. In the meantime, you can use the brushes used by the ListView as a workaround. For the TreeView specifically, WInUI could change the default brushes to the brushes used by the ListView, though there probably was a reason why those weren't chosen in the first place.

michael-hawker commented 2 years ago

@Felix-Dev was this related (or what led) to #2913?

dpaulino commented 1 year ago

Bumping