Fixing `ToolStrip` button background rendering in HC theme #5614

ArtemTatarinov · 2021-08-30T12:11:40Z

Fixes #5502

Proposed changes

false flag is being passed while creating ToolStripHighContrastRenderer instance at ToolStripSystemRenderer, so after this flag is inverted at ToolStripHighContrastRenderer's constructor, the FillWhenSelected flag will be true (ToolStripProfessionalRenderer already has the same logic)
If there's a special overridden renderer for High Contrast theme, then base class should call that renderer's method for rendering backgrounds of ToolStripButton, ToolStripDropDownButton, ToolStripSplitButton (ToolStripProfessionalRenderer already has the same logic)

Customer Impact

Before fix

After fix

Regression?

No

Risk

Minimal

Test methodology

Manually
Unit tests
CTI team

Accessibility testing

Inspect
Accessibility Insights

Test environment(s)

Microsoft Windows [Version 10.0.19043.1165]
.NET SDK 6.0.100-rc.1.21416.15

Microsoft Reviewers: Open in CodeFlow

src/System.Windows.Forms/src/System/Windows/Forms/ToolStripSystemRenderer.cs

dreddy-work · 2021-08-30T21:14:37Z

As per @merriemcgaw , we would need this servicing to 6.0 RC2.

Tanya-Solyanik

Looks good, but please describe testing done. Specifically it is important to test renderers for all possible styles and states of tool strip buttons, checked, CHeckState, and in response to different mouse actions(mouse over, left button down).

Consider adding a test case that enumerates these variations like buttons.cs does to our test app.

src/System.Windows.Forms/src/System/Windows/Forms/ToolStripSystemRenderer.cs

RussKie · 2021-09-01T04:03:19Z

Generally we should write rendering tests, if possible. There are a few example in the codebase, e.g. #4739.

Please ping me if you have any Qs.

dreddy-work

LGTM.

ArtemTatarinov · 2021-09-03T14:52:19Z

Looks good, but please describe testing done. Specifically it is important to test renderers for all possible styles and states of tool strip buttons, checked, CHeckState, and in response to different mouse actions(mouse over, left button down).
Consider adding a test case that enumerates these variations like buttons.cs does to our test app.

I have added ToolStrip buttons to the ToolStripTests form and manually checked different mouse actions (mouse hover, LMB and RMB clicking), different DisplayStyles (Image, Text, ImageAndText) and Checked/CheckStates (Checked, Unchecked, Indeterminate) in Black and White HC themes. I found one issue: for CheckState.Indeterminate the background is rendered wrong just like the original cases of the issue but this time it doesn't matter if the button was selected:

I fixed it and tested again manually all the cases, looks like now it's good.

Issue Description
Background of the buttons with CheckState.Checked and CheckState.Indeterminate is rendered the same way in non-HC mode at .NET 6.0 and .NET Framework 4.7.2 , both states look like each other.

.NET 6.0

.NET Framework 4.7.2

This is happening because Checked property is used for rendering the ToolStrip items, not CheckState property, and the Checked property of the ToolStripButton class is false only for CheckState.Unchecked and is true for CheckState.Checked or CheckState.Indeterminate:

I used the same logic for fixing this issue. At the OnRenderButtonBackground method of the ToolStripHighContrastRenderer class there were two checks:

So, if we want to render the background of the button with CheckState.Indeterminate state in HC mode as it's done in non-HC mode, we should remove the redundant check for CheckState.Checked, because it will leave buttons with CheckState.Indeterminate without correct background rendering:

Tanya-Solyanik · 2021-09-03T19:32:58Z

Background of the buttons with CheckState.Checked and CheckState.Indeterminate is rendered the same way in non-HC mode at .NET 6.0 and .NET Framework 4.7.2 , both states look like each other.

This is unfortunate... But this is an existing bug, we'll wait for external reports before we triage it.

Your fix for indeterminate HC makes sense. Thank you for the follow up!

Tanya-Solyanik

Looks good

Tanya-Solyanik · 2021-09-03T19:40:09Z

Issue to track rendering tests - #5670

vladimir-krestov

Looks good ✔️
Waiting for testing results...

vladimir-krestov · 2021-09-04T08:03:57Z

