Skip to content

fix(kbm): Handle SendVirtualInput failures and release modifiers for text remap#9

Closed
crutkas wants to merge 5 commits into
mainfrom
fix-stuck-keys/c-keyboard-manager
Closed

fix(kbm): Handle SendVirtualInput failures and release modifiers for text remap#9
crutkas wants to merge 5 commits into
mainfrom
fix-stuck-keys/c-keyboard-manager

Conversation

@crutkas

@crutkas crutkas commented Apr 13, 2026

Copy link
Copy Markdown
Owner

Summary

Fix Keyboard Manager stuck key bugs where SendVirtualInput failure causes keys to vanish, and held modifiers corrupt text injection.

Changes

C1: SendVirtualInput returns bool — pass through on failure

  • Changed InputInterface::SendVirtualInput from void to bool
  • Returns false when SendInput injects fewer events than expected
  • All 8 call sites that suppress the original key (return 1) now check the return value — if injection fails, return 0 passes through the original key instead of eating it
  • Updated MockedInput test double to match new signature

C4: Release held modifiers before text injection

  • In HandleSingleKeyToTextRemapEvent, detect held modifiers via GetAsyncKeyState
  • Release Ctrl, Shift, Alt, and Win keys before SendTextInput
  • Re-press them after text injection
  • Prevents Ctrl+text, Win+text corruption
  • Return value checked on modifier release — passes through original key if release fails

New Unit Tests (3 tests)

  • HandleSingleKeyRemapEvent_ShouldPassThrough_WhenSendVirtualInputFails — verifies key not eaten on failure
  • HandleSingleKeyToTextRemapEvent_ShouldReleaseAndRestoreCtrl_WhenCtrlIsHeld
  • HandleSingleKeyToTextRemapEvent_ShouldReleaseAndRestoreWinKey_WhenWinKeyIsHeld

How to Reproduce

C1 — Key eaten on failure: Hard to reproduce directly (requires SendInput to fail), but verified via unit test with mocked failure.

C4 — Modifier corruption:

  1. In KBM, remap key X to text "hello"
  2. Hold Ctrl and press X
  3. Before fix: Types Ctrl+h, Ctrl+e, Ctrl+l, Ctrl+l, Ctrl+o (triggering shortcuts instead of text)
  4. After fix: Releases Ctrl, types "hello", re-presses Ctrl

How to Verify

  1. All 95 unit tests pass (92 existing + 3 new)
  2. Manual: remap a key to text, hold Ctrl/Win/Shift while pressing it — text types correctly

Related Issues

PR Checklist

  • Builds clean on x64 Release
  • 95/95 unit tests pass
  • New tests added for failure paths

@github-actions

This comment has been minimized.

@yeelam-gordon

Copy link
Copy Markdown

At HandleSingleKeyToTextRemapEvent (KeyboardEventHandlers.cpp ~L1823-1862) the release / SendTextInput / restore sequence has a narrow but real race:

for (int vk : modifierKeys)
    if (ii.GetVirtualKeyState(vk))
        // queue release + remember in releasedKeys
ii.SendVirtualInput(releaseEvents);
Helpers::SendTextInput(*remapping);   // <-- user can release the physical modifier here
// unconditionally re-press releasedKeys
ii.SendVirtualInput(restoreEvents);

If the user physically lets go of the modifier during SendTextInput (very plausible for a long remap text), the unconditional re-press leaves Windows believing the modifier is virtually held while the physical key is up i.e. the exact stuck-modifier class this PR family targets. The user has to tap the modifier again to resync.

Suggested fix: query GetAsyncKeyState(vk) & 0x8000 immediately before each re-press and only re-press modifiers still physically held:

for (int vk : releasedKeys)
{
    if (GetAsyncKeyState(vk) & 0x8000)
        Helpers::SetKeyEvent(restoreEvents, INPUT_KEYBOARD, static_cast<WORD>(vk), 0,
                             KeyboardManagerConstants::KEYBOARDMANAGER_SHORTCUT_FLAG);
}

Tests in SingleKeyRemappingTests.cpp cover the synchronous failure path nicely adding one for "user releases modifier mid-text" would make this airtight.

crutkas and others added 4 commits June 12, 2026 12:10
…text remap

