Repository: microsoft/terminal (language: cpp)

Issue:
Alt + numpad combination does not work anymore
### Windows Terminal version

1.21.1272.0

### Windows build number

10.0.22631.0

### Other Software

wsl terminal, powershell, etc.

### Steps to reproduce

Sequential numpad keys combinations with the ALT key pressed to insert special characters ( for example the most common ones such as `{` with ALT+123, `~` with ALT+126, or the tilde ~ with ALT+126, etc. ) do not work anymore.

The latest working version is Microsoft.WindowsTerminal.Preview 1.20.11215.0, the next current one (1.21.1272.0) broke this functionality.




### Expected Behavior

On previous versions the combination works

raksati@wsl:$ showkey -a

Press any keys - Ctrl-D will terminate this program

`        96 0140 0x60

^D        4 0004 0x04
raksati@wsl:~$

### Actual Behavior

nothing is detected:

raksati@wsl:$ showkey -a

Press any keys - Ctrl-D will terminate this program

^D        4 0004 0x04
raksati@wsl:~$





Discussion / hints:
Thanks for the report!

I suspect this might be related to PR #17067. There are a couple of places in the code that mention Alt-Numpad being handled in `TSFInputControl`. For example see here:

https://github.com/microsoft/terminal/blob/5d1cf1a704986808d09ae7729aaed39ce5d5f909/src/cascadia/TerminalApp/TerminalPage.cpp#L1439-L1442

But `TSFInputControl` has now been replaced, so perhaps that functionality was lost in the rewrite.

/cc @lhecker 
It works in conhost because that one doesn't rely on the WinRT TSF stack via TextInputHost, and so it'll receive the Alt+Numpad key regularly via `WM_CHAR`. The weirdest thing is that even if I remove the new TSF implementation entirely (doesn't get instantiated) it'll not fix the issue. Does that mean the only way to get Alt+Numpad keys in a WinUI app is by using `CoreTextServicesManager`? That'd be quite surprising and kind of sad, because we fundamentally can't use that class.
Wow. Just after I wrote that I found that switching my keyboard layout to Korean or any other "complex" language fixes the issue. What in the heck!
So, with all that said, I think it'd be _way_ easier to just write our own Alt+Numpad support in `TermControl` from scratch. ConEmu got all the info we need: https://conemu.github.io/en/AltNumpad.html

If anyone wants to give it a shot, please do so!

---

Windows internals for the curious follow below:

I spent an unfathomable amount of time debugging this. 😄
This happens basically because the UWP implementation of TSF does not like its own non-UWP sibling very much and switches off when it sees it.

I tried solving this issue by adding this to `TermControl`:
```cpp
// The remaining callbacks must be implemented because SendInput should only be called if there's no active composition.
const auto manager = Windows::UI::Text::Core::CoreTextServicesManager::GetForCurrentView();
_editContext = manager.CreateEditContext();
_editContext.TextUpdating([this](auto&&, auto&& args) { SendInput(args.Text()); });
_editContext.TextRequested([](auto&&, auto&&) {});
_editContext.SelectionRequested([](auto&&, auto&&) {});
```

(And `NotifyFocusEnter()` / `NotifyFocusLeave()` calls must be added of course.)
The problem is that for some mysterious reason these callbacks won't get called if our new TSF implementation got focus simultaneously (which it needs because otherwise we have no IME).

We call `ITfDocumentMgr::CreateContext` here:
https://github.com/microsoft/terminal/blob/13de7c668577ae270ce9528d6a895fd169ece7cc/src/tsf/Implementation.cpp#L42

It's implemented by `CDocumentInputManager::CreateContext` which calls `QueryInterface` on our `ITfContextOwnerCompositionSink` to check if it implements `ITextInputClientOwner` (`86b4403f-c187-4a8b-b13e-c93c4ad1078c`) and passes that as a `bool` to the `CInputContext` instance that's returned as the "context".

(The TSF context is mostly an adapter between the public msctf.h interfaces and the private implementation. We need our own context because we need `TS_SS_TRANSITORY`. See `Implementation.cpp` for lots of details about this.)

`CInputContext::GetStatus` then adapts our `Implementation::GetStatus` somewhat like this:
```cpp
HRESULT CInputContext::GetStatus(TS_STATUS* status)
{
    YOUR_IMPLEMENTATION->GetStatus(status);
    if (YOU_IMPLEMENTED_THE_ITextInputClientOwner_INTERFACE?)
        status->dwStaticFlags |= TS_SS_UWPCONTROL;
    else
        status->dwStaticFlags &= ~TS_SS_UWPCONTROL;
}
```

The existence of this `TS_SS_UWPCONTROL` is what makes and breaks not just the Alt+Numpad support, but seemingly any support for any input whatsoever. If that flag is missing (= no TSF3), then somewhere something between `TextInputHost` and `CoreTextEditContext` breaks down. I couldn't quite figure out where and what exactly.

I think that's a bug in the TSF implementation. The lack of Alt+Numpad support is likely a fallout from this, because when that flag is missing and `TextInputHost` is not sending any input anymore, then it should also re-enable sending Alt+Numpad via `WM_CHAR`, just like how it works for regular Win32 applications.

In any case, if I pass any `IUnknown` instance as the `ITextInputClientOwner` in **our** `Implementation`, then that will fix the issue with the context that WinUI holds, because `TextInputHost` will then send messages again.

The only problem with all that is that it doesn't solve the focus problem: To make this work we need to give both WinUI's context (for Alt+Numpad) and our context (= our IME) focus at the same, which means that both will receive the same input at the same time. That's really really bad, because TSF3 doesn't use `TS_SS_TRANSITORY` and I don't want to deal with the terminal-specific issues that this results in.
If this is _not_ a joke, can i quote your rationale? 

```
   ╔═════╗
   ║ Duh ║
