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

Better UI extensibility: retain webviews, provide communication channel #28263

Closed
pavelfeldman opened this issue Jun 8, 2017 · 24 comments · Fixed by #47989
Closed

Better UI extensibility: retain webviews, provide communication channel #28263

pavelfeldman opened this issue Jun 8, 2017 · 24 comments · Fixed by #47989
Assignees
Labels
on-testplan under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@pavelfeldman
Copy link
Member

I was looking at porting some of the Chrome DevTools features over to VSCode: timeline / profiler to start with, maybe elements / sidebar.

All my controls have rich UI that I would like to surface either as editors or as views in the console drawer. I was using TextDocumentContentProvider to render my content. Some of the limitations of the extensibility system I hit were:

  • Webviews are re-created on switching editor tabs (removed from DOM?). That does not work for my rich stateful controls, would be nice to retain those. I know that reparenting is impossible with the regular iframes, not sure whether webview / oopif can make it work at this point. I can look into it on the browser side if you are interested in retaining them on your side. In Chrome DevTools we use the "hideOnDetach" trick for extensions where we remove elements from render tree, while retaining them in DOM to keep them alive.

  • Bi-directional communication channel between webview content and extension. I need to be able to post messages back and forth and existing uni-directional href=command: trick does not satisfy my use cases. A richer API that allows evaluating arbitrary strings (appending scripts) into webview would also be handy. I don't think these changes affect your security model, but they would enable us to do nice things.

  • Contribute UI views next to Terminal and Console. For some of my controls it makes sense to surface them beside editor as an auxiliary or shell view.

@jrieken
Copy link
Member

jrieken commented Jun 14, 2017

The design of VS Code is to not allow for UI extensions on that level but our API defined data contracts where extensions fill in data and the editor renderers it. In fact, extensions run in a separate, headless process to guarantee isolation. See https://code.visualstudio.com/docs/extensionAPI/patterns-and-principles#_our-approach-to-extensibility for all the details.

@jrieken jrieken added the *as-designed Described behavior is as designed label Jun 14, 2017
@jrieken jrieken assigned seanmcbreen and chrisdias and unassigned jrieken Jun 14, 2017
@pavelfeldman
Copy link
Member Author

Yes, I'm familiar with the existing policy and I am suggesting to extend it. I was thinking of making parts of Chrome DevTools work in VSCode environment as well (I'm TL of the Chrome DevTools team). And present UI extensibility model feels rather limiting. More than that, I can get pretty far with the existing policy:

  • use local server for interaction between the TextDocumentContentProvider content and extension
  • refactor some of our code to re-build based on the external state to address tab switch scenario.

But the experiences I create end up sub-optimal, at the expense of the user. If you think your longer term goal is to allow richer UI extensibility, I can look into the browser limitations that prevent you from getting there. Oopifs should allow security and performance isolation sufficient to run extensions in the UI without compromising your core experience. I know reparenting is not there, but it is more important if you are willing to get there. Alternatively, if at this point you are strong on not letting features such as Chrome's performance tooling into your framework as extensions, I'll drop the idea altogether.

@egamma
Copy link
Member

egamma commented Jun 16, 2017

@pavelfeldman we are definitely interested in experiments in this area. I'll look into this in our side and I will get back to you.

@egamma
Copy link
Member

egamma commented Jun 20, 2017

@pavelfeldman having the Chrome DevTools performance tooling available inside VS Code is very interesting to us. What do you think of the approach of pushing the integration as far as we can get with the current support and then we collaborate on improving the experience and the extension API.

I suggest to start with an integration of the perf tools in an editor using the existing htmlPreview support. This is similar to what we did for the SQL extension. The SQL extension provides rich UI to present query results. The SQL externalizes the state into an express server.

I'm adding some team members that can chime in if you are game to start on this and you have more questions:
@jrieken htmlPreview API
@weinand debugger architecture and protocol
@roblourens chrome debugger extension

@jrieken
Copy link
Member

jrieken commented Jun 22, 2017

