Fix Tab Trap in SettingsHotkey Custom Control (#7136)

* basic logic working

* Added a literal for ignore flag which cna be shared by all the files

* Added a condition that the other modifier keys should not be pressed

* Added comments to describe each scenario

* sometimes when multiple modified keys were involved the shift+tab key press was also being invoked, so added an additional check in the IsValid function

* use variable for vk_tab

* remove new line before initializing dwextraInfo

* move flag check if the filterKeyboardevent function

* use windows.system.virtualkey.shift instead of defining a constant for the shift key code

* removed latest settings to use internal settings instead. Removed the validity check while still within the hotkey other than if it's tab or shift+tab

* add a function to send input to the system instead of duplicating the send input code

* remove VKSHIFT declaration

* display all shortcuts/keys except tab and shift+tab

* remove header that is no longer needed

src/common/interop/KeyboardHook.cpp

@@ -60,7 +60,10 @@ LRESULT __clrcall KeyboardHook::HookProc(int nCode, WPARAM wParam, LPARAM lParam
         KeyboardEvent ^ ev = gcnew KeyboardEvent();
         ev->message = wParam;
         ev->key = reinterpret_cast<KBDLLHOOKSTRUCT*>(lParam)->vkCode;
-        if (filterKeyboardEvent != nullptr && !filterKeyboardEvent->Invoke(ev))
+        ev->dwExtraInfo = reinterpret_cast<KBDLLHOOKSTRUCT*>(lParam)->dwExtraInfo;
+
+        // Ignore the keyboard hook if the FilterkeyboardEvent returns false.
+        if ((filterKeyboardEvent != nullptr && !filterKeyboardEvent->Invoke(ev)))
         {
             return CallNextHookEx(hookHandle, nCode, wParam, lParam);
         }

src/common/interop/KeyboardHook.h

@@ -10,6 +10,7 @@ public
     {
         WPARAM message;
         int key;
+        DWORD dwExtraInfo;
     };
 
 public

src/core/Microsoft.PowerToys.Settings.UI.Lib/HotkeySettings.cs

@@ -10,6 +10,8 @@ namespace Microsoft.PowerToys.Settings.UI.Lib
 {
     public class HotkeySettings
     {
+        private const int VKTAB = 0x09;
+
         public HotkeySettings()
         {
             Win = false;
@@ -100,6 +102,11 @@ namespace Microsoft.PowerToys.Settings.UI.Lib
 
         public bool IsValid()
         {
+            if (IsAccessibleShortcut())
+            {
+                return false;
+            }
+
             return (Alt || Ctrl || Win || Shift) && Code != 0;
         }
 
@@ -107,5 +114,17 @@ namespace Microsoft.PowerToys.Settings.UI.Lib
         {
             return !Alt && !Ctrl && !Win && !Shift && Code == 0;
         }
+
+        public bool IsAccessibleShortcut()
+        {
+            // Shift+Tab and Tab are accessible shortcuts
+            if ((!Alt && !Ctrl && !Win && Shift && Code == VKTAB)
+                || (!Alt && !Ctrl && !Win && !Shift && Code == VKTAB))
+            {
+                return true;
+            }
+
+            return false;
+        }
     }
 }

src/core/Microsoft.PowerToys.Settings.UI.Lib/HotkeySettingsControlHook.cs

@@ -2,6 +2,7 @@
 // The Microsoft Corporation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using System;
 using interop;
 
 namespace Microsoft.PowerToys.Settings.UI.Lib
@@ -10,6 +11,8 @@ namespace Microsoft.PowerToys.Settings.UI.Lib
 
     public delegate bool IsActive();
 
+    public delegate bool FilterAccessibleKeyboardEvents(int key, UIntPtr extraInfo);
+
     public class HotkeySettingsControlHook
     {
         private const int WmKeyDown = 0x100;
@@ -22,12 +25,15 @@ namespace Microsoft.PowerToys.Settings.UI.Lib
         private KeyEvent _keyUp;
         private IsActive _isActive;
 
-        public HotkeySettingsControlHook(KeyEvent keyDown, KeyEvent keyUp, IsActive isActive)
+        private FilterAccessibleKeyboardEvents _filterKeyboardEvent;
+
+        public HotkeySettingsControlHook(KeyEvent keyDown, KeyEvent keyUp, IsActive isActive, FilterAccessibleKeyboardEvents filterAccessibleKeyboardEvents)
         {
             _keyDown = keyDown;
             _keyUp = keyUp;
             _isActive = isActive;
-            _hook = new KeyboardHook(HotkeySettingsHookCallback, IsActive, null);
+            _filterKeyboardEvent = filterAccessibleKeyboardEvents;
+            _hook = new KeyboardHook(HotkeySettingsHookCallback, IsActive, FilterKeyboardEvents);
             _hook.Start();
         }
 
@@ -51,6 +57,11 @@ namespace Microsoft.PowerToys.Settings.UI.Lib
             }
         }
 
+        private bool FilterKeyboardEvents(KeyboardEvent ev)
+        {
+            return _filterKeyboardEvent(ev.key, (UIntPtr)ev.dwExtraInfo);
+        }
+
         public void Dispose()
         {
             // Dispose the KeyboardHook object to terminate the hook threads

src/core/Microsoft.PowerToys.Settings.UI/Controls/HotkeySettingsControl.xaml.cs

@@ -3,6 +3,7 @@
 // See the LICENSE file in the project root for more information.
 
 using System;
+using Microsoft.PowerToys.Settings.UI.Helpers;
 using Microsoft.PowerToys.Settings.UI.Lib;
 using Windows.UI.Core;
 using Windows.UI.Xaml;
@@ -12,6 +13,12 @@ namespace Microsoft.PowerToys.Settings.UI.Controls
 {
     public sealed partial class HotkeySettingsControl : UserControl
     {
+        private readonly UIntPtr ignoreKeyEventFlag = (UIntPtr)0x5555;
+
+        private bool _shiftKeyDownOnEntering = false;
+
+        private bool _shiftToggled = false;
+
         public string Header { get; set; }
 
         public string Keys { get; set; }
@@ -93,7 +100,7 @@ namespace Microsoft.PowerToys.Settings.UI.Controls
             HotkeyTextBox.GettingFocus += HotkeyTextBox_GettingFocus;
             HotkeyTextBox.LosingFocus += HotkeyTextBox_LosingFocus;
             HotkeyTextBox.Unloaded += HotkeyTextBox_Unloaded;
-            hook = new HotkeySettingsControlHook(Hotkey_KeyDown, Hotkey_KeyUp, Hotkey_IsActive);
+            hook = new HotkeySettingsControlHook(Hotkey_KeyDown, Hotkey_KeyUp, Hotkey_IsActive, FilterAccessibleKeyboardEvents);
         }
 
         private void HotkeyTextBox_Unloaded(object sender, RoutedEventArgs e)
@@ -123,6 +130,7 @@ namespace Microsoft.PowerToys.Settings.UI.Controls
                 case Windows.System.VirtualKey.Shift:
                 case Windows.System.VirtualKey.LeftShift:
                 case Windows.System.VirtualKey.RightShift:
+                    _shiftToggled = true;
                     internalSettings.Shift = matchValue;
                     break;
                 case Windows.System.VirtualKey.Escape:
@@ -135,15 +143,99 @@ namespace Microsoft.PowerToys.Settings.UI.Controls
             }
         }
 
+        // Function to send a single key event to the system which would be ignored by the hotkey control.
+        private void SendSingleKeyboardInput(short keyCode, uint keyStatus)
+        {
+            NativeKeyboardHelper.INPUT inputShift = new NativeKeyboardHelper.INPUT
+            {
+                type = NativeKeyboardHelper.INPUTTYPE.INPUT_KEYBOARD,
+                data = new NativeKeyboardHelper.InputUnion
+                {
+                    ki = new NativeKeyboardHelper.KEYBDINPUT
+                    {
+                        wVk = keyCode,
+                        dwFlags = keyStatus,
+
+                        // Any keyevent with the extraInfo set to this value will be ignored by the keyboard hook and sent to the system instead.
+                        dwExtraInfo = ignoreKeyEventFlag,
+                    },
+                },
+            };
+
+            NativeKeyboardHelper.INPUT[] inputs = new NativeKeyboardHelper.INPUT[] { inputShift };
+
+            _ = NativeMethods.SendInput(1, inputs, NativeKeyboardHelper.INPUT.Size);
+        }
+
+        private bool FilterAccessibleKeyboardEvents(int key, UIntPtr extraInfo)
+        {
+            // A keyboard event sent with this value in the extra Information field should be ignored by the hook so that it can be captured by the system instead.
+            if (extraInfo == ignoreKeyEventFlag)
+            {
+                return false;
+            }
+
+            // If the current key press is tab, based on the other keys ignore the key press so as to shift focus out of the hotkey control.
+            if ((Windows.System.VirtualKey)key == Windows.System.VirtualKey.Tab)
+            {
+                // Shift was not pressed while entering and Shift is not pressed while leaving the hotkey control, treat it as a normal tab key press.
+                if (!internalSettings.Shift && !_shiftKeyDownOnEntering && !internalSettings.Win && !internalSettings.Alt && !internalSettings.Ctrl)
+                {
+                    return false;
+                }
+
+                // Shift was not pressed while entering but it was pressed while leaving the hotkey, therefore simulate a shift key press as the system does not know about shift being pressed in the hotkey.
+                else if (internalSettings.Shift && !_shiftKeyDownOnEntering && !internalSettings.Win && !internalSettings.Alt && !internalSettings.Ctrl)
+                {
+                    // This is to reset the shift key press within the control as it was not used within the control but rather was used to leave the hotkey.
+                    internalSettings.Shift = false;
+
+                    SendSingleKeyboardInput((short)Windows.System.VirtualKey.Shift, (uint)NativeKeyboardHelper.KeyEventF.KeyDown);
+
+                    return false;
+                }
+
+                // Shift was pressed on entering and remained pressed, therefore only ignore the tab key so that it can be passed to the system.
+                // As the shift key is already assumed to be pressed by the system while it entered the hotkey control, shift would still remain pressed, hence ignoring the tab input would simulate a Shift+Tab key press.
+                else if (!internalSettings.Shift && _shiftKeyDownOnEntering && !_shiftToggled && !internalSettings.Win && !internalSettings.Alt && !internalSettings.Ctrl)
+                {
+                    return false;
+                }
+
+                // Shift was pressed on entering but it was released and later pressed again.
+                // Ignore the tab key and the system already has the shift key pressed, therefore this would simulate Shift+Tab.
+                // However, since the last shift key was only used to move out of the control, reset the status of shift within the control.
+                else if (internalSettings.Shift && _shiftKeyDownOnEntering && _shiftToggled && !internalSettings.Win && !internalSettings.Alt && !internalSettings.Ctrl)
+                {
+                    internalSettings.Shift = false;
+
+                    return false;
+                }
+
+                // Shift was pressed on entering and was later released.
+                // The system still has shift in the key pressed status, therefore pass a Shift KeyUp message to the system, to release the shift key, therefore simulating only the Tab key press.
+                else if (!internalSettings.Shift && _shiftKeyDownOnEntering && _shiftToggled && !internalSettings.Win && !internalSettings.Alt && !internalSettings.Ctrl)
+                {
+                    SendSingleKeyboardInput((short)Windows.System.VirtualKey.Shift, (uint)NativeKeyboardHelper.KeyEventF.KeyUp);
+
+                    return false;
+                }
+            }
+
+            return true;
+        }
+
         private async void Hotkey_KeyDown(int key)
         {
             await Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
             {
                 KeyEventHandler(key, true, key, Lib.Utilities.Helper.GetKeyName((uint)key));
-                if (internalSettings.Code > 0)
+
+                // Tab and Shift+Tab are accessible keys and should not be displayed in the hotkey control.
+                if (internalSettings.Code > 0 && !internalSettings.IsAccessibleShortcut())
                 {
+                    HotkeyTextBox.Text = internalSettings.ToString();
                     lastValidSettings = internalSettings.Clone();
-                    HotkeyTextBox.Text = lastValidSettings.ToString();
                 }
             });
         }
@@ -163,6 +255,16 @@ namespace Microsoft.PowerToys.Settings.UI.Controls
 
         private void HotkeyTextBox_GettingFocus(object sender, RoutedEventArgs e)
         {
+            // Reset the status on entering the hotkey each time.
+            _shiftKeyDownOnEntering = false;
+            _shiftToggled = false;
+
+            // To keep track of the shift key, whether it was pressed on entering.
+            if ((NativeMethods.GetAsyncKeyState((int)Windows.System.VirtualKey.Shift) & 0x8000) != 0)
+            {
+                _shiftKeyDownOnEntering = true;
+            }
+
             _isActive = true;
         }
 

src/core/Microsoft.PowerToys.Settings.UI/Helpers/NativeKeyboardHelper.cs

@@ -0,0 +1,86 @@
+// Copyright (c) Microsoft Corporation
+// The Microsoft Corporation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Runtime.InteropServices;
+
+namespace Microsoft.PowerToys.Settings.UI.Helpers
+{
+    internal static class NativeKeyboardHelper
+    {
+        [StructLayout(LayoutKind.Sequential)]
+        [System.Diagnostics.CodeAnalysis.SuppressMessage("StyleCop.CSharp.NamingRules", "SA1307:Accessible fields should begin with upper-case letter", Justification = "Matching Native Structure")]
+        internal struct INPUT
+        {
+            internal INPUTTYPE type;
+            internal InputUnion data;
+
+            internal static int Size
+            {
+                get { return Marshal.SizeOf(typeof(INPUT)); }
+            }
+        }
+
+        [StructLayout(LayoutKind.Explicit)]
+        [System.Diagnostics.CodeAnalysis.SuppressMessage("StyleCop.CSharp.NamingRules", "SA1307:Accessible fields should begin with upper-case letter", Justification = "Matching Native Structure")]
+        internal struct InputUnion
+        {
+            [FieldOffset(0)]
+            internal MOUSEINPUT mi;
+            [FieldOffset(0)]
+            internal KEYBDINPUT ki;
+            [FieldOffset(0)]
+            internal HARDWAREINPUT hi;
+        }
+
+        [StructLayout(LayoutKind.Sequential)]
+        [System.Diagnostics.CodeAnalysis.SuppressMessage("StyleCop.CSharp.NamingRules", "SA1307:Accessible fields should begin with upper-case letter", Justification = "Matching Native Structure")]
+        internal struct MOUSEINPUT
+        {
+            internal int dx;
+            internal int dy;
+            internal int mouseData;
+            internal uint dwFlags;
+            internal uint time;
+            internal UIntPtr dwExtraInfo;
+        }
+
+        [StructLayout(LayoutKind.Sequential)]
+        [System.Diagnostics.CodeAnalysis.SuppressMessage("StyleCop.CSharp.NamingRules", "SA1307:Accessible fields should begin with upper-case letter", Justification = "Matching Native Structure")]
+        internal struct KEYBDINPUT
+        {
+            internal short wVk;
+            internal short wScan;
+            internal uint dwFlags;
+            internal int time;
+            internal UIntPtr dwExtraInfo;
+        }
+
+        [StructLayout(LayoutKind.Sequential)]
+        [System.Diagnostics.CodeAnalysis.SuppressMessage("StyleCop.CSharp.NamingRules", "SA1307:Accessible fields should begin with upper-case letter", Justification = "Matching Native Structure")]
+        internal struct HARDWAREINPUT
+        {
+            internal int uMsg;
+            internal short wParamL;
+            internal short wParamH;
+        }
+
+        internal enum INPUTTYPE : uint
+        {
+            INPUT_MOUSE = 0,
+            INPUT_KEYBOARD = 1,
+            INPUT_HARDWARE = 2,
+        }
+
+        [Flags]
+        internal enum KeyEventF
+        {
+            KeyDown = 0x0000,
+            ExtendedKey = 0x0001,
+            KeyUp = 0x0002,
+            Unicode = 0x0004,
+            Scancode = 0x0008,
+        }
+    }
+}

src/core/Microsoft.PowerToys.Settings.UI/Helpers/NativeMethods.cs

@@ -0,0 +1,17 @@
+// Copyright (c) Microsoft Corporation
+// The Microsoft Corporation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System.Runtime.InteropServices;
+
+namespace Microsoft.PowerToys.Settings.UI.Helpers
+{
+    internal static class NativeMethods
+    {
+        [DllImport("user32.dll")]
+        internal static extern uint SendInput(uint nInputs, NativeKeyboardHelper.INPUT[] pInputs, int cbSize);
+
+        [DllImport("user32.dll", CharSet = CharSet.Auto, CallingConvention = CallingConvention.StdCall, SetLastError = true)]
+        internal static extern short GetAsyncKeyState(int vKey);
+    }
+}

src/core/Microsoft.PowerToys.Settings.UI/Microsoft.PowerToys.Settings.UI.csproj

@@ -102,6 +102,8 @@
     <Compile Include="Generated Files\AssemblyInfo.cs">
       <SubType>Code</SubType>
     </Compile>
+    <Compile Include="Helpers\NativeKeyboardHelper.cs" />
+    <Compile Include="Helpers\NativeMethods.cs" />
     <Compile Include="Helpers\NavHelper.cs" />
     <Compile Include="Helpers\Observable.cs" />
     <Compile Include="Helpers\RelayCommand.cs" />