Skip to content

fix: standalone BottomSheet dismisses on Escape during IME composition-cancel - #5357

Closed
HelloOjasMutreja wants to merge 2 commits into
facebook:mainfrom
HelloOjasMutreja:fix/bottomsheet-standalone-ime-escape
Closed

fix: standalone BottomSheet dismisses on Escape during IME composition-cancel#5357
HelloOjasMutreja wants to merge 2 commits into
facebook:mainfrom
HelloOjasMutreja:fix/bottomsheet-standalone-ime-escape

Conversation

@HelloOjasMutreja

Copy link
Copy Markdown
Contributor

Fixes #5302.

Problem

The standalone BottomSheet's handleKeyDown reads a bare event.key === 'Escape', so a CJK (Korean/Japanese/Chinese) user pressing Escape to cancel an in-progress IME composition dismisses the sheet instead of just cancelling the candidate.

Dialog guards this in its native keydown listener via isImeKeyEvent(event), and BottomSheetSwitcher's own handleKeyDown guards it the same way (!isImeKeyEvent(event.nativeEvent)). The standalone sheet's handleKeyDown was the one path in this family that didn't.

Reachable via the documented purpose="form" and mobile-keyboard (height="tall") use cases, which put a text field inside a standalone sheet.

Fix

const handleKeyDown = useCallback(
  (event: React.KeyboardEvent<HTMLDialogElement>) => {
    if (event.key === 'Escape' && !isImeKeyEvent(event.nativeEvent)) {
      event.preventDefault();
      dismissOnEscape();
    }
  },
  [dismissOnEscape],
);

One deviation from the issue's suggested fix: I left handleCancel unguarded. Checked both existing precedents first — neither Dialog.handleCancel nor BottomSheetSwitcher.handleCancel check isImeKeyEvent, only handleKeyDown does in both. That's not an oversight: handleCancel only runs off the browser's native <dialog> close-watcher, which doesn't fire when handleKeyDown skips preventDefault() for a composing keydown — the browser's own IME handling takes it from there. Guarding handleCancel too would be dead code with no existing precedent for it anywhere in this component family.

Verification

Added a regression test (ignores Escape while an IME composition is active) mirroring the existing one in BottomSheetSwitcher.test.tsx. Confirmed it fails against the pre-fix code (reverted the implementation locally, test failed with onOpenChange called twice) and passes with the fix. Full BottomSheet suite (186 tests) passes, typecheck and lint are clean.

Not a visual change, so no before/after screenshot.

The standalone BottomSheet's handleKeyDown read a bare `event.key ===
'Escape'`, so a CJK user pressing Escape to cancel an in-progress IME
composition dismissed the sheet instead. Dialog and BottomSheetSwitcher
already guard their own Escape handling with isImeKeyEvent(); the
standalone sheet's handleKeyDown was the one path that didn't.

handleCancel is left unguarded, matching both of those: it only ever
fires from the browser's native close-watcher, which does not run when
handleKeyDown's preventDefault() is skipped for a composing keydown.
@vercel

vercel Bot commented Aug 23, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
astryx Error Error Aug 23, 2026 2:58pm

Request Review

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 23, 2026
@github-actions github-actions Bot added community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge labels Aug 23, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

Modified Components

BottomSheet (@astryxdesign/core) · View in Storybook
Metric Before After Delta
Bundle Size (ESM) N/A N/A N/A
Lines of Code N/A 3190 -
Complexity N/A Very High (258) -

Bundle Size Summary

Package Size (ESM) Size (CJS) Gzipped
@astryxdesign/core N/A 4.8KB 1.2KB

Accessibility Audit

Status: No accessibility violations detected.


Generated by PR Enrichment workflow | Storybook | Sandbox | View full report

github-actions Bot added a commit that referenced this pull request Aug 23, 2026
@HelloOjasMutreja

Copy link
Copy Markdown
Contributor Author

Closing as a duplicate: #5322 landed on main with the same fix (isImeKeyEvent guard on handleKeyDown, same reasoning for leaving handleCancel unguarded) while this was in review. Thanks for beating me to it!

@HelloOjasMutreja
HelloOjasMutreja deleted the fix/bottomsheet-standalone-ime-escape branch August 23, 2026 19:43
@cixzhang

Copy link
Copy Markdown
Contributor

Thanks for jumping on this — your read of the bug was right.

Two PRs came in for #5302 and we've gone with #5322, now merged. The deciding difference is small and not obvious: it calls preventDefault() before the IME check, so the key is claimed. Without that, a modal sheet's <dialog> close watcher raises its own close request and dismisses it anyway — I drove a real Hangul composition in Chromium and this shape behaves identically to main.

Worth knowing the comment at BottomSheet.tsx:320 claiming Dialog and BottomSheetSwitcher "guard the same way" is wrong — they have the same bug. That's ours to fix, not yours.

Please do send more. If you'd rather talk something through with a person, we're in Discord.

[Reviewed by Robohands]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot. community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[audit] BottomSheet: standalone Escape lacks an IME composition guard (I19)

2 participants