Repository navigation
fix(studio): respect composition and cancellation in text inputs - #6011
Conversation
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Thank you, this is a careful fix and the captures helped. I traced the draft lifecycle: Escape clears the dirty flag and restores the value, so the blur no longer writes back what was just discarded, and the Enter path no longer fires onCommit and onTrack twice. Gating blur on the dirty flag changes nothing for the untouched field case, and the new blur without Enter test is a good positive control.
One note for a follow up, not for this PR: NumberField has the same two gaps, it commits on Enter with no composition guard and commits on blur unconditionally. Only the mergeability and WIP checks have run on this head, so the full suite still needs a run before merge.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approving, agreeing with Miguel's approval at this head.
Both bugs are real on main in packages/studio/src/components/ui/Input.tsx. The Escape branch calls setDraft(valueRef.current) and then blur() in the same handler, so the blur runs with the old draft from that render and commits the value the user just cancelled. Enter has the mirror problem: commit clears the dirty flag, but valueRef doesn't change until the parent re-renders, so the blur that follows passes the next === valueRef.current check and fires onCommit and onTrack a second time. Gating blur on dirtyRef fixes both without touching the plain type-then-blur path, which the new positive test covers.
One small thing for a follow-up, not this PR: Safari ends the composition before it sends the confirming Enter, so isComposing is already false there and that Enter will still commit early. Editors usually also check keyCode === 229 for that case. Miguel's note that NumberField has the same two gaps is worth picking up in the same follow-up.
Need: yes, Escape writes a discarded value and Enter double-commits · Size: ok · Reuse: uses the field's existing dirty flag, nothing new · Simpler: already minimal
— Rames
|
Thanks, @shuaiyafei. This one is approved, and it'll go into the merge queue once these are sorted:
Once it's green and signed, we'll queue it at your new head. If you push code changes beyond a rebase, we'll take another look first. — Rames |
|
This is approved and the tests guard the change: with |
Head branch was pushed to by a user without write access
7865f32 to
c547235
Compare
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-approving after the rebase: the patch is unchanged from the version approved on 10-24.
What
Let the shared
Inputfinish an IME composition without treating its Enter/Escape keys as editor commands. Finalize a draft once: Enter followed by blur commits once, and Escape followed by blur commits nothing.Why
The keyboard handler currently intercepts composing key events. Separately, Enter or Escape clears the dirty flag, but blur still commits the captured draft unconditionally. A focused Escape therefore writes the value it just discarded; Enter can notify the caller and tracking callback twice while the parent is still persisting the value.
Related work
Self-contained correction to the shared text
Input. #5947 changesNumberFieldbounds and #4710 changesSelect; they also usevalueControls.test.tsx.How
Return early for composing key events and let blur commit only a dirty draft. This uses the field's existing draft lifecycle and dirty flag.
Before
After
These captures render the actual React component in Chromium. Each row records a fresh-page keyboard sequence; composition events are dispatched explicitly, not generated by an operating-system IME.
Test plan
Four regression cases fail on unchanged source: composing Enter/Escape, Enter then blur, and Escape then blur.
The value-control file now passes 20 tests, including ordinary blur as a positive control. The UI directory passes 68 tests.
Browser assertions reproduce all four failures on the upstream component and pass with the patch, checking committed values, raw tracking-call counts, and whether the key was prevented.
Workspace build, updated Studio build, oxlint/oxfmt, and the full pre-commit and commit-msg hooks passed.
Unit tests added
Browser reproduction performed, with before/after captures
Comment rules reviewed
AI assistance: Codex.
Rebase update — October 28, 2026
Rebased onto
mainat8b1f88b3, as requested. The approved patch's added and removed lines are unchanged. The new head isc547235b, with a GitHub-verified signature.Fresh local validation for this rebased tree:
packages/studio/src/components/ui: 71 tests across 8 files pass.git diff --checkpass.The earlier validation section describes the pre-rebase revision. Upstream CI and Windows results must be checked at the new head.
At this update, the fork workflow runs are awaiting maintainer approval. Auto-merge is currently disabled; GitHub returned
FORBIDDENwhen the author account attempted to restore it, so a maintainer needs to re-enable it.