Changed semantics of TabIndexes, TabIndex, and CanFocus relative to TabStop.

CanFocus is not coupled with the tab index or tab stop other than to automatically set TabStop when set to True.
A TabIndex of -1 is only used to indicate TabStop_set has not been called.
Once nullable is enabled, we'll change _tabIndex to be nullable.
Changing CanFocus does not impact TabIndex (except in that it sets TabStop if it's None).

Terminal.Gui/View/View.Hierarchy.cs

@@ -183,7 +183,7 @@ public partial class View // SuperView/SubView hierarchy management (SuperView,
         _subviews.Remove (view);
         _tabIndexes.Remove (view);
         view._superView = null;
-        view._tabIndex = -1;
+        //view._tabIndex = -1;
         SetNeedsLayout ();
         SetNeedsDisplay ();
 

Terminal.Gui/View/View.Navigation.cs

@@ -205,7 +205,7 @@ public partial class View // Focus and cross-view navigation management (TabStop
             {
                 case false when _tabIndex > -1:
                     // BUGBUG: This is a poor API design. Automatic behavior like this is non-obvious and should be avoided. Callers should adjust TabIndex explicitly.
-                    TabIndex = -1;
+                    //TabIndex = -1;
 
                     break;
 
@@ -215,11 +215,11 @@ public partial class View // Focus and cross-view navigation management (TabStop
                     break;
             }
 
-            if (_canFocus && _tabIndex == -1)
-            {
-                // BUGBUG: This is a poor API design. Automatic behavior like this is non-obvious and should be avoided. Callers should adjust TabIndex explicitly.
-                TabIndex = SuperView is { } ? SuperView._tabIndexes.IndexOf (this) : -1;
-            }
+            //if (_canFocus && _tabIndex == -1)
+            //{
+            //    // BUGBUG: This is a poor API design. Automatic behavior like this is non-obvious and should be avoided. Callers should adjust TabIndex explicitly.
+            //    TabIndex = SuperView is { } ? SuperView._tabIndexes.IndexOf (this) : -1;
+            //}
 
             if (TabStop == TabStop.None && _canFocus)
             {
@@ -262,7 +262,7 @@ public partial class View // Focus and cross-view navigation management (TabStop
                             view._oldCanFocus = view.CanFocus;
                             view._oldTabIndex = view._tabIndex;
                             view.CanFocus = false;
-                            view._tabIndex = -1;
+                            //view._tabIndex = -1;
                         }
                         else
                         {
@@ -667,9 +667,8 @@ public partial class View // Focus and cross-view navigation management (TabStop
     /// <value>The tabIndexes.</value>
     public IList<View> TabIndexes => _tabIndexes?.AsReadOnly () ?? _empty;
 
-    // TODO: Change this to int? and use null to indicate the view is not in the tab order.
-    // BUGBUG: It is confused to have both TabStop and TabIndex = -1.
-    private int _tabIndex = -1;
+    // TODO: Change this to int? and use null to indicate the view has not yet been added to the tab order.
+    private int _tabIndex = -1; // -1 indicates the view has not yet been added to TabIndexes
     private int _oldTabIndex;
 
     /// <summary>
@@ -700,15 +699,18 @@ public partial class View // Focus and cross-view navigation management (TabStop
         // TOOD: This should be a get-only property. Introduce SetTabIndex (int value) (or similar).
         set
         {
-            // BUGBUG: Property setters should set the property to the value passed in and not have side effects.
-            if (!CanFocus)
-            {
-                // BUGBUG: Property setters should set the property to the value passed in and not have side effects.
-                // BUGBUG: TabIndex = -1 should not be used to indicate that the view is not in the tab order. That's what TabStop is for.
-                _tabIndex = -1;
+            //// BUGBUG: Property setters should set the property to the value passed in and not have side effects.
+            //if (!CanFocus)
+            //{
+            //    // BUGBUG: Property setters should set the property to the value passed in and not have side effects.
+            //    // BUGBUG: TabIndex = -1 should not be used to indicate that the view is not in the tab order. That's what TabStop is for.
+            //    _tabIndex = -1;
 
-                return;
-            }
+            //    return;
+            //}
+
+            // Once a view is in the tab order, it should not be removed from the tab order; set TabStop to None instead.
+            Debug.Assert (value >= 0);
 
             // BUGBUG: Property setters should set the property to the value passed in and not have side effects.
             if (SuperView?._tabIndexes is null || SuperView?._tabIndexes.Count == 1)
@@ -746,6 +748,11 @@ public partial class View // Focus and cross-view navigation management (TabStop
     /// <returns>The minimum of <paramref name="idx"/> and the <see cref="SuperView"/>'s <see cref="TabIndexes"/>.</returns>
     private int GetGreatestTabIndexInSuperView (int idx)
     {
+        if (SuperView is null)
+        {
+            return 0;
+        }
+
         var i = 0;
 
         foreach (View superViewTabStop in SuperView._tabIndexes)
@@ -766,6 +773,11 @@ public partial class View // Focus and cross-view navigation management (TabStop
     /// </summary>
     private void ReorderSuperViewTabIndexes ()
     {
+        if (SuperView is null)
+        {
+            return;
+        }
+
         var i = 0;
 
         foreach (View superViewTabStop in SuperView._tabIndexes)
@@ -780,6 +792,8 @@ public partial class View // Focus and cross-view navigation management (TabStop
         }
     }
 
+    private TabStop _tabStop = TabStop.None;
+
     /// <summary>
     ///     Gets or sets whether the view is a stop-point for keyboard navigation.
     /// </summary>
@@ -793,7 +807,25 @@ public partial class View // Focus and cross-view navigation management (TabStop
     ///     modifying the key bindings (see <see cref="KeyBindings.Add(Key, Command[])"/>) of the SuperView.
     /// </para>
     /// </remarks>
-    public TabStop TabStop { get; set; } = TabStop.None;
+    public TabStop TabStop
+    {
+        get => _tabStop;
+        set
+        {
+            if (_tabStop == value)
+            {
+                return;
+            }
+            _tabStop = value;
+
+            // If TabIndex is -1 it means this view has not yet been added to TabIndexes (TabStop has not been set previously).
+            if (TabIndex == -1)
+            {
+                TabIndex = SuperView is { } ? SuperView._tabIndexes.Count : 0;
+            }
+            ReorderSuperViewTabIndexes();
+        }
+    }
 
     #endregion Tab/Focus Handling
 }

Terminal.Gui/View/View.cs

@@ -186,7 +186,7 @@ public partial class View : Responder, ISupportInitializeNotification
         SetupText ();
 
         CanFocus = false;
-        TabIndex = -1;
+        //TabIndex = -1;
         TabStop = TabStop.None;
     }
 

UnitTests/View/NavigationTests.cs

@@ -274,8 +274,8 @@ public class NavigationTests (ITestOutputHelper output) : TestsAllViews
         Assert.Equal (r.TabIndexes.IndexOf (v1), v1.TabIndex);
         Assert.Equal (1, v1.TabIndex);
         Assert.Equal (TabStop.TabStop, v1.TabStop);
-        Assert.NotEqual (r.TabIndexes.IndexOf (v2), v2.TabIndex);
-        Assert.Equal (-1, v2.TabIndex);
+        Assert.Equal (r.TabIndexes.IndexOf (v2), v2.TabIndex); // TabIndex is not changed
+        Assert.NotEqual (-1, v2.TabIndex);
         Assert.Equal (TabStop.TabStop, v2.TabStop); // TabStop is not changed
         Assert.Equal (r.TabIndexes.IndexOf (v3), v3.TabIndex);
         Assert.Equal (2, v3.TabIndex);
@@ -621,133 +621,133 @@ public class NavigationTests (ITestOutputHelper output) : TestsAllViews
         top1.Dispose ();
     }
 
