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 Safari responds to the EnterKey from IM #2901

Merged
merged 8 commits into from
Dec 11, 2024

Conversation

miku1958
Copy link
Contributor

@miku1958 miku1958 commented Dec 9, 2024

How to repro: Press Enter when IM shows candidates

Screen.Recording.2024-12-09.at.13.14.33.mov

And its keycode is 229
image

Which is dead code
image

@@ -181,7 +181,8 @@ export class EditPlugin implements EditorPlugin {
break;

case 'Enter':
if (!hasCtrlOrMetaKey) {
if (!hasCtrlOrMetaKey && event.rawEvent.keyCode == 13) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting. If we are sure that keyCode will be 229 for IME enter, I prefer to check keyCode != 229 here which makes it more clear what it is doing.

Please add a const declaration for this key name and add comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@JiuqingSong
Copy link
Collaborator

I created a PR to fix it from core plugin. #2905

/**
* Key code enumeration
*/
export const enum KeyCode {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just put it back in EditPlugin.ts and no need to export it.
And use a regular const instead of const enum since const enum has a lot of problem.

@JiuqingSong JiuqingSong merged commit db04e83 into microsoft:master Dec 11, 2024
4 checks passed
@miku1958 miku1958 deleted the Safari-IM-Eeter branch December 16, 2024 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants