Add conditional cancellation to default keydown event handler

[neet]

What:

This PR improves default handling for KeyDown event which triggered with KeyboardEvent.which === 229 || KeyboardEvent.isComposing

Why:

Since Japanese and Chinese keyboard users convert the native characters by IME after typing them in alphabets, We have 2 types of input: before and after composition.

But the default keydown event handler triggers events despite composition is not completed. So even while choosing the native chars using Arrow or select it by Enter, states will be changed.

GIF: The front window is the IME and the back is a React component with Downshift

Of course, you can prevent them like this. but this is too long as a boilerplate 😥

<Input
  {...getInputProps({
    onKeyDown: (e: React.KeyboardEvent<HTMLInputElement>) => {
      if (e.which === 229 || e.isComposing) {
        (e as any).nativeEvent.preventDownshiftDefault = true;
      }
    },
  })}
/>

How:

I added conditional cancellation to inputHandleKeyDown. Because of browser compatibility issue, there are 2 conditions.

Checklist:

• Documentation N/A
• Tests
• Ready to be merged
  
• Added myself to contributors table
  

[silviuaavram]

Thank you for your contribution. I will take a look at the changes and get some context on this. Will come back to you!

[silviuaavram]

maybe we can have a better condition for reading. why don't we add an if to the top of the function, checking event.which and is composing and return from function if one of them is true? in that way we don't need to normalize any arrow key and we can have two clean conditions.

Also I think the event.which !== 229 was useless there since normalizeArrowKey checks if key is 37-40.

Suggested change

	if (
	inputHandleKeyDown = event => {
	if (event.which === 229 || event.isComposing) { return }
	const key = normalizeArrowKey(event)
	// bla bla the rest
	}

What do you think? Also I tried to find out what is the exact issue with this. Probably I kind of got the idea, but since you are native to it and know the whole thing, can you explain in detail what happens here? That GIF you posted is helpful, true, but for me, without knowing that IME composition existed, is hard to figure out. :D

[silviuaavram]

also before the first IF we can add a comment with this PR and also a short description of the problem, like IME composing issue

[neet]

maybe we can have a better condition for reading.

I agree with it is a bit unreadable. Initially, I've implemented it as you said above. (febf5a4)
But since I'm not familiar with RTL, I couldn't find the way to test what returned in the event handler. That's why I couldn't avoid it.

Also I think the event.which !== 229 was useless there since normalizeArrowKey checks if key is 37-40.

Oh, I didn't realize it. KeyboardEvent.isComposing was available in my browser so it worked correctly.

Anyway, If you know how to test the return value of the event handler could you teach me? It will also resolve the normalizeArrowKey issue.

[silviuaavram]

Hey sorry for coming back to you so late, but I had to take the mobile web specialist test and had to study for it. Left some comments, overall looks good and I'm happy we can make Downshift even more inclusive. What I wanted is to get more background on the issue and have the changes a bit more clear. Take a look and let me know what you think. Thanks!

Oh, you might want to rebase with the original master branch too :)

[NMinhNguyen]

FYI you might be interested in how Material-UI have solved this issue: mui/material-ui#19435

[silviuaavram]

Closing this one and following up with #993