Fixes #3707 - Wizard focus (#3708)

* Initial commit.

* More fixes

* Fixed AdvanceFocus issue with nested subviews. E.g. Wizard->WizardStep->_contentView->subview

* Fixed DatePicker tests

* Fixed DatePicker tests

Terminal.Gui/View/View.Navigation.cs

@@ -14,7 +14,7 @@ public partial class View // Focus and cross-view navigation management (TabStop
     /// </summary>
     /// <remarks>
     ///     <para>
-    ///         If there is no next/previous view, the focus is set to the view itself.
+    ///         If there is no next/previous view to advance to, the focus is set to the view itself.
     ///     </para>
     /// </remarks>
     /// <param name="direction"></param>
@@ -37,17 +37,19 @@ public partial class View // Focus and cross-view navigation management (TabStop
             return true;
         }
 
-        // AdvanceFocus did not advance
-        View [] index = GetSubviewFocusChain (direction, behavior);
+        // AdvanceFocus did not advance - do we wrap, or move up to the superview?
 
-        if (index.Length == 0)
+        View [] focusChain = GetSubviewFocusChain (direction, behavior);
+
+        if (focusChain.Length == 0)
         {
             return false;
         }
 
+        // Special case TabGroup
         if (behavior == TabBehavior.TabGroup)
         {
-            if (direction == NavigationDirection.Forward && focused == index [^1] && SuperView is null)
+            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 = GetSubviewFocusChain (NavigationDirection.Forward, TabBehavior.TabGroup);
@@ -66,7 +68,7 @@ public partial class View // Focus and cross-view navigation management (TabStop
                 }
             }
 
-            if (direction == NavigationDirection.Backward && focused == index [0])
+            if (direction == NavigationDirection.Backward && focused == focusChain [0])
             {
                 // We're at the bottom of the focus chain
                 View [] views = GetSubviewFocusChain (NavigationDirection.Forward, TabBehavior.TabGroup);
@@ -86,38 +88,38 @@ public partial class View // Focus and cross-view navigation management (TabStop
             }
         }
 
-        int focusedIndex = index.IndexOf (Focused); // Will return -1 if Focused can't be found or is null
-        int next = 0;
+        int focusedIndex = focusChain.IndexOf (Focused); // Will return -1 if Focused can't be found or is null
+        var next = 0; // Assume we wrap to start of the focus chain
 
-        if (focusedIndex < index.Length - 1)
+        if (focusedIndex < focusChain.Length - 1)
         {
             // We're moving w/in the subviews
             next = focusedIndex + 1;
         }
         else
         {
-            // We're moving beyond the last subview
-
             // Determine if focus should remain in this focus chain, or move to the superview's focus chain
-
-            // If we are TabStop and our SuperView has at least one other TabStop subview, move to the SuperView's chain
-            if (TabStop == TabBehavior.TabStop && SuperView is { } && SuperView.GetSubviewFocusChain (direction, behavior).Length > 1)
+            if (SuperView is { })
             {
-                return false;
-            }
-
-            // TabGroup is special-cased. 
-            if (focused?.TabStop == TabBehavior.TabGroup)
-            {
-                if (SuperView?.GetSubviewFocusChain (direction, TabBehavior.TabGroup)?.Length > 0)
+                // If we are TabStop, and we have at least one other focusable peer, move to the SuperView's chain
+                if (TabStop == TabBehavior.TabStop && SuperView is { } && SuperView.GetSubviewFocusChain (direction, behavior).Length > 1)
                 {
-                    // Our superview has a TabGroup subview; signal we couldn't move so we nav out to it
                     return false;
                 }
+
+                // TabGroup is special-cased. 
+                if (focused?.TabStop == TabBehavior.TabGroup)
+                {
+                    if (SuperView?.GetSubviewFocusChain (direction, TabBehavior.TabGroup)?.Length > 0)
+                    {
+                        // Our superview has a TabGroup subview; signal we couldn't move so we nav out to it
+                        return false;
+                    }
+                }
             }
         }
 