Ok, lets brainstorm more on this. As you have said, the document provider and preview command is a way of making custom UI but was definitely not designed for that. It is what its name implies, previewing a resource. We acknowledge that it's used for building UI and time to make this more official and supported will come. We had similar ideas, in short along those lines

  • have an API function ala createWebview(name:string, location: ViewColumn | OtherLocation): WebView. That function will create a part in the UI that can be positioned in some predefined places and that can be talked to. So, the 'document preview preview command'-workaround won't be needed anymore.
  • Similar to an iframe or web worker a WebView will have some sort of communication channel, think of WebView.sendMessage and WebView.onMessage. That will allow easy bidi-communication between an extension and its UI piece, no more servers listening on localhost.

An API could look like this

interface Webview {
	readonly name: string;
	location: ViewColumn;
	markup: string;
	onDidReceiveMessage: Event<any>;
	sendMessage(message: any): Thenable<any>;
	dispose(): void;
}

export function createWebview(name: string): Webview;

Reasons why we haven't pursued that idea yet are

  • As you know, things are implemented with a WebView which is buggy and a source of frustration. For security and process isolation reasons (out mantra is that the cursor is always blinking, you can always save) we need to run things in a separate process. I'd love to learn more about OOIF, I know they weren't there when we started with this 1.5 years ago.
  • The reparenting challenge. Also, I'd love to know more about that. Please shed some light on you hideOnDetach trick. We used to have the web view sitting on the body, would hide it when not shown, and would absolutely position it when needed. Apart from slowness it is a big accessibility no-no. Also, when shuffling editors (swap column, move out, merge) around we reparent, again to keep the tab-index working.
  • Apart from the technical challenges there is look and feel. Things like font-face, size, colours etc are easy'ish. Already today, the previewed document gets those style set but thinks like widget reuse is a different story. How do we make sure extension use the native context menu, how do we align the basics like buttons, file labels. etc?
  • Also, there is source for conflicts when it comes to keybindings, link clicking, drag&drop etc. I'd say that something like Cmd+Q should always be handled in the host, where something like Cmd+C should maybe not.

In short, many questions and tradeoffs. @pavelfeldman - I'd like to hear your thoughts on this.

@petrbrzek
Copy link

Is this thing on the roadmap or it's not a priority right now? @egamma

@ivankravets
Copy link

This issue totally blocks @platformio

@egamma
Copy link
Member

egamma commented Nov 23, 2017

@petrbrzek this is our current roadmap. There are some UI API improvements on there, but opening up VS Code for general UI extensibility is not on our 6-12 month roadmap.

With regard to WebView, given our focus on performance and stability, we will investigate intoBrowserViews.

@egamma
Copy link
Member

egamma commented Nov 23, 2017

@ivankravets what is the @platformio scenario that is blocked?

@ivankravets
Copy link

ivankravets commented Nov 23, 2017

@egamma. @platformio has "PIO Home" web-based application written in React+Redux which developers use to manage embedded libraries, development platforms, and other things. PIO Home depends on backend server through WebSocket.

We have different extensions for different IDEs including for VSCode. Each IDE's extension launches own instance of PIO Home Server in the background and waits for a connection from WebView/IFrame components. It takes 2-5secs for the first loading (establish WebSocket connection ws://localhost:8xxx/yyy, configure PIO Home workspace).

Currently, we recommend using PlatformIO IDE for Atom because it can keep an opened tab with PIO Home. VSCode destroys PIO Home (with <IFRAME ../>) each time when developer switches between tabs. As result, they can't use it for the most operation because they need to wait 5 secs again and again.
A temporary solution is to split VSCode into 2 vertical Layouts and keep PIO Home in one of them. However, that is not useful with small screens.

Finally, we plan to move a lot of features to PIO Home which makes it portable between different IDEs including desktop browsers. Indeed, we need just the one option which will allow explaining VSCode to don't destroy our IFRAME.

Thanks!

@jrieken
Copy link
Member

jrieken commented Nov 24, 2017

VSCode destroys PIO Home (with <IFRAME ../>) each time when developer switches between tabs

Yeah, that unfortunate... Things run inside a webview-process because that isolates us well from extension code. The downside is that the process restarts whenever it is detached from the document, e.g reparented. This happens when you drag an editor around between two columns. It also seems to happens when selecting different tabs within an editor column. Not sure why, @bpasero might know if that on purpose or not

@bpasero
Copy link
Member

bpasero commented Nov 25, 2017

@jrieken up to the editor to decide, when the tab gets hidden clearInput is called and most editors in that case dispose their resources because the editor is no longer visible, which imho is the right thing to do.

