Skip to content

Review 5322

Jump to bottom
Cindy Zhang edited this page Aug 23, 2026 · 1 revision

Review 5322 — BottomSheet: Escape during IME composition

#5322 · AKnassa · community · merged 2026-08-23 Head reviewed 7ec88210c812f1131420e82eeccffb704c54d82d Verdict approve, 2 nits · competing set with #5357 (HelloOjasMutreja, draft, closed) First live run of R6d — and it found the competitor.

Problem

A standalone BottomSheet closes on the Escape a CJK user presses to cancel a half-typed word. Reproduced in Chromium on main with a real Hangul composition: the sheet and everything typed into it are gone. Two routes reach it — the sheet's own onKeyDown, and, on the default modal sheet, the browser's close request landing on onCancel.

Solution

handleKeyDown calls preventDefault() first, then early-returns on isImeKeyEvent. The hoist is the half that closes the second route: it claims the key so the <dialog> close watcher never raises a close request.

Impact · API · Theme · Breaking · Performance

Everyone on the next core patch, no opt-in. No API change — isImeKeyEvent was already public. No theme targets. Breaking: no on all three axes. Nothing: no effect, listener or observer; one predicate per Escape keydown. Low-risk (R19): all four.

The decision table

core-bottomsheet--mobile-keyboard, one axis, same harness, real composition driven through CDP Input.imeSetComposition:

build modal cancel fired sheet survives
main yes no no
#5322 yes no yes
#5357's shape yes yes no
main, plain Escape yes no no (correct)
#5322, plain Escape yes no no (correct)

Isolated on a bare page across all four handler shapes × modal/non-modal × composing: non-modal show() has no close watcher so both fixes work there; modal showModal() has one, and only #5322 survives it.

The preventDefault hoist looks like over-engineering next to two siblings that do not do it, and it is the entire fix. jsdom models neither composition nor the close watcher, so nothing but a real browser could have shown that (R14i). Without the Chromium run the review would have written the opposite nit.

Judgement

approve

1. [not blocking] BottomSheet.tsx:320 says Dialog and BottomSheetSwitcher
   "guard the same way" — they don't; neither claims the key
   → the next contributor copies the sibling shape and ships the bug back.
     #5357 did exactly that, six hours earlier      · BottomSheet.tsx:320
2. [not blocking] the changeset credits the issue filer, not the author
   → AKnassa's name is missing from the release notes for their own fix
                     · .changeset/bottom-sheet-ime-escape-guard.md:7

The family, and it is ours

Root cause: an IME guard that returns early without claiming the key, on an element whose onCancel dismisses. Same shape at useFocusTrap.ts:388, Dialog.tsx:492, BottomSheetSwitcher.tsx:583. Dialog is measured broken on main (frame captured). AlertDialog renders Dialog; the modal switcher routes Escape through useFocusTrap. Not the author's to fix, and #5322 freezes no surface, so it is a step toward the real fix rather than a detour. Owed: the family fix, as ours.

Evidence

Real Chromium, 480×900, story iframe. Before (main) / after / #5357's shape / Dialog-on-main, all opened. Two reusable probes added: probe-ime-close-request.cjs, probe-bottomsheet-ime.cjs.

The PR's six tests are load-bearing — verified by swapping four implementations under them: main → 5 red; #5357's shape → exactly 1 red; hoist removed → 1 red.

TIME  total 21m
  setup      3m   one worktree, cloned node_modules, one build, storybook dev
  reading    5m
  measuring  6m   3 bare-page matrices, 8 probe runs, 2 test-variant matrices
  writing    4m
  waste      3m   a setContent reset silently stopped CDP key events reaching
                  the page — a whole 16-row matrix of keydown=NONE

Clone this wiki locally