feat(injection): Add keyboard input injection on Skia - #23970
feat(injection): Add keyboard input injection on Skia#23970MartinZikmund wants to merge 10 commits into
Conversation
Sorry, something went wrong.
|
Claude finished @MartinZikmund's task in 4m 25s —— View job PR Review: feat(injection): Add keyboard input injection on SkiaTasks:
Review SummaryVerdict: ship — the architecture is sound and the approach of joining at KeyboardManager.OnKey is correct. A handful of small issues below. Inline findings posted (4 substantive, 1 informational):
Cross-cutting notes (no matching diff line):
|
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23970/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-23970/docs/index.html |
Sorry, something went wrong.
|
The build 225873 found UI Test snapshots differences: skia-linux-screenshots: 57, skia-windows-screenshots: 2348, wasm: 11 Details
|
Sorry, something went wrong.
|
⚠️⚠️ The build 225873 has failed on Uno.UI - CI. |
Sorry, something went wrong.
|
Claude finished @MartinZikmund's task in 3m 43s —— View job PR Review: feat(injection): Add keyboard input injection on SkiaTasks:
Review SummaryVerdict: fix-first — the architecture and approach are sound; a small cluster of issues from the prior review were not addressed and need fixing before merge. Inline findings (5 posted):
What's correct:
|
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23970/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
Claude finished @MartinZikmund's task in 4m 10s —— View job PR Review: feat(injection): Add keyboard input injection on SkiaTasks:
Review SummaryVerdict: fix-first — the two new commits (7db720cb refactor: drop dead #if HAS_INPUT_INJECTOR guard; 72aef409 fix: retarget DevTools input namespace for 7.0) are both clean and introduce no new issues. However, all four substantive findings from the prior reviews remain unaddressed. Inline findings (4 posted):
Cross-cutting notes (not in diff, cannot post inline):
What's correct in the new commits:
|
Sorry, something went wrong.
|
The build 229579 found UI Test snapshots differences: skia-linux-screenshots: 73, skia-windows-screenshots: 123 Details
|
Sorry, something went wrong.
- ArgumentException now reports "input" as paramName instead of a method name. - InjectKeyboardInputAsync validates the whole batch before dispatching, so the no-partial-delivery contract of the synchronous overload holds there too. - Caps Lock toggling moves behind a static helper, keeping the static write out of the instance method. - Logging uses this.Log().Warn(...), matching the rest of the file, and the KeyboardManager init error no longer says "PointerManager". - The surrogate-pair test releases VK 255 instead of leaving it latched down. - Documented why the XamlIsland skip uses Assert.Inconclusive. - Dropped an unused using from IInputInjectorTarget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmZ63vuPQNDmg8H6nSGFSq
|
Claude finished @MartinZikmund's task in 1m 43s —— View job PR Review: feat(injection): Add keyboard input injection on SkiaTasks:
Review SummaryVerdict: ship — all four substantive findings from the prior two review rounds are confirmed fixed. No new issues. Verified fixes:
Cross-cutting notes (no new concerns):
|
Sorry, something went wrong.
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-23970/docs/index.html |
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23970/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-23970/docs/index.html |
Sorry, something went wrong.
|
The build 230295 found UI Test snapshots differences: skia-linux-screenshots: 57, skia-windows-screenshots: 2348, wasm: 11 Details
|
Sorry, something went wrong.
|
Claude finished @MartinZikmund's task in 1m 26s —— View job PR Review: feat(injection): Add keyboard input injection on SkiaTasks:
Review SummaryVerdict: ship — all findings from three prior review rounds are confirmed fixed in the current branch HEAD. No new issues found. Verified fixes:
What's correct (no new concerns):
|
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Adds Skia-only support for Windows.UI.Input.Preview.Injection.InputInjector.InjectKeyboardInput by routing injected key events through InputManager.KeyboardManager, enabling focus routing, accelerators, navigation, and text input, while also fixing Skia text layout to tolerate unpaired Unicode surrogates.
Changes:
- Implement Skia keyboard injection end-to-end (new InjectedInputKeyboardInfo, character synthesis, active-window targeting via IInputInjectorTarget).
- Propagate tracked keyboard modifiers into injected pointer events to enable composed gestures (e.g., Ctrl+Click).
- Harden Skia text layout against lone surrogate code units and add runtime tests + design note.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file| File | Description |
|---|---|
| src/Uno.UWP/UI/Input/Preview.Injection/InputInjector.cs | Tracks per-thread injector targets, implements keyboard injection, and forwards tracked modifiers into pointer injection. |
| src/Uno.UWP/UI/Input/Preview.Injection/InjectedInputKeyboardInfo.cs | Adds real InjectedInputKeyboardInfo implementation and conversion to KeyEventArgs. |
| src/Uno.UWP/UI/Input/Preview.Injection/InjectedInputKeyboardCharacterMap.cs | Provides invariant-US character synthesis for non-Unicode injected keys. |
| src/Uno.UWP/UI/Input/Preview.Injection/IInputInjectorTarget.cs | Extends injector target contract with keyboard injection methods and activation state. |
| src/Uno.UWP/Generated/3.0.0.0/Windows.UI.Input.Preview.Injection/InputInjector.cs | Generated sync update to avoid emitting a stub for an already-implemented method. |
| src/Uno.UWP/Generated/3.0.0.0/Windows.UI.Input.Preview.Injection/InjectedInputKeyboardInfo.cs | Generated sync update to avoid emitting stubs for properties now implemented manually. |
| src/Uno.UI/UI/Xaml/Internal/InputManager.Pointers.Managed.cs | Removes injector registration from pointer-manager construction (registration moved to InputManager ctor). |
| src/Uno.UI/UI/Xaml/Internal/InputManager.Keyboard.skia.cs | Adds injection entry point into KeyboardManager and fixes a log message label. |
| src/Uno.UI/UI/Xaml/Internal/InputManager.Keyboard.cs | Implements new IInputInjectorTarget keyboard members + activation targeting on InputManager. |
| src/Uno.UI/UI/Xaml/Internal/InputManager.cs | Registers InputInjector target from InputManager constructor for Skia injector support. |
| src/Uno.UI/UI/Xaml/Documents/UnicodeText.skia.cs | Prevents char.ConvertToUtf32 from throwing on unpaired surrogates during layout. |
| src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Documents/Given_UnicodeText.skia.cs | Adds coverage ensuring unpaired surrogates don’t crash Skia layout paths. |
| src/Uno.UI.RuntimeTests/Tests/Windows_UI_Input_Preview_Injection/Given_InputInjector_Keyboard.cs | Adds Skia runtime tests for injected keyboard behavior (text, accelerators, Tab navigation, Unicode, modifiers). |
| specs/keyboard-injection-design.md | Documents measured WinUI behavior, Skia design/limitations, and test plan. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-23970/docs/index.html |
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23970/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
The build 230310 found UI Test snapshots differences: skia-linux-screenshots: 57, skia-windows-screenshots: 2348, wasm: 11 Details
|
Sorry, something went wrong.
|
⚠️⚠️ The build 230310 has failed on Uno.UI - CI. |
Sorry, something went wrong.
Real input reads the OS key state after the transition is applied, so a Shift key-down already reports Shift. The tracker is only updated once the routed event reaches UIElement, so injected args reported stale modifiers. Also resolves the Unicode key (255) before querying WasKeyDown, which was previously looked up against VirtualKey 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012PF59BzvEyASdeFtsFHrox
Select-all is Cmd+A on Apple keyboards, so the hardcoded Ctrl+A failed on macOS and iOS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012PF59BzvEyASdeFtsFHrox
An unpaired surrogate in a test name or assertion message made the whole NUnit results document unwritable on WebAssembly, where it is POSTed to a server that encodes as UTF-16. The shard then aborted with "Root element is missing" and no diagnostics. Escape non-XML characters when generating the document, and stop passing lone surrogates as data-row values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012PF59BzvEyASdeFtsFHrox
|
Claude finished @MartinZikmund's task in 1m 7s —— View job PR Review: feat(injection): Add keyboard input injection on SkiaTasks:
Review SummaryVerdict: ship — both new Copilot findings from the latest build are false positives. The issues they describe were already addressed in the current code. Copilot finding 1 — stale modifiers / wrong wasKeyDown key for Unicode (line 361): Already fixed. InjectKeyboardInputCore now uses info.EffectiveVirtualKey (which returns 255 for Unicode keys, not 0), and manually folds the current key transition into modifiers via GetModifierFlag(key) before building the args. The comment in the code explains this exactly: // Real input reads the OS key state after the transition has been applied, so pressing Shift
// already reports Shift. The tracker is only updated once the routed event reaches UIElement,
// so fold the current transition in by hand to keep injected args identical to real ones.
var modifiers = GetTrackedModifiers();
if (GetModifierFlag(key) is { } flag)
{
modifiers = info.IsKeyUp ? modifiers & ~flag : modifiers | flag;
}
var wasKeyDown = KeyboardStateTracker.GetKeyState(key).HasFlag(CoreVirtualKeyStates.Down);Copilot finding 2 — WasKeyDown always true for key-up (line 93): Intentional Win32 parity. The code comment explains the design: "Win32 defines the previous-key-state bit as always 1 for WM_KEYUP, so a key-up reports WasKeyDown even when no key-down preceded it." WM_KEYUP always has bit 30 of lParam set to 1 by the OS; Copilot's suggestion to pass wasKeyDown directly would break that parity. All findings from the three prior review rounds remain confirmed fixed:
No new inline comments needed. |
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (1)Previously missed (1) — in code that hasn't changed since the last review.
src/Uno.UWP/UI/Input/Preview.Injection/InputInjector.cs:58
- GetFirstTarget() scans the thread-static WeakReference list but never removes dead entries. Over time (open/close windows), this can leave the list full of dead WeakReferences, making TryCreate() increasingly expensive and keeping the stale list around indefinitely until another registration happens.
Consider pruning dead entries while scanning, similarly to SetTargetForCurrentThread().
private static IInputInjectorTarget? GetFirstTarget()
{
if (_inputManagers is { } managers)
{
foreach (var weak in managers)
{
if (weak.TryGetTarget(out var target))
{
return target;
}
}
}
return null;
}
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23970/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-23970/docs/index.html |
Sorry, something went wrong.
|
The build 230356 found UI Test snapshots differences: skia-linux-screenshots: 57, skia-windows-screenshots: 2348, wasm: 11 Details
|
Sorry, something went wrong.
| /// </remarks> | ||
| internal static class InjectedInputKeyboardCharacterMap | ||
| { | ||
| // VK codes without a Windows.System.VirtualKey name. |
There was a problem hiding this comment.
| // VK codes without a Windows.System.VirtualKey name. | |
| // VK codes without a Windows.System.VirtualKey name: // value (without/with)-Shift key |
Sorry, something went wrong.
| VkOem1 => shift ? ':' : ';', | ||
| VkOemPlus => shift ? '+' : '=', | ||
| VkOemComma => shift ? '<' : ',', | ||
| VkOemMinus => shift ? '_' : '-', | ||
| VkOemPeriod => shift ? '>' : '.', | ||
| VkOem2 => shift ? '?' : '/', | ||
| VkOem3 => shift ? '~' : '`', | ||
| VkOem4 => shift ? '{' : '[', | ||
| VkOem5 => shift ? '|' : '\\', | ||
| VkOem6 => shift ? '}' : ']', | ||
| VkOem7 => shift ? '"' : '\'', |
There was a problem hiding this comment.
consider using !shift and swap the sides?
Sorry, something went wrong.
| } | ||
| } | ||
|
|
||
| // TODO: Move as extension method |
There was a problem hiding this comment.
todo?
Sorry, something went wrong.
| modifiers |= VirtualKeyModifiers.Menu; | ||
| } | ||
|
|
||
| if (IsDown(VirtualKey.LeftWindows) || IsDown(VirtualKey.RightWindows)) |
There was a problem hiding this comment.
why only win-key has this left/right shenanigans, but not the rest/above?
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
GitHub Issue: closes #12962
PR Type:
✨ Feature
What changed? 🚀
InputInjector.InjectKeyboardInput was unimplemented on every Uno target — the generated stub threw. This implements it for all Skia targets.
The implementation joins the input pipeline at InputManager.KeyboardManager, the single chokepoint every Skia host already feeds through IUnoKeyboardInputSource. Injected keys therefore get focus routing, accelerators, Tab navigation and text input for free, with no per-host changes.
Keyboard injection targets the active window, mirroring the foreground-window behavior of the Windows implementation. The injector registration moves from the managed pointer manager into the InputManager constructor and now tracks every ContentRoot on the thread, so a second window is reachable.
Also included:
Non-Skia targets keep the previous behavior: the new IInputInjectorTarget keyboard members follow the existing pointer pattern (cross-platform explicit interface implementation delegating to a partial void that only the Skia file implements), so they compile to a no-op everywhere else.
PR Checklist ✅