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

Stop dispatching to keybindings in editing coposition text. #6673

Conversation

monaka
Copy link
Contributor

@monaka monaka commented Dec 2, 2019

Signed-off-by: Masaki Muranaka monaka@monami-ya.com

What it does

Ignores keydown event when IME is working.
This will fix #6459 and gitpod-io/gitpod#875 .

How to test

Editing texts like the description of #6459.

Review checklist

Reminder for reviewers

This patch was tested on my Chrome only.
I suppose this will work also on another modern browsers, but not tested on them.

@akosyakov akosyakov added the keybindings issues related to keybindings label Dec 2, 2019
@akosyakov
Copy link
Member

@502647092 Does it fix the issue for you? Could you check please?

@akosyakov
Copy link
Member

Does it prevent Monaco from receiving composition events? From code it seems that it expects to receive and have special handling logic, i.e. like here microsoft/vscode@31c7e3b Why is not enough to let Monaco to deal with it?

@akosyakov akosyakov added the monaco issues related to monaco label Dec 2, 2019
@monaka
Copy link
Contributor Author

monaka commented Dec 2, 2019

And @mouse484 also will be one of the important reviewer.

@mouse484
Copy link

mouse484 commented Dec 2, 2019

Expected behavior 👍

This makes Theia and its associations better for those who need IME.

@502647092
Copy link
Contributor

#6459 is fixed
#6603 not fixed
temp

@monaka
Copy link
Contributor Author

monaka commented Dec 3, 2019

@akosyakov I cannot be sure as I'm not a Monaco specialist.
But I suspect the base version of @typefox/monaco-editor-core is old.

The similar issue was raised on VSCode also and fixed by microsoft/vscode#40546.
src/vs/editor/browser/controller/textAreaInput.ts was fixed. But there is no same filename in the fork by TypeFox.

@monaka
Copy link
Contributor Author

monaka commented Dec 3, 2019

@502647092 Hmm... I'm using Japanese IME on ChromeOS and it looks working well.
Could you tell me your environment? OS, Browser, and IME.

replace with Japanese IME

@502647092
Copy link
Contributor

@monaka
Windows10
Chrome
Chinese IME

@502647092
Copy link
Contributor

temp
when I select content and input at IME
editor have two cursor then will freeze
can't input anything
must reopen it

@akosyakov
Copy link
Member

akosyakov commented Dec 3, 2019

@akosyakov I cannot be sure as I'm not a Monaco specialist.
But I suspect the base version of @typefox/monaco-editor-core is old.

We need to catch up with Monaco then. Maybe it would be feasible at the beginning of next year. I would be fine though to accept this PR as a temporary solution if it works.

Here is the explanation why Theia is using custom monaco-editor-core and which other packages are necessary: https://github.com/eclipse-theia/theia/wiki/LSP-and-Monaco-integration

For previous update we used this branch: https://github.com/TypeFox/vscode/tree/monaco/0.18.0.

We will need a new branch which is forked from latest master, tested with upstream with bug fixes provided to upstream, tree shaking should be removed as in previous version, then it should be published for testing and tested with Theia like here #5901. We have plans to work on auto testing it in mid December and next January to make Monaco migration easier.

@monaka
Copy link
Contributor Author

monaka commented Dec 3, 2019

@akosyakov I agree it's a best to fixed by upgrading Monaco to the latest. And you might not recognized, this issue is a show-stopper to the Chinese/Japanese/Korean people.
IMO, this is a hot-fix. Users will move to VSCode based services if this issue was left as is.

@akosyakov
Copy link
Member

akosyakov commented Dec 4, 2019

@502647092 What do you think? Is it better despite #6673 (comment)? @mouse484 Do you have something like #6673 (comment)?

@mouse484
Copy link

mouse484 commented Dec 4, 2019

@akosyakov
"IME" to be different depending on country and language.
No problem in Japanese IME.

@mouse484
Copy link

mouse484 commented Dec 4, 2019

It may be different from this problem.
use the chrome input tool, I can't input Japanese Chinese etc

@svenefftinge
Copy link
Contributor

@502647092 I understood that the freezing exists with or without this change (i.e. #6603)?
If that is the case, I'd like to merge this as an interim solution.

@monaka monaka force-pushed the pr-fix-unexpected-behaviors-in-composition-editing branch from 4588241 to 49e4a20 Compare December 6, 2019 05:00
@monaka
Copy link
Contributor Author

monaka commented Dec 6, 2019

@mouse484 It's better to raise new issue for your report, IMO. But your report is interesting to me.
Just my guess for now, your report may be a hint to resolve #6603 that @502647092 reported.

@monaka
Copy link
Contributor Author

monaka commented Dec 6, 2019

@svenefftinge #6603 was raised on 14 days ago. And this PR was raised on 4 days ago.
So at least #6603 was caused with or without this patch.
@502647092 how about your comment above?

Signed-off-by: Masaki Muranaka <monaka@monami-ya.com>
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

let's give it a try, at least it seems to fix some cases and we can revert it later

@akosyakov akosyakov merged commit 27745bf into eclipse-theia:master Dec 6, 2019
@monaka monaka deleted the pr-fix-unexpected-behaviors-in-composition-editing branch December 8, 2019 23:30
@kittaakos
Copy link
Contributor

As always; beamers never work during the presentations and hotfixes contain typos -> Comosition 😊

@RomanNikitenko
Copy link
Contributor

@monaka
I removed this fix within upgrade Monaco to 0.19.3.
Could you test if it works well for #7149

thanks in advance!

@akosyakov
Copy link
Member

@502647092 @mouse484 It would help if you also give it a try: #6673 (comment) 🙏

@mouse484
Copy link

mouse484 commented Mar 17, 2020

not Fixed in Japanese IME

(Works before removing the fix - 32b26b2)

@RomanNikitenko
Copy link
Contributor

not Fixed in Japanese IME

thanks, will try to reproduce it on my machine and investigate it.

@RomanNikitenko RomanNikitenko mentioned this pull request Jun 15, 2020
47 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keybindings issues related to keybindings monaco issues related to monaco
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[editor] chinese input method error
7 participants