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

Update background color of selected ListView item to meet contrast ratio requirement #2962

Conversation

Felix-Dev
Copy link
Contributor

@Felix-Dev Felix-Dev commented Jul 21, 2020

Description

This PR updates the brushes used by the ListView to set the background of selected ListViewItems. A new SystemControlHighlightListAccentMediumLowBrush has been added to set the background of a selected ListViewItem in rest state. Additionally, the brush used by ListViewItemBackgroundSelectedPointerOver has been updated in Dark mode to create a better visual contrast to the selected rest and selected pressed states.

Here are the new opacities used as per this PR:

Selection State Light Dark
Selection Rest 0.75 (previously 0.4) 0.75 (previously 0.6)
Selection Hover 0.6 0.6 (previously 0.8)
Selection Pressed 0.7 0.9

Motivation and Context

Closes #2908

How Has This Been Tested?

Visually (see GIFs below)

GIFs:

Light Dark
listviewitem-selected-light listviewitem-selected-dark

And in Highcontrast (only shown in HC black. HC Light is identical just with a different accent color):
listviewitem-selected-highcontrast-dark

Remarks:

@YuliKl In light theme, the selected pressed opacity (0.7) is now slightly less than the selected rest opacity (0.75). Previously, the selected pressed color had the highest opacity in Light theme of all three selection state colors (as can be seen in the table above). Here is how it would look like if the were to update the selection pressed opacity in Light theme to 0.9 (and thus matching Dark theme):
listviewitem-selected-light-selectionpressed-update

This would also resemble the color scheme used for the Button control:
button-light-theme

What do you think? Should we update the selection pressed opacity from 0.7 to 0.9 in Light theme? If yes, that would probably require us to introduce a new brush, as the SystemControlHighlightListAccentHighBrush used here is defined in Light theme as:

<SolidColorBrush x:Key="SystemControlHighlightListAccentHighBrush" Color="{ThemeResource SystemAccentColor}" Opacity="0.7" />

We likely shouldn't update that brush to 0.9 opacity as that could affect controls besides the ListViewItem. Thus, similar to the newly introduced SystemControlHighlightListAccentMediumLowBrush brush as part of this PR, we could introduce a new brush here which will be used by the ListViewItem in Light theme:

<SolidColorBrush x:Key="SystemControlHighlightListAccent[NewTermHere]Brush" Color="{ThemeResource SystemAccentColor}" Opacity="0.9" />

<ResourceDictionary x:Key="Light">
    <StaticResource x:Key="ListViewItemBackgroundSelectedPressed" ResourceKey="SystemControlHighlightListAccent[NewTermHere]Brush" />
</ResourceDictionary>

What would be a good name here for such a brush? Is it even a good idea to introduce a new system-wide brush here which might only be used by the ListView in Light theme mode for now? Or should we use a solution local to the ListView here?

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Jul 21, 2020
@StephenLPeters
Copy link
Contributor

The remarks sections would be good to have in the the original issue.

@YuliKl
Copy link

YuliKl commented Jul 21, 2020

Agreed that we don't want to change values/opacities of existing brushes.
Let me noodle on the rest of the question and get back to you shortly.

@StephenLPeters StephenLPeters added area-Lists ListView, GridView, ListBox, etc team-Controls Issue for the Controls team accessibility Narrator, keyboarding, UIA, etc and removed needs-triage Issue needs to be triaged by the area owners labels Jul 21, 2020
@YuliKl
Copy link

YuliKl commented Jul 21, 2020

For light theme, the design philosophy we recently introduced is for a control to look lighter (closer to the user) on hover, and darker (further from the user) on press, as compared to the rest state. Button is currently the canonical implementation and I agree with the goal of emulating the same value changes as button's for list view items.

I would prefer adding a new resource globally even if ListViewItem is the only style to use it. As for the name, I don't love this but can't think of anything better than SystemControlHighlightListAccentHigherBrush

@Felix-Dev
Copy link
Contributor Author

@YuliKl So, in Light theme, we would define that new brush like this:

<SolidColorBrush x:Key="SystemControlHighlightListAccentHigherBrush" Color="{ThemeResource SystemAccentColor}" Opacity="0.9" />

In dark theme SystemControlHighlightListAccentHighBrush is already set to opacity 0.9, so I guess a possible value here would be 0.95 then?

As for Highcontrast: We should be fine just copying what the other SystemControlHighlightListAccent* brushes do here, which is using the SystemColorHighlightColor theme resource.

Put together, the new brush would be defined like this then:

<ResourceDictionary x:Key="Light">
    <SolidColorBrush x:Key="SystemControlHighlightListAccentHigherBrush" Color="{ThemeResource SystemAccentColor}" Opacity="0.9" />
</ResourceDictionary>
<ResourceDictionary x:Key="Dark">
    <SolidColorBrush x:Key="SystemControlHighlightListAccentHigherBrush" Color="{ThemeResource SystemAccentColor}" Opacity="0.95" />
</ResourceDictionary>
<ResourceDictionary x:Key="HighContrast">
    <SolidColorBrush x:Key="SystemControlHighlightListAccentHigherBrush" Color="{ThemeResource SystemColorHighlightColor}" />
</ResourceDictionary>

As for the resource name...it does look quite as an outlier considering the naming scheme used. Some brushes use the terms "MediumHigh" to differentiate from "High", like SystemControlHighlightBaseMediumHighBrush. Is that something we could make use of here? Just that we would got the other way around. Instead of creating a "medium" version of a brush we create a "very" version, like SystemControlHighlightListAccentVeryHighBrush? Looking at the values we are assigning to this brush in Light/Dark mode, I doubt there is any room left for a SystemControlHighlightListAccentHigh**est**Brush, so replacing "AccentHigherBrush" with "AccentVeryHighBrush" should be fine. Personally, to me that term would also fit better into the current naming system used for the different brushes.

Your thoughts?

@YuliKl
Copy link

YuliKl commented Jul 22, 2020

I'm fine with the VeryHigh name. For dark theme, I recommend keeping the new resource at the 0.9 opacity that we want list view items to keep using. So:

<ResourceDictionary x:Key="Light">
    <SolidColorBrush x:Key="SystemControlHighlightListAccentVeryHighBrush" Color="{ThemeResource SystemAccentColor}" Opacity="0.9" />
</ResourceDictionary>
<ResourceDictionary x:Key="Dark">
    <SolidColorBrush x:Key="SystemControlHighlightListAccentVeryHighBrush" Color="{ThemeResource SystemAccentColor}" Opacity="0.9" />
</ResourceDictionary>
<ResourceDictionary x:Key="HighContrast">
    <SolidColorBrush x:Key="SystemControlHighlightListAccentVeryHighBrush" Color="{ThemeResource SystemColorHighlightColor}" />
</ResourceDictionary>

This will allow all three declarations of ListViewItemBackgroundSelectedPressed to be consistently

<StaticResource x:Key="ListViewItemBackgroundSelectedPressed" ResourceKey="SystemControlHighlightListAccentVeryHighBrush" />

@Felix-Dev
Copy link
Contributor Author

@YuliKl Added the SystemControlHighlightListAccentVeryHighBrush theme resource as you've suggested.

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters StephenLPeters merged commit 043ed5a into microsoft:master Jul 31, 2020
Kinnara added a commit to Kinnara/ModernWpf that referenced this pull request Aug 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Narrator, keyboarding, UIA, etc area-Lists ListView, GridView, ListBox, etc team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update background color of selected List View item to meet contrast ratio requirement
4 participants