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

Explore moving webcontents around when moving editors with webviews #5257

Closed
jrieken opened this issue Apr 14, 2016 · 10 comments
Closed

Explore moving webcontents around when moving editors with webviews #5257

jrieken opened this issue Apr 14, 2016 · 10 comments
Assignees
Labels
debt Code quality issues *out-of-scope Posted issue is not in scope of VS Code webview Webview issues workbench-editors Managing of editor widgets in workbench window

Comments

@jrieken
Copy link
Member

jrieken commented Apr 14, 2016

I know you were waiting for it. Please don't reparent editors during slot assignment because some dom element don't survive such action.

Related to #5039

@bpasero bpasero changed the title Don't reparent editors when shuffled Prevent DOM reparenting when moving editors around Apr 15, 2016
@bpasero bpasero added the bug Issue identified by VS Code Team member as probable bug label Apr 15, 2016
@bpasero bpasero added this to the Backlog milestone Apr 15, 2016
@bpasero
Copy link
Member

bpasero commented Apr 15, 2016

Idea is to maybe use absolute positioning (or flex layout?). Potential issues I see:

  • accessibility with tab navigation will be broken if the order of elements in the DOM is different from the visual order
  • layout might flicker more if the elements moved are not in order in the DOM as visually appearing (have seen this with the panel and editor area before)

@bpasero bpasero changed the title Prevent DOM reparenting when moving editors around Explore moving webcontents around when moving editors with webviews Oct 13, 2016
@bpasero
Copy link
Member

bpasero commented Oct 13, 2016

Electron adds support to move webcontents between webview instances (electron/electron#7157). We should explore if this is a solution for the problem.

@bpasero bpasero added electron-update upstream Issue identified as 'upstream' component related (exists outside of VS Code) debt Code quality issues and removed bug Issue identified by VS Code Team member as probable bug labels Oct 13, 2016
@mikes-gh
Copy link

In vscode 1.8 / electron 1.4.6 changing tabs back and forth still gives a new guestintance id for an open webview each time the web view is shown hidden.
AFAIK it should b e possible to keep the guestinstance id and prevent the reload now with electron update?

@bpasero bpasero added electron Issues and items related to Electron and removed electron-update labels Mar 8, 2017
@bpasero bpasero removed the upstream Issue identified as 'upstream' component related (exists outside of VS Code) label Apr 10, 2017
@bpasero bpasero added upstream Issue identified as 'upstream' component related (exists outside of VS Code) webview Webview issues and removed workbench electron Issues and items related to Electron upstream Issue identified as 'upstream' component related (exists outside of VS Code) labels Nov 12, 2017
@bpasero bpasero removed this from the Backlog milestone Nov 15, 2017
@bpasero bpasero added the workbench-editors Managing of editor widgets in workbench window label Nov 15, 2017
@bpasero bpasero assigned mjbvz and unassigned bpasero Mar 8, 2018
@bpasero
Copy link
Member

bpasero commented Mar 8, 2018

@mjbvz it looks like you solved it with your new approach of positioning the webview absolute?

@bpasero
Copy link
Member

bpasero commented Mar 8, 2018

@mjbvz fyi this might be interesting to you: electron/electron#7157

When I played with it I could not really get it to work. Nevertheless I think #5257 (comment) applies to your solution that you picked now.

@mjbvz
Copy link
Collaborator

mjbvz commented Mar 8, 2018

Yes we can explore using guestInstance. Accessibility seem to work using the existing absolute position approach but there are a few edge cases around positioning as you found

We also need to be able to keep webviews alive when their editor is no longer visible. We may be able to use a hidden webview element as a cache for this case

@mjbvz
Copy link
Collaborator

mjbvz commented Mar 13, 2018

Blocked by electron/electron#12251 . Out editors destroy their content before webview has a chance to transfer itself over to the new webview inside the new editor. Not sure if there is any workaround

@bpasero
Copy link
Member

bpasero commented May 24, 2018

@mjbvz not sure if it makes your life easier, but with the introduction of grid layout, an editor will always stay fixed to its parent container, even if the view is moved around. Of course, in the end if the view is moved, the grid widget will still remove the element from the DOM and put it somewhere else. I am also not sure if you are still using the hack to position webviews absolutely, maybe you could test this in the grid branch (ben/editor).

@mjbvz
Copy link
Collaborator

mjbvz commented May 24, 2018

@bpasero Thanks. I think we may have to stick with the absolute positioning in order to support the retainContextWhileHidden option that keeps webviews alive while they are hidden

@bpasero
Copy link
Member

bpasero commented Aug 21, 2018

It looks like electron/electron#12251 was closed and will not be supported with Electron 3.0.x onwards.

@mjbvz mjbvz closed this as completed Sep 10, 2018
@mjbvz mjbvz added the *out-of-scope Posted issue is not in scope of VS Code label Sep 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Code quality issues *out-of-scope Posted issue is not in scope of VS Code webview Webview issues workbench-editors Managing of editor widgets in workbench window
Projects
None yet
Development

No branches or pull requests

4 participants