Fixed microsoft#521 - AltGr combinations not working (microsoft#1436)

This commit changes how TerminalControl/TerminalCore handle key events to give it better knowledge about modifier states at the lower levels.

src/cascadia/TerminalControl/TermControl.cpp

@@ -634,15 +634,18 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
             return;
         }
 
-        auto modifiers = _GetPressedModifierKeys();
-
+        const auto modifiers = _GetPressedModifierKeys();
         const auto vkey = static_cast<WORD>(e.OriginalKey());
 
         bool handled = false;
         auto bindings = _settings.KeyBindings();
         if (bindings)
         {
-            KeyChord chord(modifiers, vkey);
+            KeyChord chord(
+                WI_IsAnyFlagSet(modifiers, CTRL_PRESSED),
+                WI_IsAnyFlagSet(modifiers, ALT_PRESSED),
+                WI_IsFlagSet(modifiers, SHIFT_PRESSED),
+                vkey);
             handled = bindings.TryKeyChord(chord);
         }
 
@@ -652,10 +655,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
             // If the terminal translated the key, mark the event as handled.
             // This will prevent the system from trying to get the character out
             // of it and sending us a CharacterRecieved event.
-            handled = _terminal->SendKeyEvent(vkey,
-                                              WI_IsFlagSet(modifiers, KeyModifiers::Ctrl),
-                                              WI_IsFlagSet(modifiers, KeyModifiers::Alt),
-                                              WI_IsFlagSet(modifiers, KeyModifiers::Shift));
+            handled = _terminal->SendKeyEvent(vkey, modifiers);
 
             if (_cursorTimer.has_value())
             {
@@ -1556,8 +1556,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
     //   find out which modifiers (ctrl, alt, shift) are pressed in events that
     //   don't necessarily include that state.
     // Return Value:
-    // - a KeyModifiers value with flags set for each key that's pressed.
-    Settings::KeyModifiers TermControl::_GetPressedModifierKeys() const
+    // - The combined ControlKeyState flags as a bitfield.
+    DWORD TermControl::_GetPressedModifierKeys() const
     {
         CoreWindow window = CoreWindow::GetForCurrentThread();
         // DONT USE
@@ -1567,17 +1567,31 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         // Sometimes with the key down, the state is Down | Locked.
         // Sometimes with the key up, the state is Locked.
         // IsFlagSet(Down) is the only correct solution.
-        const auto ctrlKeyState = window.GetKeyState(VirtualKey::Control);
-        const auto shiftKeyState = window.GetKeyState(VirtualKey::Shift);
-        const auto altKeyState = window.GetKeyState(VirtualKey::Menu);
 
-        const auto ctrl = WI_IsFlagSet(ctrlKeyState, CoreVirtualKeyStates::Down);
-        const auto shift = WI_IsFlagSet(shiftKeyState, CoreVirtualKeyStates::Down);
-        const auto alt = WI_IsFlagSet(altKeyState, CoreVirtualKeyStates::Down);
+        struct KeyModifier
+        {
+            VirtualKey vkey;
+            DWORD flag;
+        };
+
+        constexpr std::array<KeyModifier, 5> modifiers{ {
+            { VirtualKey::RightMenu, RIGHT_ALT_PRESSED },
+            { VirtualKey::LeftMenu, LEFT_ALT_PRESSED },
+            { VirtualKey::RightControl, RIGHT_CTRL_PRESSED },
+            { VirtualKey::LeftControl, LEFT_CTRL_PRESSED },
+            { VirtualKey::Shift, SHIFT_PRESSED },
+        } };
+
+        DWORD flags = 0;
+
+        for (const auto& mod : modifiers)
+        {
+            const auto state = window.GetKeyState(mod.vkey);
+            const auto isDown = WI_IsFlagSet(state, CoreVirtualKeyStates::Down);
+            flags |= isDown ? mod.flag : 0;
+        }
 
-        return KeyModifiers{ (ctrl ? Settings::KeyModifiers::Ctrl : Settings::KeyModifiers::None) |
-                             (alt ? Settings::KeyModifiers::Alt : Settings::KeyModifiers::None) |
-                             (shift ? Settings::KeyModifiers::Shift : Settings::KeyModifiers::None) };
+        return flags;
     }
 
     // Method Description:

src/cascadia/TerminalControl/TermControl.h

@@ -152,7 +152,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         void _ScrollbarUpdater(Windows::UI::Xaml::Controls::Primitives::ScrollBar scrollbar, const int viewTop, const int viewHeight, const int bufferSize);
         static Windows::UI::Xaml::Thickness _ParseThicknessFromPadding(const hstring padding);
 
-        Settings::KeyModifiers _GetPressedModifierKeys() const;
+        DWORD _GetPressedModifierKeys() const;
 
         const COORD _GetTerminalPosition(winrt::Windows::Foundation::Point cursorPosition);
 

src/cascadia/TerminalCore/ITerminalInput.hpp

@@ -9,7 +9,7 @@ namespace Microsoft::Terminal::Core
     public:
         virtual ~ITerminalInput() {}
 
-        virtual bool SendKeyEvent(const WORD vkey, const bool ctrlPressed, const bool altPressed, const bool shiftPressed) = 0;
+        virtual bool SendKeyEvent(const WORD vkey, const DWORD modifiers) = 0;
 
         // void SendMouseEvent(uint row, uint col, KeyModifiers modifiers);
         [[nodiscard]] virtual HRESULT UserResize(const COORD size) noexcept = 0;

src/cascadia/TerminalCore/Terminal.cpp

@@ -195,16 +195,11 @@ void Terminal::Write(std::wstring_view stringView)
 //   real character out of the event.
 // Arguments:
 // - vkey: The vkey of the key pressed.
-// - ctrlPressed: true iff either ctrl key is pressed.
-// - altPressed: true iff either alt key is pressed.
-// - shiftPressed: true iff either shift key is pressed.
+// - modifiers: The current ControlKeyState flags.
 // Return Value:
 // - true if we translated the key event, and it should not be processed any further.
 // - false if we did not translate the key, and it should be processed into a character.
-bool Terminal::SendKeyEvent(const WORD vkey,
-                            const bool ctrlPressed,
-                            const bool altPressed,
-                            const bool shiftPressed)
+bool Terminal::SendKeyEvent(const WORD vkey, const DWORD modifiers)
 {
     if (_snapOnInput && _scrollOffset != 0)
     {
@@ -213,10 +208,23 @@ bool Terminal::SendKeyEvent(const WORD vkey,
         _NotifyScrollEvent();
     }
 
-    DWORD modifiers = 0 |
-                      (ctrlPressed ? LEFT_CTRL_PRESSED : 0) |
-                      (altPressed ? LEFT_ALT_PRESSED : 0) |
-                      (shiftPressed ? SHIFT_PRESSED : 0);
+    KeyEvent keyEv{ true, 0, vkey, 0, UNICODE_NULL, modifiers };
+
+    // AltGr key combinations don't always contain any meaningful,
+    // pretranslated unicode character during WM_KEYDOWN.
+    // E.g. on a German keyboard AltGr+Q should result in a "@" character,
+    // but actually results in "Q" with Alt and Ctrl modifier states.
+    // By returning false though, we can abort handling this WM_KEYDOWN
+    // event and let the WM_CHAR handler kick in, which will be
+    // provided with an appropriate unicode character.
+    if (keyEv.IsAltGrPressed())
+    {
+        return false;
+    }
+
+    const auto ctrlPressed = keyEv.IsCtrlPressed();
+    const auto altPressed = keyEv.IsAltPressed();
+    const auto shiftPressed = keyEv.IsShiftPressed();
 
     // Alt key sequences _require_ the char to be in the keyevent. If alt is
     // pressed, manually get the character that's being typed, and put it in the
@@ -250,9 +258,9 @@ bool Terminal::SendKeyEvent(const WORD vkey,
         ch = UNICODE_SPACE;
     }
 
-    const bool manuallyHandled = ch != UNICODE_NULL;
+    keyEv.SetCharData(ch);
 
-    KeyEvent keyEv{ true, 0, vkey, 0, ch, modifiers };
+    const bool manuallyHandled = ch != UNICODE_NULL;
     const bool translated = _terminalInput->HandleKey(&keyEv);
 
     return translated && manuallyHandled;

src/cascadia/TerminalCore/Terminal.hpp

@@ -76,10 +76,7 @@ class Microsoft::Terminal::Core::Terminal final :
 
 #pragma region ITerminalInput
     // These methods are defined in Terminal.cpp
-    bool SendKeyEvent(const WORD vkey,
-                      const bool ctrlPressed,
-                      const bool altPressed,
-                      const bool shiftPressed) override;
+    bool SendKeyEvent(const WORD vkey, const DWORD modifiers) override;
     [[nodiscard]] HRESULT UserResize(const COORD viewportSize) noexcept override;
     void UserScrollViewport(const int viewTop) override;
     int GetScrollOffset() override;

src/cascadia/UnitTests_TerminalCore/InputTest.cpp

@@ -48,19 +48,19 @@ namespace TerminalCoreUnitTests
         // Verify that Alt+a generates a lowercase a on the input
         expectedinput = L"\x1b"
                         "a";
-        VERIFY_IS_TRUE(term.SendKeyEvent(L'A', false, true, false));
+        VERIFY_IS_TRUE(term.SendKeyEvent(L'A', LEFT_ALT_PRESSED));
 
         // Verify that Alt+shift+a generates a uppercase a on the input
         expectedinput = L"\x1b"
                         "A";
-        VERIFY_IS_TRUE(term.SendKeyEvent(L'A', false, true, true));
+        VERIFY_IS_TRUE(term.SendKeyEvent(L'A', LEFT_ALT_PRESSED | SHIFT_PRESSED));
     }
 
     void InputTest::AltSpace()
     {
         // Make sure we don't handle Alt+Space. The system will use this to
         // bring up the system menu for restore, min/maximimize, size, move,
         // close
-        VERIFY_IS_FALSE(term.SendKeyEvent(L' ', false, true, false));
+        VERIFY_IS_FALSE(term.SendKeyEvent(L' ', LEFT_ALT_PRESSED));
     }
 }

src/terminal/adapter/ut_adapter/inputTest.cpp

@@ -339,6 +339,7 @@ void InputTest::TerminalInputModifierKeyTests()
     unsigned int uiKeystate = uiActualKeystate;
 
     const TerminalInput* const pInput = new TerminalInput(s_TerminalInputTestCallback);
+    const BYTE slashVkey = LOBYTE(VkKeyScanW(L'/'));
 
     Log::Comment(L"Sending every possible VKEY at the input stream for interception during key DOWN.");
     for (BYTE vkey = 0; vkey < BYTE_MAX; vkey++)
@@ -356,6 +357,12 @@ void InputTest::TerminalInputModifierKeyTests()
         irTest.Event.KeyEvent.wVirtualKeyCode = vkey;
         irTest.Event.KeyEvent.bKeyDown = TRUE;
 
+        // Ctrl-/ is handled in another test, because it's weird.
+        if (ControlPressed(uiKeystate) && (vkey == VK_DIVIDE || vkey == slashVkey))
+        {
+            continue;
+        }
+
         // Set up expected result
         switch (vkey)
         {
@@ -373,9 +380,7 @@ void InputTest::TerminalInputModifierKeyTests()
             continue;
         case VK_BACK:
             // Backspace is kinda different from other keys - we'll handle in another test.
-        case VK_DIVIDE:
         case VK_OEM_2:
-            // Ctrl-/ is also handled in another test, because it's weird.
             // VK_OEM_2 is typically the '/?' key
             continue;
             // wcscpy_s(s_pwsInputBuffer, L"\x7f");