Skip to content

Fix Caps lock bug when using some IME on macOS#3728

Merged
Tyriar merged 10 commits intoxtermjs:masterfrom
serkodev:fix-caps-lock-ime
Jun 26, 2022
Merged

Fix Caps lock bug when using some IME on macOS#3728
Tyriar merged 10 commits intoxtermjs:masterfrom
serkodev:fix-caps-lock-ime

Conversation

@serkodev
Copy link
Copy Markdown
Contributor

@serkodev serkodev commented Apr 6, 2022

Fixes #2457
Fixes #3155

The reason for this bug is that when Chrome uses some specific input methods and keydown, it will enter a key that is inconsistent with the actual one.

But in onkeypress, it can return the correct keyCode. Therefore, the solution to this problem is to only process characters (A-Za-z) during onkeypress.

In addition to the macOS built-in Pinyin input method proposed in the earlier issue, some other macOS third-party input methods such as OpenVanilla will encounter the same problem.


Fixes microsoft/vscode#109565
Fixes microsoft/vscode#69367
Fixes microsoft/vscode#138028

@serkodev serkodev changed the title Fix Caps lock bug on macOS IME Fix Caps lock bug when using some IME on macOS Apr 7, 2022
@serkodev
Copy link
Copy Markdown
Contributor Author

serkodev commented May 13, 2022

@Tyriar Please let me know if you have any comments on this as this bug is really annoying when using VSCode.
@meganrogge I would appreciate it if you could help to review too.

Copy link
Copy Markdown
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Won't this break non-US keyboard layouts that use characters beyond a-zA-Z?

@serkodev
Copy link
Copy Markdown
Contributor Author

I've tried Taiwan and Japanese layout keyboard and there is no problems.

@serkodev
Copy link
Copy Markdown
Contributor Author

serkodev commented May 13, 2022

@Tyriar If there are some other keyboard layouts or something else need me to help to test, just let me know. 😊
And welcome to discuss if there is other way to fix this bug.

This bug has affected me for at least 6 years when I was new to web development with VSCode.

@serkodev
Copy link
Copy Markdown
Contributor Author

serkodev commented May 13, 2022

If concern about event.key is non-English char, I think it can also add a condition to confirm the event.key is a-zA-Z before returning.

if (event.key && !event.ctrlKey && !event.altKey && !event.metaKey && event.key.length === 1 && (
    (event.keyCode >= 65 && event.keyCode <= 90) ||
    (event.keyCode >= 97 && event.keyCode <= 122)
  )) {
    // Include only keys in A-Z/a-z.
    // HACK: Need process these key events in 'onkeypress' to fix a IME bug on macOS Chrome. fix for #2457
-   return true;
+   const c = event.key.charCodeAt(0)
+   if ((c >= 65 && c <= 90) || (c >= 97 && c <= 122)) {
+       return true;
+   }
}

I did a benchmark test on using event.key.charCodeAt(0) and Regex /[a-z]/i.test(event.key), the result is using charCodeAt is 91.65% faster then Regex.

If you think this solution is acceptable, I can push a commit with this.

@serkodev
Copy link
Copy Markdown
Contributor Author

@Tyriar May I know is there feed back? Thank you

)) {
// Include only keys in A-Z/a-z.
// HACK: Need process these key events in 'onkeypress' to fix a IME bug on macOS Chrome. fix for #2457
return true;
Copy link
Copy Markdown
Contributor Author

@serkodev serkodev May 24, 2022

Choose a reason for hiding this comment

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

Related to #3728 (comment), here is the review comment with suggestion change. Welcome to commit suggestion If you think this is a better solution.

Suggested change
return true;
const c = event.key.charCodeAt(0);
if ((c >= 65 && c <= 90) || (c >= 97 && c <= 122)) {
return true;
}

@serkodev
Copy link
Copy Markdown
Contributor Author

serkodev commented Jun 7, 2022

@Tyriar @meganrogge World you please take a look on this? Hope to give some advice if possible.
This bug is still bothering me a lot.

@Tyriar Tyriar added this to the 4.19.0 milestone Jun 26, 2022
Copy link
Copy Markdown
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

@serkodev thanks a bunch for looking into this and sorry about the delay. I pushed a change to move off the deprecated keyCode and it still works great. This should land in the next version of VS Code 🙂

@Tyriar Tyriar enabled auto-merge June 26, 2022 16:24
@Tyriar Tyriar merged commit 3845008 into xtermjs:master Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment