From b2ac8290422a79df59fb57a55bedb537589af9d5 Mon Sep 17 00:00:00 2001 From: Eran Markus Date: Sat, 6 Jun 2026 12:15:00 +0300 Subject: [PATCH] fix: correct tab order in New Connection dialog (General and Groups) The General and Groups sections of the New Connection dialog had random and colliding TabIndex values (e.g. multiple controls at TabIndex 0/1 in Groups, and Save-password at TabIndex 6 in General), which made keyboard navigation jump around the form. Reassign TabIndex on both user controls to follow the visible reading order (top to bottom, then left to right). No layout or behavior changes; only TabIndex values are altered. Adds NewTerminalTabOrderTests asserting each section's tab order follows reading order and that focusable controls do not share a tab index. Fixes #132 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../GeneralPropertiesUserControl.Designer.cs | 28 ++--- .../EditFavorite/GroupsControl.Designer.cs | 10 +- Source/Tests/Tests.csproj | 1 + .../UserInterface/NewTerminalTabOrderTests.cs | 107 ++++++++++++++++++ 4 files changed, 127 insertions(+), 19 deletions(-) create mode 100644 Source/Tests/UserInterface/NewTerminalTabOrderTests.cs diff --git a/Source/Terminals/Forms/EditFavorite/GeneralPropertiesUserControl.Designer.cs b/Source/Terminals/Forms/EditFavorite/GeneralPropertiesUserControl.Designer.cs index bc95f2da..904a684b 100644 --- a/Source/Terminals/Forms/EditFavorite/GeneralPropertiesUserControl.Designer.cs +++ b/Source/Terminals/Forms/EditFavorite/GeneralPropertiesUserControl.Designer.cs @@ -54,7 +54,7 @@ private void InitializeComponent() this.chkAddtoToolbar.Location = new System.Drawing.Point(136, 272); this.chkAddtoToolbar.Name = "chkAddtoToolbar"; this.chkAddtoToolbar.Size = new System.Drawing.Size(96, 17); - this.chkAddtoToolbar.TabIndex = 39; + this.chkAddtoToolbar.TabIndex = 13; this.chkAddtoToolbar.Text = "Add to &Toolbar"; this.chkAddtoToolbar.UseVisualStyleBackColor = true; // @@ -65,7 +65,7 @@ private void InitializeComponent() this.NewWindowCheckbox.Location = new System.Drawing.Point(136, 252); this.NewWindowCheckbox.Name = "NewWindowCheckbox"; this.NewWindowCheckbox.Size = new System.Drawing.Size(130, 17); - this.NewWindowCheckbox.TabIndex = 40; + this.NewWindowCheckbox.TabIndex = 12; this.NewWindowCheckbox.Text = "&Open in New Window"; this.NewWindowCheckbox.UseVisualStyleBackColor = true; // @@ -74,7 +74,7 @@ private void InitializeComponent() this.httpUrlTextBox.Location = new System.Drawing.Point(133, 45); this.httpUrlTextBox.Name = "httpUrlTextBox"; this.httpUrlTextBox.Size = new System.Drawing.Size(259, 20); - this.httpUrlTextBox.TabIndex = 38; + this.httpUrlTextBox.TabIndex = 4; this.httpUrlTextBox.Text = "https://github.com/Terminals-Origin/Terminals/issues"; this.httpUrlTextBox.Visible = false; this.httpUrlTextBox.TextChanged += new System.EventHandler(this.HttpUrlTextBox_TextChanged); @@ -84,14 +84,14 @@ private void InitializeComponent() this.txtPort.Location = new System.Drawing.Point(443, 43); this.txtPort.Name = "txtPort"; this.txtPort.Size = new System.Drawing.Size(46, 20); - this.txtPort.TabIndex = 26; + this.txtPort.TabIndex = 6; // // txtName // this.txtName.Location = new System.Drawing.Point(133, 75); this.txtName.Name = "txtName"; this.txtName.Size = new System.Drawing.Size(325, 20); - this.txtName.TabIndex = 28; + this.txtName.TabIndex = 8; // // chkSavePassword // @@ -100,7 +100,7 @@ private void InitializeComponent() this.chkSavePassword.Location = new System.Drawing.Point(136, 232); this.chkSavePassword.Name = "chkSavePassword"; this.chkSavePassword.Size = new System.Drawing.Size(99, 17); - this.chkSavePassword.TabIndex = 6; + this.chkSavePassword.TabIndex = 11; this.chkSavePassword.Text = "S&ave password"; this.chkSavePassword.UseVisualStyleBackColor = true; // @@ -112,7 +112,7 @@ private void InitializeComponent() this.pictureBox2.Name = "pictureBox2"; this.pictureBox2.Size = new System.Drawing.Size(16, 16); this.pictureBox2.SizeMode = System.Windows.Forms.PictureBoxSizeMode.StretchImage; - this.pictureBox2.TabIndex = 36; + this.pictureBox2.TabIndex = 9; this.pictureBox2.TabStop = false; this.pictureBox2.Click += new System.EventHandler(this.PictureBox2_Click); // @@ -122,7 +122,7 @@ private void InitializeComponent() this.lblPort.Location = new System.Drawing.Point(398, 46); this.lblPort.Name = "lblPort"; this.lblPort.Size = new System.Drawing.Size(29, 13); - this.lblPort.TabIndex = 25; + this.lblPort.TabIndex = 5; this.lblPort.Text = "Port:"; // // ProtocolComboBox @@ -133,7 +133,7 @@ private void InitializeComponent() this.ProtocolComboBox.MaxDropDownItems = 10; this.ProtocolComboBox.Name = "ProtocolComboBox"; this.ProtocolComboBox.Size = new System.Drawing.Size(356, 21); - this.ProtocolComboBox.TabIndex = 35; + this.ProtocolComboBox.TabIndex = 1; this.ProtocolComboBox.SelectedIndexChanged += new System.EventHandler(this.ProtocolComboBox_SelectedIndexChanged); // // ProtocolLabel @@ -142,7 +142,7 @@ private void InitializeComponent() this.ProtocolLabel.Location = new System.Drawing.Point(6, 17); this.ProtocolLabel.Name = "ProtocolLabel"; this.ProtocolLabel.Size = new System.Drawing.Size(49, 13); - this.ProtocolLabel.TabIndex = 34; + this.ProtocolLabel.TabIndex = 0; this.ProtocolLabel.Text = "&Protocol:"; // // label5 @@ -151,7 +151,7 @@ private void InitializeComponent() this.label5.Location = new System.Drawing.Point(6, 78); this.label5.Name = "label5"; this.label5.Size = new System.Drawing.Size(93, 13); - this.label5.TabIndex = 27; + this.label5.TabIndex = 7; this.label5.Text = "Connection na&me:"; // // cmbServers @@ -161,7 +161,7 @@ private void InitializeComponent() this.cmbServers.Location = new System.Drawing.Point(133, 45); this.cmbServers.Name = "cmbServers"; this.cmbServers.Size = new System.Drawing.Size(259, 21); - this.cmbServers.TabIndex = 24; + this.cmbServers.TabIndex = 3; this.cmbServers.SelectedIndexChanged += new System.EventHandler(this.CmbServers_SelectedIndexChanged); this.cmbServers.TextChanged += new System.EventHandler(this.CmbServers_TextChanged); this.cmbServers.Leave += new System.EventHandler(this.CmbServers_Leave); @@ -172,7 +172,7 @@ private void InitializeComponent() this.lblServerName.Location = new System.Drawing.Point(6, 46); this.lblServerName.Name = "lblServerName"; this.lblServerName.Size = new System.Drawing.Size(55, 13); - this.lblServerName.TabIndex = 23; + this.lblServerName.TabIndex = 2; this.lblServerName.Text = "&Computer:"; // // securityPanel1 @@ -180,7 +180,7 @@ private void InitializeComponent() this.securityPanel1.Location = new System.Drawing.Point(6, 101); this.securityPanel1.Name = "securityPanel1"; this.securityPanel1.Size = new System.Drawing.Size(483, 128); - this.securityPanel1.TabIndex = 41; + this.securityPanel1.TabIndex = 10; // // GeneralPropertiesUserControl // diff --git a/Source/Terminals/Forms/EditFavorite/GroupsControl.Designer.cs b/Source/Terminals/Forms/EditFavorite/GroupsControl.Designer.cs index eb0fa5e9..df360f3b 100644 --- a/Source/Terminals/Forms/EditFavorite/GroupsControl.Designer.cs +++ b/Source/Terminals/Forms/EditFavorite/GroupsControl.Designer.cs @@ -73,7 +73,7 @@ private void InitializeComponent() this.btnRemoveTag.Location = new System.Drawing.Point(521, 74); this.btnRemoveTag.Name = "btnRemoveTag"; this.btnRemoveTag.Size = new System.Drawing.Size(21, 21); - this.btnRemoveTag.TabIndex = 1; + this.btnRemoveTag.TabIndex = 5; this.btnRemoveTag.UseVisualStyleBackColor = true; this.btnRemoveTag.Click += new System.EventHandler(this.BtnRemoveTag_Click); // @@ -83,7 +83,7 @@ private void InitializeComponent() this.lvConnectionTags.Location = new System.Drawing.Point(12, 74); this.lvConnectionTags.Name = "lvConnectionTags"; this.lvConnectionTags.Size = new System.Drawing.Size(503, 66); - this.lvConnectionTags.TabIndex = 0; + this.lvConnectionTags.TabIndex = 4; this.lvConnectionTags.UseCompatibleStateImageBehavior = false; this.lvConnectionTags.View = System.Windows.Forms.View.List; this.lvConnectionTags.DoubleClick += new System.EventHandler(this.LvConnectionTags_DoubleClick); @@ -94,7 +94,7 @@ private void InitializeComponent() this.AllTagsAddButton.Location = new System.Drawing.Point(521, 176); this.AllTagsAddButton.Name = "AllTagsAddButton"; this.AllTagsAddButton.Size = new System.Drawing.Size(21, 21); - this.AllTagsAddButton.TabIndex = 1; + this.AllTagsAddButton.TabIndex = 8; this.AllTagsAddButton.UseVisualStyleBackColor = true; this.AllTagsAddButton.Click += new System.EventHandler(this.AllTagsAddButton_Click); // @@ -104,7 +104,7 @@ private void InitializeComponent() this.AllTagsListView.Location = new System.Drawing.Point(9, 176); this.AllTagsListView.Name = "AllTagsListView"; this.AllTagsListView.Size = new System.Drawing.Size(506, 150); - this.AllTagsListView.TabIndex = 0; + this.AllTagsListView.TabIndex = 7; this.AllTagsListView.UseCompatibleStateImageBehavior = false; this.AllTagsListView.View = System.Windows.Forms.View.List; this.AllTagsListView.DoubleClick += new System.EventHandler(this.AllTagsListView_DoubleClick); @@ -124,7 +124,7 @@ private void InitializeComponent() this.allLabel.Location = new System.Drawing.Point(9, 160); this.allLabel.Name = "allLabel"; this.allLabel.Size = new System.Drawing.Size(101, 13); - this.allLabel.TabIndex = 4; + this.allLabel.TabIndex = 6; this.allLabel.Text = "All available groups:"; // // GroupsControl diff --git a/Source/Tests/Tests.csproj b/Source/Tests/Tests.csproj index 4c76e091..910c3589 100644 --- a/Source/Tests/Tests.csproj +++ b/Source/Tests/Tests.csproj @@ -190,6 +190,7 @@ + diff --git a/Source/Tests/UserInterface/NewTerminalTabOrderTests.cs b/Source/Tests/UserInterface/NewTerminalTabOrderTests.cs new file mode 100644 index 00000000..b5bf9a6f --- /dev/null +++ b/Source/Tests/UserInterface/NewTerminalTabOrderTests.cs @@ -0,0 +1,107 @@ +using System.Collections.Generic; +using System.Linq; +using System.Windows.Forms; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Terminals.Forms.EditFavorite; + +namespace Tests.UserInterface +{ + /// + /// Regression tests for issue #132: the General and Groups sections of the + /// New Connection dialog had random/colliding tab order indices, which made + /// keyboard navigation jump around the form. These tests assert the tab order + /// of each section follows the visible reading order (top to bottom, then left + /// to right) and that focusable controls do not share a tab index. + /// + [TestClass] + public class NewTerminalTabOrderTests + { + /// + /// Controls on the same visual row may differ slightly in Top (a label and + /// its field are not pixel aligned). Treat anything within this band as one row. + /// + private const int RowTolerance = 12; + + [TestMethod] + public void GeneralProperties_TabOrder_FollowsReadingOrder() + { + using (var control = new GeneralPropertiesUserControl()) + AssertReadingOrder(control); + } + + [TestMethod] + public void Groups_TabOrder_FollowsReadingOrder() + { + using (var control = new GroupsControl()) + AssertReadingOrder(control); + } + + [TestMethod] + public void GeneralProperties_FocusableControls_HaveUniqueTabIndex() + { + using (var control = new GeneralPropertiesUserControl()) + AssertUniqueTabIndex(control); + } + + [TestMethod] + public void Groups_FocusableControls_HaveUniqueTabIndex() + { + using (var control = new GroupsControl()) + AssertUniqueTabIndex(control); + } + + /// + /// Walks the focusable controls in tab order and verifies each next control is + /// either below the previous one, or on the same row but further right - never + /// jumping back up the form. + /// + private static void AssertReadingOrder(Control container) + { + List ordered = FocusableControls(container) + .OrderBy(c => c.TabIndex) + .ToList(); + + for (int i = 1; i < ordered.Count; i++) + { + Control previous = ordered[i - 1]; + Control current = ordered[i]; + + bool sameRow = System.Math.Abs(current.Top - previous.Top) <= RowTolerance; + bool isBelow = current.Top - previous.Top > RowTolerance; + bool rightOnSameRow = sameRow && current.Left >= previous.Left; + + string message = string.Format( + "Tab order jumps backwards: '{0}' (TabIndex {1}, {2}) should not come before '{3}' (TabIndex {4}, {5}).", + previous.Name, previous.TabIndex, FormatLocation(previous), + current.Name, current.TabIndex, FormatLocation(current)); + + Assert.IsTrue(isBelow || rightOnSameRow, message); + } + } + + private static void AssertUniqueTabIndex(Control container) + { + List focusable = FocusableControls(container).ToList(); + var duplicates = focusable + .GroupBy(c => c.TabIndex) + .Where(g => g.Count() > 1) + .Select(g => string.Format("TabIndex {0}: {1}", g.Key, string.Join(", ", g.Select(c => c.Name)))) + .ToList(); + + Assert.AreEqual(0, duplicates.Count, + "Focusable controls must not share a tab index. Collisions: " + string.Join(" | ", duplicates)); + } + + private static IEnumerable FocusableControls(Control container) + { + // Labels (TabStop == false) and hidden alternates are skipped by real tab + // navigation, so they are excluded here too. + return container.Controls.Cast().Where(c => c.TabStop && c.Visible); + } + + private static string FormatLocation(Control control) + { + return string.Format("Top={0},Left={1}", control.Top, control.Left); + } + } +}