Skip to content

Conversation

@LeafShi1
Copy link
Member

@LeafShi1 LeafShi1 commented Jun 13, 2024

Fixes #2053861

Proposed changes

  • Separate DrawPopUpCombo from DrawFlatCombo method,draw the outer black border only when the ComboBox mouse is over

Customer Impact

  • Combobox no longer flickers when hovering over it

Regression?

  • No

Risk

  • Minimal

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

  • Manually

Test environment(s)

  • .net 9.0.0-preview.6.24305.2
Microsoft Reviewers: Open in CodeFlow

@LeafShi1 LeafShi1 requested a review from a team as a code owner June 13, 2024 07:50
@LeafShi1 LeafShi1 requested a review from Tanya-Solyanik June 13, 2024 07:50
@codecov
Copy link

codecov bot commented Jun 13, 2024

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
@@                 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 Coverage Δ
Debug 74.46985% <9.37500%> (+0.07125%) ⬆️
integration 17.97590% <0.00000%> (-0.02466%) ⬇️
production 47.32729% <9.37500%> (+0.08032%) ⬆️
test 96.96223% <ø> (-0.00449%) ⬇️
unit 44.31360% <9.37500%> (+0.08465%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@Tanya-Solyanik
Copy link
Contributor

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.

@Tanya-Solyanik Tanya-Solyanik added the waiting-author-feedback The team requires more information from the author label Jun 13, 2024
@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback The team requires more information from the author label Jun 14, 2024
@LeafShi1
Copy link
Member Author

LeafShi1 commented Jun 14, 2024

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

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

@Tanya-Solyanik
Copy link
Contributor

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

@elachlan elachlan added the waiting-author-feedback The team requires more information from the author label Jun 19, 2024
@LeafShi1
Copy link
Member Author

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

@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback The team requires more information from the author label Jun 19, 2024
@merriemcgaw
Copy link
Member

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

@Tanya-Solyanik
Copy link
Contributor

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 Tanya-Solyanik added waiting-author-feedback The team requires more information from the author waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) labels Jun 20, 2024
@Tanya-Solyanik
Copy link
Contributor

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

@Tanya-Solyanik
Copy link
Contributor

Tanya-Solyanik commented Jun 20, 2024

@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

@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback The team requires more information from the author label Jun 21, 2024
@LeafShi1
Copy link
Member Author

LeafShi1 commented Jun 21, 2024

@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

@LeafShi1 LeafShi1 added waiting-review This item is waiting on review by one or more members of team and removed waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) labels Jun 25, 2024
@LeafShi1 LeafShi1 requested a review from SimonZhao888 July 3, 2024 07:58
Copy link
Member

@ricardobossan ricardobossan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All LGTM

@Tanya-Solyanik
Copy link
Contributor

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

No

@Tanya-Solyanik Tanya-Solyanik removed the waiting-review This item is waiting on review by one or more members of team label Jul 21, 2024
@Tanya-Solyanik Tanya-Solyanik merged commit d89527c into dotnet:main Jul 21, 2024
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0 Preview7 milestone Jul 21, 2024
@Syareel-Sukeri
Copy link
Contributor

Verified this issue on 9.0.100-preview.7.24371.4 with dlls built from winforms repo of main branch, it was fixed. Flickering effect is no longer there when hovering over ComboBox. Results are as below:

PopupStyle

Popup.mp4

FlatStyle

Flat.mp4

Different Themes

Desert.mp4
Dusk.mp4
Night.mp4
Aquatic.mp4

@Olina-Zhang
Copy link
Member

Verified this PR fixing in .NET 9.0.100-preview7.24402.8 test pass build from provided VS VAL build, it was fixed for ComboBox flickering issue when setting this control's FlatStyle == PopUp. But still repro for ComboBox's FlatStyle == Flat case. These are same as the testing result we tested before.

ComboBox_flickerFixing.mp4

@github-actions github-actions bot locked and limited conversation to collaborators Sep 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants