Fixes #4080. TabGroup not always navigate correctly across groups (#4085)

* Fixes #4080. TabGroup not always navigate correctly across groups

* Remove duplicated code

* Improves code by removing duplicate code

* Made requested changes

* Change to Theory unit test

* Cleanup to run git actions again

* Trying fix racing fail unit tests

---------

Co-authored-by: Tig <[email protected]>

Terminal.Gui/App/Application.Keyboard.cs

@@ -98,17 +98,11 @@ public static partial class Application // Keyboard handling
             }
             else
             {
-                // BUGBUG: this seems unneeded.
-                if (!KeyBindings.TryGet (key, out KeyBinding keybinding))
-                {
-                    return null;
-                }
-
                 bool? toReturn = null;
 
-                foreach (Command command in keybinding.Commands)
+                foreach (Command command in binding.Commands)
                 {
-                    toReturn = InvokeCommand (command, key, keybinding);
+                    toReturn = InvokeCommand (command, key, binding);
                 }
 
                 handled = toReturn ?? true;

Terminal.Gui/ViewBase/View.Hierarchy.cs

@@ -469,10 +469,29 @@ public partial class View // SuperView/SubView hierarchy management (SuperView,
 
     /// <summary>
     ///     Moves <paramref name="subview"/> to the end of the <see cref="SubViews"/> list.
+    ///     If the <see cref="Arrangement"/> is <see cref="ViewArrangement.Overlapped"/>, keeps the original sorting.
     /// </summary>
     /// <param name="subview">The subview to move.</param>
     public void MoveSubViewToEnd (View subview)
     {
+        if (Arrangement.HasFlag (ViewArrangement.Overlapped))
+        {
+            PerformActionForSubView (
+                                     subview,
+                                     x =>
+                                     {
+                                         while (InternalSubViews!.IndexOf (x) != InternalSubViews.Count - 1)
+                                         {
+                                             View v = InternalSubViews [0];
+                                             InternalSubViews!.Remove (v);
+                                             InternalSubViews.Add (v);
+                                         }
+                                     }
+                                    );
+
+            return;
+        }
+
         PerformActionForSubView (
                                  subview,
                                  x =>

Terminal.Gui/ViewBase/View.Navigation.cs

@@ -62,38 +62,18 @@ public partial class View // Focus and cross-view navigation management (TabStop
             if (direction == NavigationDirection.Forward && focused == focusChain [^1] && SuperView is null)
             {
                 // We're at the top of the focus chain. Go back down the focus chain and focus the first TabGroup
-                View [] views = GetFocusChain (NavigationDirection.Forward, TabBehavior.TabGroup);
-
-                if (views.Length > 0)
+                if (AdvanceFocusChain ())
                 {
-                    View [] subViews = views [0].GetFocusChain (NavigationDirection.Forward, TabBehavior.TabStop);
-
-                    if (subViews.Length > 0)
-                    {
-                        if (subViews [0].SetFocus ())
-                        {
-                            return true;
-                        }
-                    }
+                    return true;
                 }
             }
 
-            if (direction == NavigationDirection.Backward && focused == focusChain [0])
+            if (direction == NavigationDirection.Backward && focused == focusChain [0] && SuperView is null)
             {
                 // We're at the bottom of the focus chain
-                View [] views = GetFocusChain (NavigationDirection.Forward, TabBehavior.TabGroup);
-
-                if (views.Length > 0)
+                if (AdvanceFocusChain ())
                 {
-                    View [] subViews = views [^1].GetFocusChain (NavigationDirection.Forward, TabBehavior.TabStop);
-
-                    if (subViews.Length > 0)
-                    {
-                        if (subViews [0].SetFocus ())
-                        {
-                            return true;
-                        }
-                    }
+                    return true;
                 }
             }
         }
@@ -149,6 +129,46 @@ public partial class View // Focus and cross-view navigation management (TabStop
         (bool focusSet, bool _) = view.SetHasFocusTrue (Focused);
 
         return focusSet;
+
+        bool AdvanceFocusChain ()
+        {
+            if (focusChain.Length > 0)
+            {
+                // Get the index of the currently focused view
+                int focusedTabGroupIndex = focusChain.IndexOf (Focused); // Will return -1 if Focused can't be found or is null
+
+                if (focusedTabGroupIndex + 1 > focusChain.Length - 1)
+                {
+                    focusedTabGroupIndex = 0;
+                }
+                else
+                {
+                    focusedTabGroupIndex++;
+                }
+
+                View [] subViews = focusChain [focusedTabGroupIndex].GetFocusChain (NavigationDirection.Forward, TabBehavior.TabStop);
+
+                if (subViews.Length > 0)
+                {
+                    if (focusChain [focusedTabGroupIndex]._previouslyFocused is { }
+                        && subViews.Any (v => v == focusChain [focusedTabGroupIndex]._previouslyFocused))
+                    {
+                        if (focusChain [focusedTabGroupIndex]._previouslyFocused!.SetFocus ())
+                        {
+                            return true;
+                        }
+                    }
+
+                    // We have a subview that can be focused
+                    if (subViews [0].SetFocus ())
+                    {
+                        return true;
+                    }
+                }
+            }
+
+            return false;
+        }
     }
 
     private bool RaiseAdvancingFocus (NavigationDirection direction, TabBehavior? behavior)

Tests/IntegrationTests/FluentTests/BasicFluentAssertionTests.cs

@@ -12,7 +12,6 @@ public class BasicFluentAssertionTests
         _out = new TestOutputWriter (outputHelper);
     }
 
