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.

Sign up for GitHub

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

Jump to bottom

Electron 3.0.x: Webview issues #56988

Closed
3 of 5 tasks
bpasero opened this issue Aug 22, 2018 · 8 comments
Closed
3 of 5 tasks

Electron 3.0.x: Webview issues #56988

bpasero opened this issue Aug 22, 2018 · 8 comments
Assignees
@bpasero @Tyriar @mjbvz
Labels
electron Issues and items related to Electron upstream Issue identified as 'upstream' component related (exists outside of VS Code) webview Webview issues
Milestone
December/January ...

Comments

@bpasero
Copy link
Member

bpasero commented Aug 22, 2018 •
edited
Loading

Electron 3.0.0-beta.8 changed the <webview> implementation to "Out of Process Iframe". This issue tracks regressions found by that move.
@bpasero bpasero added upstream Issue identified as 'upstream' component related (exists outside of VS Code) electron Issues and items related to Electron electron-blocker Issues in next update of Electron preventing update electron-3.0.x-update labels Aug 22, 2018
bpasero added a commit that referenced this issue Aug 29, 2018
@bpasero
webview - bubble up keyboard events (workaround for #56988)
34a635e
@bpasero
Copy link
Member Author

bpasero commented Aug 29, 2018

@mjbvz fyi see my change in 34a635e to bubble out keyboard events from <webview> to our keybinding service.

bpasero added a commit that referenced this issue Sep 2, 2018
@bpasero
workaround more webview focus issues (for #56988)
135032b
@bpasero
Copy link
Member Author

bpasero commented Sep 2, 2018

@mjbvz fyi see my change in 135032b to workaround the new focus behaviour of webview.

@bpasero
Copy link
Member Author

bpasero commented Sep 3, 2018 •
edited
Loading

There is still lots of flakiness in <webview> with regards to focus. It seems that the fact that we use an <iframe> within the <webview> makes proper focus/blur detection harder.

I think the context key in webviewElement.ts should not work with focus/blur but rather just be enabled when the active editor is a <webview> editor. Ideally we do not have to rely on focus/blur at all for the commands (select all, find) to work.

More things to test: use an extension such as negokaz.live-server-preview-0.1.2 that leverages HTMLPreviewPart and check if focus handling works properly. We should also let the MS SQL extension team do a test with Electron 3.0.x because they seem to rely most on the <webview> from all extensions.

@bpasero
Copy link
Member Author

bpasero commented Sep 5, 2018 •
edited
Loading

I pushed another comment to move away from DOM.trackFocus() for the webview element and rather send an explicit event.

There is still focus issues when doing this:

  • install negokaz.live-server-preview-0.1.2
  • open a empty HTML file and type some words
  • open preview to the side
  • click between editor and right hand preview

=> at one point focus is no longer detected

@bpasero bpasero removed the electron-blocker Issues in next update of Electron preventing update label Sep 5, 2018
@bpasero
Copy link
Member Author

bpasero commented Sep 6, 2018 •
edited
Loading

Another issue (macOS):

  • open one editor (markdown)
  • open preview to the side
  • hit Cmd+W to close
  • => both editors close

It looks like both the Cmd+W is fired from within the webview as well as the File > Close Editor menu entry from the global menu.

I opened electron/electron#14514

Update: it looks like calling webContents.setIgnoreMenuShortcuts(true) (somewhere here) fixes this issue however has one downside: any menu item on macOS that has a role associated (cut, copy, paste for example) needs to be implemented manually in the webview, otherwise we loose that functionality.

bpasero added a commit that referenced this issue Sep 10, 2018
@bpasero
webview - do not rely on DOM focus for certain commands (for #56988)
c47790a
@bpasero
Copy link
Member Author

bpasero commented Sep 10, 2018

c47790a changes to not rely on DOM focus/blur for context keys in the webview.

@bpasero bpasero added this to the November 2018 milestone Oct 25, 2018
@bpasero
Copy link
Member Author

bpasero commented Oct 25, 2018

#61762 seems related, moving to November.

@bpasero bpasero mentioned this issue Nov 18, 2018
Closed
@Tyriar Tyriar modified the milestones: November 2018, December 2018 Dec 3, 2018
@bpasero bpasero added the webview Webview issues label Dec 5, 2018
bpasero added a commit that referenced this issue Dec 10, 2018
@bpasero
Update to Electron 3.0.10 (#56149)
0bcf5cf 
* update to 3.0.0-beta.3

* update d.ts files; rename NodeBuffer => Buffer

* update to 3.0.0-beta.4

* undo invalid change

* remove some Electron 2.0.x specific workarounds

* pinch zoom is now disabled by default

* update vscode-nsfw

* change vscode-nsfw

* update smoke test electron version

* streams: use destroy() over close()

* workaround broken tests

* bump distro and OSS

* try to bump node version for build

* update macOS build to use node.js 10.8.0

* fix extension tests

* use node.js 10.2.1 for all builds

* remove nsfw from dev dependencies

* back to node 8.x for build

* Revert "back to node 8.x for build"

This reverts commit 90ea2b7.

* update distro

* disable test run on macOS prod build for now

* bump distro

* ensure proper nsfw dep

* fix more native dependencies

* temp disable failing test

* fix packages

* update deps

* fix deps

* update deps

* enable macOS unit tests again

* fix deprecated buffer use

* Electron 3.0.0.beta.5

* bump deps

* fix tree accidentally treating auxclick as click

* improve overlay cleanup scheduler (fixes flicker seen with Electron 3.0.x)

* update distro

* remove obsolete disableBlinkFeatures: 'Auxclick'

* update to Electron 3.0 beta 6

* fix compile

* workaround #56994

* do not use backgroundColor to find shared process (causes flicker)

* fix flicker on windows from shared process background color

* webview - bubble up keyboard events (workaround for #56988)

* bump electron to 3.0.0-beta.8

* webview - fix deprecation

* webview - fix another deprecation

* debt - handle SIGPIPE on more processes

* workaround more webview focus issues (for #56988)

* webview - use proper way to focus()

* debt - avoid window-focus/blur and use native focus events instead

* webview - restore previous focus method

* webview - improve focus tracking (do not rely on DOM events)

* bump to electron 3.0.0-beta.9

* update deps

* update electron@3.0.0-beta.10

* webview - do not rely on DOM focus for certain commands (for #56988)

* update to electron@3.0.0-beta.11

* electron@3.0.0-beta.12

* update to beta 13

* update to electron 3.0.0

* update to Electron 3.0.1

* electron@3.0.2

* revert build changes (node.js version)

* try with: enable mojave dark mode support

* fix types

* electron 3.0.3

* electron@3.0.4

* fix deps

* bump electron@3.0.6

* bump electron@3.0.9

* fix strict null issue

* reset format

* bump electron@3.0.10

* fix strict null issue

* webview - print error when revive fails

* electron 3.0.x - try to fix keybindings in webviews (#64417)

* bump @types/node => ^10.12.12

* 💄

* update distro
@bpasero bpasero mentioned this issue Dec 10, 2018
Closed
@bpasero
Copy link
Member Author

bpasero commented Dec 10, 2018

I have extracted #64724 into its own issue.

@bpasero bpasero closed this as completed Dec 10, 2018
@bpasero bpasero mentioned this issue Dec 10, 2018
Closed
@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bpasero bpasero

@Tyriar Tyriar

@mjbvz mjbvz

Labels
electron Issues and items related to Electron upstream Issue identified as 'upstream' component related (exists outside of VS Code) webview Webview issues
Projects
None yet
Milestone
December/January 2019
Development

No branches or pull requests
3 participants
@bpasero @Tyriar @mjbvz