-        View view = index [next];
+        View view = focusChain [next];
 
         if (view.HasFocus)
         {
@@ -259,7 +261,7 @@ public partial class View // Focus and cross-view navigation management (TabStop
     {
         if (Focused is null && _subviews?.Count > 0)
         {
-            if (_previouslyMostFocused is { } /* && (behavior is null || _previouslyMostFocused.TabStop == behavior)*/)
+            if (_previouslyMostFocused is { })
             {
                 return _previouslyMostFocused.SetFocus ();
             }
@@ -355,6 +357,11 @@ public partial class View // Focus and cross-view navigation management (TabStop
         return focusSet;
     }
 
+    /// <summary>
+    ///     Caches the most focused subview when this view is losing focus. This is used by <see cref="RestoreFocus"/>.
+    /// </summary>
+    private View? _previouslyMostFocused;
+
     /// <summary>
     ///     INTERNAL: Called when focus is going to change to this view. This method is called by <see cref="SetFocus"/> and
     ///     other methods that
@@ -625,7 +632,7 @@ public partial class View // Focus and cross-view navigation management (TabStop
 
         if (SuperView is { })
         {
-            SuperView._previouslyMostFocused = focusedPeer;
+            //SuperView._previouslyMostFocused = focusedPeer;
         }
 
         // Post-conditions - prove correctness
@@ -637,11 +644,6 @@ public partial class View // Focus and cross-view navigation management (TabStop
         SetNeedsDisplay ();
     }
 
-    /// <summary>
-    ///     Caches the most focused subview when this view is losing focus. This is used by <see cref="RestoreFocus"/>.
-    /// </summary>
-    private View? _previouslyMostFocused;
-
     private void NotifyFocusChanged (bool newHasFocus, View? previousFocusedView, View? focusedVew)
     {
         // Call the virtual method

Terminal.Gui/Views/DatePicker.cs

@@ -192,6 +192,7 @@ public class DatePicker : View
 
         _calendar = new TableView
         {
+            Id = "_calendar",
             X = 0,
             Y = Pos.Bottom (_dateLabel),
             Height = 11,
@@ -206,6 +207,7 @@ public class DatePicker : View
 
         _dateField = new DateField (DateTime.Now)
         {
+            Id = "_dateField",
             X = Pos.Right (_dateLabel),
             Y = 0,
             Width = Dim.Width (_calendar) - Dim.Width (_dateLabel),
@@ -215,6 +217,7 @@ public class DatePicker : View
 
         _previousMonthButton = new Button
         {
+            Id = "_previousMonthButton",
             X = Pos.Center () - 2,
             Y = Pos.Bottom (_calendar) - 1,
             Width = 2,
@@ -234,6 +237,7 @@ public class DatePicker : View
 
         _nextMonthButton = new Button
         {
+            Id = "_nextMonthButton",
             X = Pos.Right (_previousMonthButton) + 2,
             Y = Pos.Bottom (_calendar) - 1,
             Width = 2,

Terminal.Gui/Views/Wizard/Wizard.cs

@@ -351,14 +351,11 @@ public class Wizard : Dialog
 
         UpdateButtonsAndTitle ();
 
-        // Set focus to the nav buttons
-        if (BackButton.HasFocus)
+
+        // Set focus on the contentview
+        if (newStep is { })
         {
-            BackButton.SetFocus ();
-        }
-        else
-        {
-            NextFinishButton.SetFocus ();
+            newStep.Subviews.ToArray () [0].SetFocus ();
         }
 
         if (OnStepChanged (oldStep, _currentStep))
@@ -543,7 +540,7 @@ public class Wizard : Dialog
 
         SetNeedsLayout ();
         LayoutSubviews ();
-        Draw ();
+        //Draw ();
     }
 
     private void Wizard_Closing (object sender, ToplevelClosingEventArgs obj)

Terminal.Gui/Views/Wizard/WizardStep.cs

@@ -13,7 +13,7 @@
 ///     the step is active; see also: <see cref="Wizard.StepChanged"/>. To enable or disable a step from being shown to the
 ///     user, set <see cref="View.Enabled"/>.
 /// </remarks>
-public class WizardStep : FrameView
+public class WizardStep : View
 {
     ///// <summary>
     ///// The title of the <see cref="WizardStep"/>. 
@@ -36,19 +36,32 @@ public class WizardStep : FrameView
     //private string title = string.Empty;
 
     // The contentView works like the ContentView in FrameView.
-    private readonly View _contentView = new () { Id = "WizardContentView" };
-    private readonly TextView _helpTextView = new ();
+    private readonly View _contentView = new ()
+    {
+        CanFocus = true,
+        TabStop = TabBehavior.TabStop,
+        Id = "WizardStep._contentView"
+    };
+    private readonly TextView _helpTextView = new ()
+    {
+        CanFocus = true,
+        TabStop = TabBehavior.TabStop,
+        ReadOnly = true,
+        WordWrap = true,
+        AllowsTab = false,
+        Id = "WizardStep._helpTextView"
+    };
 
     /// <summary>
     ///     Initializes a new instance of the <see cref="Wizard"/> class.
     /// </summary>
     public WizardStep ()
     {
+        TabStop = TabBehavior.TabStop;
+        CanFocus = true;
         BorderStyle = LineStyle.None;
         base.Add (_contentView);
 
-        _helpTextView.ReadOnly = true;
-        _helpTextView.WordWrap = true;
         base.Add (_helpTextView);
 
         // BUGBUG: v2 - Disabling scrolling for now
@@ -144,11 +157,6 @@ public class WizardStep : FrameView
     /// <remarks></remarks>
     public override View Remove (View view)
     {
-        if (view is null)
-        {
-            return view;
-        }
-
         SetNeedsDisplay ();
         View container = view?.SuperView;
 

UICatalog/Scenarios/Wizards.cs

@@ -26,7 +26,7 @@ public class Wizards : Scenario
         };
         win.Add (frame);
 
-        var label = new Label { X = 0, Y = 0, TextAlignment = Alignment.End, Text = "Width:" };
+        var label = new Label { X = 0, Y = 0, TextAlignment = Alignment.End, Text = "_Width:", Width = 10 };
         frame.Add (label);
 
         var widthEdit = new TextField
@@ -39,7 +39,7 @@ public class Wizards : Scenario
         };
         frame.Add (widthEdit);
 
-        label = new()
+        label = new ()
         {
             X = 0,
             Y = Pos.Bottom (label),
@@ -47,7 +47,7 @@ public class Wizards : Scenario
             Width = Dim.Width (label),
             Height = 1,
             TextAlignment = Alignment.End,
-            Text = "Height:"
+            Text = "_Height:"
         };
         frame.Add (label);
 
@@ -61,7 +61,7 @@ public class Wizards : Scenario
         };
         frame.Add (heightEdit);
 
-        label = new()
+        label = new ()
         {
             X = 0,
             Y = Pos.Bottom (label),
@@ -69,7 +69,7 @@ public class Wizards : Scenario
             Width = Dim.Width (label),
             Height = 1,
             TextAlignment = Alignment.End,
-            Text = "Title:"
+            Text = "_Title:"
         };
         frame.Add (label);
 
@@ -91,7 +91,7 @@ public class Wizards : Scenario
 
         win.Loaded += Win_Loaded;
 
-        label = new()
+        label = new ()
         {
             X = Pos.Center (), Y = Pos.AnchorEnd (1), TextAlignment = Alignment.End, Text = "Action:"
         };
@@ -105,7 +105,7 @@ public class Wizards : Scenario
 
         var showWizardButton = new Button
         {
-            X = Pos.Center (), Y = Pos.Bottom (frame) + 2, IsDefault = true, Text = "Show Wizard"
+            X = Pos.Center (), Y = Pos.Bottom (frame) + 2, IsDefault = true, Text = "_Show Wizard"
         };
 
         showWizardButton.Accept += (s, e) =>
@@ -162,6 +162,13 @@ public class Wizards : Scenario
 
                                            firstStep.HelpText =
                                                "This is the End User License Agreement.\n\n\n\n\n\nThis is a test of the emergency broadcast system. This is a test of the emergency broadcast system.\nThis is a test of the emergency broadcast system.\n\n\nThis is a test of the emergency broadcast system.\n\nThis is a test of the emergency broadcast system.\n\n\n\nThe end of the EULA.";
+
+                                           RadioGroup radioGroup = new ()
+                                           {
+                                               RadioLabels = ["_One", "_Two", "_3"]
+                                           };
+                                           firstStep.Add (radioGroup);
+
                                            wizard.AddStep (firstStep);
 
                                            // Add 2nd step
@@ -178,6 +185,13 @@ public class Wizards : Scenario
                                                Text = "Press Me to Rename Step", X = Pos.Right (buttonLbl), Y = Pos.Top (buttonLbl)
                                            };
 
+                                           RadioGroup radioGroup2 = new ()
+                                           {
+                                               RadioLabels = ["_A", "_B", "_C"],
+                                               Orientation = Orientation.Horizontal
+                                           };
+                                           secondStep.Add (radioGroup2);
+
                                            button.Accept += (s, e) =>
                                                             {
                                                                 secondStep.Title = "2nd Step";
@@ -193,7 +207,7 @@ public class Wizards : Scenario
                                            var firstNameField =
                                                new TextField { Text = "Number", Width = 30, X = Pos.Right (lbl), Y = Pos.Top (lbl) };
                                            secondStep.Add (lbl, firstNameField);
-                                           lbl = new() { Text = "Last Name:  ", X = 1, Y = Pos.Bottom (lbl) };
+                                           lbl = new () { Text = "Last Name:  ", X = 1, Y = Pos.Bottom (lbl) };
                                            var lastNameField = new TextField { Text = "Six", Width = 30, X = Pos.Right (lbl), Y = Pos.Top (lbl) };
                                            secondStep.Add (lbl, lastNameField);
 
@@ -213,7 +227,8 @@ public class Wizards : Scenario
                                                Y = Pos.Bottom (thirdStepEnabledCeckBox) + 2,
                                                Width = Dim.Fill (),
                                                Height = 4,
-                                               Title = "A Broken Frame (by Depeche Mode)"
+                                               Title = "A Broken Frame (by Depeche Mode)",
+                                               TabStop = TabBehavior.NoStop
                                            };
                                            frame.Add (new TextField { Text = "This is a TextField inside of the frame." });
                                            secondStep.Add (frame);
@@ -286,7 +301,7 @@ public class Wizards : Scenario
                                                                      }
                                                                  };
                                            fourthStep.Add (hideHelpBtn);
-                                           fourthStep.NextButtonText = "Go To Last Step";
+                                           fourthStep.NextButtonText = "_Go To Last Step";
                                            var scrollBar = new ScrollBarView (someText, true);
 
                                            scrollBar.ChangedPosition += (s, e) =>
@@ -355,4 +370,9 @@ public class Wizards : Scenario
         win.Dispose ();
         Application.Shutdown ();
     }
+
+    private void Wizard_StepChanged (object sender, StepChangeEventArgs e)
+    {
+        throw new NotImplementedException ();
+    }
 }

