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(searchbox): only refine if composition ended when using an IME #5963

Merged
merged 4 commits into from
Dec 14, 2023

Conversation

dhayab
Copy link
Member

@dhayab dhayab commented Dec 12, 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 refining on input when composing with an IME and instead rely on the compositionend event in that case.

Result

Characters entered using an IME do not trigger search requests until relevant.

CR-5106

Copy link

codesandbox-ci bot commented Dec 12, 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 26e1aaf:

Sandbox Source
InstantSearch.js Configuration
react-instantsearch-app Configuration
example-react-instantsearch-default-theme Configuration
example-react-instantsearch-next-app-dir-example Configuration
example-vue-instantsearch-default-theme Configuration

@Haroenv
Copy link
Contributor

Haroenv commented Dec 12, 2023

just checking, does IE have composition support? I sas FF sometimes triggers in wrong order too: w3c/uievents#202 (comment)

@dhayab
Copy link
Member Author

dhayab commented Dec 13, 2023

just checking, does IE have composition support? I sas FF sometimes triggers in wrong order too: w3c/uievents#202 (comment)

IE11 doesn't implement event.isComposing so the current behaviour will stay (search requests will be triggered during composition). There should be no issue with the order of events, as the input value will be the same, so it is caught by the search client cache.

@dhayab dhayab marked this pull request as ready for review December 13, 2023 11:44
@dhayab dhayab requested review from a team, sarahdayan and aymeric-giraudet and removed request for a team December 13, 2023 11:44
Copy link
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.

thanks!

@@ -184,6 +189,9 @@ class SearchBox extends Component<
spellCheck="false"
maxLength={512}
onInput={this.onInput}
// see: https://github.com/preactjs/preact/issues/1978
// eslint-disable-next-line react/no-unknown-property
oncompositionend={this.onInput}
Copy link
Contributor

Choose a reason for hiding this comment

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

not relevant now, but I wonder how we'll deal with these differences with only one JSX source

tests/common/widgets/search-box/options.ts Outdated Show resolved Hide resolved
await act(async () => {
await wait(0);

searchClient.search.mockClear();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
searchClient.search.mockClear();
castToJestMock(searchClient.search).mockClear();

also why not right after setup + wait 0, before starting to compose?

await act(async () => {
  await wait(0);
  castToJestMock(searchClient.search).mockClear();
});

// ... character etc

Copy link
Member Author

Choose a reason for hiding this comment

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

I need to wrap the events inside of act() so I'm reusing the one that waits for initialization.

@dhayab dhayab merged commit 0820455 into master Dec 14, 2023
12 checks passed
@dhayab dhayab deleted the fix/searchbox-ime-composition branch December 14, 2023 14:52
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