[web] Refactor inner implementation of web input#2838
[web] Refactor inner implementation of web input#2838
Conversation
Following changes are introduced: * Reschedule checkpoint after successful NativeInputEventsProcessor::runCheckpoint run * Reduce amount of expect/actual implementations (in favour of using entities in webMain) * ComposeCommandCommunicator::sendKeyboardEvent return boolean which indicates whethre compose actually processed event (this logic used for finer Backspace resolution)
There was a problem hiding this comment.
Pull request overview
Refactors the web text input pipeline to reduce platform-specific implementations, improve checkpoint scheduling behavior, and make keyboard event handling aware of whether Compose actually consumed an event (for more accurate Backspace resolution).
Changes:
- Updates web text input startup to pass a
PlatformTextInputMethodRequestthrough to the web service implementation. - Refactors native DOM event processing (checkpointing + Backspace/beforeinput interplay) and removes timestamp interop expect/actual utilities.
- Changes
ComposeCommandCommunicator.sendKeyboardEventto return aBooleanindicating whether Compose handled the event, and adapts call sites/tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| compose/ui/ui/src/webTest/kotlin/androidx/compose/ui/platform/NativeInputEventsProcessorTest.kt | Updates mock communicator to return Boolean from sendKeyboardEvent. |
| compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/window/WebTextInputSession.kt | Switches to starting input via webTextInputService.startInput(request). |
| compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/platform/WebTextInputService.kt | Introduces request-based startInput and adapts communicator keyboard event return type. |
| compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/platform/NativeInputEventsProcessor.kt | Refactors event processing and uses keyboard-event consumption result to refine beforeinput Backspace handling. |
| compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/platform/DomInputStrategy.kt | Simplifies input event interop and tightens “Compose-only” key handling in keydown. |
| compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/platform/BackingDomInput.kt | Updates communicator API to return Boolean from sendKeyboardEvent. |
| compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/internal/jsinterop/JsInteropUtils.kt | Removes timestamp expect/actual utilities. |
| compose/ui/ui/src/wasmJsMain/kotlin/androidx/compose/ui/platform/DomInputStrategy.wasmJs.kt | Removes wasmJs actual for asInputEventExt. |
| compose/ui/ui/src/wasmJsMain/kotlin/androidx/compose/ui/internal/jsinterop/JsInteropUtils.wasmJs.kt | Removes wasmJs actual timestamp utilities. |
| compose/ui/ui/src/jsMain/kotlin/androidx/compose/ui/platform/DomInputStrategy.js.kt | Removes js actual for asInputEventExt. |
| compose/ui/ui/src/jsMain/kotlin/androidx/compose/ui/internal/jsinterop/JsInteropUtils.js.kt | Removes js actual timestamp utilities. |
| compose/foundation/foundation/src/webMain/kotlin/androidx/compose/foundation/text/TextFieldKeyInput.web.kt | Refactors typed-event detection to rely on the DOM keyboard event flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/platform/NativeInputEventsProcessor.kt
Show resolved
Hide resolved
| isInIMEComposition = false | ||
| collectedEvents.clear() | ||
| isCheckpointScheduled = true | ||
| } |
There was a problem hiding this comment.
At the end of runCheckpoint() the code sets isCheckpointScheduled = true after clearing events. With the current scheduling logic this can prevent future checkpoints from being scheduled after this run. Typically the flag should be cleared when the scheduled checkpoint starts running, and only set when a new checkpoint is actually scheduled (not unconditionally at the end of processing).
There was a problem hiding this comment.
Setting isCheckpointScheduled = true is not correct.
It should be set in:
if (!isCheckpointScheduled) {
scheduleCheckpoint()
// here:
isCheckpointScheduled = true
}
There was a problem hiding this comment.
Are you sure? I might be missing something - mine approach is most likely wrong - but this way - won't we end up with immidiately resetting checkpoing while in scheduleCheckpoint something is still runinng in requestAnimationFrame?
There was a problem hiding this comment.
the idea of setting isCheckpointScheduled = true in
if (!isCheckpointScheduled) {
scheduleCheckpoint()
isCheckpointScheduled = true
}
is to avoid having more than one call to scheduleCheckpoint until the checkpoint is executed.
It should be reset back to false isCheckpointScheduled = false in fun runCheckpoint(currentTextFieldValue: TextFieldValue)
| class ComposeKeyDownStatus( | ||
| val event: KeyboardEvent, | ||
| val processed: Boolean | ||
| ) |
There was a problem hiding this comment.
ComposeKeyDownStatus is declared as a public top-level class but is only used inside NativeInputEventsProcessor.kt. Making it public expands the module’s API surface unnecessarily; it should be private (or at most internal) and ideally a private data class if it’s just a value holder.
| override fun startInput( | ||
| value: TextFieldValue, | ||
| imeOptions: ImeOptions, | ||
| onEditCommand: (List<EditCommand>) -> Unit, | ||
| onImeActionPerformed: (ImeAction) -> Unit | ||
| ) { | ||
| // This method is not used in the new API, but we keep it for backward compatibility. | ||
| } |
There was a problem hiding this comment.
The overridden startInput(value, imeOptions, onEditCommand, onImeActionPerformed) is currently a no-op, which breaks the PlatformTextInputService contract for any code path still using the legacy API. If this is truly kept for backward compatibility, it should delegate to the new startInput(request) (by adapting the parameters) or preserve the previous behavior instead of doing nothing.
| lastKeydownStatus = ComposeKeyDownStatus( | ||
| evt, | ||
| composeSender.sendKeyboardEvent(evt.toComposeEvent()) | ||
| ) |
There was a problem hiding this comment.
New behavior relies on the boolean result of composeSender.sendKeyboardEvent(...) (stored in lastKeydownStatus) to decide whether to ignore a subsequent deleteContentBackward. There isn’t a test covering the processed = false path (i.e., when Compose doesn’t consume Backspace and the beforeinput should still generate commands). Add a unit test with a communicator that returns false from sendKeyboardEvent and assert that deleteContentBackward is handled.
| } | ||
| }) | ||
|
|
||
| htmlInput.addEventListener("compositionstart", { evt -> |
There was a problem hiding this comment.
isInIMEComposition includes compositionstart, so I think this event listener should remain here?
There was a problem hiding this comment.
updated this logic - basically, we need to register this event but don't care about any handlers
…i:compileWebMainKotlinMetadata
…rocessing compositionstart and compositionupdate
| get() = isTypedEvent(this) | ||
|
|
||
| private fun isTypedEvent(evt: KeyboardEvent): Boolean = | ||
| js("!evt.metaKey && !evt.ctrlKey && evt.key.charAt(0) === evt.key") |
There was a problem hiding this comment.
So now we have 3 different declarations.
We can't avoid implementing the expect/actual and we can't avoid extracting the js calls into a separate function.
But do we need private val KeyboardEvent.isTypedEvent: Boolean?
I think 2 declarations would be enough. WDYT?
| // we need to reset this each time we consider something to be typed | ||
| // see https://youtrack.jetbrains.com/issue/CMP-8773 | ||
| lastProcessedEventIsBackspace = evt.key == "Backspace" | ||
| lastKeydownStatus = null |
There was a problem hiding this comment.
lastProcessedEventIsBackspace = evt.key == "Backspace" has been added here #2559 as a fix for iOS.
But now lastProcessedEventIsBackspace = evt.key == "Backspace" is removed. Won't it cause a regression on iOS?
| val editCommands = buildList { | ||
| when (inputType) { | ||
| "deleteContentBackward" -> { | ||
| if (lastKeydownStatus?.getProcessedEvent()?.isBackspace() == true) return@buildList |
There was a problem hiding this comment.
I think the simple bolean lastProcessedEventIsBackspace check looked more readable than the chain of nullable calls with the same result.
Do we need lastKeydownStatus for anything else or only for this specific check?
There was a problem hiding this comment.
Simple Boolean lastProcessedEventIsBackspace does not take into account that compose indeed processed this event
There was a problem hiding this comment.
but it can take it into account if it's set to true only when compose actually handled it, right?
Following changes are introduced:
Testing
manual and
gradlew testWebRelease Notes
N/A