UnitTests/View/Navigation/AdvanceFocusTests.cs

@@ -1,4 +1,5 @@
-using Xunit.Abstractions;
+using System.Runtime.Intrinsics;
+using Xunit.Abstractions;
 
 namespace Terminal.Gui.ViewTests;
 
@@ -72,6 +73,38 @@ public class AdvanceFocusTests ()
         r.Dispose ();
     }
 
+
+    [Fact]
+    public void AdvanceFocus_Subviews_TabStop ()
+    {
+        TabBehavior behavior = TabBehavior.TabStop;
+        var top = new View { Id = "top", CanFocus = true };
+
+        var v1 = new View { Id = "v1", CanFocus = true, TabStop = behavior };
+        var v2 = new View { Id = "v2", CanFocus = true, TabStop = behavior };
+        var v3 = new View { Id = "v3", CanFocus = false, TabStop = behavior };
+
+        top.Add (v1, v2, v3);
+
+        // Cycle through v1 & v2
+        top.AdvanceFocus (NavigationDirection.Forward, behavior);
+        Assert.True (v1.HasFocus);
+        top.AdvanceFocus (NavigationDirection.Forward, behavior);
+        Assert.True (v2.HasFocus);
+
+        // Should cycle back to v1
+        top.AdvanceFocus (NavigationDirection.Forward, behavior);
+        Assert.True (v1.HasFocus);
+
+        // Go backwards
+        top.AdvanceFocus (NavigationDirection.Backward, behavior);
+        Assert.True (v2.HasFocus);
+        top.AdvanceFocus (NavigationDirection.Backward, behavior);
+        Assert.True (v1.HasFocus);
+
+        top.Dispose ();
+    }
+
     [Fact]
     public void AdvanceFocus_Compound_Subview_TabStop ()
     {
@@ -145,6 +178,102 @@ public class AdvanceFocusTests ()
         top.Dispose ();
     }
 
