Skip to content

fix(poweraccent): Add injection hygiene and focus-loss cleanup#10

Closed
crutkas wants to merge 3 commits into
mainfrom
fix-stuck-keys/d-poweraccent
Closed

fix(poweraccent): Add injection hygiene and focus-loss cleanup#10
crutkas wants to merge 3 commits into
mainfrom
fix-stuck-keys/d-poweraccent

Conversation

@crutkas

@crutkas crutkas commented Apr 13, 2026

Copy link
Copy Markdown
Owner

Summary

Add injection hygiene and focus-loss cleanup to PowerAccent (Quick Accent) to prevent cross-module stuck keys and stuck accent mode.

Note: PR microsoft#46593 on microsoft/PowerToys is also touching PowerAccent with shift key state fixes. Coordinate before merging to avoid conflicts.

Changes

D1: Add LLKHF_INJECTED filter to LL keyboard hook

Skip injected events in LowLevelKeyboardProc. Prevents Keyboard Manager remapped keys from triggering accent mode — e.g., if KBM remaps Q to A, pressing Q should not activate the accent toolbar for 'A'.

D2: Tag all SendInput output with dwExtraInfo

  • Added POWERTOYS_INJECTED_TAG = 0x110 constant
  • Set dwExtraInfo on all KEYBDINPUT structs (backspace, character insertion)
  • Added SendInput return value checking with error logging
  • Other hooks (centralized, KBM) can now identify PowerAccent output and skip it

D3: Add ForceReset() for focus-loss cleanup

  • New ForceReset() method on KeyboardListener clears all state: letterPressed, m_toolbarVisible, trigger flags, shift flags
  • Exposed through PowerAccent.Core.ForceResetKeyboardState()
  • Wired into Selector.OnDeactivated — accent mode now cleans up when the window loses focus (Alt+Tab, click away, etc.)

D4: Replace SendKeys.SendWait with tagged SendInput

  • Replaced SendKeys.SendWait("{RIGHT}") / "{LEFT}" with WindowsFunctions.SendArrowKey()
  • Uses tagged SendInput instead of blocking SendKeys.SendWait
  • Arrow key events now carry dwExtraInfo so other hooks identify them as synthetic

How to Reproduce

D1 — Cross-module trigger:

  1. In KBM, remap Q to A
  2. Press and hold Q, then press Space
  3. Before fix: Accent toolbar appears for 'A' (triggered by KBM-injected input)
  4. After fix: Accent toolbar does NOT appear (injected input filtered)

D3 — Stuck accent mode:

  1. Trigger accent toolbar (hold a letter with accents, press Space)
  2. While toolbar is visible, press Alt+Tab to switch apps
  3. Before fix: Accent state stays active, Space/arrows swallowed globally
  4. After fix: Toolbar hides, state resets on deactivation

Coordination

PR Checklist

  • Builds clean on x64 Release
  • Manual verification per repro steps
  • No interference with KBM when both active

@github-actions

This comment has been minimized.

@yeelam-gordon

Copy link
Copy Markdown

Incomplete locking around the new m_state_mutex.

m_state_mutex (KeyboardListener.h L69) is acquired in ForceReset() (KeyboardListener.cpp L54-68), but not in OnKeyDown / OnKeyUp, which mutate the same fields letterPressed, m_toolbarVisible, m_triggeredWithSpace, m_triggeredWithLeftArrow, m_triggeredWithRightArrow, m_leftShiftPressed, m_rightShiftPressed.

OnKeyDown / OnKeyUp run on the LL keyboard hook thread. ForceReset() is called from Selector_Deactivated (Selector.xaml.cs L60), which is a WPF window event runs on the Dispatcher (UI) thread.

So this is real cross-thread mutation: the hook-thread writers race the UI-thread reset. Without a guard on OnKeyDown/OnKeyUp the reset can observe partial state, miss writes, or race a flag flip and on the hook thread we can lose a write that ForceReset just did.

Two safe fixes:

  1. Add std::lock_guard<std::recursive_mutex> lock(m_state_mutex); at the top of OnKeyDown / OnKeyUp. The mutex is already recursive_mutex, so any callback into PowerAccent.cs that ends up calling back is safe.
  2. Or, since these are tight LL-hook paths, convert the flags to std::atomic<bool> and std::atomic<LetterKey> and drop the mutex on the reset side. Lock-free is friendlier for the hot path.

Either way, the current "lock only one writer" pattern is the canonical incomplete-locking shape and undoes most of the safety the mutex was meant to add.

crutkas and others added 2 commits June 12, 2026 15:46
- Filter LLKHF_INJECTED events in LL hook to prevent cross-module triggers
- Tag all SendInput output with dwExtraInfo for cross-hook identification
- Add ForceReset() for focus-loss cleanup
- Replace SendKeys.SendWait with tagged SendInput for arrow keys

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Selector_Deactivated now calls ForceReset, so losing focus mid-accent clears
any half-pressed state instead of leaving the toolbar or shift modifiers stuck.

OnKeyDown/OnKeyUp run on the low-level keyboard hook thread while ForceReset
runs on the UI thread. The PowerAccent.cs callbacks Dispatcher.Invoke
synchronously onto that UI thread, so holding a lock across the hook callbacks
would deadlock against ForceReset. The shared flags and letterPressed are
therefore std::atomic rather than mutex-guarded - race-free and lock-free on
the hot hook path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@crutkas crutkas force-pushed the fix-stuck-keys/d-poweraccent branch from 2dcbe36 to f421f92 Compare June 12, 2026 22:51
@crutkas

crutkas commented Jun 12, 2026

Copy link
Copy Markdown
Owner Author

Went with the atomics option and dropped the mutex.

Locking OnKeyDown/OnKeyUp would deadlock: their PowerAccent.cs callbacks Dispatcher.Invoke synchronously onto the UI thread, and ForceReset (now wired to Selector_Deactivated) runs on that same UI thread — so holding a lock across the hook callbacks would have the hook thread waiting on the UI thread while the UI thread waits on the lock. The shared flags and letterPressed are now std::atomic instead, which is race-free and lock-free on the hot hook path.

I also removed a broad LLKHF_INJECTED filter that had been added here — same accessibility regression as in the centralized-hook PR (it blocked OSK/AT/macro injection). The dwExtraInfo tagging on PowerAccent's own injected keys stays, so the centralized keyboard hook still ignores them.

PowerAccent injects VK_LEFT / VK_RIGHT to move the caret while selecting an
accent. Without KEYEVENTF_EXTENDEDKEY the synthesized arrow keys carry the
non-extended scan code, which some apps interpret as numpad navigation
(affected by NumLock) rather than the dedicated arrow cluster. Set the
extended-key flag on both the key-down and key-up INPUTs, matching how
WinForms SendKeys synthesizes these keys.

Also resolve two StyleCop errors in code this change set introduced, so the
project builds clean: rename the POWERTOYS_INJECTED_TAG constant to
PascalCase PowerToysInjectedTag (SA1310) and add the required blank line
after the backspace send-failure check (SA1513).

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#48572 (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

Development

Successfully merging this pull request may close these issues.

3 participants