[web] backing fields for inputs are actually contenteditable dom and spans#3167
[web] backing fields for inputs are actually contenteditable dom and spans#3167Shagen Ogandzhanian (Schahen) wants to merge 17 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Compose Web text input so the backing DOM elements are contenteditable span/div nodes (instead of input/textarea), aligning DOM selection/input handling with the new strategy and updating the affected Web tests and styling.
Changes:
- Replace backing field DOM elements with contenteditable
span(single-line) anddiv(multi-line), and update selection tracking to use the DOMSelectionAPI. - Adjust input event processing to use
textRangeCollapsedand centralize selection-command creation. - Update Web tests and backing-field CSS to target the new
.compose-backing-fieldelements and expected behavior.
Reviewed changes
Copilot reviewed 8 out of 8 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/input/TextFieldFocusTest.kt | Updates single-line backing field lookup from input to span.compose-backing-field. |
| compose/ui/ui/src/webTest/kotlin/androidx/compose/ui/input/specs/TextFieldTestSpec.kt | Updates test helpers to use div.compose-backing-field instead of textarea. |
| compose/ui/ui/src/webTest/kotlin/androidx/compose/ui/input/specs/CompositeInputTestSpec.kt | Updates composite input test to assert backing field is a div. |
| compose/ui/ui/src/webTest/kotlin/androidx/compose/ui/input/MouseTextInputTests.kt | Updates hit-testing to use the new backing div.compose-backing-field. |
| compose/ui/ui/src/webTest/kotlin/androidx/compose/ui/input/ExternalSelectionChangeListenerTest.kt | Updates selection-change test to use div backing field and new selection-range helper. |
| compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/window/ComposeWindow.web.kt | Adjusts backing-field CSS (overflow/white-space) for contenteditable behavior. |
| compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/platform/NativeInputEventsProcessor.kt | Updates selection deletion/insert logic to use textRangeCollapsed and a selection helper. |
| compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/platform/DomInputStrategy.kt | Core migration to contenteditable backing fields and Selection API-based range handling. |
Comments suppressed due to low confidence (1)
compose/ui/ui/src/webMain/kotlin/androidx/compose/ui/platform/DomInputStrategy.kt:89
beforeinputrelies onlatestSelection, butlatestSelectionis never updated when Compose programmatically sets selection viaupdateState(). If aselectionchangeevent doesn’t arrive (or is skipped due topauseSelectionChangeListener),textRangeStart/textRangeEndcan become stale and input events may be processed with incorrect selection offsets. UpdatelatestSelectionwhen callingsetSelectionRange()inupdateState().
if (needsTextUpdate || needsSelectionUpdate) {
pauseSelectionChangeListener = true
htmlInput.setSelectionRange(textFieldValue.selection.min, textFieldValue.selection.max)
pauseSelectionChangeListener = false
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!textRangeCollapsed) { | ||
| add(SetSelectionCommand(textRangeStart, textRangeEnd)) | ||
| } |
| val backintInput = getShadowRoot().querySelector("div.compose-backing-field") | ||
| assertIs<HTMLDivElement>(backintInput) | ||
|
|
||
| val textAreaRect = textArea.getBoundingClientRect() | ||
| val textAreaRect = backintInput.getBoundingClientRect() |
| import androidx.compose.ui.OnCanvasTests | ||
| import androidx.compose.ui.WebApplicationScope | ||
| import androidx.compose.ui.focus.FocusRequester | ||
| import androidx.compose.ui.focus.focusRequester | ||
| import androidx.compose.ui.platform.setSelectionRange |
…typoef selection where it's possible
| if (needsTextUpdate || needsSelectionUpdate) { | ||
| pauseSelectionChangeListener = true | ||
| htmlInput.setSelectionRange(textFieldValue.selection.min, textFieldValue.selection.max) | ||
| setSelectionRange(htmlInput,textFieldValue.selection.min, textFieldValue.selection.max) | ||
| pauseSelectionChangeListener = false | ||
| } |
| } | ||
| }) | ||
|
|
||
| htmlInput.addEventListener("compositionstart", {evt -> |
| if (!textRangeCollapsed) { | ||
| add(SetSelectionCommand(textRangeStart, textRangeEnd)) | ||
| } |
…insertCompositionText beforeinput events
|
Could you please add the motivation of this change in the PR description so it will be easier to recall it when viewing the commits history later? As I understood, we want to use |
| val inputExt = evt.asInputEventExt() | ||
| inputExt.textRangeStart = htmlInput.selectionStart | ||
| inputExt.textRangeEnd = htmlInput.selectionEnd | ||
|
|
||
| inputExt.firstRange = inputExt.getTargetRanges()[0] | ||
|
|
||
| nativeInputEventsProcessor.registerEvent(evt) |
| "deleteWordBackward" -> buildList { | ||
| if (lastProcessedKeydown?.isBackspace() != true) return@buildList | ||
|
|
||
| // This would mean event was triggered by long press on mobile device (iOS) | ||
| if (lastProcessedKeydown?.repeat == true) { | ||
| val layoutResult = composeSender.currentTextLayoutResult() ?: return@buildList | ||
|
|
||
|
|
||
| val offset = layoutResult.getPrevWordOffset(textRangeEnd) | ||
| val deleteCommand = DeleteSurroundingTextCommand((textRangeEnd - offset).coerceAtLeast(0), 0) | ||
| add(deleteCommand) | ||
| firstRange?.let { targetRange -> | ||
| add( | ||
| DeleteSurroundingTextCommand( | ||
| targetRange.endOffset - targetRange.startOffset, |
|
|
||
| val needsTextUpdate = lastMeaningfulUpdate.text != textFieldValue.text | ||
| val needsSelectionUpdate = lastMeaningfulUpdate.selection != textFieldValue.selection | ||
| val needsTextUpdate = (lastMeaningfulUpdate.text != textFieldValue.text) && !isInCompositionMode |
There was a problem hiding this comment.
since isInCompositionMode is a simple boolean, checking it first is a bit better so most of the times comparing the text values will be skipped.
UPD: " most of the times" is not true, but sometimes
There was a problem hiding this comment.
btw, why is additional check for !isInCompositionMode necessary?
| private fun isTypedEvent(evt: KeyboardEvent): Boolean = | ||
| js("!evt.metaKey && !evt.ctrlKey && evt.key.charAt(0) === evt.key") | ||
|
|
||
| internal fun isModifyingEvent(evt: KeyboardEvent): Boolean { |
There was a problem hiding this comment.
wdyt about making this entire function implemented in js?
Looks like isTypedEvent is used only here, so isTypedEvent can be changed to isModifyingEvent and include the check for Backspace?
|
|
||
| // Resetting `pauseSelectionChangeListener` synchronously right after is not enough | ||
| // TODO: this is the cheapest way to make sure that DOM <=> Compose sync won't self-trigger but we need to consider better possible options | ||
| window.requestAnimationFrame { |
There was a problem hiding this comment.
Looks like this paragraph in the documentation explains this - https://www.w3.org/TR/selection-api/#scheduling-selectionchange-event
To schedule a selectionchange event on a node target, run these steps:
If target's has scheduled selectionchange event is true, abort these steps.
Set target's has scheduled selectionchange event to true.
Queue a task on the user interaction task source to fire a selectionchange event on target.
So the selectionchange event listeners do not run synchronously.
Could you please leave the link to the w3c documentation?
| text-shadow: none; | ||
| user-select: none; | ||
| white-space: pre; | ||
| white-space: break-spaces; |
There was a problem hiding this comment.
why was this change necessary?
| ).sendToHtmlInput() | ||
|
|
||
| textFieldValue.awaitAndAssertTextEquals("홀", "hangul second time") | ||
| //TODO: this is disabled because this test is a lie - I've check manually dozens of time on different devices and the sequence of events matches, as well as the exptected result |
There was a problem hiding this comment.
what exactly is a lie here? Does the assert fail now?
The goal of this PR is to use as backing DOM entities divs and spans (rather than textareas and inputs)
Currently implemented logic of input processing in Compose Web conceptually is following:
Don't process keyboard typed event
We do not process (and by processing we mean sending keyboard event to the compose scene via scene.sendKeyEvent(keyEvent))
any keyboard event that we considered to be a so-called typed event, that is, an event that in a html context will lead for
modification of the text for introducing new symbols
Don't process keyboard in composite mode (in a wide sense)
We do not process any keyboard event that we do believe happened when composite input or, say, accent dialogue is open.
All modern browsers simply lacks the API that will help us to control or at leas monitor events inside such dialogues, we
don't even know for sure whether they are opened or not
Each compose input has a dom counterpart on which we are listening keyboard events
It's very important nuance: we can not delegate events to the native DOM element, in order to process events
in such element we are supposed to be focused (in a browser document sense, not in Compose sense) in the DOM element.
We call such element backing input node
We never sync state of backing input node in the direction of the state of corresponding Compose Input directly
Insted we rely on bunch of js beforeinput events (as of now, "deleteContentBackward", "deleteWordBackward", "insertReplacementText", "insertText"
and "insertCompositionText" to be precise) from wich we are deducing what Compose Edit commands should be created in order to be send to the Compose scene.
We trying to deduce how exactly the backing input node was affected to resolve relevant params we need to pass to the edit commands
We always sync selection and text state from Compose input to the backing DOM field
In both cases there are guards that prevent us from resetting text and selection states when they are already equivalent and thus, synced.
Problem we have with this approach
As long as we've introduced this approach, the major source of bugs and issues we've encountered was related to deducing the correct params from the
input events. The thing is that input events (in most cases) in theory contain all the information that we need to mimic it precisely, that is, create Edit Commands
that we will send to the scene and, after being applied, will lead to syncing ot the texts states.
In textareas and inputs, however, one very important part of such information is missing - we don't now the range of text currently affected byt this changes.
Examples
when insertText or insertReplacementText happens, targetRange contains information on what text should be replaced, in current approach we just trying to guess
what is happening based on what is the current selection and whether we, for instance, pressed Backspace
when deleteContentBackward or deleteWordBackward is happening we compute the borders of the words via standard Compose methods, relying on the fact that
our current cursor position is correct.
What is the problem with such approach
There's a practical problem with this approach that we need to take into consideration and retest thoroughly inputs in multiple brtowsers and on multiple devices after any
such fix. But, most importantly, this will be never enough - what we've learned from mobile devices that there can be a lot of very special cases
when particular input events happens (and affects the actual output) but we just don't know.
What this PR introduces
Testing
manual +
./gradlew testWebRelease Notes
N/A