+
+    [Fact]
+    public void AdvanceFocus_CompoundCompound_Subview_TabStop ()
+    {
+        TabBehavior behavior = TabBehavior.TabStop;
+        var top = new View { Id = "top", CanFocus = true };
+        var topv1 = new View { Id = "topv1", CanFocus = true, TabStop = behavior };
+        var topv2 = new View { Id = "topv2", CanFocus = true, TabStop = behavior };
+        var topv3 = new View { Id = "topv3", CanFocus = false, TabStop = behavior };
+        top.Add (topv1, topv2, topv3);
+
+        var compoundSubview = new View
+        {
+            CanFocus = true,
+            Id = "compoundSubview",
+            TabStop = behavior
+        };
+        var v1 = new View { Id = "v1", CanFocus = true, TabStop = behavior };
+        var v2 = new View { Id = "v2", CanFocus = true, TabStop = behavior };
+        var v3 = new View { Id = "v3", CanFocus = false, TabStop = behavior };
+
+        compoundSubview.Add (v1, v2, v3);
+
+
+        var compoundCompoundSubview = new View
+        {
+            CanFocus = true,
+            Id = "compoundCompoundSubview",
+            TabStop = behavior
+        };
+        var v4 = new View { Id = "v4", CanFocus = true, TabStop = behavior };
+        var v5 = new View { Id = "v5", CanFocus = true, TabStop = behavior };
+        var v6 = new View { Id = "v6", CanFocus = false, TabStop = behavior };
+
+        compoundCompoundSubview.Add (v4, v5, v6);
+
+        compoundSubview.Add (compoundCompoundSubview);
+
+        top.Add (compoundSubview);
+
+        top.SetFocus ();
+        Assert.True (topv1.HasFocus);
+
+        // Cycle through topv2
+        top.AdvanceFocus (NavigationDirection.Forward, behavior);
+        Assert.True (topv2.HasFocus);
+
+        // Cycle v1, v2, v4, v5
+        top.AdvanceFocus (NavigationDirection.Forward, behavior);
+        Assert.True (v1.HasFocus);
+        top.AdvanceFocus (NavigationDirection.Forward, behavior);
+        Assert.True (v2.HasFocus);
+        top.AdvanceFocus (NavigationDirection.Forward, behavior);
+        Assert.True (v4.HasFocus);
+        top.AdvanceFocus (NavigationDirection.Forward, behavior);
+        Assert.True (v5.HasFocus);
+
+        // Should cycle back to topv1
+        top.AdvanceFocus (NavigationDirection.Forward, behavior);
+        Assert.True (topv1.HasFocus);
+        top.AdvanceFocus (NavigationDirection.Forward, behavior);
+        Assert.True (topv2.HasFocus);
+
+        // Add another top subview. Should cycle to it after v5
+        View otherSubview = new ()
+        {
+            CanFocus = true,
+            TabStop = behavior,
+            Id = "otherSubview"
+        };
+
+        top.Add (otherSubview);
+
+        // Adding a focusable subview causes advancefocus
+        Assert.True (otherSubview.HasFocus);
+
+        // Cycle through topv1, topv2, v1, v2, v4, v5
+        top.AdvanceFocus (NavigationDirection.Forward, behavior);
+        Assert.True (topv1.HasFocus);
+        top.AdvanceFocus (NavigationDirection.Forward, behavior);
+        Assert.True (topv2.HasFocus);
+        top.AdvanceFocus (NavigationDirection.Forward, behavior);
+
+        // the above should have cycled to v5 since it was the previously most focused subview of compoundSubView
+        Assert.True (v5.HasFocus);
+
+        top.AdvanceFocus (NavigationDirection.Forward, behavior);
+        Assert.True (otherSubview.HasFocus);
+
+        // Should cycle back to topv1
+        top.AdvanceFocus (NavigationDirection.Forward, behavior);
+        Assert.True (topv1.HasFocus);
+
+        top.Dispose ();
+    }
+
     [Fact]
     public void AdvanceFocus_Compound_Subview_TabGroup ()
     {

UnitTests/Views/DatePickerTests.cs

@@ -71,18 +71,25 @@ public class DatePickerTests
         top.Add (datePicker);
         Application.Begin (top);
 
+        Assert.Equal (datePicker.Subviews.First (v => v.Id == "_dateField"), datePicker.Focused);
+
         // Set focus to next month button
         datePicker.AdvanceFocus (NavigationDirection.Forward, TabBehavior.TabStop);
+        Assert.Equal (datePicker.Subviews.First (v => v.Id == "_calendar"), datePicker.Focused);
         datePicker.AdvanceFocus (NavigationDirection.Forward, TabBehavior.TabStop);
+        Assert.Equal (datePicker.Subviews.First (v => v.Id == "_previousMonthButton"), datePicker.Focused);
         datePicker.AdvanceFocus (NavigationDirection.Forward, TabBehavior.TabStop);
+        Assert.Equal (datePicker.Subviews.First (v => v.Id == "_nextMonthButton"), datePicker.Focused);
 
         // Change month to December
         Assert.True (Application.OnKeyDown (Key.Enter));
         Assert.Equal (12, datePicker.Date.Month);
 
-        // Date should change as next month button was disabled, causing focus to advance
-        Assert.True (Application.OnKeyDown (Key.Enter));
-        Assert.Equal (11, datePicker.Date.Month);
+        // Next month button is disabled, so focus advanced to edit field
+        Assert.Equal (datePicker.Subviews.First (v => v.Id == "_dateField"), datePicker.Focused);
+
+        // Pressing enter on datefield does nothing
+        Assert.False (Application.OnKeyDown (Key.Enter));
         top.Dispose ();
     }
 