src/System.Windows.Forms/src/System/Windows/Forms/ToolStripSystemRenderer.cs

@@ -42,7 +42,7 @@ internal ToolStripRenderer HighContrastRenderer
            {
                if (toolStripHighContrastRenderer is null)
                {
-                    toolStripHighContrastRenderer = new ToolStripHighContrastRenderer(/*renderLikeSystem*/true);
+                    toolStripHighContrastRenderer = new ToolStripHighContrastRenderer(/*renderLikeSystem*/systemRenderMode: false);


Suggested change

toolStripHighContrastRenderer = new ToolStripHighContrastRenderer(/*renderLikeSystem*/systemRenderMode: false);

toolStripHighContrastRenderer = new ToolStripHighContrastRenderer(systemRenderMode: false);

RussKie

Rendering tests?

…tripHighContrastRenderer

Olina-Zhang · 2021-09-07T09:08:14Z

Tested this fixing with no new issue found, hitting known GH issue: #5676

dreddy-work · 2021-09-07T17:27:33Z

Rendering tests?

Rendering tests are tracked by #5670 .

ArtemTatarinov requested a review from a team as a code owner August 30, 2021 12:11

ghost assigned ArtemTatarinov Aug 30, 2021

ArtemTatarinov requested review from vladimir-krestov and SergeySmirnov-Akvelon August 30, 2021 12:11

dreddy-work reviewed Aug 30, 2021

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/ToolStripSystemRenderer.cs Outdated Show resolved Hide resolved

dreddy-work reviewed Aug 30, 2021

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/ToolStripSystemRenderer.cs Show resolved Hide resolved

dreddy-work reviewed Aug 30, 2021

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/ToolStripSystemRenderer.cs Outdated Show resolved Hide resolved

dreddy-work reviewed Aug 30, 2021

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/ToolStripSystemRenderer.cs Outdated Show resolved Hide resolved

merriemcgaw added the a11yMAS High Priority - Accessibility violation of Microsoft Accessibility Standards label Aug 30, 2021

RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Aug 31, 2021

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Aug 31, 2021

ArtemTatarinov force-pushed the Issue#5502_ToolStripButton_NoText_HighContrast branch from 68c662b to 7bfb681 Compare August 31, 2021 05:24

ArtemTatarinov added waiting-review This item is waiting on review by one or more members of team and removed a11yMAS High Priority - Accessibility violation of Microsoft Accessibility Standards labels Aug 31, 2021

Tanya-Solyanik reviewed Sep 1, 2021

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/ToolStripSystemRenderer.cs Outdated Show resolved Hide resolved

src/System.Windows.Forms/src/System/Windows/Forms/ToolStripSystemRenderer.cs Outdated Show resolved Hide resolved

Tanya-Solyanik added 📭 waiting-author-feedback The team requires more information from the author and removed waiting-review This item is waiting on review by one or more members of team labels Sep 1, 2021

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Sep 1, 2021

ArtemTatarinov marked this pull request as draft September 1, 2021 11:38

ArtemTatarinov added the a11yMAS High Priority - Accessibility violation of Microsoft Accessibility Standards label Sep 1, 2021

ArtemTatarinov marked this pull request as ready for review September 1, 2021 17:12

ArtemTatarinov added waiting-review This item is waiting on review by one or more members of team and removed waiting-review This item is waiting on review by one or more members of team labels Sep 1, 2021

dreddy-work previously approved these changes Sep 1, 2021

View reviewed changes

ArtemTatarinov mentioned this pull request Sep 2, 2021

Write rendering tests for ToolStrip Button background rendering fix #5670

Closed

ArtemTatarinov dismissed dreddy-work’s stale review via 75e7e85 September 2, 2021 06:12

ArtemTatarinov force-pushed the Issue#5502_ToolStripButton_NoText_HighContrast branch 2 times, most recently from 75e7e85 to 0bc7e9e Compare September 2, 2021 06:15

dreddy-work previously approved these changes Sep 2, 2021

View reviewed changes

dreddy-work added ready-to-merge PRs that are ready to merge but worth notifying the internal team. and removed waiting-review This item is waiting on review by one or more members of team labels Sep 2, 2021

Tanya-Solyanik added 📭 waiting-author-feedback The team requires more information from the author and removed ready-to-merge PRs that are ready to merge but worth notifying the internal team. labels Sep 3, 2021

ArtemTatarinov dismissed dreddy-work’s stale review via 2d77eb9 September 3, 2021 12:25

ArtemTatarinov force-pushed the Issue#5502_ToolStripButton_NoText_HighContrast branch from f625517 to 2d77eb9 Compare September 3, 2021 12:25

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Sep 3, 2021

ArtemTatarinov force-pushed the Issue#5502_ToolStripButton_NoText_HighContrast branch from 2d77eb9 to 5e45c76 Compare September 3, 2021 13:46

Tanya-Solyanik previously approved these changes Sep 3, 2021

View reviewed changes

Tanya-Solyanik added the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Sep 3, 2021

vladimir-krestov previously approved these changes Sep 4, 2021

View reviewed changes

RussKie reviewed Sep 6, 2021

View reviewed changes

RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Sep 6, 2021

Backgrounds of selected ToolStrip buttons are rendered now with ToolS…

113816f

…tripHighContrastRenderer

ArtemTatarinov dismissed stale reviews from vladimir-krestov and Tanya-Solyanik via 113816f September 6, 2021 04:29

ArtemTatarinov force-pushed the Issue#5502_ToolStripButton_NoText_HighContrast branch from 5e45c76 to 113816f Compare September 6, 2021 04:29

Olina-Zhang removed the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Sep 7, 2021

dreddy-work approved these changes Sep 7, 2021

View reviewed changes

dreddy-work removed the 📭 waiting-author-feedback The team requires more information from the author label Sep 7, 2021

dreddy-work merged commit a7761d1 into dotnet:main Sep 7, 2021

ghost added this to the 7.0 alpha1 milestone Sep 7, 2021

ArtemTatarinov deleted the Issue#5502_ToolStripButton_NoText_HighContrast branch September 8, 2021 05:54

ghost locked as resolved and limited conversation to collaborators Jan 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing `ToolStrip` button background rendering in HC theme #5614

Fixing `ToolStrip` button background rendering in HC theme #5614

ArtemTatarinov commented Aug 30, 2021 •

edited by ghost

Loading

dreddy-work commented Aug 30, 2021

Tanya-Solyanik left a comment

RussKie commented Sep 1, 2021

dreddy-work left a comment

ArtemTatarinov commented Sep 3, 2021

Tanya-Solyanik commented Sep 3, 2021

Tanya-Solyanik left a comment •

edited

Loading

Tanya-Solyanik commented Sep 3, 2021

vladimir-krestov left a comment

vladimir-krestov Sep 4, 2021

RussKie left a comment

Olina-Zhang commented Sep 7, 2021

dreddy-work commented Sep 7, 2021

	toolStripHighContrastRenderer = new ToolStripHighContrastRenderer(/renderLikeSystem/systemRenderMode: false);
	toolStripHighContrastRenderer = new ToolStripHighContrastRenderer(systemRenderMode: false);

Fixing ToolStrip button background rendering in HC theme #5614

Fixing ToolStrip button background rendering in HC theme #5614

Conversation

ArtemTatarinov commented Aug 30, 2021 • edited by ghost Loading

Proposed changes

Customer Impact

Regression?

Risk

Test methodology

Accessibility testing

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

dreddy-work commented Aug 30, 2021

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

RussKie commented Sep 1, 2021

dreddy-work left a comment

Choose a reason for hiding this comment

ArtemTatarinov commented Sep 3, 2021

Tanya-Solyanik commented Sep 3, 2021

Tanya-Solyanik left a comment • edited Loading

Choose a reason for hiding this comment

Tanya-Solyanik commented Sep 3, 2021

vladimir-krestov left a comment

Choose a reason for hiding this comment

vladimir-krestov Sep 4, 2021

Choose a reason for hiding this comment

RussKie left a comment

Choose a reason for hiding this comment

Olina-Zhang commented Sep 7, 2021

dreddy-work commented Sep 7, 2021

Fixing `ToolStrip` button background rendering in HC theme #5614

Fixing `ToolStrip` button background rendering in HC theme #5614

ArtemTatarinov commented Aug 30, 2021 •

edited by ghost

Loading

Tanya-Solyanik left a comment •

edited

Loading