Fix DarkMode (a) StatusStrip background renderer A11Y Regression. #13360
Conversation
KlausLoeffelmann
force-pushed
the
DarkMode_a_FixStatusStripRegression
branch
from
aae8556 to
85d701e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13360 +/- ##
===================================================
- Coverage 76.60074% 76.55212% -0.04862%
===================================================
Files 3230 3231 +1
Lines 639097 639593 +496
Branches 47289 47352 +63
===================================================
+ Hits 489553 489622 +69
- Misses 145962 146403 +441
+ Partials 3582 3568 -14
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
KlausLoeffelmann
force-pushed
the
DarkMode_a_FixStatusStripRegression
branch
3 times, most recently
from
8e9ff57 to
5ea89b6
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR addresses several A11Y regressions related to DarkMode rendering by removing experimental diagnostic attributes and refining related rendering logic. Key changes include the removal of experimental DarkMode attributes in form APIs, the introduction and integration of a dedicated dark mode renderer in ToolStripSystemRenderer, and various rendering improvements in ToolStripRenderer and StatusStrip.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| FormCornerPreference.cs | Removed experimental diagnostic attribute for DarkMode support. |
| Form.cs | Removed experimental attribute on DarkMode–related properties and events. |
| ToolStripSystemRenderer.cs | Introduced a dedicated dark mode renderer and updated RendererOverride logic. |
| ToolStripRenderer.cs | Updated rendering paths and optimized array initializations for DarkMode disabled images. |
| ToolStrip.cs | Modified background painting to use renderer overrides when available. |
| StatusStrip.cs | Updated comment spelling for clarity. |
| PublicAPI.Unshipped.txt | Updated API documentation entries for FormCornerPreference and related events. |
Comments suppressed due to low confidence (6)
src/System.Windows.Forms/System/Windows/Forms/FormCornerPreference.cs:11
- Verify that the removal of the experimental DarkMode diagnostic attribute is intended and that any related tooling or documentation has been updated accordingly.
-[Experimental(DiagnosticIDs.ExperimentalDarkMode, UrlFormat = DiagnosticIDs.UrlFormat)]
src/System.Windows.Forms/System/Windows/Forms/Form.cs:2185
- Ensure that removing the experimental attribute from the Form properties aligns with production readiness for DarkMode, and update any related diagnostic handling as needed.
- [Experimental(DiagnosticIDs.ExperimentalDarkMode, UrlFormat = DiagnosticIDs.UrlFormat)]
src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/StatusStrip.cs:121
- Correct the spelling of 'accomodate' to 'accommodate' in the comment for clarity.
// we do some custom stuff with padding to accomodate size grip.
src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStripSystemRenderer.cs:18
- Confirm that prioritizing high contrast mode over dark mode in the RendererOverride is intended and that the newly introduced dark mode renderer meets accessibility guidelines.
+ private ToolStripRenderer? _toolStripDarkModeRenderer;
src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStripRenderer.cs:855
- Review the logic change for rendering item check images to ensure that returning early when the image rectangle is empty or the image is null does not inadvertently skip intended rendering in edge cases.
+ if (imageRect == Rectangle.Empty || image is null)
src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStrip.cs:3651
- [nitpick] Ensure that using the renderer override to draw the ToolStrip background and returning early does not conflict with any custom rendering requirements that downstream consumers might have.
+ if (Renderer.RendererOverride is ToolStripRenderer renderer)
3a1b788 to
031811f
Compare
KlausLoeffelmann
force-pushed
the
DarkMode_a_FixStatusStripRegression
branch
from
e78aef4 to
f65907a
Compare
| @@ -546,24 +546,12 @@ public static bool VisualStylesEnabled | |||
|
|
|||
| internal static ToolStripRenderer CreateRenderer(ToolStripManagerRenderMode renderMode) | |||
| { | |||
| switch (renderMode) | |||
| return renderMode switch | |||
There was a problem hiding this comment.
nit: you can turn this into => now.
| @@ -6,47 +6,83 @@ | |||
|
|
|||
| namespace System.Windows.Forms; | |||
|
|
|||
| /// <summary> | |||
| /// Provides system rendering for ToolStrip controls with support for dark mode. | |||
There was a problem hiding this comment.
Please indent all XML comments by a space.
| private ToolStripRenderer? _toolStripHighContrastRenderer; | ||
| private ToolStripRenderer? _toolStripDarkModeRenderer; |
There was a problem hiding this comment.
Shouldn't these be static? I don't see mutable state after construction for the high contrast renderer.
There was a problem hiding this comment.
We can revisit this. The HighContrastRenderer was here before and wasn't static.
So, I will leave the DarkModeRenderer here for the time being.
It's not critical, because StatusStrip is not a control which you'll have in hundreds on a Form.
| @@ -203,28 +239,34 @@ protected override void OnRenderToolStripBackground(ToolStripRenderEventArgs e) | |||
| } | |||
| else if (DisplayInformation.LowResolution) | |||
| { | |||
| FillBackground(g, bounds, (toolStrip is ToolStripDropDown) ? SystemColors.ControlLight : e.BackColor); | |||
| FillBackground(g, bounds, (toolStrip is ToolStripDropDown) | |||
There was a problem hiding this comment.
nit:
| FillBackground(g, bounds, (toolStrip is ToolStripDropDown) | |
| FillBackground(g, bounds, toolStrip is ToolStripDropDown |
| } | ||
| else if (toolStrip.IsDropDown) | ||
| { | ||
| FillBackground(g, bounds, (!ToolStripManager.VisualStylesEnabled) ? | ||
| e.BackColor : SystemColors.Menu); | ||
| FillBackground(g, bounds, (!ToolStripManager.VisualStylesEnabled) |
There was a problem hiding this comment.
Reversing the ! would be clearer and the parenthesis isn't needed.
| new(8, 0, 2, 2), | ||
| private static readonly Rectangle[] s_baseSizeGripRectangles = | ||
| [ | ||
| new(12, 0, 2, 2), |
There was a problem hiding this comment.
See added screenshots for explanaition.
| if (isDarkMode) | ||
| { | ||
| // Dark mode color matrix | ||
| float[][] greyscale = |
There was a problem hiding this comment.
This is very inefficient. There is now a new constructor in .NET 9 that you can pass this as a span of floats. We should do a follow up where we convert this code and the helper to work with ColorMatrix directly.
There was a problem hiding this comment.
Agree to follow-up later on this.
Instead of doing the work outselves, however, I suggest a different approach.
See #13441. I will leave this for now.
| /// <param name="e">The event arguments containing rendering information.</param> | ||
| /// <param name="arrowColor">The color to use for the arrow.</param> | ||
| /// <returns>The rendered arrow points.</returns> | ||
| private protected Point[] RenderArrowCore( |
There was a problem hiding this comment.
Why are we returning the array here?
|
|
||
| arrow = e.Direction switch | ||
| Point[] arrow = e.Direction switch |
There was a problem hiding this comment.
Should stackalloc Span<Point> now and pass that to FillPolygon
| } | ||
|
|
||
| private protected void OnRenderStatusStripSizingGrip( | ||
| ToolStripRenderEventArgs eArgs, |
There was a problem hiding this comment.
| ToolStripRenderEventArgs eArgs, | |
| ToolStripRenderEventArgs e, |
dotnet-policy-service
bot
added
waiting-author-feedback
KlausLoeffelmann
force-pushed
the
DarkMode_a_FixStatusStripRegression
branch
from
f65907a to
82e62d9
Compare
@merriemcgaw, @Tanya-Solyanik, @Shyam-Gupta: First of all, @jeremy, thanks for the review! While reading @JeremyKuhne review comments, I got the idea, that many of his comments can be in fact generelized Copilot instructions, which we could just "run" before we submit a PR. I think that would drastically reduce the workload, both for review and for keeping all those little optinization details in mind, and we could actually concentrate more on the domain-specific work. I created #13441, and my suggestion would be, that if someone sees an optimization potential, which would also work in a generic way, that they not only comment, but also add those to a list of general CoPilot instructions. This list can grow over time, and will exponentially save time, with this kind of work. What do y'all think? I have experimented over the weekend with a few approaches, and I think this looks promesing. |
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Recording.2025-05-13.030723.mp4
@KlausLoeffelmann
@JeremyKuhne
Why not just to be
private static void RenderStatusStripBackground(ToolStripRenderEventArgs e)
{
if (Application.RenderWithVisualStyles)
{
VisualStyleRenderer vsRenderer = VisualStyleRenderer!;
#pragma warning disable WFO5001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
VisualStyleElement visualStyleElement = Application.IsDarkModeEnabled
? VisualStyleElement.CreateElement("DarkMode::ExplorerStatusBar", 0, 0)
: VisualStyleElement.Status.Bar.Normal;
#pragma warning restore WFO5001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
;
vsRenderer.SetParameters(visualStyleElement);
vsRenderer.DrawBackground(e.Graphics, new Rectangle(0, 0, e.ToolStrip.Width - 1, e.ToolStrip.Height - 1));
}
else
{
if (!SystemInformation.InLockedTerminalSession())
{
e.Graphics.Clear(e.BackColor);
}
}
}
Can you use Dark Mode Render instead of accessibility drawing please?
dotnet-policy-service
bot
added
the
waiting-author-feedback
Hey @RussKie, I cannot see the screenshot you assumingly posted - can you fix that? Also, the plan is: |
dotnet-policy-service
bot
added
untriaged
KlausLoeffelmann
added
area-DarkMode
This comment was marked as resolved.
This comment was marked as resolved.
…background painting.
...proof, that the existing Unit Tests are doing a fantastic job!
KlausLoeffelmann
force-pushed
the
DarkMode_a_FixStatusStripRegression
branch
from
cd0eb39 to
b999aad
Compare
Fixes #12909.
Fixes #12663.
Fixes #12041.
Fixes #12040.
Fixes #12027.
As for the approach.
We already have a series of Renderers, where we need to avoid passing rendering down to the visual styles. In fact, for the ToolStrip and everything around it, we do most of the render work ourselves.
That also means, we're in many cases no longer compatible with subtle details. The rounded corners of the Windows 11 windows for example, can obscure the symbolic Grip, depending on the Window border color and the resulting Antialias, and leave not enough distance for a good contrast:
And then in addition, since we have WAY higher resolutions since back then, we also needed a bigger area for better contrast and visibility. This change addresses that, and also reposition the grip pixel to follow the shape of the rounded corners:
Microsoft Reviewers: Open in CodeFlow