@@ -98,17 +105,22 @@ public class DatePickerTests
         top.Add (datePicker);
         Application.Begin (top);
 
-        // set focus to the previous month button
+        Assert.Equal (datePicker.Subviews.First (v => v.Id == "_dateField"), datePicker.Focused);
+
         datePicker.AdvanceFocus (NavigationDirection.Forward, TabBehavior.TabStop);
+        Assert.Equal (datePicker.Subviews.First (v => v.Id == "_calendar"), datePicker.Focused);
         datePicker.AdvanceFocus (NavigationDirection.Forward, TabBehavior.TabStop);
+        Assert.Equal (datePicker.Subviews.First (v => v.Id == "_previousMonthButton"), datePicker.Focused);
 
         // Change month to January 
         Assert.True (datePicker.NewKeyDownEvent (Key.Enter));
         Assert.Equal (1, datePicker.Date.Month);
 
-        // Previous month button is disabled, so focus advanced to edit field
-        Assert.True (datePicker.NewKeyDownEvent (Key.Enter));
-        Assert.Equal (1, datePicker.Date.Month);
+        // Next prev button is disabled, so focus advanced to edit button
+        Assert.Equal (datePicker.Subviews.First (v => v.Id == "_dateField"), datePicker.Focused);
+
+        // Pressing enter on datefield does nothing
+        Assert.False (Application.OnKeyDown (Key.Enter));
         top.Dispose ();
     }
 }