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

webview: ignore Ctrl+W and Ctrl+N in webview for PWA #164981

Merged
merged 1 commit into from Nov 1, 2022

Conversation

zhuowei
Copy link
Contributor

@zhuowei zhuowei commented Oct 29, 2022

This fixes closing Release Notes / Markdown Preview with Ctrl+W closing whole PWA.

PWAs can intercept the Ctrl+W/Ctrl+N keystrokes, and VSCode does this for editor tabs. However, webviews do not intercept these keystrokes.

If a webview such as Release Notes or Markdown Preview has focus:

  • when the user presses Ctrl+N, another instance of the PWA is opened in a new window.
  • when the user presses Ctrl+W, the whole PWA window closes.

See #150735 for details.

This fixes the issue by ignoring Ctrl+W and Ctrl+N in webviews.

Tested on macOS 12.6 / Chrome Dev 109.0.5384.0:

  • patch environmentService.ts webviewExternalEndpoint to use http://localhost:8080/static/sources/out/vs/workbench/contrib/webview/browser/pre/
  • followed the contributing instruction to run code-web
  • in Chrome, More Tools -> Create Shortcut -> Open as window
  • closed and re-opened Code OSS PWA window
  • opened chrome://inspect and inspected the Code OSS window
  • added an uncaught exception handler
  • opened file.md and chose "Open preview"
  • when it hit Expected '${parentOriginHash}' as hostname or subdomain! manually ran start(parentOrigin) in console and continued
  • the markdown preview now displayed
  • pressed Command+W

Before: the whole window closes
After: only the Markdown preview tab closes

@gjsjohnmurray
Copy link
Contributor

/assign @mjbvz

@zhuowei zhuowei changed the title webview: ignore Ctrl+W in webview when running as PWA to fix closing Release Notes / Markdown Preview with Ctrl+W closing whole PWA webview: ignore Ctrl+W and Ctrl+W in webview when running as PWA Oct 30, 2022
This fixes closing Release Notes / Markdown Preview with Ctrl+W closing whole PWA.

PWAs can intercept the Ctrl+W/Ctrl+N keystrokes, and VSCode does this for editor tabs. However, webviews do not intercept these keystrokes.

If a webview such as Release Notes or Markdown Preview has focus:

- when the user presses Ctrl+N, another instance of the PWA is opened in a new window.
- when the user presses Ctrl+W, the whole PWA window closes.

See microsoft#150735 for details.

This fixes the issue by ignoring Ctrl+W and Ctrl+N in webviews.

Tested on macOS 12.6 / Chrome Dev 109.0.5384.0:

- patch environmentService.ts webviewExternalEndpoint to use
  http://localhost:8080/static/sources/out/vs/workbench/contrib/webview/browser/pre/
- followed the contributing instruction to run code-web
- in Chrome, More Tools -> Create Shortcut -> Open as window
- closed and re-opened Code OSS PWA window
- opened chrome://inspect and inspected the Code OSS window
- added an uncaught exception handler
- opened file.md and chose "Open preview"
- when it hit
  `Expected '${parentOriginHash}' as hostname or subdomain!`
  manually ran `start(parentOrigin)` in console and continued
- the markdown preview now displayed
- pressed Command+W

Before: the whole window closes
After: only the Markdown preview tab closes
@zhuowei zhuowei changed the title webview: ignore Ctrl+W and Ctrl+W in webview when running as PWA webview: ignore Ctrl+W and Ctrl+N in webview for PWA Oct 30, 2022
@zhuowei
Copy link
Contributor Author

zhuowei commented Oct 30, 2022

Updated PR to handle Ctrl+N as well.

Questions:

  • should I just remove the isElectron check and put the new keys to ignore alongside the existing keys? I added that check since I don't want to cause any issues with Electron, but I don't think Electron uses these shortcuts anyways?
  • The webview now calls preventDefault for multiple keys. Each has a similar checking function. Should we refactor this to remove some of the duplication?

@mjbvz mjbvz added this to the November 2022 milestone Nov 1, 2022
Copy link
Contributor

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Thank you!

@mjbvz mjbvz merged commit a9bf311 into microsoft:main Nov 1, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2022
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.

None yet

3 participants