-
     [Theory]
     [ClassData (typeof (V2TestDrivers))]
     public void GuiTestContext_NewInstance_Runs (V2TestDriver d)
@@ -24,7 +23,6 @@ public class BasicFluentAssertionTests
         context.Stop ();
     }
 
-
     [Theory]
     [ClassData (typeof (V2TestDrivers))]
     public void GuiTestContext_QuitKey_Stops (V2TestDriver d)
@@ -153,4 +151,76 @@ public class BasicFluentAssertionTests
                                      .WriteOutLogs (_out);
         Assert.True (clicked);
     }
+
+    [Theory]
+    [ClassData (typeof (V2TestDrivers))]
+    public void Toplevel_TabGroup_Forward_Backward (V2TestDriver d)
+    {
+        var v1 = new View { Id = "v1", CanFocus = true };
+        var v2 = new View { Id = "v2", CanFocus = true };
+        var v3 = new View { Id = "v3", CanFocus = true };
+        var v4 = new View { Id = "v4", CanFocus = true };
+        var v5 = new View { Id = "v5", CanFocus = true };
+        var v6 = new View { Id = "v6", CanFocus = true };
+
+        using GuiTestContext c = With.A<Window> (50, 20, d)
+                                     .Then (
+                                            () =>
+                                            {
+                                                var w1 = new Window { Id = "w1" };
+                                                w1.Add (v1, v2);
+                                                var w2 = new Window { Id = "w2" };
+                                                w2.Add (v3, v4);
+                                                var w3 = new Window { Id = "w3" };
+                                                w3.Add (v5, v6);
+                                                Toplevel top = Application.Top!;
+                                                Application.Top!.Add (w1, w2, w3);
+                                            })
+                                     .WaitIteration ()
+                                     .Then (() => Assert.True (v5.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6)
+                                     .Then (() => Assert.True (v1.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6)
+                                     .Then (() => Assert.True (v3.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6.WithShift)
+                                     .Then (() => Assert.True (v1.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6.WithShift)
+                                     .Then (() => Assert.True (v5.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6.WithShift)
+                                     .Then (() => Assert.True (v3.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6)
+                                     .Then (() => Assert.True (v5.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6)
+                                     .Then (() => Assert.True (v1.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6)
+                                     .Then (() => Assert.True (v3.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6.WithShift)
+                                     .Then (() => Assert.True (v1.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6.WithShift)
+                                     .Then (() => Assert.True (v5.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6.WithShift)
+                                     .Then (() => Assert.True (v3.HasFocus))
+                                     .RaiseKeyDownEvent (Key.Tab)
+                                     .Then (() => Assert.True (v4.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6)
+                                     .Then (() => Assert.True (v5.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6)
+                                     .Then (() => Assert.True (v1.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6.WithShift)
+                                     .Then (() => Assert.True (v5.HasFocus))
+                                     .RaiseKeyDownEvent (Key.Tab)
+                                     .Then (() => Assert.True (v6.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6.WithShift)
+                                     .Then (() => Assert.True (v4.HasFocus))
+                                     .RaiseKeyDownEvent (Key.F6)
+                                     .Then (() => Assert.True (v6.HasFocus))
+                                     .WriteOutLogs (_out)
+                                     .Stop ();
+        Assert.False (v1.HasFocus);
+        Assert.False (v2.HasFocus);
+        Assert.False (v3.HasFocus);
+        Assert.False (v4.HasFocus);
+        Assert.False (v5.HasFocus);
+        Assert.False (v6.HasFocus);
+    }
 }

Tests/UnitTests/Application/KeyboardTests.cs

@@ -215,7 +215,7 @@ public class KeyboardTests
                                      Assert.True (v3.HasFocus);
 
                                      Application.RaiseKeyDownEvent (Key.F6);
-                                     Assert.True (v1.HasFocus);
+                                     Assert.True (v2.HasFocus); // previously focused view was preserved
 
                                      Application.RequestStop ();
                                  };

Tests/UnitTestsParallelizable/View/SubviewTests.cs

@@ -118,6 +118,40 @@ public class SubViewTests
         Assert.Equal (new (5, 5), view.GetContentSize ());
     }
 
+    [Theory]
+    [InlineData (ViewArrangement.Fixed)]
+    [InlineData (ViewArrangement.Overlapped)]
+    public void MoveSubViewToEnd_ViewArrangement (ViewArrangement arrangement)
+    {
+        View superView = new () { Arrangement = arrangement };
+
+        var subview1 = new View
+        {
+            Id = "subview1"
+        };
+
+        var subview2 = new View
+        {
+            Id = "subview2"
+        };
+
+        var subview3 = new View
+        {
+            Id = "subview3"
+        };
+
+        superView.Add (subview1, subview2, subview3);
+
+        superView.MoveSubViewToEnd (subview1);
+        Assert.Equal ([subview2, subview3, subview1], superView.SubViews.ToArray ());
+
+        superView.MoveSubViewToEnd (subview2);
+        Assert.Equal ([subview3, subview1, subview2], superView.SubViews.ToArray ());
+
+        superView.MoveSubViewToEnd (subview3);
+        Assert.Equal ([subview1, subview2, subview3], superView.SubViews.ToArray ());
+    }
+
     [Fact]
     public void MoveSubViewToStart ()
     {