fix(BottomSheet): guard standalone Escape against IME composition - #5322
Open
AKnassa wants to merge 2 commits into
Open
fix(BottomSheet): guard standalone Escape against IME composition#5322AKnassa wants to merge 2 commits into
AKnassa wants to merge 2 commits into
Conversation
The standalone sheet's Escape handler read a bare `event.key === 'Escape'`, so the keydown an IME fires to cancel an in-progress composition — which arrives before `compositionend` — dismissed the sheet instead. A CJK user composing in a `purpose="form"` field, or in the mobile-keyboard `height="tall"` case, lost the whole sheet on the keystroke that was only meant to cancel a candidate. `handleKeyDown` now early-returns on `isImeKeyEvent`, the guard `Dialog` and the sibling `BottomSheetSwitcher` already carry. It claims the key with `preventDefault` first, before deciding what the key means: an unclaimed Escape lets the browser raise its own close request, which arrives at `handleCancel` and dismisses on the same keypress. `handleCancel` itself needs no guard, and gets a comment saying why — `cancel` is a plain Event with no composition state to read, so an `isImeKeyEvent` call there could never return true. Two regression tests, mirroring the switcher's: both IME signals (`isComposing`, and the legacy keyCode 229) leave `onOpenChange` untouched, and the composing Escape is claimed. Both were red before the fix; a mutation to the naive early-return-before-preventDefault shape kills the second one on its own. Component Audit Rubric v1.6 §9 I19. Fixes facebook#5302
Four behaviors the first pass left unpinned, each proven to catch a real
regression rather than passing by accident:
- A composing Escape raised on a focused field inside a `purpose="form"`
sheet — the exact path the report names — has to survive the bubble to
the `<dialog>` and leave the sheet open.
- The guard releases: the Escape that follows a cancelled composition
still dismisses. A guard that over-blocks would trap the user in the
sheet, which is a worse bug than the one being fixed.
- A non-modal sheet (`hasScrim={false}`) ignores it too. There is no
native close request behind that one, so `handleKeyDown` is the whole
dismissal route and the only thing standing in the way.
- Enter and ArrowDown mid-composition are left unclaimed. They commit a
candidate and walk the candidate window; claiming them would take keys
the IME owns.
Red proofs, since the fix already landed in the previous commit. Reverting
`handleKeyDown` to its pre-fix unguarded shape turns the first three red on
their behavior assertions (`onOpenChange` called once, when it must not be
at all) and leaves the fourth green. Hoisting `preventDefault` above the
`key !== 'Escape'` check turns only the fourth red. So no test here passes
vacuously, and the two halves of the fix are pinned separately.
BottomSheet suite 77/77.
Part of facebook#5302
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
community
AKnassa
marked this pull request as ready for review
August 22, 2026 03:48
Contributor
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR Modified ComponentsBottomSheet (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
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
1 participant
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.
What this does
A bottom sheet no longer closes when someone typing Korean, Japanese or Chinese presses Escape to back out of a half-typed word.
Why
While an IME is composing, the browser sends an Escape keydown to cancel the pending characters, and it sends it before
compositionend. The standalone sheet read that as a plain "close me" command, so a CJK user correcting a word inside a sheet lost the whole sheet, along with anything already typed into it. The documentedpurpose="form"andheight="tall"mobile-keyboard cases both put a text field in a standalone sheet, so this is an ordinary path rather than a corner.The base
Dialogalready guards this, and so does the siblingBottomSheetSwitcher. Only the standalone sheet was missing it.What changed
isImeKeyEventcheck the rest of the library uses.onCanceland closes it on the very same keypress. The guard on its own is not enough.handleCancelis unchanged, and gains a comment saying why:cancelis a plainEventcarrying no composition state, so a guard there could never fire.Evidence
This is a bug fix, so the bar is red before and green after.
The tests are red at
fba7b4009(the branch base) and green after the fix. RevertinghandleKeyDownto its previous shape turns four of the five red on their behaviour assertion (onOpenChangecalled once where it must not be called at all), and hoistingpreventDefaultabove the key check turns the fifth red on its own. No test here passes vacuously, and the two halves of the fix are pinned separately.Covered: both IME signals (
isComposing, and the legacy keyCode 229); a composing Escape raised on a focused field and bubbling to the<dialog>; the release case, where the Escape after a cancelled composition still dismisses; the non-modal sheet, where Escape is the only route out; and Enter and ArrowDown mid-composition staying unclaimed, since the IME owns those keys.Local:
pnpm lint:strictclean, core typecheck clean,pnpm buildgreen, BottomSheet suite 77/77, full suite 11289/11292. The three remaining failures are pre-existing and not from this branch: two cli tests that assume a case-sensitive filesystem, and oneDateInputTouchtest that times out at 5s only under load (127/127 in isolation, andpackages/core/src/DateInput/is byte-identical tomainon this branch).No screenshots: nothing rendered changes, and a real IME composition cannot be driven from browser automation. A synthetic
KeyboardEventis untrusted, so it would not exercise the close-request path either.Rubric
Triage: bug fix · non-breaking · low blast radius → fast path · checks: §1 A1-A7/A11-A12, §7 C1-C8/C11, Q1-Q12Component Audit Rubric v1.6, check §9 I19. §1: Escape stays operable; no ARIA, focus or live region touched. §7: no Effect, state, ref or observer added, and the shared
isImeKeyEventis composed rather than reimplemented. The component is not audited around the fix.One note for the ledger row: the issue asks for the guard in both
handleKeyDownandhandleCancel. Only the first is implementable, for the reason above. ThepreventDefaulthoist is what actually closes the cancel path.Fixes #5302