- SendVirtualInput now returns bool; callers pass through original key on failure
- Release held modifiers before text injection in single-key-to-text remap
  to prevent Ctrl+text corruption

Fixes: microsoft#17035, microsoft#29015, microsoft#9778

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…urns, add tests

- Add VK_LWIN/VK_RWIN to modifierKeys[] in HandleSingleKeyToTextRemapEvent
  so Win key is released before text injection (prevents Win+h/e/l shortcuts)
- Check SendVirtualInput return on modifier release — pass through original
  key if release fails (prevents Ctrl+text corruption on SendInput failure)
- Add MockedInput::SetKeyboardState for test setup
- Add ClearSingleKeyToTextRemaps to ResetTestEnv
- Add 2 new tests:
  - HandleSingleKeyToTextRemapEvent_ShouldReleaseAndRestoreWinKey_WhenWinKeyIsHeld
  - HandleSingleKeyToTextRemapEvent_ShouldReleaseAndRestoreCtrl_WhenCtrlIsHeld

All 94 KBM unit tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The single-key-to-text handler releases any held modifiers before
injecting Unicode text (so Ctrl/Shift/etc. don't corrupt it) and
previously re-pressed them afterwards. Re-pressing is unsafe: once we
inject a modifier KEYUP, GetAsyncKeyState reports that key as up, so we
cannot tell whether the user is still physically holding it or has let
go. If they released it during injection, the re-press would leave the
modifier stuck down - the exact bug this series fixes.

Leave released modifiers released; the user taps the key again to
re-engage it. Update the tests to assert modifiers are never re-pressed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@crutkas crutkas force-pushed the fix-stuck-keys/c-keyboard-manager branch from 0bb92cb to edf82ef Compare June 12, 2026 22:51
@crutkas

crutkas commented Jun 12, 2026

Copy link
Copy Markdown
Owner Author

Good catch on the release/re-press race. I dug into the GetAsyncKeyState & 0x8000 "re-press only if still physically held" suggestion and it turns out to be unworkable: injecting the modifier's own KEYUP flips GetAsyncKeyState to up, so by the time we'd evaluate the guard it always reports the modifier released — the re-press would become dead code.

So I dropped the re-press entirely. Modifiers are released before the text is injected (SendTextInput uses KEYEVENTF_UNICODE, so it's modifier-independent) and never re-pressed, which guarantees this path can't leave a modifier stuck down. Cost is that the user re-taps the modifier to re-engage it. Added tests asserting the modifiers end released and that there are zero modifier re-press injections, including the "user releases the modifier mid-text" case.

…ection

Three correctness fixes for the single-key-to-text remap path plus test
coverage for the stuck-key hardening:

1. Insert a dummy key event before releasing held modifiers. Releasing a
   lone Win or Alt key-up otherwise triggers the Start Menu / menu bar; the
   dummy key absorbs the modifier so the release is inert. The dummy and
   releases are only injected when a modifier is actually held.

2. Accept WM_SYSKEYDOWN as well as WM_KEYDOWN. While Alt is held the system
   sends WM_SYSKEYDOWN, so the previous WM_KEYDOWN-only guard silently
   dropped the remap whenever Alt was down.

3. Route Helpers::SendTextInput through the InputInterface instead of calling
   Win32 SendInput directly. Previously the unit tests for this handler
   injected real keystrokes into the OS during the test run. Text is still
   flushed per character to preserve the existing batching workaround.

Also add a MockedInput failure seam (SetSendVirtualInputShouldFail) and
tests: RemappedKey_ShouldPassOriginalKeyThrough_WhenInjectionFails verifies
the handler passes the original key through when injection fails - the core
stuck-key behavior introduced by this change set, previously untestable
because the mock always succeeded; and
HandleSingleKeyToTextRemapEvent_ShouldFireAndReleaseAlt_WhenAltIsHeld covers
fix 2 by asserting the remap still fires (releasing the held Alt) when the
key arrives as WM_SYSKEYDOWN.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@crutkas

crutkas commented Jun 13, 2026

Copy link
Copy Markdown
Owner Author

Superseded by the upstream PR against microsoft/PowerToys: microsoft#48571 (rebased onto current main, builds/tests verified). Closing this intra-fork review PR.

@crutkas crutkas closed this Jun 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants