winforms icon indicating copy to clipboard operation
winforms copied to clipboard

Visual Styles enabled or disabled, remove differentiation for border style in the left side of the row header when the style would have been set to `Outset`

Open ricardobossan opened this issue 9 months ago • 13 comments

Fixes #5961

Proposed changes

  • If DataGridView is RightToLeft, with either visual styles enabled or not, remove differentiation for border style in the left side of the row header when the style would have been set to Outset

Customer Impact

  • Will be able to see left border of the row header when control is set to RightToLeft

Regression?

  • No

Risk

  • Minimal

Screenshots

Before

  • Visual styles enabled

before_visual_style_on

  • Visual styles disabled

before_visual_style_off

After

  • Visual styles enabled

after_visual_style_on

  • Visual styles disabled

after_visual_style_off

Test methodology

  • Manual

Accessibility testing

  • Accessibility insights

Test environment(s)

  • dotnet 9.0.100-preview.3.24204.13
Microsoft Reviewers: Open in CodeFlow

ricardobossan avatar May 09 '24 20:05 ricardobossan

Please investigate the failing tests

Tanya-Solyanik avatar May 10 '24 00:05 Tanya-Solyanik

Please investigate the failing tests

@Tanya-Solyanik In my fix, I discovered that the transparency issue stems from the DataGridViewRow header's left border being set to Outset when RightToLeft is defined. By changing it to OutsetDouble, which matches the configuration when RightToLeft isn't chosen, the transparency problem is resolved.

However, this fix conflicts with existing tests, which expect the DataGridViewRow header's left border to be set to Outset. To ensure the integrity of the codebase, I'm considering modifying the tests to accommodate this fix.

I believe implementing this fix is crucial for resolving the reported issue and ensuring consistent behavior across different globalization settings. I'd appreciate your input on how best to proceed.

ricardobossan avatar May 11 '24 01:05 ricardobossan

However, this fix conflicts with existing tests, which expect the DataGridViewRow header's left border to be set to Outset. To ensure the integrity of the codebase, I'm considering modifying the tests to accommodate this fix.

I agree!

Tanya-Solyanik avatar May 11 '24 01:05 Tanya-Solyanik

In this image - image

the column border is not visible between the column headers. Could you please open a new issue for that?

Tanya-Solyanik avatar May 11 '24 01:05 Tanya-Solyanik

In this image - image

the column border is not visible between the column headers. Could you please open a new issue for that?

@Tanya-Solyanik, Leaf's suggestion fixed it:

image

ricardobossan avatar May 17 '24 21:05 ricardobossan

@ricardobossan - please investigate unit test failures

Tanya-Solyanik avatar May 17 '24 23:05 Tanya-Solyanik

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 75.03208%. Comparing base (f53f153) to head (9bda02d). Report is 217 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main      #11345         +/-   ##
===================================================
+ Coverage   74.29395%   75.03208%   +0.73812%     
===================================================
  Files           3026        3122         +96     
  Lines         627152      656198      +29046     
  Branches       46758       51715       +4957     
===================================================
+ Hits          465936      492359      +26423     
- Misses        157863      160436       +2573     
- Partials        3353        3403         +50     
Flag Coverage Δ
Debug 75.03208% <83.33333%> (+0.73812%) :arrow_up:
integration 18.95253% <0.00000%> (+0.96238%) :arrow_up:
production 48.91950% <50.00000%> (+1.88922%) :arrow_up:
test 97.05592% <100.00000%> (+0.06824%) :arrow_up:
unit 45.95277% <50.00000%> (+1.94120%) :arrow_up:

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

codecov[bot] avatar May 20 '24 20:05 codecov[bot]

LGTM!

LeafShi1 avatar May 21 '24 02:05 LeafShi1

@Olina-Zhang - could you please test this change when you have time?

Tanya-Solyanik avatar May 21 '24 05:05 Tanya-Solyanik

During the review of this PR we identified two issues when RightToLeft is set to Yes and VisualStyles is not enabled:

  1. Column borders misalignment between header and body.
  2. The top row header's left border disappears when there's a horizontal scrollbar and it is moved to the left.

These issues are not regressions from PR 11345. We will investigate these problems further and work towards a resolution in #11431 .

ricardobossan avatar May 28 '24 10:05 ricardobossan

@merriemcgaw @Tanya-Solyanik

Considering Merrie's message on #11431, where it was mentioned that we should not modify anything with VisualStyles turned off:

In this PR, I have already fixed the missing column between the row header and the DataGridView body for both VisualStyles enabled and disabled. Should I revert that change for when VisualStyles is off? I can easily revert that change without removing the fix for when VisualStyles is on.

ricardobossan avatar May 31 '24 08:05 ricardobossan

@ricardobossan if you've already got the fix, then I don't mind taking it. Mostly I wanted to make sure we don't invest too much time in Visual Styles Off.

merriemcgaw avatar Jun 04 '24 18:06 merriemcgaw

@merriemcgaw Got it. I'll keep it then. Thank you!

ricardobossan avatar Jun 04 '24 20:06 ricardobossan

Changes LGTM, In regard to previous conversation:

In this image - image the column border is not visible between the column headers. Could you please open a new issue for that?

@Tanya-Solyanik, Leaf's suggestion fixed it:

image

It looks like we have gone with a different change in this PR than what was Leaf's suggestion, curious if this still resolves the issue Tanya pointed out here https://github.com/dotnet/winforms/pull/11345#issuecomment-2105434405

lonitra avatar Jul 26 '24 00:07 lonitra

@lonitra Thanks! I just ran the code from this PR and the issue that Tanya mentioned in her comment is solved, as seen in the image bellow:

image

ricardobossan avatar Jul 31 '24 01:07 ricardobossan