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

complete support of webviews #6465

Merged
merged 21 commits into from
Nov 22, 2019
Merged

complete support of webviews #6465

merged 21 commits into from
Nov 22, 2019

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented Oct 29, 2019

What it does

How to test

Use following VS Code extensions:

Review checklist

Reminder for reviewers

@akosyakov akosyakov added vscode issues related to VSCode compatibility webviews issues related to webviews labels Oct 29, 2019
@akosyakov akosyakov force-pushed the ak/vscode_webviews branch 6 times, most recently from b504bc6 to 838c71d Compare October 30, 2019 14:48
test.http Outdated Show resolved Hide resolved
@akosyakov akosyakov force-pushed the ak/vscode_webviews branch 3 times, most recently from 2f86eed to f2660f8 Compare November 1, 2019 14:41
@akosyakov
Copy link
Member Author

akosyakov commented Nov 6, 2019

@BroKun @amiramw @502647092 Could you check please how well this PR works for your webviews? Also whether you have any issues with running it on windows (if it is applicable to your case).

@akosyakov
Copy link
Member Author

@marcdumais-work I had to copy webview iframe code from 1.40.0 version. It is not available in previous versions. CQ: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=21051

@akosyakov
Copy link
Member Author

akosyakov commented Nov 7, 2019

@olexii4 @benoitf Could you point please to theia plugins contributing webview for testing?

Important difference in this PR that it switches to vscode way of styling webviews, i.e. https://code.visualstudio.com/api/extension-guides/webview#theming-webview-content

@akosyakov
Copy link
Member Author

akosyakov commented Nov 8, 2019

A CQ was already approved: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=21051#c3 🏎

@akosyakov akosyakov force-pushed the ak/vscode_webviews branch 8 times, most recently from 5cd1432 to 730158f Compare November 12, 2019 08:23
@benoitf
Copy link
Contributor

benoitf commented Nov 12, 2019

@akosyakov
Copy link
Member Author

akosyakov commented Nov 22, 2019

Merging, please file issues to ask for help. From my experience for last weeks, webview is not trivial thing from user (especially to make it work remotely with localhost services) and implementation perspectives.

@akosyakov akosyakov merged commit fc49b2c into master Nov 22, 2019
@marcdumais-work
Copy link
Contributor

Thanks for your hard work on this feature @akosyakov !


protected async resolveCache(): Promise<void> {
try {
this.cache.resolve(await caches.open('webview:v1'));

Choose a reason for hiding this comment

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

Hi,

Where should 'caches' be defined?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question - I didn't know either so I opened this file in Gitpod and using "Peek Definition" from the context menu, over that symbol, I found it's from the DOM API:

image

So this is implemented in/by your web browser. Hope this help.

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s a browser API. It’s not available over http, webviews work only over https or localhost for testing.

Copy link
Member

Choose a reason for hiding this comment

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

Just a thought, but maybe prefixing such variables like window.caches would help readers?

@DanTup
Copy link
Contributor

DanTup commented Nov 28, 2019

@akosyakov I was just doing some testing on GitPod (so I don't know if this PR is deployed there), but calling openExternal seems to not map the URL (eg. I'm calling it with localhost and it's spawning a new browser tab that's pointing at localhost rather than the exposed URL).

I don't know if it's just because there are undeployed changes, or if this is something that isn't done yet. openExternal should internally do the same as asExternalUri (expose and transform the URI) prior to opening the window.

@akosyakov
Copy link
Member Author

akosyakov commented Nov 28, 2019

@DanTup this PR was not deployed in Gitpod yet, I think Gitpod is targeting next week to get it there cc @svenefftinge

openExternal should internally do the same as asExternalUri (expose and transform the URI) prior to opening the window.

yes, it is how it's implemented:

async open(uri: URI): Promise<Window | undefined> {

@DanTup
Copy link
Contributor

DanTup commented Nov 28, 2019

Ah great - I did look through the code but couldn't find the implementation. Thanks!

@akosyakov akosyakov deleted the ak/vscode_webviews branch January 3, 2020 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment