Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix correctly handle IME Input in Combobox #6239

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ryo-manba
Copy link
Contributor

@ryo-manba ryo-manba commented Apr 20, 2024

related to nextui-org/nextui#2121

This PR addresses a bug in the combobox component where inputs from an IME were being incorrectly committed before the user completed their text entry.

βœ… Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

before

combobox-ime-before.mov

after

combobox-ime-after.mov

πŸ“ Test Instructions:

Setup for MacOS

Add the Japanese input method

  • Settings > Keyboard > Input Sources > Add a language > Japanese

Test Steps

  1. Choose Hiragana in the menu bar.
  2. Navigate to http://localhost:9003/?path=/story/react-aria-components--combo-box-ime-example&providerSwitcher-express=false&strict=true
  3. Type nihon followed by the enter key. This should result in "にほん" or "ζ—₯本".
  4. Continue typing go, and the combobox should suggest "にほんご". Press enter.
  5. Previously, the initial input "にほん" would be cleared, leaving only "ご". After this fix, "にほんご" should be correctly inputted without clearing the previous characters.

🧒 Your Project:

https://github.com/nextui-org/nextui

@ryo-manba ryo-manba marked this pull request as draft April 20, 2024 02:56
@snowystinger
Copy link
Member

GET_BUILD

@rspbot
Copy link

rspbot commented Apr 21, 2024

@rspbot
Copy link

rspbot commented Apr 21, 2024

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, this is indeed better.

I noticed that the popover keeps opening and closing in the background. I think we could introduce some behavior based on the composition events from useTextfield, onCompositiionStart and onCompositionEnd. Then we could keep the popover closed while composing and reopen it when composition ends.

Possibly closing when composition starts and opening when it ends? I tried it, but I don't know enough about composition to know what feels natural to a user. I'm guessing I'm missing something because the popover didn't always reopen, but maybe I just don't know how to end compositions correctly.

I'm happy for this to be handled separately in a new issue. Just let us know if it's something you'd like to take care of.

@ryo-manba
Copy link
Contributor Author

@snowystinger

I think we could introduce some behavior based on the composition events from useTextfield, onCompositiionStart and onCompositionEnd.

This is a good idea. I'll see if I can come up with another issue to address what state would be desirable for users!
Thanks for the feedback!

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.

None yet

3 participants