-//    [Fact]
-//    [AutoInitShutdown]
-//    public void HotKey_Will_Invoke_KeyPressed_Only_For_The_MostFocused_With_Top_KeyPress_Event ()
-//    {
-//        var sbQuiting = false;
-//        var tfQuiting = false;
-//        var topQuiting = false;
-
-//        var sb = new StatusBar (
-//                                new Shortcut []
-//                                {
-//                                    new (
-//                                         KeyCode.CtrlMask | KeyCode.Q,
-//                                         "Quit",
-//                                         () => sbQuiting = true
-//                                        )
-//                                }
-//                               );
-//        var tf = new TextField ();
-//        tf.KeyDown += Tf_KeyPressed;
-
-//        void Tf_KeyPressed (object sender, Key obj)
-//        {
-//            if (obj.KeyCode == (KeyCode.Q | KeyCode.CtrlMask))
-//            {
-//                obj.Handled = tfQuiting = true;
-//            }
-//        }
-
-//        var win = new Window ();
-//        win.Add (sb, tf);
-//        Toplevel top = new ();
-//        top.KeyDown += Top_KeyPress;
-
-//        void Top_KeyPress (object sender, Key obj)
-//        {
-//            if (obj.KeyCode == (KeyCode.Q | KeyCode.CtrlMask))
-//            {
-//                obj.Handled = topQuiting = true;
-//            }
-//        }
-
-//        top.Add (win);
-//        Application.Begin (top);
-
-//        Assert.False (sbQuiting);
-//        Assert.False (tfQuiting);
-//        Assert.False (topQuiting);
-
-//        Application.Driver?.SendKeys ('Q', ConsoleKey.Q, false, false, true);
-//        Assert.False (sbQuiting);
-//        Assert.True (tfQuiting);
-//        Assert.False (topQuiting);
-
-//#if BROKE_WITH_2927
-//        tf.KeyPressed -= Tf_KeyPress;
-//        tfQuiting = false;
-//        Application.Driver?.SendKeys ('q', ConsoleKey.Q, false, false, true);
-//        Application.MainLoop.RunIteration ();
-//        Assert.True (sbQuiting);
-//        Assert.False (tfQuiting);
-//        Assert.False (topQuiting);
-
-//        sb.RemoveItem (0);
-//        sbQuiting = false;
-//        Application.Driver?.SendKeys ('q', ConsoleKey.Q, false, false, true);
-//        Application.MainLoop.RunIteration ();
-//        Assert.False (sbQuiting);
-//        Assert.False (tfQuiting);
-
-//// This test is now invalid because `win` is focused, so it will receive the keypress
-//        Assert.True (topQuiting);
-//#endif
-//        top.Dispose ();
-//    }
-
-//    [Fact]
-//    [AutoInitShutdown]
-//    public void HotKey_Will_Invoke_KeyPressed_Only_For_The_MostFocused_Without_Top_KeyPress_Event ()
-//    {
-//        var sbQuiting = false;
-//        var tfQuiting = false;
-
-//        var sb = new StatusBar (
-//                                new Shortcut []
-//                                {
-//                                    new (
-//                                         KeyCode.CtrlMask | KeyCode.Q,
-//                                         "~^Q~ Quit",
-//                                         () => sbQuiting = true
-//                                        )
-//                                }
-//                               );
-//        var tf = new TextField ();
-//        tf.KeyDown += Tf_KeyPressed;
-
-//        void Tf_KeyPressed (object sender, Key obj)
-//        {
-//            if (obj.KeyCode == (KeyCode.Q | KeyCode.CtrlMask))
-//            {
-//                obj.Handled = tfQuiting = true;
-//            }
-//        }
-
-//        var win = new Window ();
-//        win.Add (sb, tf);
-//        Toplevel top = new ();
-//        top.Add (win);
-//        Application.Begin (top);
-
-//        Assert.False (sbQuiting);
-//        Assert.False (tfQuiting);
-
-//        Application.Driver?.SendKeys ('Q', ConsoleKey.Q, false, false, true);
-//        Assert.False (sbQuiting);
-//        Assert.True (tfQuiting);
-
-//        tf.KeyDown -= Tf_KeyPressed;
-//        tfQuiting = false;
-//        Application.Driver?.SendKeys ('Q', ConsoleKey.Q, false, false, true);
-//        Application.MainLoop.RunIteration ();
-//#if BROKE_WITH_2927
-//        Assert.True (sbQuiting);
-//        Assert.False (tfQuiting);
-//#endif
-//        top.Dispose ();
-//    }
+    //    [Fact]
+    //    [AutoInitShutdown]
+    //    public void HotKey_Will_Invoke_KeyPressed_Only_For_The_MostFocused_With_Top_KeyPress_Event ()
+    //    {
+    //        var sbQuiting = false;
+    //        var tfQuiting = false;
+    //        var topQuiting = false;
+
+    //        var sb = new StatusBar (
+    //                                new Shortcut []
+    //                                {
+    //                                    new (
+    //                                         KeyCode.CtrlMask | KeyCode.Q,
+    //                                         "Quit",
+    //                                         () => sbQuiting = true
+    //                                        )
+    //                                }
+    //                               );
+    //        var tf = new TextField ();
+    //        tf.KeyDown += Tf_KeyPressed;
+
+    //        void Tf_KeyPressed (object sender, Key obj)
+    //        {
+    //            if (obj.KeyCode == (KeyCode.Q | KeyCode.CtrlMask))
+    //            {
+    //                obj.Handled = tfQuiting = true;
+    //            }
+    //        }
+
+    //        var win = new Window ();
+    //        win.Add (sb, tf);
+    //        Toplevel top = new ();
+    //        top.KeyDown += Top_KeyPress;
+
+    //        void Top_KeyPress (object sender, Key obj)
+    //        {
+    //            if (obj.KeyCode == (KeyCode.Q | KeyCode.CtrlMask))
+    //            {
+    //                obj.Handled = topQuiting = true;
+    //            }
+    //        }
+
+    //        top.Add (win);
+    //        Application.Begin (top);
+
+    //        Assert.False (sbQuiting);
+    //        Assert.False (tfQuiting);
+    //        Assert.False (topQuiting);
+
+    //        Application.Driver?.SendKeys ('Q', ConsoleKey.Q, false, false, true);
+    //        Assert.False (sbQuiting);
+    //        Assert.True (tfQuiting);
+    //        Assert.False (topQuiting);
+
+    //#if BROKE_WITH_2927
+    //        tf.KeyPressed -= Tf_KeyPress;
+    //        tfQuiting = false;
+    //        Application.Driver?.SendKeys ('q', ConsoleKey.Q, false, false, true);
+    //        Application.MainLoop.RunIteration ();
+    //        Assert.True (sbQuiting);
+    //        Assert.False (tfQuiting);
+    //        Assert.False (topQuiting);
+
+    //        sb.RemoveItem (0);
+    //        sbQuiting = false;
+    //        Application.Driver?.SendKeys ('q', ConsoleKey.Q, false, false, true);
+    //        Application.MainLoop.RunIteration ();
+    //        Assert.False (sbQuiting);
+    //        Assert.False (tfQuiting);
+
+    //// This test is now invalid because `win` is focused, so it will receive the keypress
+    //        Assert.True (topQuiting);
+    //#endif
+    //        top.Dispose ();
+    //    }
+
+    //    [Fact]
+    //    [AutoInitShutdown]
+    //    public void HotKey_Will_Invoke_KeyPressed_Only_For_The_MostFocused_Without_Top_KeyPress_Event ()
+    //    {
+    //        var sbQuiting = false;
+    //        var tfQuiting = false;
+
+    //        var sb = new StatusBar (
+    //                                new Shortcut []
+    //                                {
+    //                                    new (
+    //                                         KeyCode.CtrlMask | KeyCode.Q,
+    //                                         "~^Q~ Quit",
+    //                                         () => sbQuiting = true
+    //                                        )
+    //                                }
+    //                               );
+    //        var tf = new TextField ();
+    //        tf.KeyDown += Tf_KeyPressed;
+
+    //        void Tf_KeyPressed (object sender, Key obj)
+    //        {
+    //            if (obj.KeyCode == (KeyCode.Q | KeyCode.CtrlMask))
+    //            {
+    //                obj.Handled = tfQuiting = true;
+    //            }
+    //        }
+
+    //        var win = new Window ();
+    //        win.Add (sb, tf);
+    //        Toplevel top = new ();
+    //        top.Add (win);
+    //        Application.Begin (top);
+
+    //        Assert.False (sbQuiting);
+    //        Assert.False (tfQuiting);
+
+    //        Application.Driver?.SendKeys ('Q', ConsoleKey.Q, false, false, true);
+    //        Assert.False (sbQuiting);
+    //        Assert.True (tfQuiting);
+
+    //        tf.KeyDown -= Tf_KeyPressed;
+    //        tfQuiting = false;
+    //        Application.Driver?.SendKeys ('Q', ConsoleKey.Q, false, false, true);
+    //        Application.MainLoop.RunIteration ();
+    //#if BROKE_WITH_2927
+    //        Assert.True (sbQuiting);
+    //        Assert.False (tfQuiting);
+    //#endif
+    //        top.Dispose ();
+    //    }
 
     [Fact]
     [SetupFakeDriver]
