Darken TabControl in dark mode #12471
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #12471 +/- ##
===================================================
+ Coverage 61.34945% 61.35198% +0.00253%
===================================================
Files 1547 1547
Lines 158479 158479
Branches 14751 14751
===================================================
+ Hits 97226 97230 +4
+ Misses 60551 60548 -3
+ Partials 702 701 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
This is not an area I specialize in and could use some guidance. My plan is to help tackle the remaining dark mode help wanted issues if this approach is acceptable. I have done my best to glean from the work that has already been done and have been experimenting with the dark mode themes that I see in .msstyles. Am I correct that in an ideal world these controls would not require this additional styling? |
|
These are also tackled in #11985 |
To me it looks like that PR could be the start of a new API proposal. In this PR, my goal is to fix the issue without introducing any new public APIs. |
| @@ -1771,6 +1779,9 @@ private bool ShouldSerializeItemSize() | |||
| return !_padding.Equals(s_defaultPaddingPoint); | |||
| } | |||
|
|
|||
| private BOOL StyleUpDown(HWND handle) | |||
| => PInvoke.SetWindowTheme(handle, $"{DarkModeIdentifier}_{ExplorerThemeIdentifier}", null).Succeeded; | |||
There was a problem hiding this comment.
We are adding line breaks after the => - https://github.com/dotnet/winforms/blob/main/docs/coding-style.md#line-breaks
There was a problem hiding this comment.
Maybe we should theme only immediate child windows, the ones whose .Parent in this TabControl?
There was a problem hiding this comment.
Maybe we should theme only immediate child windows, the ones whose .Parent in this TabControl?
Is this covered in the design document somewhere? 😄
Let me know what you would like me to do, but my thought is if a user enables dark mode and the control is ultimately not dark, not dark enough, or not consistently dark, then the user might object. However, like you eluded to, some users might expect for immediate and child controls to be styled dark, but not any of the grandchildren. It's a little harder for me to envision why users would expect this scenario but my knowledge, experience, and use cases are more limited than yours. If I were the user I would expect a sensible default with an option to override.
There was a problem hiding this comment.
I thought that goal of our PR was to theme page headers and the spinner control, page bodies and their children are already themed. Is {DarkModeIdentifier}_{ExplorerThemeIdentifier} an appropriate theme identifier for other windows?
|
@willibrandon - had you tested tab controls with various child controls in the pages? Including nested tab controls? |
Tanya-Solyanik
added
the
waiting-author-feedback
|
@Tanya-Solyanik - I need 24 to 48 hours before I can get back to this but I am eager to do so. I've been studying quite a lot about color science and WCAG compliant contrast ratios. |
@Tanya-Solyanik - Just getting back to this now. I will manually test the tab control with various child controls in the pages now. In the meantime, do you have suggestions on how I might test this change in an automated way? Does it make sense to put this in an automated test? And lastly, is there already an established way of testing dark mode support? Please let me know, and if you can describe it or show me an example, then I will automate the test and iterate until everyone is confident in this change. I assume that the assertion would be on the color or background of the child controls, as well as any contrast ratio requirements that should be met. Is there anything else that I should consider? |
|
Hey, @willibrandon. I would like to take reactivate this, test this with other recent patches for DarkMode, and run this to completion. If you have the time to rebase this, I will wait, if not, I would cherry pick your commits into another PR. Thanks, Klaus |
|
@KlausLoeffelmann Thank you for your interest in moving this forward. I've rebased the branch against main and the builds are passing successfully, as you can see in the pipeline. Let me know if there's anything else needed from my side to get this across the finish line. |
Fixes #11953
Proposed changes
Customer Impact
Regression?
Risk
Screenshots
Before
After
Test methodology
Test environment(s)
Microsoft Reviewers: Open in CodeFlow