o.O╜═════╝
```
So, whats the best way to achieve something more complex, say √² or https://brltty.app/details.html 

...




 
Rationale for what? And what exactly did you feel like was a joke?

The way you're phrasing it, it sounds like you believe I have no intention of seeing this fixed, but literally the very first paragraph of what I wrote explains how we should do it. 😅
Right. Hast je ein terminal verwendet in deinem Leben der weniger mühsam
ist als er?

On Mon, Jul 8, 2024, 19:38 Leonard Hecker ***@***.***> wrote:

> Rationale for what? And what exactly did you feel like was a joke?
>
> The way you're phrasing it, it sounds like you believe I have no intention
> of seeing this fixed, but literally the very first paragraph of what I
> wrote explains how we should do it. 😅
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/microsoft/terminal/issues/17327#issuecomment-2214796236>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAL7SAW5DGMZ3H2N643JBXLZLLFBBAVCNFSM6AAAAABIMKLFJKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJUG44TMMRTGY>
> .
> You are receiving this because you commented.Message ID:
> ***@***.***>
>


Fix patch:
diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp
index be9de020b54..0e571132752 100644
--- a/src/cascadia/TerminalControl/TermControl.cpp
+++ b/src/cascadia/TerminalControl/TermControl.cpp
@@ -1454,61 +1454,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     // - Whether the key was handled.
     bool TermControl::OnDirectKeyEvent(const uint32_t vkey, const uint8_t scanCode, const bool down)
     {
-        // Short-circuit isReadOnly check to avoid warning dialog
-        if (_core.IsInReadOnlyMode())
-        {
-            return false;
-        }
-
         const auto modifiers{ _GetPressedModifierKeys() };
-        auto handled = false;
-
-        if (vkey == VK_MENU && !down)
-        {
-            // Manually generate an Alt KeyUp event into the key bindings or terminal.
-            //   This is required as part of GH#6421.
-            (void)_TrySendKeyEvent(VK_MENU, scanCode, modifiers, false);
-            handled = true;
-        }
-        else if ((vkey == VK_F7 || vkey == VK_SPACE) && down)
-        {
-            // Manually generate an F7 event into the key bindings or terminal.
-            //   This is required as part of GH#638.
-            // Or do so for alt+space; only send to terminal when explicitly unbound
-            //  That is part of #GH7125
-            auto bindings{ _core.Settings().KeyBindings() };
-            auto isUnbound = false;
-            const KeyChord kc = {
-                modifiers.IsCtrlPressed(),
-                modifiers.IsAltPressed(),
-                modifiers.IsShiftPressed(),
-                modifiers.IsWinPressed(),
-                gsl::narrow_cast<WORD>(vkey),
-                0
-            };
-
-            if (bindings)
-            {
-                handled = bindings.TryKeyChord(kc);
-
-                if (!handled)
-                {
-                    isUnbound = bindings.IsKeyChordExplicitlyUnbound(kc);
-                }
-            }
-
-            const auto sendToTerminal = vkey == VK_F7 || (vkey == VK_SPACE && isUnbound);
-
-            if (!handled && sendToTerminal)
-            {
-                // _TrySendKeyEvent pretends it didn't handle F7 for some unknown reason.
-                (void)_TrySendKeyEvent(gsl::narrow_cast<WORD>(vkey), scanCode, modifiers, true);
-                // GH#6438: Note that we're _not_ sending the key up here - that'll
-                // get passed through XAML to our KeyUp handler normally.
-                handled = true;
-            }
-        }
-        return handled;
+        return _KeyHandler(gsl::narrow_cast<WORD>(vkey), gsl::narrow_cast<WORD>(scanCode), modifiers, down);
     }
 
     void TermControl::_KeyDownHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/,
@@ -1525,13 +1472,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
 
     void TermControl::_KeyHandler(const Input::KeyRoutedEventArgs& e, const bool keyDown)
     {
-        // If the current focused element is a child element of searchbox,
-        // we do not send this event up to terminal
-        if (_searchBox && _searchBox->ContainsFocus())
-        {
-            return;
-        }
-
         const auto keyStatus = e.KeyStatus();
         const auto vkey = gsl::narrow_cast<WORD>(e.OriginalKey());
         const auto scanCode = gsl::narrow_cast<WORD>(keyStatus.ScanCode);
@@ -1542,6 +1482,18 @@ namespace winrt::Microsoft::Terminal::Control::implementation
             modifiers |= ControlKeyStates::EnhancedKey;
         }
 
+        e.Handled(_KeyHandler(vkey, scanCode, modifiers, keyDown));
+    }
+
+    bool TermControl::_KeyHandler(WORD vkey, WORD scanCode, ControlKeyStates modifiers, bool keyDown)
+    {
+        // If the current focused element is a child element of searchbox,
+        // we do not send this event up to terminal
+        if (_searchBox && _searchBox->ContainsFocus())
+        {
+            return false;
+        }
+
         // GH#11076:
         // For some weird reason we sometimes receive a WM_KEYDOWN
         // message without vkey or scanCode if a user drags a tab.
@@ -1550,8 +1502,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         // accidental insertion of invalid KeyChords into classes like ActionMap.
         if (!vkey && !scanCode)
         {
-            e.Handled(true);
-            return;
+            return true;
         }
 
         // Mark the event as handled and do nothing if we're closing, or the key
@@ -1564,26 +1515,151 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         // about.
         if (_IsClosing() || vkey == VK_LWIN || vkey == VK_RWIN)
         {
-            e.Handled(true);
-            return;
+            return true;
         }
 
         // Short-circuit isReadOnly check to avoid warning dialog
         if (_core.IsInReadOnlyMode())
         {
-            e.Handled(!keyDown || _TryHandleKeyBinding(vkey, scanCode, modifiers));
-            return;
+            return !keyDown || _TryHandleKeyBinding(vkey, scanCode, modifiers);
         }
 
-        // Alt-Numpad# input will send us a character once the user releases
-        // Alt, so we should be ignoring the individual keydowns. The character
-        // will be sent through the TSFInputControl. See GH#1401 for more
-        // details
-        if (modifiers.IsAltPressed() && !modifiers.IsCtrlPressed() &&
-            (vkey >= VK_NUMPAD0 && vkey <= VK_NUMPAD9))
+        // Our custom TSF input control doesn't receive Alt+Numpad inputs,
+        // and we don't receive any via WM_CHAR as a xaml island app either.
+        // So, we simply implement our own Alt-Numpad handling here.
+        //
+        // This handles the case where the Alt key is released.
+        // We'll flush any ongoing composition in that case.
+        if (vkey == VK_MENU && !keyDown && _altNumpadState.active)
         {
-            e.Handled(true);
-            return;
+            auto& s = _altNumpadState;
+            auto encoding = s.encoding;
+            wchar_t buf[4]{};
+            size_t buf_len = 0;
+
+            if (encoding == AltNumpadEncoding::Unicode)
+            {
+                // UTF-32 -> UTF-16
+                if (s.accumulator <= 0xffff)
+                {
+                    buf[buf_len++] = static_cast<uint16_t>(s.accumulator);
+                }
+                else
+                {
+                    buf[buf_len++] = static_cast<uint16_t>((s.accumulator >> 10) + 0xd7c0);
+                    buf[buf_len++] = static_cast<uint16_t>((s.accumulator & 0x3ff) | 0xdc00);
+                }
+            }
+            else
+            {
+                const auto ansi = encoding == AltNumpadEncoding::ANSI;
+                const auto acp = GetACP();
+                auto codepage = ansi ? acp : CP_OEMCP;
+
+                // Alt+Numpad inputs are always a single codepoint, be it UTF-32 or ANSI.
+                // Since DBCS code pages by definition are >1 codepoint, we can't encode those.
+                // Traditionally, the OS uses the Latin1 or IBM code page instead.
+                if (acp == CP_JAPANESE ||
+                    acp == CP_CHINESE_SIMPLIFIED ||
+                    acp == CP_KOREAN ||
+                    acp == CP_CHINESE_TRADITIONAL ||
+                    acp == CP_UTF8)
+                {
+                    codepage = ansi ? 1252 : 437;
+                }
+
+                // The OS code seemed to also simply cut off the last byte in the accumulator.
+                const auto ch = gsl::narrow_cast<char>(s.accumulator & 0xff);
+                const auto len = MultiByteToWideChar(codepage, 0, &ch, 1, &buf[0], 2);
+                buf_len = gsl::narrow_cast<size_t>(std::max(0, len));
+            }
+
+            if (buf_len != 0)
+            {
+                // WinRT always needs null-terminated strings, because HSTRING is dumb.
+                // If it encounters a string that isn't, cppwinrt will abort().
+                // It should already be null-terminated, but let's make sure to not crash.
+                buf[buf_len] = L'\0';
+                _core.SendInput(std::wstring_view{ &buf[0], buf_len });
+            }
+
+            s = {};
+            return true;
+        }
+        // As a continuation of the above, this handles the key-down case.
+        if (modifiers.IsAltPressed())
+        {
+            // The OS code seems to reset the composition if shift is pressed, but I couldn't
+            // figure out how exactly it worked. We'll simply ignore any such inputs.
+            static constexpr DWORD permittedModifiers =
+                RIGHT_ALT_PRESSED |
+                LEFT_ALT_PRESSED |
+                NUMLOCK_ON |
+                SCROLLLOCK_ON |
+                CAPSLOCK_ON;
+
+            if (keyDown && (modifiers.Value() & ~permittedModifiers) == 0)
+            {
+                auto& s = _altNumpadState;
+
+                if (vkey == VK_ADD)
+                {
+                    // Alt '+' <number> is used to input Unicode code points.
+                    // Every time you press + it resets the entire state
+                    // in the original OS implementation as well.
+                    s.encoding = AltNumpadEncoding::Unicode;
+                    s.accumulator = 0;
+                    s.active = true;
+                }
+                else if (vkey == VK_NUMPAD0 && s.encoding == AltNumpadEncoding::OEM && s.accumulator == 0)
+                {
+                    // Alt '0' <number> is used to input ANSI code points.
+                    // Otherwise, they're OEM codepoints.
+                    s.encoding = AltNumpadEncoding::ANSI;
+                    s.active = true;
+                }
+                else
+                {
+                    // Otherwise, append the pressed key to the accumulator.
+                    const uint32_t base = s.encoding == AltNumpadEncoding::Unicode ? 16 : 10;
+                    uint32_t add = 0xffffff;
+
+                    if (vkey >= VK_NUMPAD0 && vkey <= VK_NUMPAD9)
+                    {
+                        add = vkey - VK_NUMPAD0;
+                    }
+                    else if (vkey >= 'A' && vkey <= 'F')
+                    {
+                        add = vkey - 'A' + 10;
+                    }
+
+                    // Pressing Alt + <not a number> should not activate the Alt+Numpad input, however.
+                    if (add < base)
+                    {
+                        s.accumulator = std::min(s.accumulator * base + add, 0x10FFFFu);
+                        s.active = true;
+                    }
+                }
+
+                // If someone pressed Alt + <not a number>, we'll skip the early
+                // return and send the Alt key combination as per usual.
+                if (s.active)
+                {
+                    return true;
+                }
+
+                // Unless I didn't code the above correctly, active == false should imply
+                // that _altNumpadState is in the (default constructed) base state.
+                assert(s.encoding == AltNumpadEncoding::OEM);
+                assert(s.accumulator == 0);
+            }
+        }
+        else if (_altNumpadState.active)
+        {
+            // If the user Alt+Tabbed in the middle of an Alt+Numpad sequence, we'll not receive a key-up event for
+            // the Alt key. There are several ways to detect this. Here, we simply check if the user typed another
+            // character, it's not an alt-up event, and we still have an ongoing composition.
+            _altNumpadState = {};
         }
 
         // GH#2235: Terminal::Settings hasn't been modified to differentiate
@@ -1599,20 +1675,18 @@ namespace winrt::Microsoft::Terminal::Control::implementation
             keyDown &&
             _TryHandleKeyBinding(vkey, scanCode, modifiers))
         {
-            e.Handled(true);
-            return;
+            return true;
         }
 
         if (_TrySendKeyEvent(vkey, scanCode, modifiers, keyDown))
         {
-            e.Handled(true);
-            return;
+            return true;
         }
 
         // Manually prevent keyboard navigation with tab. We want to send tab to
         // the terminal, and we don't want to be able to escape focus of the
         // control with tab.
-        e.Handled(vkey == VK_TAB);
+        return vkey == VK_TAB;
     }
 
     // Method Description:
diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h
index e8cf2cd20f6..d1b53f01dd6 100644
--- a/src/cascadia/TerminalControl/TermControl.h
+++ b/src/cascadia/TerminalControl/TermControl.h
@@ -248,6 +248,23 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         TsfDataProvider _tsfDataProvider{ this };
         winrt::com_ptr<SearchBoxControl> _searchBox;
 
+        enum class AltNumpadEncoding
+        {
+            OEM,
+            ANSI,
+            Unicode,
+        };
+        struct AltNumpadState
+        {
+            AltNumpadEncoding encoding = AltNumpadEncoding::OEM;
+            uint32_t accumulator = 0;
+            // Checking for accumulator != 0 to see if we have an ongoing Alt+Numpad composition is insufficient.
+            // The state can be active, while the accumulator is 0, if the user pressed Alt+Numpad0 which enabled
+            // the OEM encoding mode (= active), and then pressed Numpad0 again (= accumulator is still 0).
+            bool active = false;
+        };
+        AltNumpadState _altNumpadState;
+
         bool _closing{ false };
         bool _focused{ false };
         bool _initializedTerminal{ false };
@@ -375,6 +392,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         void _UpdateAutoScroll(const Windows::Foundation::IInspectable& sender, const Windows::Foundation::IInspectable& e);
 
         void _KeyHandler(const Windows::UI::Xaml::Input::KeyRoutedEventArgs& e, const bool keyDown);
+        bool _KeyHandler(WORD vkey, WORD scanCode, ::Microsoft::Terminal::Core::ControlKeyStates modifiers, bool keyDown);
         static ::Microsoft::Terminal::Core::ControlKeyStates _GetPressedModifierKeys() noexcept;
         bool _TryHandleKeyBinding(const WORD vkey, const WORD scanCode, ::Microsoft::Terminal::Core::ControlKeyStates modifiers) const;
         static void _ClearKeyboardState(const WORD vkey, const WORD scanCode) noexcept;


Test patch: