google / mesop

Rapidly build AI apps in Python
https://google.github.io/mesop/
Apache License 2.0
5.38k stars 259 forks source link

Unintentional on_input_enter trigger when confirming IME composition in chat component #588

Closed kyohei3 closed 2 months ago

kyohei3 commented 3 months ago

Describe the bug

This is an issue in the chat component.

When using an IME that requires composition, such as Japanese or Chinese, the on_input_enter event is unintentionally triggered when pressing Enter to confirm the composition.

To Reproduce

  1. Go to the demo gallery and open Chat demo.
  2. Focus on the input element below and type something using a Japanese/Chinese IME (or any other IME that requires composition).
  3. Press "Enter" to confirm the IME conversion.

Expected behavior

The composition should be confirmed, but the text should not be passed to the on_input_enter event.

Screenshots

Desktop System Info

Additional context

richard-to commented 3 months ago

Thanks for the catch. That's a tricky one.

As a temporary fix, you can make a copy of https://github.com/google/mesop/blob/main/mesop/labs/chat.py and remove the on enter event on line 13. This means you will only be able to send the chat input by pressing the button. But probably better than the current behavior.

Basically, copy the chat.py file to your own chat.py file. Then import your custom chat.py into your main mesop application.

richard-to commented 3 months ago

Maybe a solution is to check is this isComposing field: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/isComposing

Saw this from this pull request in vuejs: https://github.com/vuejs/vitepress/pull/3454/files

Would just need to update this function here: https://github.com/google/mesop/blob/main/mesop/components/input/input.ts#L86

kyohei3 commented 3 months ago

@richard-to Thank you for the advice.

I tried to change code like this, but it does not work with my environment (Mac and Chrome).

const keyboardEvent = event as KeyboardEvent;
if (keyboardEvent.key === 'Enter' && !keyboardEvent.isComposing) {
  // ...
}

As far as I observed key events on my environment and read some Japanese articles, isComposing is set to true on keydown event but set to false on keyup event. I locally changed here to (keydown)=... and found it worked as expected.

https://github.com/google/mesop/blob/273c65d26f710f19504525b7295e101430f284d5/mesop/components/input/input.ng.html#L54

Do you think it makes scene to replace keyup event with keydown?

richard-to commented 3 months ago

Thanks for looking into this. That's very interesting to know.

Key up would be preferable since it won't fire that many events to Mesop server. If necessary, it's possible we can debounce the keydown if someone decides to hold down the enter key for whatever reason.


I wonder if we can have both a key up and key down event. So the key down event would check if isComposing is True (this would be stored as a variable on the component. Then on key up we could check the stored variable.

Not sure if that will work or not. But the hope is that the first key down will log isComposing is True. Then the second key down will log isComposing is False. Can you give that a try?

richard-to commented 2 months ago

I added a fix for the IME issue here: https://github.com/google/mesop/pull/593 I'm not completely sure if I tested it right. I don't normally use IME, but I tested by enabling IME for Japanese on MacOS.

On Chrome, MacOS, the isComposing was set to true on key up. But it was on the first enter (when selecting an option). Then on the confirmation, the second enter (it was returning false as you said). So the solution to track the isComposing via key down seems to work. But I don't know if behavior differs from browser to browser.

richard-to commented 2 months ago

I tested on Chrome and Firefox and the behavior was the same. However, Safari didn't work.

Seems like this is a long time Safari issue: https://bugs.webkit.org/show_bug.cgi?id=165004

I guess one solution in that thread is to compare the keycode even though it is deprecated:

> This makes comparing `keyCode === 229` the only reliable way to detect whether IME is being used or not, even though modern APIs are available, because they are giving the wrong results.

I think alternate hacky way to do this is to check for isComposing == false twice which is kind of ridiculous but could work.

richard-to commented 2 months ago

Ok, I submitted a fix. Now just need to wait for it to reach the next Mesop release.