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

incorrectly handle insertText (macOS) #3925

Open
ksqsf opened this issue Sep 20, 2024 · 8 comments · May be fixed by #4087
Open

incorrectly handle insertText (macOS) #3925

ksqsf opened this issue Sep 20, 2024 · 8 comments · May be fixed by #4087
Labels
B - bug Dang, that shouldn't have happened DS - macos

Comments

@ksqsf
Copy link

ksqsf commented Sep 20, 2024

Description

Description

Currently, the implementation of insertText is insufficient to cover some input methods that can commit text without setting a preedit. (e.g. when the input method translates a certain key and directly commits the translation.)

Known affected downstream programs include Alacritty and Neovide.

Examples

  • A typical example is the full-width punctuation. If you hit the period key with the input method enabled, it will commit to the program without ever setting a preedit.
  • Another example is SKK-style Japanese input methods, with which, e.g. a directly commits without any preedit. Currently, both full-width punctuation and SKK Japanese input method doesn't work properly.

Analysis

Upon inspection, I found that insertText will only commit text when there is a preedit. (Hence the description above.)

I removed the condition and it almost worked for me. However, it seems to be added in commit 3fd7384 for a good reason.

// Commit only if we have marked text.
if unsafe { self.hasMarkedText() } && self.is_ime_enabled() && !is_control {
self.queue_event(WindowEvent::Ime(Ime::Preedit(String::new(), None)));
self.queue_event(WindowEvent::Ime(Ime::Commit(string)));
self.ivars().ime_state.set(ImeState::Committed);
}

Notes

By "almost" above, I mean it's still not fully satisfactory.

  • On startup, the IME is disabled and only gets enabled after a preedit is set once. That is, for full-width punctuation to work, one has to input something to enter the IME state. For SKK, the situation could get a bit problematic. This should also be fixed. (I think this probably belongs to a separate issue? It could be fixed by setting Ground to be the default. But I might be missing some details here.)
  • Some input methods support toggle keys. For example, in SKK, q toggles Hiragana and Katagana input. If I removed the preedit check, whenever I type q, the input method can receive q (toggling katagana) and another q will also be forwarded to the application.

Another thing is that I noticed the system input methods can commit full-width punctuation just fine. But to my surprise I didn't manage to find any difference in the logs. So I don't really understand what's going on.

macOS version

ProductName:		macOS
ProductVersion:		15.0
BuildVersion:		24A335

Winit version

0.30.3

@fredizzimo
Copy link

On startup, the IME is disabled and only gets enabled after a preedit is set once.

I think this might additionally explain the following two bugs (just based on the symptoms, so I might be wrong)

ksqsf added a commit to ksqsf/winit that referenced this issue Jan 21, 2025
fixes rust-windowing#3925

on macOS, when IME is allowed, always send text to IME and use that
result when possible.  Even if the keyboard is a simple one, like US
keyboard.

Committed text is now inserted regardless of the presence of a
preedit.
@ksqsf ksqsf linked a pull request Jan 21, 2025 that will close this issue
4 tasks
@kchibisov
Copy link
Member

kchibisov commented Feb 2, 2025

I removed the condition and it almost worked for me. However, it seems to be added in commit 3fd7384 for a good reason.

That's the reason you have KeyboardInput in the first place, without this line, you don't have it anymore, thus, said apps can not really function, since everything is a Commit regardless of what layout are you using, which is just wrong. So if anyone want to solve this, they should ensure that KeyboardInput is still there, if it's not, your solution doesn't work.

And no, checking for e.g. ASCII is also wrong, since Russian layout shouldn't go through IME.

@lifei
Copy link

lifei commented Feb 18, 2025

what's the status of this pr?

@ksqsf
Copy link
Author

ksqsf commented Feb 18, 2025

@lifei No progress. I don't have an idea yet.

I guess the correct way forward is to reinterpret system results as key events rather than Ime::Committed and such, similar to how Emacs handles it. To make that work, the client app must be aware whether it is expecting text or not, but that doesn't seem to be feasible currently.

OTOH, if the client app is willing to indicate whether it wants keys or text by calling set_ime_allowed, this PR should work as-is: say in a GUI app, the user focuses a text entry and the widget lib calls set_ime_allowed(true), and calls with false when it's unfocused.

@fredizzimo
Copy link

You can still deliver normal KeyboardInput events even when IME is allowed, but the same input can't generate both KeyboardInput and Ime.

@ksqsf
Copy link
Author

ksqsf commented Feb 18, 2025

@fredizzimo Yep, I'm aware of that. The current version behaves that way by heuristically deciding whether to send key events to IME.

I'll try to summarize the problem here:

  1. There is no way to tell if a key should be handled by IME => we have to forward all keys to IME first, or we cannot support special IMEs. (SKK is an extreme example because it basically breaks all IME conventions.)
  2. IME invokes certain ObjC methods in the app. However, it does not distinguish unhandled raw strings (from the perspective of the user, not necessarily technically unhandled) from converted strings. For example, the US keyboard layout will call insertText, in exactly the same way how a CJK IME would behave.

To winit, without a hint from the app itself, we cannot tell with certainty if the key event should be directly forwarded to the app, or it is the text from IME should be sent to the app. The root problem is due to winit distinguishing between the two, which may make sense for Windows or Linux (I don't know) but definitely causes some problems for macOS.

@kchibisov
Copy link
Member

I'm pretty sure it's possible to fix this, since the sequence should be generally different, but given that I'm not that interested myself in figuring out macOS nor apple allows me to use newer versions, I'm not that much interested myself.

The thing is that certain characters which don't work have likely a special pattern, since they are not being inserted in a normal way at all IIRC, so figuring that pattern out is how it should be done.

I know that maybe unmarked text may be used for that, from what I've seen in certain toolkits, but generally I don't see how it's impossible, since browsers somehow can deal with it, given that they implement W3C spec on macOS as well.

@kchibisov
Copy link
Member

I also don't see any issue in e.g. looking into how firefox builds the sequence and how it deals with the IME input, or e.g. chrome. I'm sure they should have the same issue and probably solved it somehow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B - bug Dang, that shouldn't have happened DS - macos
Development

Successfully merging a pull request may close this issue.

4 participants