@@ -1011,7 +1011,7 @@ public class NavigationTests (ITestOutputHelper output) : TestsAllViews
 
         // top
         Assert.Equal (new Point (-3, -2), top.ScreenToFrame (new (0, 0)));
-        var  screen = top.Margin.ViewportToScreen (new Point (-3, -2));
+        var screen = top.Margin.ViewportToScreen (new Point (-3, -2));
         Assert.Equal (0, screen.X);
         Assert.Equal (0, screen.Y);
         screen = top.Border.ViewportToScreen (new Point (-3, -2));
@@ -1223,7 +1223,7 @@ public class NavigationTests (ITestOutputHelper output) : TestsAllViews
         v1.TabIndex = 0;
         Assert.True (r.Subviews.IndexOf (v1) == 0);
         Assert.True (r.TabIndexes.IndexOf (v1) == 0);
-        Assert.Equal (-1, v1.TabIndex);
+        Assert.NotEqual (-1, v1.TabIndex);
         r.Dispose ();
     }
 
@@ -1270,7 +1270,7 @@ public class NavigationTests (ITestOutputHelper output) : TestsAllViews
 
         r.Add (v1, v2, v3);
 
-        v1.TabIndex = -1;
+        //v1.TabIndex = -1;
         Assert.True (r.Subviews.IndexOf (v1) == 0);
         Assert.True (r.TabIndexes.IndexOf (v1) == 0);
         r.Dispose ();
@@ -1541,11 +1541,11 @@ public class NavigationTests (ITestOutputHelper output) : TestsAllViews
     {
         var view = new View { CanFocus = canFocus, TabStop = tabStop };
 
-        Assert.Equal(canFocus, view.CanFocus);
+        Assert.Equal (canFocus, view.CanFocus);
         Assert.Equal (tabStop, view.TabStop);
     }
 
-    [Fact (Skip="Causes crash on Ubuntu in Github Action. Bogus test anyway.")]
+    [Fact (Skip = "Causes crash on Ubuntu in Github Action. Bogus test anyway.")]
     public void WindowDispose_CanFocusProblem ()
     {
         // Arrange
@@ -1569,7 +1569,7 @@ public class NavigationTests (ITestOutputHelper output) : TestsAllViews
     // View.Focused & View.MostFocused tests
 
     // View.Focused - No subviews
-    [Fact, Trait("BUGBUG", "Fix in Issue #3444")]
+    [Fact, Trait ("BUGBUG", "Fix in Issue #3444")]
     public void Focused_NoSubviews ()
     {
         var view = new View ();