Conversation
b4cc152 to
28d7b06
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved IME ordering, synchronization, and read-only input issues remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes Android soft-keyboard IME synchronization for Skia WebAssembly TextBoxes.
Changes:
- Synchronizes hidden-input text, caret, and composition state.
- Adds composition start-index reporting and key-name fallback mapping.
- Adds runtime coverage for composition ordering and Backspace behavior.
File summaries
| File | Summary |
|---|---|
src/Uno.UI/UI/Xaml/Controls/TextBoxCore/TextBoxCore.IME.cs |
Tracks composition indices and platform-applied text. |
src/Uno.UI/UI/Xaml/Controls/TextBox/Extensions/ImeCompositionEventArgs.cs |
Adds StartIndex. |
src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_TextBox.BrowserSoftKeyboard.cs |
Adds soft-keyboard runtime tests. |
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/UI/Xaml/Controls/TextBox/WasmImeTextBoxExtension.cs |
Bridges browser composition lifecycle events. |
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts |
Synchronizes browser input, selection, and composition state. |
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/Devices/Input/BrowserKeyboardInputSource.cs |
Falls back from key codes to key names. |
Review details
Suppressed comments (5)
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/Devices/Input/BrowserKeyboardInputSource.cs:55
- The new
FromKeyfallback is not exercised by the added soft-keyboard tests: those events are stopped at the hidden input before the document-levelBrowserKeyboardInputSourcecallback runs. Add a directOnNativeKeyboardEventregression case such ascode="",key="Backspace"and assertVirtualKey.Back; otherwise this mapping can regress without a test.
var virtualKey = BrowserVirtualKeyHelper.FromCode(code);
if (virtualKey is VirtualKey.None)
{
// Soft keyboards report an empty code, so the key name is all there is to go on.
virtualKey = BrowserVirtualKeyHelper.FromKey(key);
}
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:167
- Unlike
OnCompositionUpdated,syncTextFromInputdoes not honorcore.IsReadOnly;ProcessTextInputaccepts the value. Because the hidden element is never marked read-only, a focused read-only TextBox can be mutated by a composing soft-keyboard input that this new unconditional sync forwards. Reject native input for read-only cores (or configure the hidden input accordingly) before syncing it.
BrowserInvisibleTextBoxViewExtension.syncTextFromInput(input);
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:167
- This raises
TextCompositionChangedbeforesyncTextFromInputupdates the managed TextBox. On the first preedit, handlers can therefore observe a composition range that is outsideText(the existing IME sample reads that range fromText), and the event ordering differs from the normal IME path, which applies text before raising the event. Update the managed value first while preserving the platform-apply guard, then raise the composition event.
BrowserInvisibleTextBoxViewExtension._imeExports.OnCompositionUpdated(
BrowserInvisibleTextBoxViewExtension.compositionText,
BrowserInvisibleTextBoxViewExtension.findCompositionStart(input));
}
BrowserInvisibleTextBoxViewExtension.syncTextFromInput(input);
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:216
- With Safari's ordering, the committed
inputevent arrives aftercompositionend. ClearingisComposingand completing here means the lateroninputskipsfindCompositionStart; when an IME has reopened over existing text, the managed session still has the caret captured atcompositionstart, soTextCompositionEndedEventArgs.StartIndexreports the wrong range. Defer completion until the following input, or otherwise carry that input's computed range into completion.
BrowserInvisibleTextBoxViewExtension.isComposing = false;
if (ev.data.length > 0) {
BrowserInvisibleTextBoxViewExtension._imeExports.OnCompositionCompleted(ev.data);
src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_TextBox.BrowserSoftKeyboard.cs:58
- This test does not actually model the Safari ordering described in its comment:
ComposePreeditalready dispatches an input event beforecompositionend, soabhas been synced before completion and the post-end input is only a duplicate. It therefore cannot catch a completion callback observing stale managed text; construct the sequence with the first committed input aftercompositionendand assert the composition event state as well as the final value.
ComposePreedit("ab", "ab", caret: 2);
DispatchComposition("compositionend", "ab");
DispatchInput("insertCompositionText", "ab", isComposing: false);
SetHiddenInputValue("ab ", caret: 3);
DispatchInput("insertText", " ", isComposing: false);
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/Devices/Input/BrowserKeyboardInputSource.cs:55
- The new
FromKeyfallback is not exercised by the added soft-keyboard tests: those dispatch events on the hidden input, where the TypeScript handler stops propagation beforeOnNativeKeyboardEventis called. The existing browser keyboard tests cover only non-emptyevent.code, so a regression in mapping an empty-codeBackspace/Enterto its key name would pass; add a directOnNativeKeyboardEventtest for the fallback.
var virtualKey = BrowserVirtualKeyHelper.FromCode(code);
if (virtualKey is VirtualKey.None)
{
// Soft keyboards report an empty code, so the key name is all there is to go on.
virtualKey = BrowserVirtualKeyHelper.FromKey(key);
}
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/UI/Xaml/Controls/TextBox/WasmImeTextBoxExtension.cs:71
- In Safari's ordering,
compositionendarrives before the committedinputevent. This raisesTextCompositionEndedwithTextAlreadyAppliedbefore the hidden input's value has been synchronized, so managed handlers observe the oldText(the IME sample computes the committed substring from that property); only the later input event fixes the final value. Defer completion/ended notification until after the trailing input sync, or synchronize managed text before raising these events.
Instance._isComposing = false;
Instance.CompositionCompleted?.Invoke(Instance, new ImeCompositionEventArgs(text, textAlreadyApplied: true));
Instance.CompositionEnded?.Invoke(Instance, EventArgs.Empty);
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:481
- This guard suppresses every
setTextcall during an input callback, including the writeback fromTextBoxView.UpdateTextFromNativewhenBeforeTextChangingor text coercion changes/rejects the DOM value. In that case the hidden input keeps the rejected value while managedTexthas another value; composition completion is marked already applied, so no later path necessarily resynchronizes them. Suppress only the exact echo, or restore the managed value and end the composition when the input was rejected.
if (BrowserInvisibleTextBoxViewExtension.isComposing && BrowserInvisibleTextBoxViewExtension.isSyncingFromInput) {
return;
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:216
- Safari can deliver the committed
inputevent aftercompositionend, but this invokesOnCompositionCompletedimmediately and marks the managed composition complete before that input syncsTextBox.Text.TextCompositionEndedhandlers therefore see stale text and a range that may not exist; the existing IME sample's substring lookup will report the committed preedit as out of range in this ordering. Buffer the completion notification until the value has been synchronized, or otherwise apply the committed value before raising the managed event.
BrowserInvisibleTextBoxViewExtension.isComposing = false;
if (ev.data.length > 0) {
BrowserInvisibleTextBoxViewExtension._imeExports.OnCompositionCompleted(ev.data);
src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_TextBox.BrowserSoftKeyboard.cs:58
- This test does not actually replay the Safari ordering described in its comment:
ComposePreeditdispatches the finalinputbeforecompositionend, and the post-end input carries the same already-synchronized value. Consequently it cannot catch handlers observing staleTextCompositionEndedstate in Safari order. Model the final preedit withcompositionupdateonly, then set the value and dispatch the committedinputaftercompositionend.
// Safari order: the input event carrying the committed preedit follows compositionend.
DispatchComposition("compositionstart", "");
ComposePreedit("ab", "ab", caret: 2);
DispatchComposition("compositionend", "ab");
DispatchInput("insertCompositionText", "ab", isComposing: false);
SetHiddenInputValue("ab ", caret: 3);
DispatchInput("insertText", " ", isComposing: false);
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
28d7b06 to
75f5172
Compare
75f5172 to
483e25f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate issues remain unresolved in keyboard fallback and composition synchronization.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:50
- The described key-name fallback is not implemented here: for an empty or unmapped
event.code, this branch only stops propagation, whileBrowserKeyboardInputSource.OnNativeKeyboardEventstill callsBrowserVirtualKeyHelper.FromCode(code)and never usesKeyboardEvent.key. Consequently such events still becomeVirtualKey.Nonewherever they are routed to managed input; add that fallback in the keyboard source or remove the claim from the PR description.
private static isSoftKeyboardKey(ev: KeyboardEvent): boolean {
return ev.keyCode === BrowserInvisibleTextBoxViewExtension.ANDROID_IME_KEYCODE || ev.code === "";
}
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review details
Suppressed comments (4)
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:50
- This only recognizes an empty
code(or keyCode 229) as a soft-keyboard event. The document path still maps onlyFromCode(code); for a key such as{ key: 'Backspace', code: 'Unidentified' }, this predicate is false, the handler's laterpreventDefault()runs, and managed code receivesVirtualKey.None, so deletion still fails. Add the key-name fallback or classify these unmapped events as native before they are prevented.
private static isSoftKeyboardKey(ev: KeyboardEvent): boolean {
return ev.keyCode === BrowserInvisibleTextBoxViewExtension.ANDROID_IME_KEYCODE || ev.code === "";
}
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:69
- The active-element check still admits only
HTMLInputElement, whilecreateInputcreates anHTMLTextAreaElementfor multiline (acceptsReturn) TextBoxes. Native selection changes in those controls therefore never reachOnSelectionChanged, leaving the managed caret stale when the IME or native keyboard moves it during composition. IncludeHTMLTextAreaElementin this guard.
if (input.selectionDirection == "backward") {
BrowserInvisibleTextBoxViewExtension._exports.OnSelectionChanged(input.selectionEnd, input.selectionStart - input.selectionEnd);
} else {
BrowserInvisibleTextBoxViewExtension._exports.OnSelectionChanged(input.selectionStart, input.selectionEnd - input.selectionStart);
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:267
- An empty
compositionupdateis valid when an IME clears or cancels its preedit, butvalue.startsWith("", editEnd)is always true here. This returns the end of the value (or the suffix boundary) instead of the caret captured atcompositionstart, so the managedStartIndexand ended range can jump to the end of existing text. Preserve the composition-start caret for the empty-preedit case before using the diff.
// The preedit ends where the edited region ends, or starts where it starts.
const editEnd = value.length - suffix;
if (editEnd >= text.length && value.startsWith(text, editEnd - text.length)) {
return editEnd - text.length;
src/Uno.UI/UI/Xaml/Controls/TextBoxCore/TextBoxCore.IME.cs:156
syncFromInputinvokesOnCompositionUpdatedbefore syncing the input value into managed text. For an empty preedit,IsCompositionTextApplied("")is always true because the zero-length comparison succeeds, so this branch raisesTextCompositionChangedwhileTextstill has the old value during delete/cancel updates. Keep that update pending until the input value has been applied, then flush it after the sync so handlers never observe a stale composition range.
if (textAlreadyApplied && !IsCompositionTextApplied(compositionText))
{
_compositionChangedPending = true;
}
else
{
_host.RaiseTextCompositionChanged(new TextCompositionChangedEventArgs(_compositionStartIndex, _compositionLength));
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
483e25f to
10d0f4d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Two critical review findings remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:50
- The PR description says unmapped
event.codevalues fall back to the key name, but this change only classifies empty/Unidentifiedcodes as native soft-keyboard input. The keyboard bridge still passes non-empty codes directly toBrowserVirtualKeyHelper.FromCode, whose unknown-code path returnsVirtualKey.None, so that promised fallback is not implemented for other unmapped codes. Please add the fallback in the bridge (with coverage) or remove that claim from the PR description.
private static isSoftKeyboardKey(ev: KeyboardEvent): boolean {
return ev.keyCode === BrowserInvisibleTextBoxViewExtension.ANDROID_IME_KEYCODE || ev.code === "" || ev.code === "Unidentified";
}
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
10d0f4d to
1f0a23f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate unresolved synchronization issues remain in the browser TypeScript implementation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:161
- The hidden
<textarea>normalizes CR/CRLF to LF, but this records the pre-normalized managed string. On a multiline TextBox,compositionendtherefore sees a phantom value change even before the user edits anything. Initialize the synchronization marker from the actual DOM value.
BrowserInvisibleTextBoxViewExtension.lastSyncedValue = text;
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
1f0a23f to
6558e9c
Compare
|
@morning4coffe-dev - Addressed. The completion now follows the input event that puts the committed text in place, whichever side of it WebKit delivers |
There was a problem hiding this comment.
🟡 Changes recommended
Four unresolved moderate issues remain in browser composition and selection handling.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/UI/Xaml/Controls/TextBox/BrowserInvisibleTextBoxViewExtension.cs:50
- When the input change is rejected (for example by
BeforeTextChanging),UpdateTextFromNativehas already applied the input's post-edit selection throughSetPendingSelection; its unchanged-text path only queues the saved pre-input selection for later. Selectingcore.SelectionStarthere therefore re-applies the rejected caret immediately and leaves the hidden input at that position until the queued callback runs, contradicting the synchronous restoration expected byWhen_SoftKeyboard_Input_Rejected_Mid_Text. Restore the saved selection specifically for the unchanged-text rejection path rather than using the post-input core selection.
// The change was rejected or coerced and the TextBox's text written back to the input, whose
// caret then has to follow the TextBox's; an unchanged selection would not be pushed on its own.
core.TextBoxView.Select(core.SelectionStart, core.SelectionLength);
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:660
- When WebKit emits its standalone
deleteCompositionText,syncFromInputintentionally leaveslastSyncedValueat the preedit while the DOM value is already the restored text. If managed code then assigns that restored value, this equality check skipsendComposition();TextBoxCore.CancelCompositionOnExternalChangeonly ends the managed/IME extension state, not the JSisComposingflag. The later browser commit events can therefore be processed as a live composition and reapply stale text. Distinguish this case from the normal input-event echo (for example, usinglastSyncedValue) and close the JS composition even when the value write is a no-op.
if (input.value != inputText) {
// Replacing the value moves the caret to the end. Put it back right away rather than on the
// selectionchange the browser fires later: by then the IME may already be composing the next
// word, and a stale caret applied under it lands that word in the wrong place.
const { selectionStart, selectionEnd, selectionDirection } = input;
input.value = inputText;
BrowserInvisibleTextBoxViewExtension.lastSyncedValue = input.value;
input.setSelectionRange(selectionStart, selectionEnd, selectionDirection ?? "none");
// Notified last: a handler may set the text again, and its write must not be overwritten.
if (BrowserInvisibleTextBoxViewExtension.isComposing) {
BrowserInvisibleTextBoxViewExtension.endComposition();
}
}
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:659
- This synchronously re-enters managed code with
TextCompositionEndedwhile the outerTextBoxCore.OnTextChangedCoreis still processing the external/coerced text change. In the added shorter-coercion scenario, a composition at index 2 with length 1 can therefore raiseTextCompositionEndedwith range2+1whileTextis already empty, leaving handlers with an out-of-bounds composition range. Complete the native composition after the managed change has settled, or clamp/reset the range before raising the event.
// Notified last: a handler may set the text again, and its write must not be overwritten.
if (BrowserInvisibleTextBoxViewExtension.isComposing) {
BrowserInvisibleTextBoxViewExtension.endComposition();
}
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
665dec1 to
a80e267
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review details
Suppressed comments (2)
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:373
- Every composition input performs two full prefix/suffix scans over the entire input/base value, and the candidate checks below create additional
slicestrings. This makes ordinary IME typing over a large TextBox O(n) with several transient allocations per keystroke; please add a fast path for the previous/caret candidate or otherwise bound/reuse this search so long documents do not make typing progressively more expensive.
const maxCommon = Math.min(base.length, value.length);
let prefix = 0;
while (prefix < maxCommon && base[prefix] === value[prefix]) {
prefix++;
}
let suffix = 0;
while (suffix < maxCommon - prefix && base[base.length - 1 - suffix] === value[value.length - 1 - suffix]) {
suffix++;
}
src/Uno.UI/UI/Xaml/Controls/TextBoxCore/TextBoxCore.IME.cs:219
- On Android,
CompositionCompletedis raised from the editing-state callback beforeTextInputConnection.EndBatchEditcallsActiveTextBox.ProcessTextInput(seeAndroidImeTextBoxExtension.cs:18-23andTextInputConnection.cs:177-195). ThusText.Lengthhere can still be the previous preedit when the committed candidate has a different/longer length, so this clamp truncatesTextCompositionEndedEventArgsand handlers still observe stale text. Preserve the committed range or defer the completion until the platform text sync has run for this text-already-applied path.
_host.RaiseTextCompositionEnded(new TextCompositionEndedEventArgs(start, Math.Min(length, Text.Length - start)));
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
morning4coffe-dev
left a comment
There was a problem hiding this comment.
Re-reviewed a80e267d507b against my previous reviewed tree b818b9c5ae31. The commit series was replaced, but both trees have the same master base; the follow-up delta is four files, not a base merge.
The earlier WebKit completion-order blocker is addressed. I executed both exact TypeScript versions against real headless Edge DOM inputs with synthetic composition/input sequences and managed-export stand-ins: all 14 current-head cases passed, while the previous version failed six. Differing-candidate and cancellation completion now wait for the synchronized value; the cases also covered both positions of WebKit's separate removal event, the author's reported iOS insertion-before-end sequence, repeated preedit, a never-applied prefix commit, external replacement and detach. This confirms the earlier fix without claiming that Edge is Safari or that I reran the author's physical iPhone test.
One new shared-code regression still needs fixing before approval: the new unconditional completion-range clamp also runs on Android's text-already-applied path, whose completion callback actually precedes the managed text sync. I independently reproduced the concern noted in the latest automated review using the exact Android composition callback and EndBatchEdit methods plus the old/new TextBoxCore completion methods. With preedit ab x and committed value ab longer, the previous code reports start 3 / length 6; this head reports start 3 / length 1, then updates Text to ab longer without another completion. Equal-length candidates do not expose it.
The stale Text observed during Android completion already existed; the new regression is truncating the committed range against that stale value. Please preserve the range until the Android text sync has completed, or limit this defensive clamping to the external-change/coercion paths it is intended to protect, and cover a candidate longer than its preedit.
Validation scope: real browser DOM replay of exact JS with managed stand-ins, selected exact C# method execution with native editing-state/host stand-ins, and source comparison with WinUI's text-composition event contract. Not a full Uno WASM app, Android device run, or new native WinUI session. No source changes or other reviewers' thread states were made.
a80e267 to
b59a87e
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review details
Suppressed comments (1)
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:228
endComposition()invokes the managedCompositionEndedhandlers synchronously, and those handlers can detach or replace this input. If that happens, this callback still reinitializesisComposingand reportsCompositionStartedfor the now-stale element, leaving the new focused control out of sync with the browser. Re-check thatinputis still current after ending the previous session before starting the new one.
if (BrowserInvisibleTextBoxViewExtension.isComposing) {
BrowserInvisibleTextBoxViewExtension.endComposition();
}
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
morning4coffe-dev
left a comment
There was a problem hiding this comment.
Re-reviewed b59a87e56994 against a80e267d507b, with the same master base. The Android committed-range regression is fixed: normal completion now publishes committedText.Length directly, while the bounded helper remains for completion without a commit. ResetComposition clears the session state before invoking the event.
I reran the exact Android callback/EndBatchEdit and old/new shared completion methods with the same editing-state and host stand-ins. The longer-candidate case changes from the previous incorrect range 3+1 to 3+6, and the equal-length control remains 3+3. The cancellation/coercion helper still reports 0+0 when the text has been cleared. This restores the committed range without pretending the pre-existing Android callback-before-text-sync timing was also changed.
The TypeScript refactoring was also executed in a real headless Edge DOM using the same fourteen synthetic composition/input sequences and managed-export stand-ins: 14/14 passed for both this head and the previously reviewed WebKit fix. The differing-candidate/cancellation ordering, separate WebKit removal events, repeated preedit, never-applied commit, external replacement and detach behavior remain covered. I also read the test-helper consolidation and the added per-keystroke selection assertion.
Both findings I raised in the earlier reviews are now addressed. Validation scope is the exact C# callback comparison and real browser DOM replay, not a new physical Android/iPhone test or full Uno/WinUI runtime suite.
b59a87e to
e3bc40b
Compare
morning4coffe-dev
left a comment
There was a problem hiding this comment.
Follow-up on e3bc40b775fa, which arrived after my approval of b59a87e56994. The earlier WebKit completion-order and Android committed-range findings remain fixed. The new input-identity guard also fixes the reentrant focus handoff when the second control requires a different DOM element, as the added multiline test demonstrates.
A same-kind handoff still escapes that guard. I executed the exact previous/current TypeScript in a real headless Edge DOM, with a synchronous completion-export handler calling the actual bridge focus method for a second control. The previous revision incorrectly started a composition on the second control in both cases. This revision passes the replacement-input case, but two ordinary single-line TextBoxes reuse the same input: currentHandle changes from 1 to 2 while isCurrentInput(input) stays true, and the stale callback raises CompositionStarted for the second control and leaves isComposing=true.
The managed callback is synchronously reentrant: TextBox's ITextBoxHost.RaiseTextCompositionEnded directly invokes TextCompositionEnded. The input object is therefore not a sufficient identity for the owning TextBox. Please guard the owner/session generation across endComposition as well, and exercise the new test with a same-kind second control. A reviewer-only currentHandle guard made both real-DOM cases pass; no product code was modified.
Validation scope: exact JavaScript in real browser DOM with synthetic composition events and managed-export stand-ins, plus managed dispatch-source inspection. This is not a physical IME or complete Uno app run. My earlier approval applies to the earlier inspected commit; this latest-head review records the newly verified remaining case.
There was a problem hiding this comment.
🟡 Changes recommended
Address the critical stale-input routing issue and moderate composition-state reset issue.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/BrowserInvisibleTextBoxViewExtension.ts:144
focus()captureswasComposingand callsendComposition()after creating/replacing the hidden input, butcreateInput()resets the sharedisComposingflag before that call. Therefore a focus move during composition leaves the managedTextBoxCorecomposition open: the laterendComposition()is a no-op, and the old element'scompositionendis ignored as stale. The addedWhen_SoftKeyboard_Same_TextBox_Refocused_During_Compositionscenario should fail atAssert.IsFalse(SUT.IsComposing). Keep the browser composition state untilfocus()has notified the managed side (or explicitly notify before resetting it).
// A previous input may have been removed mid-composition without a compositionend;
// never carry that state over to a fresh element.
BrowserInvisibleTextBoxViewExtension.resetComposition();
src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_TextBox.BrowserSoftKeyboard.cs:1026
- This new comment is grammatically incomplete: “with the text handlers see” should use a gerund here so the sentence reads clearly.
// Records the composition events in order, with the text handlers see at that point.
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
Platforms whose native input applies the text itself may open a composition on text that already exists (an IME re-composing a committed word), so the caret position captured at CompositionStarted is not always where the preedit begins. ImeCompositionEventArgs.StartIndex lets the platform report it so the underline and composition events cover the right range. When such a platform reports that the text change of a preedit update is still to be synced, TextCompositionChanged is raised once that sync has landed, so handlers read the text the range refers to; the range is clamped to the text in case the sync coerced it shorter. TextCompositionEnded's range is clamped the same way when a composition ends without its commit (a text change from outside it, a coercion, Text set from code, may have left the text shorter than the range); a completed composition keeps the committed text's own range, which a platform may sync into the text only after the completion callback. The pending platform-apply flag is reset whenever a composition ends, so a preedit update that did not change the text cannot leave it armed. IsReadOnly changes are also forwarded to the view.
The hidden input's value is now the source of truth for the text: every input event syncs it to the TextBox, and composition events only report the preedit and where it sits in the value. Previously the composition path inserted the preedit itself and skipped the next input event after compositionend, assuming the browser fires the committed preedit's input event after compositionend. Chrome fires it before, so on Android the skipped event was the space or punctuation that committed the word, leaving the TextBox out of sync with the input: spaces disappeared, backspace appeared dead, and the next composition (which Gboard opens on the whole token) was spliced at the stale caret, duplicating the text. WebKit applies a commit through input events of its own, with no compositionupdate for the committed text: the preedit is removed, then the committed text is put in place (the insertion's data), and compositionend is delivered either after them (iOS, as observed on a device) or before them, with the preedit still in the input (selected, about to be replaced or, for a cancel, removed). Whichever the order, the completion follows the input event that puts the committed text in place, so TextCompositionChanged and the text change precede TextCompositionEnded and its handlers see the committed text rather than the preedit, as they do on Chrome (a commit equal to the preedit the input already holds needs no wait). The removal is reported together with what follows it, and the committed text put back where the preedit was is not reported as a change, so handlers do not see the composition emptied and refilled. A commit the browser never applies ends the composition with the text as it is at the next keystroke; a commit that merely prefixes a preedit still in the input has not landed. Soft keyboard keys (keyCode 229, or no usable code) are left to the browser so the IME's view of the text stays consistent; handling them from managed code rewrote the input's value mid-word. Text and caret changes that do not come from the input itself (a tap, the delete button, Text set from code, a value rejected by BeforeTextChanging or coerced) are applied to it even while a composition is open, ending the composition like a native app would (notified once the input holds the new text, so a handler's own text change is not overwritten); only the unchanged echo of our own sync is a no-op. Text is compared and written in the input's form, with LF line breaks, so a multiline TextBox's CR text does not read as a change to write back on every line break (which reset the IME and made Gboard pull the caret back to the end of the word). When the TextBox's text is written back that way, its selection (the one it had before a rejected input, or the coerced one) is pushed to the input in the same call rather than on a later dispatcher pass, so the input's caret never sits where a rejected edit left it. Selections the input already has are not re-applied, to avoid needless IME notifications, and selection changes in the textarea backing a multiline TextBox are reported like those of the input. The preedit is located in the value by diffing it against the value the composition started from. When the preedit repeats adjacent text, the start found for the previous update is kept while it still fits (the IME may move the caret inside the preedit), then the caret, which the IME leaves at the end of a preedit it just inserted, decides; an emptied preedit stays where it was, and a preedit no longer in the value counts as emptied. Replacing the input's value moves its caret to the end; it was put back on the selectionchange event the browser fires later, which could land once the keyboard was already composing the next word and drop that word in the wrong place. The caret is now restored right away instead. An input that focus() replaced (the element kind changed, or a composition was open) can still deliver the compositionend the browser fires when removing it; events from an input that is no longer the current one are ignored so they cannot land in the TextBox that owns the replacement. Every removal of the input (a replacement in focus(), a detach while the TextBox keeps focus for accessibility, the deferred detach after a blur) therefore ends a composition still open on it for the TextBox itself, since the same TextBox re-entering (a tap moving the caret) or handing over to its semantic element would otherwise never see it end. A composition still open at the next compositionstart (a commit the browser never applied) is ended before the new one starts. A handler run from an event may move focus, replacing the input or handing the same element over to another TextBox, so events check the TextBox they were for as well as the element: neither that new composition nor the rest of an update lands in the TextBox that took the focus. The selection an input event reports is applied by the text change itself, so the caret no longer passes through the start of the text on every keystroke (which raised SelectionChanged twice and, during a composition, briefly moved the input's caret out of the preedit). The hidden input mirrors IsReadOnly so the browser refuses soft keyboard input for a read-only TextBox instead of syncing it into the text.
Replays the browser's composition and key event sequences on the hidden input: Chrome's commit ordering and WebKit's (a commit equal to the preedit, a candidate differing from it, and a cancel, with compositionend delivered before the input events that apply them, with the preedit still in the input or already removed, or after them as on iOS, also for a tapped candidate applied as the preedit first and for a preedit repeating the text before it, and a commit the browser never applies, also one prefixing the preedit), a composition re-opened on existing text, a preedit repeating adjacent text (also with the caret moved inside it) or emptied again, the caret moved and the text cleared while a composition is open (including the browser's late selectionchange for the replaced value), a preedit rejected by BeforeTextChanging (also in the middle of the text) or coerced shorter than its start, text set by a TextCompositionEnded handler, focus moved to a TextBox of the same or another kind by a TextCompositionChanged handler mid-update or by a TextCompositionEnded handler at the next compositionstart, a compositionend from an input that focus() replaced or from a composition managed code already ended, the same TextBox re-entered with pointer focus mid-composition, the input detached for accessibility mid-composition, a single SelectionChanged per keystroke, a read-only TextBox (also made read-only mid-composition), a multiline TextBox following the textarea's caret and taking a line break mid-composition, and Backspace without a code in a TextBox and a PasswordBox.
e3bc40b to
4244ded
Compare
morning4coffe-dev
left a comment
There was a problem hiding this comment.
Re-reviewed 4244ded69456 against e3bc40b775fa. The focus-owner finding is addressed: the bridge captures currentHandle before synchronous callbacks and checks it as well as the DOM input identity afterward. This covers the same-element retarget path, not only input-to-textarea replacement, and the owner check is carried through compositionend and the input-sync completion path as well.
I reran the exact old/new TypeScript in a real headless Edge DOM. Both owner-handoff cases pass at this head, including the previously failing two-single-line-TextBox case; the previous head passes only replacement and still produces the spurious second-control composition when the element is reused. The original fourteen composition-order/lifecycle cases also remain 14/14. The added runtime tests now parameterize the second control's AcceptsReturn value and assert that its text/composition state stays untouched.
The shared TextBoxCore IME source is byte-identical to the revision where the Android committed-range fix was verified, so that correction is retained. The earlier WebKit ordering cases remain covered by the browser replay. The reported focus-owner thread was already resolved when I checked it; I did not change another reviewer's thread state.
All blocking findings I raised are addressed at this inspected commit. Evidence scope: real browser DOM with synthetic events and managed-export stand-ins, exact-source comparison and the previously recorded callback checks, not a new physical IME/device or full Uno/WinUI runtime-suite run.
|
@morning4coffe-dev Any chance this will/can get a backport to a |
|
@mikernet I think that should be possible, cc @agneszitte / @ajpinedam for more details. |
GitHub Issue: closes #22230
PR Type:
🐞 Bugfix
What changed? 🚀
On Skia WebAssembly, soft keyboards on Android browsers type through IME composition on the hidden native input and report key events without a code. Several things went wrong with that:
The composition path inserted the preedit itself and skipped the next input event after compositionend, assuming the browser fires the committed preedit's input event after compositionend (Safari's order). Chrome fires it before, so the skipped event was the space or punctuation that committed the word. The TextBox and the hidden input drifted apart: spaces disappeared, backspace looked dead, and when Gboard re-opened the composition on the whole token the preedit was spliced at the stale caret, duplicating text.
Keys were mapped from
event.codeonly, and soft keyboards send an empty code, so Backspace mapped toVirtualKey.Noneafter the keydown had already been prevented. Backspace never worked in a PasswordBox (no composition there) and stopped working in a TextBox outside a composition. Such keys are now left to the browser and synced from the input event instead.Managed text and caret changes were ignored while a composition was open, and the keyboard keeps one open for the whole word being typed: tapping elsewhere did not move the caret the keyboard inserts at, and clearing the text brought it back on the next keystroke. Replacing the input's value also restored the caret on a later selectionchange, which could land under the keyboard's next composition.
This PR makes the hidden input's value the source of truth for the text: every input event syncs it to the TextBox (through the
TextAlreadyAppliedmode the Android extension already uses), and composition events only report the preedit and where it sits in the value.ImeCompositionEventArgsgains aStartIndexso platforms can report where a composition begins when it covers existing text. Soft keyboard keys are left to the browser and synced from the input event. Taps, the delete button andTextset from code reach the hidden input even during a composition, ending it as a native app would, and the caret is restored synchronously when the value is replaced.WebKit applies a commit through input events of its own, with no
compositionupdatefor the committed text, and deliverscompositionendeither after them (iOS, verified on a device) or before them. The completion now follows the input event that puts the committed text in place, soTextCompositionChangedand the text change precedeTextCompositionEndedand its handlers see the committed text, as they do on Chrome and as WinUI documents the order. A commit the browser never applies ends the composition with the text as-is at the next keystroke.TextCompositionEndedreports its range clamped to the text as it is when a composition ends without its commit, so a coercion orTextset from a handler cannot leave it out of bounds.Validated with new Skia WASM runtime tests that replay Chrome's and WebKit's composition orderings (a commit equal to the preedit, a candidate differing from it and a cancel, with
compositionenddelivered before or after the input events that apply them), a composition re-opened on existing text, the caret moved and the text cleared while composing, and Backspace without a code in a TextBox and a PasswordBox. The existingGiven_TextBoxIME tests pass on WASM and Skia Desktop, and the fix was verified on an Android phone with Gboard.I did extensive manual testing (iOS Safari and Android Chrome) with all sorts of input sequences, clears, cuts/pastes, bindings, BeforeTextChanging, and multiline inputs, and it appears to be working correctly as far as I can tell.
PR Checklist ✅
Screenshots Compare Test Runresults.