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

Fixes webview responding shortcuts twice on linux #84967

Merged
merged 1 commit into from
Nov 18, 2019
Merged

Fixes webview responding shortcuts twice on linux #84967

merged 1 commit into from
Nov 18, 2019

Conversation

onequid
Copy link
Contributor

@onequid onequid commented Nov 16, 2019

Add a platform condition for the registering of the "did-keydown" event listener.
Avoided transfering keydown events on Linux.
This PR fixes #82670

@mjbvz mjbvz merged commit a406fd6 into microsoft:master Nov 18, 2019
@mjbvz mjbvz added this to the November 2019 milestone Nov 18, 2019
@mjbvz
Copy link
Contributor

mjbvz commented Nov 18, 2019

Looks reasonable. Let's see if this causes any regression for other comments. Please test using the next insiders build

@onequid onequid deleted the 82670-Ctrl-W-closes-window-on-linux branch November 18, 2019 23:57
mjbvz added a commit that referenced this pull request Nov 19, 2019
@mjbvz mjbvz removed this from the November 2019 milestone Nov 19, 2019
@mjbvz
Copy link
Contributor

mjbvz commented Nov 19, 2019

I had to revert this change as it broke keybindings on linux: #85102

Please take another look at the addressing the problem in a different way

@onequid
Copy link
Contributor Author

onequid commented Nov 21, 2019

I apologize for not testing all the keybindings and the inconvenience brought by it.
And sorry that I have to drop this issue.

Here's something I tried. Surrounding following lines with the !isLinux if-condition would work properly as expected. However, the codes are there for a reason, I tried but failed to figure out what are these lines for. I doubt the change would break something somewhere else.

// Support runKeybinding event
ipc.on('vscode:runKeybinding', (event: IpcEvent, request: IRunKeybindingInWindowRequest) => {
if (document.activeElement) {
this.keybindingService.dispatchByUserSettingsLabel(request.userSettingsLabel, document.activeElement);
}
});

and the ipc event is sent here, but not listened anywhere else beside lines in window.ts mentioned above.

if (invocation.type === 'commandId') {
activeWindow.sendWhenReady('vscode:runAction', { id: invocation.commandId, from: 'menu' } as IRunActionInWindowRequest);
} else {
activeWindow.sendWhenReady('vscode:runKeybinding', { userSettingsLabel: invocation.userSettingsLabel } as IRunKeybindingInWindowRequest);
}

@mjbvz
Copy link
Contributor

mjbvz commented Nov 21, 2019

Just make sure any changes:

  • Don't break a normal keybindings for menu items when a webview is open
  • Don't break a normal keybindings for menu items when a webview is not opened
  • That everything works properly with both "window.titleBarStyle": "custom" and "window.titleBarStyle": "native"

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pressing Ctrl+W on release notes closes the window
2 participants