Skip to content

fix: stop processing input when composing with an IME#1226

Merged
aymeric-giraudet merged 5 commits intonextfrom
fix/input-ime-composition
Jan 3, 2024
Merged

fix: stop processing input when composing with an IME#1226
aymeric-giraudet merged 5 commits intonextfrom
fix/input-ime-composition

Conversation

@dhayab
Copy link
Copy Markdown
Member

@dhayab dhayab commented Dec 14, 2023

Summary

Characters entered using an IME trigger search requests although the character composition is not done. This should only happen if the character has been fully composed.

This PR stops processing input events when composing with an IME and instead rely on the compositionend event in that case.

It also prevents keyboard inputs being processed by Autocomplete if the user is currently navigating through suggestions in an IME.

Result

Before

CleanShot 2023-12-14 at 10 45 34

After

CleanShot 2023-12-14 at 10 46 31

Fixes algolia/docsearch#1304
Fixes algolia/docsearch#1043
Fixes #1135

@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci Bot commented Dec 14, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a883e74:

Sandbox Source
@algolia/autocomplete-example-github-repositories-custom-plugin Configuration
@algolia/autocomplete-example-instantsearch Configuration
@algolia/autocomplete-example-playground Configuration
@algolia/autocomplete-example-preview-panel-in-modal Configuration
@algolia/autocomplete-example-react-renderer Configuration
@algolia/autocomplete-example-starter-algolia Configuration
@algolia/autocomplete-example-starter Configuration
@algolia/autocomplete-example-reshape Configuration
@algolia/autocomplete-example-vue Configuration

@Haroenv
Copy link
Copy Markdown
Contributor

Haroenv commented Dec 14, 2023

once this is ready, we should add references to close:

algolia/docsearch#1304, algolia/docsearch#1043, #1135, #901

@dhayab dhayab marked this pull request as ready for review December 14, 2023 14:51
@dhayab dhayab requested review from a team, aymeric-giraudet and sarahdayan and removed request for a team December 14, 2023 14:51
@dhayab dhayab marked this pull request as draft December 14, 2023 15:16
@dhayab
Copy link
Copy Markdown
Member Author

dhayab commented Dec 14, 2023

Putting this back to draft in order to integrate #901 .

@dhayab dhayab marked this pull request as ready for review December 15, 2023 13:31
Copy link
Copy Markdown
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

yay, this is the right approach, thanks!

@tats-u
Copy link
Copy Markdown

tats-u commented Dec 19, 2023

This change has not been reflected in the CodeSandbox of @algolia/autocomplete-example-react-renderer yet.

@dhayab dhayab marked this pull request as draft December 19, 2023 10:18
@dhayab
Copy link
Copy Markdown
Member Author

dhayab commented Dec 19, 2023

Hi @tats-u , thanks for your feedback. There are indeed additional changes required for the fix to work within a React environment. We'll work on those and let you know once they're released.

@aymeric-giraudet
Copy link
Copy Markdown
Member

Hi @tats-u, thank you for your patience !

It should work with a React renderer as well now, personally tested it on both desktop and mobile and it seems to work great.

@aymeric-giraudet aymeric-giraudet marked this pull request as ready for review January 3, 2024 14:28
@tats-u
Copy link
Copy Markdown

tats-u commented Jan 3, 2024

@aymeric-giraudet It seems to work properly now. (To play the following video in Safari in Apple devices, you must use M3 Mac, iPhone 15 Pro, or other newer devices)

react-renderer.mp4

(event as unknown as Event).currentTarget as HTMLInputElement
).value;

if (getNativeEvent(event as unknown as InputEvent).isComposing) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see the compatibility layer in core makes sense here, but I wonder if it actually should be part of the react layer? I guess once that's not just an example anymore?)

Copy link
Copy Markdown
Member

@aymeric-giraudet aymeric-giraudet Jan 3, 2024

Choose a reason for hiding this comment

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

You mean as user land code as there's no React layer ?
I think it's fine to just have a condition checking whether there's nativeEvent or not, doesn't add much overhead

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, I was thinking as user land code, but it's indeed easy to forget

@aymeric-giraudet aymeric-giraudet merged commit 7f5ba08 into next Jan 3, 2024
@aymeric-giraudet aymeric-giraudet deleted the fix/input-ime-composition branch January 3, 2024 16:15
dhayab added a commit that referenced this pull request Jan 16, 2024
dhayab added a commit that referenced this pull request Jan 16, 2024
@dhayab dhayab restored the fix/input-ime-composition branch January 25, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants