dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.28k stars 954 forks source link

Issue 2053861 fix combo box flickering issue #11529

Open LeafShi1 opened 2 weeks ago

LeafShi1 commented 2 weeks ago

Fixes #2053861

Proposed changes

Customer Impact

Regression?

Risk

Screenshots

Before

Combo box flashes when you move the mouse over it BeforeChange

After

Combo boxes no longer flicker when you move the mouse over them AfterChange

Test methodology

Test environment(s)

Microsoft Reviewers: Open in CodeFlow
codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 9.37500% with 29 lines in your changes missing coverage. Please review.

Project coverage is 74.46985%. Comparing base (fc56416) to head (86889e0). Report is 42 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #11529 +/- ## =================================================== + Coverage 74.39859% 74.46985% +0.07125% =================================================== Files 3032 3039 +7 Lines 628149 629068 +919 Branches 46828 46836 +8 =================================================== + Hits 467334 468466 +1132 + Misses 157465 157242 -223 - Partials 3350 3360 +10 ``` | [Flag](https://app.codecov.io/gh/dotnet/winforms/pull/11529/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | Coverage Δ | | |---|---|---| | [Debug](https://app.codecov.io/gh/dotnet/winforms/pull/11529/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `74.46985% <9.37500%> (+0.07125%)` | :arrow_up: | | [integration](https://app.codecov.io/gh/dotnet/winforms/pull/11529/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `17.97590% <0.00000%> (-0.02466%)` | :arrow_down: | | [production](https://app.codecov.io/gh/dotnet/winforms/pull/11529/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `47.32729% <9.37500%> (+0.08032%)` | :arrow_up: | | [test](https://app.codecov.io/gh/dotnet/winforms/pull/11529/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `96.96223% <ø> (-0.00449%)` | :arrow_down: | | [unit](https://app.codecov.io/gh/dotnet/winforms/pull/11529/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `44.31360% <9.37500%> (+0.08465%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet#carryforward-flags-in-the-pull-request-comment) to find out more.
Tanya-Solyanik commented 2 weeks ago

This link - https://devdiv.visualstudio.com/DevDiv/_queries/edit/2053861/?queryId=7c94b3b5-6b84-4f3d-9f76-ff1a7a7b7581 does not work, please fix it.

Had you tested this change at different themes and RtL applications?

The change looks reasonable.

LeafShi1 commented 2 weeks ago

This link - https://devdiv.visualstudio.com/DevDiv/_queries/edit/2053861/?queryId=7c94b3b5-6b84-4f3d-9f76-ff1a7a7b7581 does not work, please fix it.

Had you tested this change at different themes and RtL applications?

The change looks reasonable.

Updated the link of the description and test in different environment Test in RTL app: RTL

Test in different theme: ThemeAquatuc ThemeDesert

Test in Flat Style: FlatStyle

There are still a flickering in Flat Style, and this flickering issue can be resolved by remove https://github.com/dotnet/winforms/blob/889fca6df2aa684e3557ef7737b443ed3af32455/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ComboBox/ComboBox.cs#L3880

But I am not sure if we can remove this directly, @Tanya-Solyanik What do you think?

Tanya-Solyanik commented 1 week ago

@LeafShi1 - How do the high contrast themes look before your changes, did it get better or worse with the fix? FYI @merriemcgaw

LeafShi1 commented 1 week ago

Before the modification, the Popup state was not obvious, and only a black border was displayed when the mouse entered the control white black

merriemcgaw commented 1 week ago

This is definitely not worse from my perspective. @Tanya-Solyanik?

Tanya-Solyanik commented 1 week ago

This is definitely not worse from my perspective. @Tanya-Solyanik?

The new behavior looks more intentional, i.e. we are adding a border when mouse is hovering. I don't mind taking this fix.

Tanya-Solyanik commented 1 week ago

@Olina-Zhang - could you please test this fix?

Tanya-Solyanik commented 1 week ago

@LeafShi1

But I am not sure if we can remove this directly, @Tanya-Solyanik What do you think?

No, we shouldn't do that. I agree. To fix that we would have to redesign the control look, maybe add a prominent border in the state when the mouse is not above the control

LeafShi1 commented 1 week ago

@LeafShi1

But I am not sure if we can remove this directly, @Tanya-Solyanik What do you think?

No, we shouldn't do that. I agree. To fix that we would have to redesign the control look, maybe add a prominent border in the state when the mouse is not above the control

@Tanya-Solyanik Do we need to redesign the controls appearance now?

Before drawing Flat, the combobox control is as follows. Pic1: image

The method of drawing Flat is to draw Outboder and innerBorder. The result after drawing is as follows Pic2: image

When the cursor enters, it is necessary to complete the drawing from Pic1 to Pic2,the actual cause of the flickering is the use of g.DrawRectangle four times in a row when drawing the Flat type. So adding a protruding border when the mouse is not over the control does not seem to solve the flickering problem