Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alt + numpad combination does not work anymore #17327

Closed
raksati opened this issue May 28, 2024 · 8 comments · Fixed by #17637
Closed

Alt + numpad combination does not work anymore #17327

raksati opened this issue May 28, 2024 · 8 comments · Fixed by #17637
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal.

Comments

@raksati
Copy link

raksati commented May 28, 2024

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:~$

@raksati raksati added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels May 28, 2024
@j4james
Copy link
Collaborator

j4james commented May 28, 2024

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:

// 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

But TSFInputControl has now been replaced, so perhaps that functionality was lost in the rewrite.

/cc @lhecker

@lhecker
Copy link
Member

lhecker commented May 28, 2024

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.

@lhecker
Copy link
Member

lhecker commented May 28, 2024

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!

@lhecker
Copy link
Member

lhecker commented May 29, 2024

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:

// 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:

THROW_IF_FAILED(_documentMgr->CreateContext(_clientId, 0, static_cast<ITfContextOwnerCompositionSink*>(this), _context.addressof(), &ecTextStore));

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:

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.

@lhecker lhecker added Help Wanted We encourage anyone to jump in on these. Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. Priority-3 A description (P3) labels May 29, 2024
@lhecker lhecker added this to the Terminal v1.22 milestone May 29, 2024
@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label May 29, 2024
@inoperable

This comment was marked as off-topic.

@inoperable

This comment was marked as off-topic.

@lhecker

This comment was marked as resolved.

@inoperable

This comment was marked as abuse.

@lhecker lhecker closed this as completed in 2fab986 Aug 7, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added In-PR This issue has a related PR Needs-Tag-Fix Doesn't match tag requirements labels Aug 7, 2024
DHowett pushed a commit that referenced this issue Aug 23, 2024
This adds an indirection for `_KeyHandler` so that `OnDirectKeyEvent`
can call `_KeyHandler`. This allows us to consistently handle
Alt-key-up events. Then I added custom handling for Alt+ddd (OEM),
Alt+0ddd (ANSI), and Alt+'+'+xxxx (Unicode) sequences, due to the
absence of Alt-key events with xaml islands and our TSF control.

Closes #17327

## Validation Steps Performed
* Tested it according to https://conemu.github.io/en/AltNumpad.html
* Unbind Alt+Space
* Run `showkey -a`
* Alt+Space generates `^[ `
* F7 generates `^[[18~`

(cherry picked from commit 2fab986)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgSCpCg
Service-Version: 1.21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants