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

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

Merged
merged 10 commits into from
Jun 26, 2022

Conversation

serkodev
Copy link
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
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
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
Contributor Author

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

@serkodev
Copy link
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
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
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
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
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
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 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment