Fixing switching focus between ToolStrips where TabStop=true with Shift-Tab

[ArtemTatarinov]

Fixes #5795

Proposed changes

• Adapted the logic of the forward block of the if statement to the backward block - no special treatment for the ToolStrip is needed
• Changed forward condition at the GetNextItem method at the ToolStrip class because it is called after moving focus to the ToolStrip, but when TabStop is true the first item should be always focused
• Added unit test for checking focus cycling

Customer Impact

• Before the fix (focus was trapped at the last ToolStrip item while pressing Shift+Tab):
  

• After the fix (focus isn't trapped at the last ToolStrip item while pressing Shift+Tab):
  

Regression?

• No

Risk

• Minimal

Test methodology

• Manual testing
• Unit test
• CTI team

Test environment(s)

Microsoft Windows [Version 10.0.19043.1237]
.NET SDK 7.0.0-alpha.1.21471.1

Microsoft Reviewers: Open in CodeFlow

[Tanya-Solyanik]

Could you please explain why tool strip is a special case?

[ArtemTatarinov]

@Tanya-Solyanik, thank you very much for your review points! While fixing them I've got another solution for the issue and it seems to me more compact and consistent than the previous two. I have adapted the logic of the forward block of the if statement to the backward block - and now no special treatment for the ToolStrip is needed (also I have applied your review points where it was possible).

In the future PR I can refactor both of the forward and backward blocks: extract the common code to separate methods and so on, but now, I guess, it's better not to touch forward code so we can isolate fixing the issue without any other factors. Also, I haven't extracted new methods from the backward block too (except for the one tiny helper method) to keep the changes as close to the original code as possible.

I have manually tested the ToolStrips - Shift+Tab works fines.
RTL=No

RTL=Yes

Also, I have manually tested all the controls at the child test forms called by these buttons: "Buttons", "ListView", "Calendar", "FontNameEditor", "Multiple Controls", "CollectionEditors", "ComboBoxes", "RichTextBoxes", "ComboBoxes with ScrollBars", "PictureBoxes", "Dialogs", "DataGridView", "FileDialog", "DataGridView in Virtual mode", "ErrorProvider", "Task Dialog", "ContentAlignment", "MessageBox", "Menus", "Panels", "TrackBars", "ScrollBars", "MDI Parent", "ToolTips", "PropertyGrid" (at our test application WinformsControlsTest). Moving focus backward works fine there too.

And I have run all unit tests for Control and ToolStrip classes - they're ok.

And just to be sure this solution doesn't affect moving focus when TabStop is false I tested this case for ToolStrips too for both RTL=Yes and RTL=No modes, everything is fine.

[Tanya-Solyanik]

@ArtemTatarinov - thank you for the detailed description of testing! This is very helpful!
I agree that follow up clean up is better to put into a separate PR, but please do it while the code is fresh in your mind.

[Tanya-Solyanik]

Looks good, please address the comment about localization.

[Tanya-Solyanik]

Looks good

[Tanya-Solyanik]

Tests failed in release:

• RichTextBox_Text_GetWithHandle_ReturnsExpected - Flaky test: RichTextBox_Text_GetWithHandle_ReturnsExpected #4347

at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_Text_GetWithHandle_ReturnsExpected() in /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs:line 6919
Assert.Equal() Failure\r\n          ↓ (pos 0)\r\nExpected: a b\r\nActual:    b\r\n          ↑ (pos 0)

• DataGridView_RowHeadersWidth_SetWithParentWithHandle_GetReturnsExpected - Flaky test: DataGridView_RowHeadersWidth_SetWithParentWithHandle_GetReturnsExpected #4736

 at System.Windows.Forms.Tests.DataGridViewTests.DataGridView_RowHeadersWidth_SetWithParentWithHandle_GetReturnsExpected(DataGridViewRowHeadersWidthSizeMode rowHeadersWidthSizeMode, Boolean rowHeadersVisible, Boolean autoSize, Int32 value, Int32 expectedValue, Int32 expectedInvalidatedCallCount, Int32 expectedLayoutCallCount, Int32 expectedParentLayoutCallCount) in /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataGridViewTests.cs:line 1208

Assert.Equal() Failure\r\nExpected: 1\r\nActual:   3

[Tanya-Solyanik]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[ghost]

Hello @Tanya-Solyanik!

Because this pull request has the :octocat: automerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[ghost]

Happy to oblige