Skip to content

fix: Input IME enter to complete composition bug#4338

Open
alexisareyn wants to merge 2 commits intocloudscape-design:mainfrom
alexisareyn:input-ime-composition-fix
Open

fix: Input IME enter to complete composition bug#4338
alexisareyn wants to merge 2 commits intocloudscape-design:mainfrom
alexisareyn:input-ime-composition-fix

Conversation

@alexisareyn
Copy link
Contributor

Fix race condition where compositionend fires before onKeyDown handler, causing premature search submission when users press Enter to complete character composition.

This was resolved for the Autosuggest component in : #4182

  • Add useIMEComposition hook to track composition state with requestAnimationFrame
  • Modify Input to use isComposing() check
  • Add comprehensive tests for IME composition scenarios
  • Ensure Enter key during composition completion doesn't trigger search

Description

This PR fixes a critical accessibility issue affecting Korean (and other IME) users where pressing Enter to complete character composition would accidentally trigger search submission instead of just completing the character.

Problem: When Korean users type characters like 가 (ga), pressing Enter to complete the character would incorrectly trigger search submission because the browser's compositionend event fires before the onKeyDown handler executes, creating a race condition.

Root Cause: The timing issue meant there was no reliable way to distinguish between "Enter pressed to complete character composition" vs "Enter pressed to trigger search".

Solution: Created a useIMEComposition hook that tracks composition state and uses requestAnimationFrame to maintain the composition flag briefly after compositionend, allowing the onKeyDown handler to correctly identify composition-related Enter keypresses and prevent inappropriate search triggering.

How has this been tested?

Automated Testing:

  • Added 13 comprehensive unit tests for the new useIMEComposition hook covering character formation, multiple composition cycles, edge cases, and proper cleanup
  • Modified existing autosuggest integration tests to verify IME scenarios
  • All tests pass: npm test

Manual Testing:

  • Start the dev server: npm start

  • Navigate to the autosuggest search page at http://localhost:8080

  • Switch to Korean 2-set keyboard input method

  • Type Korean characters (e.g., ㄱ + ㅏ to form 가) and press Enter to complete - should NOT trigger search

  • Type regular text and press Enter - should trigger search normally

  • Run automated tests: npm test

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • [N/A] Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@alexisareyn alexisareyn requested a review from a team as a code owner March 6, 2026 22:41
@alexisareyn alexisareyn requested review from georgylobko and removed request for a team March 6, 2026 22:41
@codecov
Copy link

codecov bot commented Mar 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.44%. Comparing base (3dcd7e0) to head (16ac3dc).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4338      +/-   ##
==========================================
- Coverage   97.45%   97.44%   -0.01%     
==========================================
  Files         902      902              
  Lines       26468    26474       +6     
  Branches     9540     9542       +2     
==========================================
+ Hits        25794    25798       +4     
- Misses        631      633       +2     
  Partials       43       43              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@alexisareyn
Copy link
Contributor Author

alexisareyn commented Mar 11, 2026

Update:

  1. I removed the integration test I originally included. I forgot that when making fix: resolve IME enter to complete composition bug #4182 I was not able to add one. Browser automations cannot simulate real IME composition events b/c IME lives at OS level which means that we cannot simulate the IME events on a 2 set keyboard.

  2. The failing tests in /src\/app-layout\/__integ__\/app-layout-error-boundaries.test.ts fail locally on main for me - has this issue been seen before?

  3. As for the measure failure, Unhandled error: HttpError: Resource not accessible by integration , I was not super sure what to do about this. Copilot said "The error "Resource not accessible by integration" happened during a step using actions/github-script@v7 to report failure in the bundle size workflow. This is usually caused by missing or incorrect GitHub token permissions.". @georgylobko do you know how to resolve this?

alexisareyn added 2 commits March 20, 2026 09:04
Fix race condition where compositionend fires before onKeyDown handler,
causing premature search submission when users press Enter to
complete character composition.
@georgylobko georgylobko force-pushed the input-ime-composition-fix branch from fa81bbc to 16ac3dc Compare March 20, 2026 08:04
Copy link
Member

@georgylobko georgylobko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Got it, so I assume pages/input/korean-ime.page.tsx isn't needed then?

  2. No, this is a new one, and from what I can see, the tests are passing in the pipeline

  3. No worries, we don't run certain workflows for external contributions

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants