fix: DH-22684: ui.text_field preserves user input while focused#1350
Open
mofojed wants to merge 1 commit into
Open
fix: DH-22684: ui.text_field preserves user input while focused#1350mofojed wants to merge 1 commit into
mofojed wants to merge 1 commit into
Conversation
- Do not overwrite the input value with server-pushed propValue while the text field is focused; sync on focus->blur transition instead. - Stop passing defaultValue as the controlled value when no value prop is provided. - Add current input value to the serialized FocusEvent payload so on_focus / on_blur handlers can read what the user typed. - Add e2e test covering the laggy-on_change focused-typing scenario, focus and change events.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses DH-22684 by preventing ui.text_field from overwriting in-progress user typing while the field is focused, and by enriching focus/blur event payloads so server-side handlers can read the current input value.
Changes:
- Add focus-tracking + updated debounced value syncing so server-pushed
valueupdates are deferred until blur. - Extend serialized focus events to include the current input element
value. - Add Playwright e2e coverage (including a slow server
on_changescenario) and a test app panel to exercise focus/change/blur payloads.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils.ts | Type-only import cleanup for Playwright types. |
| tests/ui_text_field.spec.ts | New e2e coverage for focused typing + focus/change/blur payload assertions. |
| tests/app.d/ui_text_field.py | Adds test panels used by the new e2e spec (slow on_change + event logging). |
| tests/app.d/tests.app | Registers the new test app panel module. |
| plugins/ui/src/js/src/elements/hooks/useTextInputProps.ts | Tracks focus state and passes it into debounced change handling. |
| plugins/ui/src/js/src/elements/hooks/useFocusEventCallback.ts | Serializes current target value into focus/blur events when available. |
| plugins/ui/src/js/src/elements/hooks/useDebouncedOnChange.ts | Skips server→UI value sync while focused; syncs on blur transition. |
| plugins/ui/src/deephaven/ui/components/types/events.py | Updates Python FocusEvent typing to include value. |
Comments suppressed due to low confidence (1)
plugins/ui/src/js/src/elements/hooks/useTextInputProps.ts:51
useTextInputPropsnow callsuseDebouncedOnChange(propValue, ...)but still always returns avalueprop. WhenpropValueis omitted (uncontrolled usage with onlydefaultValue), the hook initializesvaluetoundefinedand then sets it to a string on first user edit, which causes an uncontrolled→controlled transition (React warning) and effectively reintroduces local control of the field. Consider branching: ifpropValueisundefined, omitvaluefrom the returned props (and avoid storing localvaluestate), while still wiringonChangeto call the debounced server callback; only provide a controlledvaluewhenpropValueis defined.
const [value, onChange] = useDebouncedOnChange(
propValue,
propOnChange,
isFocused
);
return {
defaultValue,
value,
onChange,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+18
to
33
| // Update local value to match a new propValue from the server when no user | ||
| // changes are queued. Skip while the input is focused so we don't change the | ||
| // value out from under the user while they are typing. When the input loses | ||
| // focus, sync to the latest propValue even if it didn't change on this | ||
| // render. | ||
| const justBlurred = prevIsFocused === true && isFocused === false; | ||
| const propValueChanged = propValue !== prevPropValue; | ||
| if ( | ||
| propValue !== prevPropValue && | ||
| (propValueChanged || justBlurred) && | ||
| propValue !== value && | ||
| propValue !== undefined && | ||
| !pending | ||
| !pending && | ||
| !isFocused | ||
| ) { | ||
| setValue(propValue); | ||
| } |
Comment on lines
18
to
+25
| The name of the related target element of the focus event. | ||
| """ | ||
|
|
||
| value: str | None | ||
| """ | ||
| The current `value` of the underlying input element, when available | ||
| (e.g. for text fields, text areas, search fields, number fields). | ||
| """ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
text field is focused; sync on focus->blur transition instead.
provided.
/ on_blur handlers can read what the user typed.