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

Proposal: Update background color of selected ListBox item to meet contrast ratio requirement #2979

Closed
Felix-Dev opened this issue Jul 23, 2020 · 7 comments · Fixed by #3053
Labels
accessibility Narrator, keyboarding, UIA, etc area-Lists ListView, GridView, ListBox, etc area-Styling help wanted Issue ideal for external contributors team-Controls Issue for the Controls team

Comments

@Felix-Dev
Copy link
Contributor

Felix-Dev commented Jul 23, 2020

Describe the bug
This is similar to the bug as reported for selected ListView items in #2908: Selected [ListBoxItems] 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 ListBoxItem uses the exaxt same brushes as the ListViewItem for its selected rest/hover/pressed visual states:

<VisualState x:Name="Selected">

This leads to a color contrast between selected and unselected items of 1.9:1 in dark mode:
image

And a color contrast of 1.7:1 in light mode:
image

As a fix I propose to use the same brushes we settled on in PR #2962 to fix the color contrast ration for the ListViewItems. This would also keep the selection brushes used consistent between the ListBoxItem and the ListViewItem.

As you saw in the linked control template code above, the ListBox currently does not have an own set of theme resources corresponding to the ListViewItems: A theme resource like ListBoxItemBackgroundSelected does not exist. Instead, the default ListBoxItem control template references the different SystemControlHighlightListAccent*Brush theme resources directly. We could either

The ListBox control seems to be in a low-priority mode based on the documentation (similar perhaps to the Pivot control?):
image

As such, it might not be worth the effort to introduce a whole bunch of ListBoxItem* theme resoures and we should just update the control template to reference the same SystemControlHighlightListAccent*Brushes the ListViewItem now does due to PR #2962. If, however, there are still valid use cases for the ListBox control in new apps (and not just ported Windows 8 apps) then we should invest the effort into creating a set of dedicated theme resources for the ListBoxItem and I would be willing to do that. I would like input from the team here (like @YuliKl).

Note: There are theme resources like ListBoxItemSelectedBackgroundThemeBrush but they all seem to be unused in the default ListBoxItem template. Can we do something about that? It's confusing to developers that the ListBox theme resource file contains a bunch of theme resources which are completely unused. If we cannot remove them for now because of the breaking apps concern, could we at least indicate via a comment that those theme resources are not in use so there is no point for developers to override them in their apps? They are also no candidate for the set of dedicated ListBoxItem theme resources we could create as part of fixing this bug here because they don't follow the current naming scheme used throughout WinUI for these theme resources.

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

YuliKl commented Jul 23, 2020

Thank you for filing this issue, I would love to see it fixed. The note in the docs is accurate, we are not making active investments in ListBox - although I would like to make an exception for this MAS requirement. To minimize code churn, I would prefer option 1 as the direction of the fix:

I'm not certain whether we can update the control template of this inbox control in WinUI 2 without waiting for WinUI 3. Hopefully we can, as this sounds similar in spirit to e.g. rounded corners changes we've already made.

The existence of unused theme resources is very annoying but right now may not be the appropriate time to fix this situation. Perhaps we need to wait until WinUI 3 to delete these unused brushes. That feels like the right time to also change ListBox's template to use light-weight styling resources with names that match the current naming convention. Although given our lack of investment in ListBox, I'm not certain this change would be a good use of anyone's time.

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Jul 23, 2020

@YuliKl

I'm not certain whether we can update the control template of this inbox control in WinUI 2 without waiting for WinUI 3. Hopefully we can, as this sounds similar in spirit to e.g. rounded corners changes we've already made.

We absolutely can! The ListBoxItem control template was moved out of the OS into the WinUI 2 repo previously and I just checked that overriding the used brushes for selection works as it does for the ListViewItem - which is the case 🙂

The existence of unused theme resources is very annoying but right now may not be the appropriate time to fix this situation. Perhaps we need to wait until WinUI 3 to delete these unused brushes. That feels like the right time to also change ListBox's template to use light-weight styling resources with names that match the current naming convention. Although given our lack of investment in ListBox, I'm not certain this change would be a good use of anyone's time.

Yep, for now it will be fine to just update the referenced brushes without investing time into creating a whole set of new ListBox specific theme resources. After all, even if we only update the referenced brushes, we will still

  • improve the color contrast for selected ListBoxItems and match ListViewItems and
  • developers can still customize the used brushes - they just have to locally override a SystemControlHighlightListAccent*Brush instead of, for example, a ListBoxItemSelectedBackground resource

I would also much rather invest time into improving the lightweight customization story for a control like the NumberBox (tracked in #2844) instead of a control like the ListBox which has already been superseded by another control like the ListView.

I will create a PR following option 1 then once PR #2962 has been merged.

@StephenLPeters StephenLPeters added area-Lists ListView, GridView, ListBox, etc area-Styling team-Controls Issue for the Controls team accessibility Narrator, keyboarding, UIA, etc help wanted Issue ideal for external contributors and removed needs-triage Issue needs to be triaged by the area owners labels Jul 23, 2020
@mdtauk
Copy link
Contributor

mdtauk commented Jul 24, 2020

This begs the question: How should deprecation be handled in the future?
Should there be a WinUI 3.x build that receives changes that are not breaking, as 3.x.y releases. With more breaking changes in 3.x (and massive overhaul changes in 4.x, 5.x releases)?

@Felix-Dev
Copy link
Contributor Author

@mdtauk It was suggested that WinUI 4.0 could be the first version where we could make these kinds of breaking changes (while that context was only about theme resources, this should be valid for other public APIs too). If semver is followed, 3.x versions shouldn't contain breaking changes.

@mdtauk
Copy link
Contributor

mdtauk commented Jul 24, 2020

@mdtauk It was suggested that WinUI 4.0 could be the first version where we could make these kinds of breaking changes (while that context was only about theme resources, this should be valid for other public APIs too). If semver is followed, 3.x versions shouldn't contain breaking changes.

I understand that Felix, but will the team be backporting fixes to 3.x.y versions, whilst releasing 4.x versions?

What will be the lifetime for a version of WinUI?

@Felix-Dev
Copy link
Contributor Author

I'm not sure on that. While @ryandemopoulos did say that WinUI 2 will be kept supported for a while after WinUI 3.0 has been released, this is arguably a special situation (host backdrop acrylic, etc...) so it might be handled differently than future major WinUI releases.

@mdtauk
Copy link
Contributor

mdtauk commented Jul 24, 2020

I'm not sure on that. While @ryandemopoulos did say that WinUI 2 will be kept supported for a while after WinUI 3.0 has been released, this is arguably a special situation (host backdrop acrylic, etc...) so it might be handled differently than future major WinUI releases.

I am not expecting there to be an answer at this stage, but it may be a conversation worth having at some point, so it can be added to public facing information, and appropriate branches and dev mechanisms can be in place once the code goes open.

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 area-Styling help wanted Issue ideal for external contributors team-Controls Issue for the Controls team
Projects
None yet
5 participants