@jrieken
Copy link
Member

jrieken commented Nov 27, 2017

Hm, this actually happens when calling setVisible(false)... Given that the webview is so fragile I'd tend to keep it alive when that happens.

@bpasero
Copy link
Member

bpasero commented Nov 27, 2017

@jrieken isn't that against our principles in some way. We never sold the webview as something that survives longer sessions. I think an extension must assume that the contents come and go dynamically.

If we wanted to have a more stable webview container for extensions, it should imho be maybe a panel that the extension can add to our panel container.

@jrieken
Copy link
Member

jrieken commented Nov 27, 2017

We never sold the webview as something that survives longer sessions.

I agree but this somehow became reality and I don't see a point in making it harder when there is already enough pain

@bpasero
Copy link
Member

bpasero commented Nov 27, 2017

@jrieken maybe this becomes opt-in so that an extension can set a flag if a webview should survive editor changes?

@bpasero
Copy link
Member

bpasero commented Nov 27, 2017

Actually one challenge will be that there could be multiple webviews open in one editor group and then that would mean to basically create multiple webviews for each extension that opened one.

@ivankravets
Copy link

maybe this becomes opt-in so that an extension can set a flag if a webview should survive editor changes?

This could help us a lot while you are looking for a better solution! Thanks!

@jrieken
Copy link
Member

jrieken commented Nov 27, 2017

lot while you are looking for a better solution!

Well, there is no better solution tbh. Yes, we can tweak the behaviour when a tab changes but as soon as webviews are re-arranged (and thereby reparented) a restart will happen. We cannot change that unfortunately and I'd recommend to keep more state inside your server such that a reconnect is fast

@ivankravets
Copy link

d I'd recommend to keep more state inside your server such that a reconnect is fast

There is not the only problem with reconnecting. We need to re-paint whole React application according to the last state each time. It requires the seconds, not the milliseconds. It depends on user's machine too, need to load "index.html", init it and only after that try to connect to the end server...

@mjbvz
Copy link
Collaborator

mjbvz commented Feb 15, 2018

@pavelfeldman I just checked in a new proposed webview API that addresses many of your points: https://github.com/Microsoft/vscode/blob/70b41f0b2dffe69f848abc0434bea9d2d11350a7/src/vs/vscode.proposed.d.ts#L528

The proposal introduces an option to keepAlive a webview so that its content not destroyed when the webview itself becomes hidden, as well as adding an official message passing API. Please take a look at the proposal and let me know if you have any questions or suggestions

@ivankravets
Copy link

The proposal introduces an option to keepAlive a webview

We can't wait to see this feature in the final release. We have over 100K downloads of our extennsion for the last months and each time users ask us about platformio/platformio-vscode-ide#32

@eamodio
Copy link
Contributor

eamodio commented Feb 15, 2018

Looks great! The onBecame* feels odd to me compared with the rest of the API tho. Maybe onDidActivate & onDidDeactivate. Also maybe onMessage could be onDidReceiveMessage

@auchenberg
Copy link
Contributor

Would the visibility events be modeled after https://developer.mozilla.org/en-US/docs/Web/API/Page_Visibility_API @mattbierner? I assume the postMessage is modeled after https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage

@mjbvz mjbvz added this to the April 2018 milestone Apr 17, 2018
mjbvz added a commit to mjbvz/vscode that referenced this issue Apr 18, 2018
mjbvz added a commit that referenced this issue Apr 19, 2018
* Promote webview Api to stable

Fixes #43713
Fixes #28263

* Rename position back to viewColumn and mark viewColumn as deprecated

This allows us to more easily re-introduce a `position` property once we have gridlayout

* Move dispose methods onto webview itself

Also better hide a few 'internal' methods / properties on the panel / webview

* Revert "Move dispose methods onto webview itself"

This reverts commit 8fab6cc.

* Move title onto webview panel

* Use _ names for private setters

* Remove unused emitter and dispose onMessageEmitter

* Preview internal emitters with _
@mjbvz mjbvz mentioned this issue Apr 23, 2018
3 tasks
@vscodebot vscodebot bot locked and limited conversation to collaborators Jun 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
on-testplan under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

Successfully merging a pull request may close this issue.