Improved menu keyboard navigation (#4480)

* Block inputs when menus are open in MainForm by moving TAStudio's logic for that into FormBase.

* Remove unneeded condition and combine methods into one properly named one.

* Fix: Pressing space while the tool strip menu was focused did not work.

* Fix: Alt+F did not work for input-blocking tools.

* Fix: Pressing alt did not focus the tool stip menu; pressing Alt then F did not work.

* Do not focus the tool strip menu if a hotkey or input was done.

Author: SuuperW

src/BizHawk.Client.Common/inputAdapters/InputManager.cs

@@ -149,10 +149,11 @@ private static AutofireController BindToDefinitionAF(
 		/// <summary>
 		/// Processes queued inputs and triggers input evets (i.e. hotkeys), but does not update output controllers.<br/>
 		/// </summary>
-		/// <param name="processUnboundInput">Events that did not do anything are forwarded out here.
+		/// <param name="processSpecialInput">All input events are forwarded out here.
 		/// This allows things like Windows' standard alt hotkeys (for menu items) to be handled by the
-		/// caller if the input didn't alrady do something else.</param>
-		public void ProcessInput(IPhysicalInputSource source, Func<string, bool> processHotkey, Config config, Action<InputEvent> processUnboundInput)
+		/// caller if the input didn't alrady do something else.
+		/// <br/>The second parameter is true if the input already did something (hotkey or controller input).</param>
+		public void ProcessInput(IPhysicalInputSource source, Func<string, bool> processHotkey, Config config, Action<InputEvent, bool> processSpecialInput)
 		{
 			// loop through all available events
 			InputEvent ie;
@@ -191,10 +192,7 @@ public void ProcessInput(IPhysicalInputSource source, Func<string, bool> process
 				}
 				bool didEmuInput = shouldDoEmuInput && isEmuInput;
 
-				if (!didHotkey && !didEmuInput)
-				{
-					processUnboundInput(ie);
-				}
+				processSpecialInput(ie, didHotkey | didEmuInput);
 			} // foreach event
 
 			// also handle axes
@@ -223,7 +221,7 @@ public void ProcessInput(IPhysicalInputSource source, Func<string, bool> process
 		}
 
 		/// <summary>
-		/// Update output controllers. Call <see cref="ProcessInput(IPhysicalInputSource, Func{string, bool}, Config, Action{InputEvent})"/> shortly before this.
+		/// Update output controllers. Call <see cref="ProcessInput(IPhysicalInputSource, Func{string, bool}, Config, Action{InputEvent, bool})"/> shortly before this.
 		/// </summary>
 		public void RunControllerChain(Config config)
 		{

src/BizHawk.Client.EmuHawk/FormBase.cs

@@ -46,6 +46,8 @@ public static void FixBackColorOnControls(Control control)
 		public virtual bool BlocksInputWhenFocused
 			=> true;
 
+		public bool MenuIsOpen { get; private set; }
+
 		public Config? Config { get; set; }
 
 		[DesignerSerializationVisibility(DesignerSerializationVisibility.Hidden)]
@@ -90,6 +92,12 @@ protected override void OnLoad(EventArgs e)
 			}
 			if (OSTailoredCode.IsUnixHost) FixBackColorOnControls(this);
 			UpdateWindowTitle();
+
+			if (MainMenuStrip != null)
+			{
+				MainMenuStrip.MenuActivate += (_, _) => MenuIsOpen = true;
+				MainMenuStrip.MenuDeactivate += (_, _) => MenuIsOpen = false;
+			}
 		}
 
 		public void UpdateWindowTitle()
@@ -98,25 +106,29 @@ public void UpdateWindowTitle()
 				: WindowTitle;
 
 		// Alt key hacks. We need this in order for hotkey bindings with alt to work.
+		private const int WM_SYSCOMMAND = 0x0112;
+		private const int SC_KEYMENU = 0xF100;
+		internal void SendAltCombination(char character)
+		{
+			var m = new Message { WParam = new IntPtr(SC_KEYMENU), LParam = new IntPtr(character), Msg = WM_SYSCOMMAND, HWnd = Handle };
+			if (character == ' ') base.WndProc(ref m);
+			else if (character >= 'a' && character <= 'z') base.ProcessDialogChar(character);
+		}
 
-		/// <summary>sends a simulation of a plain alt key keystroke</summary>
-		internal void SendPlainAltKey(int lparam)
+		internal void FocusToolStipMenu()
 		{
-			var m = new Message { WParam = new IntPtr(0xF100), LParam = new IntPtr(lparam), Msg = 0x0112, HWnd = Handle };
+			var m = new Message { WParam = new IntPtr(SC_KEYMENU), LParam = new IntPtr(0), Msg = WM_SYSCOMMAND, HWnd = Handle };
 			base.WndProc(ref m);
 		}
 
-		/// <summary>HACK to send an alt+mnemonic combination</summary>
-		internal void SendAltKeyChar(char c) => ProcessMnemonic(c);
-
 		protected override void WndProc(ref Message m)
 		{
 			if (!BlocksInputWhenFocused)
 			{
 				// this is necessary to trap plain alt keypresses so that only our hotkey system gets them
-				if (m.Msg == 0x0112) // WM_SYSCOMMAND
+				if (m.Msg == WM_SYSCOMMAND)
 				{
-					if (m.WParam.ToInt32() == 0xF100) // SC_KEYMENU
+					if (m.WParam.ToInt32() == SC_KEYMENU && m.LParam == IntPtr.Zero)
 					{
 						return;
 					}
@@ -128,6 +140,7 @@ protected override void WndProc(ref Message m)
 
 		protected override bool ProcessDialogChar(char charCode)
 		{
+			if (BlocksInputWhenFocused) return base.ProcessDialogChar(charCode);
 			// this is necessary to trap alt+char combinations so that only our hotkey system gets them
 			return (ModifierKeys & Keys.Alt) != 0 || base.ProcessDialogChar(charCode);
 		}

src/BizHawk.Client.EmuHawk/MainForm.cs

@@ -641,7 +641,7 @@ _argParser.SocketAddress is var (socketIP, socketPort)
 							? AllowInput.OnlyController
 							: AllowInput.All
 						: AllowInput.None,
-					FormBase { BlocksInputWhenFocused: false } => AllowInput.All,
+					FormBase { BlocksInputWhenFocused: false, MenuIsOpen: false } => AllowInput.All,
 					ControllerConfig => AllowInput.All,
 					HotkeyConfig => AllowInput.All,
 					LuaWinform { BlocksInputWhenFocused: false } => AllowInput.All,
@@ -899,6 +899,13 @@ private void CheckMayCloseAndCleanup(object/*?*/ closingSender, CancelEventArgs
 
 		public override bool BlocksInputWhenFocused { get; } = false;
 
+		/// <summary>
+		/// Windows does tool stip menu focus things when Alt is released, not pressed.
+		/// However, if an alt combination is pressed then those things happen at that time instead.
+		/// So we need to know if a key combination was used, so we can skip the alt release logic.
+		/// </summary>
+		private bool _skipNextAltRelease = true;
+
 		public int ProgramRunLoop()
 		{
 			// needs to be done late, after the log console snaps on top
@@ -929,26 +936,40 @@ public int ProgramRunLoop()
 				InputManager.ActiveController.PrepareHapticsForHost(finalHostController);
 				Input.Instance.Adapter.SetHaptics(finalHostController.GetHapticsSnapshot());
 
-				InputManager.ProcessInput(Input.Instance, CheckHotkey, Config, (ie) =>
+				InputManager.ProcessInput(Input.Instance, CheckHotkey, Config, (ie, handled) =>
 				{
 					if (ActiveForm is not FormBase afb) return;
 
 					// Alt key for menu items.
 					if (ie.EventType is InputEventType.Press && (ie.LogicalButton.Modifiers & LogicalButton.MASK_ALT) is not 0U)
 					{
+						// Windows will not focus the menu if any other key was pressed while Alt is held. Regardless of whether that key did anything.
+						_skipNextAltRelease = true;
+						if (handled) return;
+
 						if (ie.LogicalButton.Button.Length == 1)
 						{
 							var c = ie.LogicalButton.Button.ToLowerInvariant()[0];
-							if ((c >= 'a' && c <= 'z') || c == ' ')
-							{
-								afb.SendAltKeyChar(c);
-							}
+							afb.SendAltCombination(c);
 						}
 						else if (ie.LogicalButton.Button == "Space")
 						{
-							afb.SendPlainAltKey(32);
+							afb.SendAltCombination(' ');
 						}
 					}
+					else if (handled) return;
+					else if (ie.EventType is InputEventType.Press && ie.LogicalButton.Button == "Alt")
+					{
+						// We will only do the alt release if the alt press itself was not already handled.
+						_skipNextAltRelease = false;
+					}
+					else if (ie.EventType is InputEventType.Release
+						&& !afb.BlocksInputWhenFocused
+						&& ie.LogicalButton.Button == "Alt"
+						&& !_skipNextAltRelease)
+					{
+						afb.FocusToolStipMenu();
+					}
 
 					// same as right-click
 					if (ie.ToString() == "Press:Apps" && Config.ShowContextMenu && ContainsFocus)

src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.Designer.cs

@@ -20,7 +20,7 @@ public partial class TAStudio : ToolFormBase, IToolFormAutoConfig, IControlMainf
 		public static Icon ToolIcon
 			=> Resources.TAStudioIcon;
 
-		public override bool BlocksInputWhenFocused => IsInMenuLoop;
+		public override bool BlocksInputWhenFocused => false;
 
 		public new IMainFormForTools MainForm => base.MainForm;
 
@@ -29,8 +29,6 @@ public static Icon ToolIcon
 		// TODO: UI flow that conveniently allows to start from savestate
 		public ITasMovie CurrentTasMovie => MovieSession.Movie as ITasMovie;
 
-		public bool IsInMenuLoop { get; private set; }
-
 		private readonly List<TasClipboardEntry> _tasClipboard = new List<TasClipboardEntry>();
 		private const string CursorColumnName = "CursorColumn";
 		private const string FrameColumnName = "FrameColumn";
@@ -1093,16 +1091,6 @@ private void TasView_CellDropped(object sender, InputRoll.CellEventArgs e)
 			}
 		}
 
-		private void TASMenu_MenuActivate(object sender, EventArgs e)
-		{
-			IsInMenuLoop = true;
-		}
-
-		private void TASMenu_MenuDeactivate(object sender, EventArgs e)
-		{
-			IsInMenuLoop = false;
-		}
-
 		// Stupid designer
 		protected void DragEnterWrapper(object sender, DragEventArgs e)
 		{

src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs

@@ -48,7 +48,7 @@ public void EmulateFrameAdvance(bool lag = false)
 
 			public void BasicInputProcessing()
 			{
-				manager.ProcessInput(source, ProcessHotkey, config, (_) => { });
+				manager.ProcessInput(source, ProcessHotkey, config, (_, _) => { });
 				manager.RunControllerChain(config);
 			}
 
@@ -448,42 +448,39 @@ public void AutofireHotkeyDoesNotRespondToAlreadyHeldButton()
 		}
 
 		[TestMethod]
-		public void HotkeyIsNotSeenAsUnbound()
+		public void HotkeyIsNotSeenAsUnhandled()
 		{
 			Context context = new(_hotkeys);
 			InputManager manager = context.manager;
 			FakeInputSource source = context.source;
 			manager.ClientControls.BindMulti(_hotkeys[0], "Q");
 
 			source.MakePressEvent("Q");
-			manager.ProcessInput(source, context.ProcessHotkey, context.config, (_) => Assert.Fail("Bound key was seen as unbound."));
+			manager.ProcessInput(source, context.ProcessHotkey, context.config, (_, handled) => Assert.IsTrue(handled, "Bound key was seen as unbound."));
 		}
 
 		[TestMethod]
-		public void InputIsNotSeenAsUnbound()
+		public void InputIsNotSeenAsUnhandled()
 		{
 			Context context = new(_hotkeys);
 			InputManager manager = context.manager;
 			FakeInputSource source = context.source;
 			manager.ActiveController.BindMulti("A", "Q");
 
 			source.MakePressEvent("Q");
-			manager.ProcessInput(source, context.ProcessHotkey, context.config, (_) => Assert.Fail("Bound key was seen as unbound."));
+			manager.ProcessInput(source, context.ProcessHotkey, context.config, (_, handled) => Assert.IsTrue(handled, "Bound key was seen as unbound."));
 		}
 
 		[TestMethod]
-		public void UnboundInputIsSeen()
+		public void UnboundInputIsSeenAsUnhandled()
 		{
 			Context context = new(_hotkeys);
 			InputManager manager = context.manager;
 			FakeInputSource source = context.source;
 
 			source.MakePressEvent("A");
 
-			bool sawUnboundInput = false;
-			manager.ProcessInput(source, context.ProcessHotkey, context.config, (_) => sawUnboundInput = true);
-
-			Assert.IsTrue(sawUnboundInput);
+			manager.ProcessInput(source, context.ProcessHotkey, context.config, (_, handled) => Assert.IsFalse(handled, "Unbound key was seen as handled."));
 		}
 
 		[TestMethod]