microsoft / microsoft-ui-xaml

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

Update background color of selected List View item to meet contrast ratio requirement #2908

Closed YuliKl closed 4 years ago

YuliKl commented 4 years ago

Describe the bug

Selected ListViewItems do not currently meet the 3:1 contrast ratio requirement relative to unselected items, as specified in MAS 1.4.11 - Non-text Contrast.

The selected color needs to be lighter in dark theme (currently it's 2.2:1): image

The selected color needs to be darker in light theme (currently it's 1.8:1): image

Additional context

Copied in part from an internal bug

Related to #1233

Proposed fix

https://github.com/microsoft/microsoft-ui-xaml/issues/2908#issuecomment-659632415

mdtauk commented 4 years ago

Does this not change depending on the user's chosen Accent colour? Or is this something that needs to change with how the OS generates the lighter and darker tones of the colour?

marcelwgn commented 4 years ago

The normal Accent color generally has enough contrast, but it seems that listview is using a darker version. I think treeview also needs to update, their selected item is even darker and can have a contrast ratio of 1.266:1.

mdtauk commented 4 years ago

The normal Accent color generally has enough contrast, but it seems that listview is using a darker version. I think treeview also needs to update, their selected item is even darker and can have a contrast ratio of 1.266:1.

You say darker version - I think its using the accent colour with some transparency.

marcelwgn commented 4 years ago

Oh right, yes it needs less transparency indeed. Was there a specific reason on why we chose to make them partially transparent in the first place?

YuliKl commented 4 years ago

We need to ensure that the default blue accent color meets contrast ratio requirements. There are too many custom accent color possibilities to be able to make any assurances for other colors.

I don't know why transparent colors were chosen. It's possible that the list selection colors predate the availability of darker/lighter accent color variants to generic.xaml

marcelwgn commented 4 years ago

So for the default blue, the contrast ratios are:

Background AccentColor AccentColorDark1 AccentColorLight1
White 4.498:1 7.103:1 2.959:1
Black 4.667:1 2.735:1 7.096:1

So we either use the AccentColor or AccentColorDark1 for white background (AccentColorDark1 for black background) or we update/redefine the existing brushes,

YuliKl commented 4 years ago

TreeView is using SystemControlHighlightAccent3RevealBackgroundBrush, which as @chingucoding pointed out, also lacks the appropriate contrast ratio. As do the Accent2 variants. I'm not seeing any existing reveal brushes that use Accent1 variants, so I think one will need to be added.

I'll open a new issue for TreeView's selected color.

Felix-Dev commented 4 years ago

@YuliKl Adding the new SystemControlHighlightListAccentMediumLowBrush brush with the following values

<ResourceDictionary x:Key="Light">
    <SolidColorBrush x:Key="SystemControlHighlightListAccentMediumLowBrush" Color="{ThemeResource SystemAccentColor}" Opacity="0.75" />
</ResourceDictionary>
<ResourceDictionary x:Key="Dark">
    <SolidColorBrush x:Key="SystemControlHighlightListAccentMediumLowBrush" Color="{ThemeResource SystemAccentColor}" Opacity="0.75" />
</ResourceDictionary>
<ResourceDictionary x:Key="HighContrast">
    <SolidColorBrush x:Key="SystemControlHighlightListAccentMediumLowBrush" Color="{ThemeResource SystemColorHighlightColor}" />
</ResourceDictionary>
and changing the ListViewItemBackgroundSelected resource to consume it will give us the following result (using default blue color): Theme Proposed change vs current design
Light listview-selectionbrush-light
Dark listview-selectionbrush-dark
HC Dark listview-selectionbrush-highcontrast

As you have perhaps noticed we've lost some color contrast between the selected rest state and selected hower state in dark mode. Here are the current values in dark mode:

<StaticResource x:Key="ListViewItemBackgroundSelected" ResourceKey="SystemControlHighlightListAccentLowBrush" />
<StaticResource x:Key="ListViewItemBackgroundSelectedPointerOver" ResourceKey="SystemControlHighlightListAccentMediumBrush" />
<StaticResource x:Key="ListViewItemBackgroundSelectedPressed" ResourceKey="SystemControlHighlightListAccentHighBrush" />

<SolidColorBrush x:Key="SystemControlHighlightListAccentHighBrush" Color="{ThemeResource SystemAccentColor}" Opacity="0.9" />
<SolidColorBrush x:Key="SystemControlHighlightListAccentLowBrush" Color="{ThemeResource SystemAccentColor}" Opacity="0.6" />
<SolidColorBrush x:Key="SystemControlHighlightListAccentMediumBrush" Color="{ThemeResource SystemAccentColor}" Opacity="0.8" />

As you can see, we currently use an opacity of 0.6 (selected rest) and 0.8 (selected hover). With this change, we would get 0.75 (selected rest) vs 0.8 (selected hover). Perhaps we should increase that opacity difference here for better visibility?

In the Gifs above, the "light bulb effect" creates a greater visual difference in color than there actually is. Please see the following GIF for a demonstration (you should look at the upper right corner of the ListViewItem): listview-selectionbrush-dark-contrast

In Light theme, this issue is less present: listview-selectionbrush-light-contrast

We can also see this in the current brushes:

<StaticResource x:Key="ListViewItemBackgroundSelected" ResourceKey="SystemControlHighlightListAccentLowBrush" />
<StaticResource x:Key="ListViewItemBackgroundSelectedPointerOver" ResourceKey="SystemControlHighlightListAccentMediumBrush" />
<StaticResource x:Key="ListViewItemBackgroundSelectedPressed" ResourceKey="SystemControlHighlightListAccentHighBrush" />

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

Before this change, opacity values of 0.4 (selected rest) and 0.6 (selected hover) is used. With this update, we will change that to 0.75 (selected rest) and 0.6 (selected hover).

mdtauk commented 4 years ago

Could you check how these colours would look in an Acrylic flyout control? The off white and off black colours may have an impact on the new selection colours.

Felix-Dev commented 4 years ago

@mdtauk You mean for example in a ComboBox control?

The ComboBox currently uses the same brushes as the ListView, so those might have to be updated as well:

<StaticResource x:Key="ComboBoxItemBackgroundSelected" ResourceKey="SystemControlHighlightListAccentLowBrush" />
<StaticResource x:Key="ComboBoxItemBackgroundSelectedPointerOver" ResourceKey="SystemControlHighlightListAccentMediumBrush" />
<StaticResource x:Key="ComboBoxItemBackgroundSelectedPressed" ResourceKey="SystemControlHighlightListAccentHighBrush" />

(I am currently making a comparison how the updated brush would look in a ComboBox flyout.)

mdtauk commented 4 years ago

@mdtauk You mean for example in a ComboBox control?

The ComboBox currently uses the same brushes as the ListView, so those might have to be updated as well:

<StaticResource x:Key="ComboBoxItemBackgroundSelected" ResourceKey="SystemControlHighlightListAccentLowBrush" />
<StaticResource x:Key="ComboBoxItemBackgroundSelectedPointerOver" ResourceKey="SystemControlHighlightListAccentMediumBrush" />
<StaticResource x:Key="ComboBoxItemBackgroundSelectedPressed" ResourceKey="SystemControlHighlightListAccentHighBrush" />

(I am currently making a comparison how the updated brush would look in a ComboBox flyout.)

ComboBox, And any other flyout controls using those resources

Felix-Dev commented 4 years ago
@mdtauk Here's a GIF showing the updated brush used in a ListView which is in a flyout. The flyout has the same acrylic background as other flyout controls like the ComboBox or MenuFlyout: Theme Proposed change Current brushes
Light listview-selectionbrush-light-flyout listview-selectionbrush-light-flyout-currently
Dark listview-selectionbrush-dark-flyout listview-selectionbrush-dark-flyout-currently

I think we can see the same color contrast issue between selected rest and selected hover visual states exists here (the "light bulb effect" definitely helps here.)

@YuliKl Unfortunately, overriding the different ComboBoxItemBackgroundSelected* theme resources on the app level has no effect on the background of the ComboBoxItems in the ComboBox drop down menu. (Overriding the ListViewItemSelectedBackground resources also didn't change the background of the selected ComboBoxItem). Since the different ComboBoxItemBackgroundSelected theme resources appear ineffective to me here this is a) an issue which should be fixed and b) I'm not sure how to actually change the background of the ComboBoxItems then.

YuliKl commented 4 years ago

Purely from the accessibility perspective, the similarity in selected vs selected+hovered colors is acceptable. The WCAG 1.4.11 criterion clarifies that:

This Success Criterion does not require that changes in color that differentiate between states of an individual component meet the 3:1 contrast ratio when they do not appear next to each other. For example, there is not a new requirement that visited links contrast with the default color, or that mouse hover indicators contrast with the default state.

I'm happy to take further suggestions from @Felix-Dev, @mdtauk, and anyone else interested in this topic. I primary hope to ensure that our controls meet these accessibility requirements with as little work as possible.

I'll look into the combo box issues you've noted, thank you for pointing out that problem.

mdtauk commented 4 years ago

Have you explored using the alternative shades/tones of Accent Colour, with less transparency on hover and pressed?

YuliKl commented 4 years ago

Have you explored using the alternative shades/tones of Accent Colour, with less transparency on hover and pressed?

Yes, that's the approach used by TreeView, so I've listed the new colors in #2926. I'm not seriously considering switching ListView's colors to these variants to minimize code churn.

Felix-Dev commented 4 years ago

I would add the proposed brush like this then if nobody else is currently working on it:

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

<StaticResource x:Key="ListViewItemBackgroundSelected" ResourceKey="SystemControlHighlightListAccentMediumLowBrush" />

@YuliKl As for the color contrast between selection rets and selection hover states in dark mode: Could we follow the color scheme used by the default button? button-dark

In our ListView case, that could for example look like this then: listviewitem-dark-proposed

We would use the following brushes here:

<StaticResource x:Key="ListViewItemBackgroundSelected" ResourceKey="SystemControlHighlightListAccentMediumLowBrush" />
<StaticResource x:Key="ListViewItemBackgroundSelectedPointerOver" ResourceKey="SystemControlHighlightListAccentLowBrush" />
<StaticResource x:Key="ListViewItemBackgroundSelectedPressed" ResourceKey="SystemControlHighlightListAccentHighBrush" />

<SolidColorBrush x:Key="SystemControlHighlightListAccentMediumLowBrush" Color="{ThemeResource SystemAccentColor}" Opacity="0.75" />
<SolidColorBrush x:Key="SystemControlHighlightListAccentLowBrush" Color="{ThemeResource SystemAccentColor}" Opacity="0.6" />
<SolidColorBrush x:Key="SystemControlHighlightListAccentHighBrush" Color="{ThemeResource SystemAccentColor}" Opacity="0.9" />

We would thus use your proposed brush for the the selection rest state and use the former brush used in the selected rest state (with opacity 0.6) now for the selected hover state. The brush used for the selected pressed state would remain unchanged. Effectively, we will have these opacity differences:

Would reusing the former selected rest brush with opacity 0.6 for the selected hover state be a valid 3:1 contrast ratio concern relative to unselected ListViewItems as well?

(The brushes used in Light mode and High-contrast mode would also remain unchanged as of now.)

Felix-Dev commented 4 years ago

@YuliKl My bad, false alarm about the ComboBoxItem theme resources: I have to use the different ComboBoxItemRevealBackgroundSelected* resources. Those work just fine :)

YuliKl commented 4 years ago

Thank you, @Felix-Dev, I really like your updated suggestion of:

  • selected rest: 0.75
  • selected hover: 0.6 (this is 0.8 currently)
  • selected pressed: 0.9

Please make it so :)

Felix-Dev commented 4 years ago

@YuliKl What would be the best place to add the proposed new brush (SystemControlHighlightListAccentMediumLowBrush) to WinUI? Since this is a brush potentially not only referenced by ListViewItemSelected* brushes but also by other control brushes, I am thinking about creating a new Common_themeresources.xml file for WinUI in its CommonStyles directory. This would then contain the different SystemControlHighlightListAccentMediumLowBrush theme configurations (Light, Dark, HighContrast) and the ListViewItem_themeresources.xml file would consume this brush for its updated ListViewItemSelectedBackground brush.

Currently, these "common" brushes (like SystemControlTransparentBrush) are defined in generic.xaml which is included in the Windows SDK. On first thought, a name like Common_themeresources sounds more fitting to me here than "Generic_themeresources" for example, as the WinUI directory is named "CommonStyles" and "common" sounds more fitting than "generic" to me here.

This new resource file would be used by WinUI to add new "common" theme resources or to modify existing "common" theme resources provided by the Windows SDK.

YuliKl commented 4 years ago

Great questions that @ranjeshj or @StephenLPeters can advise on

StephenLPeters commented 4 years ago

we already have a https://github.com/microsoft/microsoft-ui-xaml/blob/2c8f540c5ed6c1c3d534adb545e816f7626cb330/dev/Common/Common_themeresources.xaml, where we put some of those things. Is this sufficient?

Felix-Dev commented 4 years ago

@StephenLPeters Nice, missed that one (would have expected it in CommonStyles I guess -- how about moving it from Common into CommonStyles?). Yep, that will work.

StephenLPeters commented 4 years ago

@StephenLPeters Nice, missed that one (would have expected it in CommonStyles I guess -- how about moving it from Common into CommonStyles?). Yep, that will work.

I think the story here was that this file existed before we decided to pull the templates for some of the system controls out into winui2, when we did that we added the CommonStyles directory to the project to house all of these templates. If we did these in the other direction I think the Common_themeresources would have ended up with the common styles so I think it makes sense to move it. @ranjeshj @kmahone or @llongley might have opinions too.

Felix-Dev commented 4 years ago

@StephenLPeters Have you heard any concerns about moving Common_themeresources to the CommonStyles folder?

StephenLPeters commented 4 years ago

@StephenLPeters Have you heard any concerns about moving Common_themeresources to the CommonStyles folder?

I haven't heard concerns and I think updating it would make sense.