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

[Plan Item] Propose new Webview API #43713

Closed
mjbvz opened this issue Feb 15, 2018 · 21 comments · Fixed by #47989
Closed

[Plan Item] Propose new Webview API #43713

mjbvz opened this issue Feb 15, 2018 · 21 comments · Fixed by #47989
Assignees
Labels
on-testplan plan-item VS Code - planned item for upcoming
Milestone

Comments

@mjbvz
Copy link
Collaborator

mjbvz commented Feb 15, 2018

Follow up on #41047 which explored improving the webview API. Part of #28263

This iteration we'd like to move the new API to be a fully documented proposed API and also self-host the markdown extension on the new API

@mjbvz mjbvz added the plan-item VS Code - planned item for upcoming label Feb 15, 2018
@mjbvz mjbvz added this to the February 2018 milestone Feb 15, 2018
@mjbvz mjbvz self-assigned this Feb 15, 2018
@mjbvz mjbvz changed the title Propose new Webview APi [Plan Item] Propose new Webview API Feb 15, 2018
@mjbvz
Copy link
Collaborator Author

mjbvz commented Feb 15, 2018

Initial version of API taken into proposed with #42690

@mjbvz
Copy link
Collaborator Author

mjbvz commented Feb 15, 2018

Proposed API changes for next iteration:

  • Add a Webview.viewColum property: readonly viewColumn?: ViewColumn;.

  • Rename WebviewOptions.keepAlive to make it more clear what this property actually does

  • Rename Webview.onMessage per Better UI extensibility: retain webviews, provide communication channel #28263 (comment)

  • Move from Webview.onDidBecomeActive/Webview.onDidBecomeInactive to an API closer to window.onDidChangeActiveTextEditor. Current proposal: window.onDidChangeActiveWebview. Would be fired with undefined when a user switches to a normal text editor. Also would be fired when a webview changes columns.

// cc @ivankravets @eamodio @pavelfeldman

mjbvz added a commit that referenced this issue Feb 16, 2018
Part of #43713

Adds a new api `onDidChangeActiveEditor` which generalizes `onDidChangeActiveTextEditor` to also detect switches to and from webviews.

This API replaces the `Webview.onBecameActive` and `Webview.onBecameInactive` events previously prototyped.
mjbvz added a commit that referenced this issue Feb 22, 2018
Part of #43713

Second try at refining the webview api. This pass specifically looks at managing webviews. Major changes:

- Adds an `id` field to webviews. The id is provided by the extension and identifies the webview. It is used with the new event handling apis

- Adds a new `onDidChangeActiveEditor` api. This is similar to `onDidChangeActiveTextEditor` but is also fired when you change webviews. It replaces the old `onFocus` and `onBlur` events on the webview itself

- Replaces `createWebview` with `getOrCreateWebview`. This new API uses the id and column together as a key. The idea is that only a single webview of id may exist in a given column

- Adds an `onDidCloseWebview` api. This is fired when a webview is closed by the user

The motivation for these changes is #27983, which tracks using the same markdown preview for any active markdown files. I believe this case is similar to how other extensions may use the webview.
@DonJayamanne
Copy link
Contributor

Deleted the comment, was using localstorage, don't need it as I can persist the webview.
👍

@DonJayamanne
Copy link
Contributor

@mjbvz Do we have an event for when a webview is closed?
Is there a way to determine whether a particular webview was closed?

Currently when using ContentProvider (via registerTextDocumentContentProvider), VSCode manages this for us with a one-to-one relationship between a URI and a window. I.e. if the window is closed and we open it again using the previewHtml, VS Code will open a new window, where as if the window isn't closed VS Code will merely activate the existing window.

With this new api, I have no way of determining whether the WebView has been closed or not and whether we need to create a new WebView for display.

@octref
Copy link
Contributor

octref commented Feb 23, 2018

Can we have API for icon/favicon of the webview?

mjbvz added a commit that referenced this issue Feb 23, 2018
Part of #43713

Third try at refining the webview api. This pass reworks  #44165.  Major changes:

- Adds an `id` field to webviews. The id is provided by the extension and identifies the webview. It is used with the new event handling apis.

- Adds a new `onDidChangeActiveEditor` api. This is similar to `onDidChangeActiveTextEditor` but is also fired when you change webviews. It replaces the old `onFocus` and `onBlur` events on the webview itself

- Adds an `onDispose` event ot webviews. This is fired when a webview is closed by the user

- Perist webview state when the editor group changes. This is enabled for all webviews, not just those with keep alive.
@mjbvz
Copy link
Collaborator Author

mjbvz commented Feb 24, 2018

@DonJayamanne Just sent out a new proposal that introduces an onDispose event on the webview: #44307

This PR introduces a different model for webviews. There is now a 1:1 correspondence between webviews and "webview editors". Previously, users could split webview editors, resulting in two different webview editors both backed by the same model. This is no longer possible

mjbvz added a commit that referenced this issue Feb 26, 2018
Part of #43713

Third try at refining the webview api. This pass reworks  #44165.  Major changes:

- Adds an `id` field to webviews. The id is provided by the extension and identifies the webview. It is used with the new event handling apis.

- Adds a new `onDidChangeActiveEditor` api. This is similar to `onDidChangeActiveTextEditor` but is also fired when you change webviews. It replaces the old `onFocus` and `onBlur` events on the webview itself

- Adds an `onDispose` event ot webviews. This is fired when a webview is closed by the user

- Perist webview state when the editor group changes. This is enabled for all webviews, not just those with keep alive.
mjbvz added a commit that referenced this issue Feb 26, 2018
* Webview API prototype 3

Part of #43713

Third try at refining the webview api. This pass reworks  #44165.  Major changes:

- Adds an `id` field to webviews. The id is provided by the extension and identifies the webview. It is used with the new event handling apis.

- Adds a new `onDidChangeActiveEditor` api. This is similar to `onDidChangeActiveTextEditor` but is also fired when you change webviews. It replaces the old `onFocus` and `onBlur` events on the webview itself

- Adds an `onDispose` event ot webviews. This is fired when a webview is closed by the user

- Perist webview state when the editor group changes. This is enabled for all webviews, not just those with keep alive.

* Throw error when trying to access disposed webview

* Improving webview documentation

* Clean up dispose management

* Throw if we receive a bad handle

* Move more event handling to input

* Simplify input updating

* Remove extra container property

* Fixing md security alert button

* Remove extra update container call

* Restore syncing of preview to active editor

* Fixing posting to webview

* Debounce preview updates

* Remove previewUri

* Enable direct window.postMessage instead of window.parent.postMessage

* Fixing scroll position not preserved when updating previews

* Revert parent.postMessage change.

Old behavior was correct

* Properly hide webview container on tab switch

* Make sure we only handle scroll events for the correct document

* Don't try setting negative scroll

* Revert vs code whitespace change
@mjbvz mjbvz mentioned this issue Feb 27, 2018
3 tasks
@mjbvz mjbvz modified the milestones: February 2018, March 2018 Mar 1, 2018
@mjbvz
Copy link
Collaborator Author

mjbvz commented Mar 13, 2018

Thanks all for the feedback! A few initial proposed changes for this month:

  • Rename Webview to WebviewEditor to make it clear that it is an editor
  • Merge vscode-extension-resource and vscode-workspace-resource schemes into vscode-resource. This should solve the cross origin problem @stef-levesque noted.
  • Add option to enable find in the webview. (should find be on or off by default?)
  • Move onDidChangeViewColumn to be a global event

This is just a first set of proposed tweaks. We'll keep refining the API throughout the month. Let me know if you have any other suggestions or feedback

Beyond webview 1.0, I'm also thinking about how the API may be able support a few additional cases:

  • Restoration, i.e. when vs code closes and then reopens, we restore the opened webviews automatically. Using resources uris for this would probably make the most sense. This ironically would be very similar to TextDocumentContentProvider which webview seeks to replace.

  • Splitting. Currently this is not supported. I think it could be added to the existing api pretty easily however.

  • Icons. Also would be easy to add to the API. Not sure if we want to allow these to be themed by an extension however

No commitment on these, but I think it is good to think about how these features would play with the currently proposed API

/cc @jrieken

@Almenon
Copy link

Almenon commented Mar 14, 2018

What percentage of the VSCode userbase uses the latest VSCode version? One thing I'm worried about with switching to the new webview is that users of older vscode versions will no longer be able to use my extension.

@jrieken
Copy link
Member

jrieken commented Mar 14, 2018

Rename Webview to WebviewEditor to make it clear that it is an editor

👎

@mjbvz
Copy link
Collaborator Author

mjbvz commented Mar 14, 2018

Please explain

@jrieken
Copy link
Member

jrieken commented Mar 14, 2018

As discussed, the WebView is not an editor but a UI-part that happens to be positioned like an editor but it must not be like that. Be prepared for showing web views in the bottom panel or as peek view

@eamodio
Copy link
Contributor

eamodio commented Mar 14, 2018

🤞 maybe even in the sidebar as its own activity? 😉 🙏

@mjbvz
Copy link
Collaborator Author

mjbvz commented Mar 14, 2018

@jrieken Our commitment here is getting a better webview api for editors and I do not think we agreed to support webviews other contexts. I see a few options on updating the API to future proof it for such without blocking the original goal:

  • Rename Webview to WebviewEditor. This gives us room to later introduce a new Webview that can be used in other UI contexts. This is the easiest

  • Try to make the current Webview interface more generic. We still would need the concept of a WebviewEditor however for events such as onDidChangeActiveEditor (or we would need to seriously rethink those) so this ends up being very similar to the next proposal.

  • Split the concept of a webview out of the existing Webview (which would renamed to WebviewEditor).

export interface Webview {
    title: string;

    html: string;

    readonly onDidReceiveMessage: Event<any>;

    postMessage(message: any): Thenable<boolean>;

    ...
}

export interface WebviewEditor {
    readonly viewColumn?: ViewColumn;

    readonly webview: Webview;

    ...
}

If we go with something like the third approach, I'd be interested in exploring a webview editor API that is more like TextDocumentContentProvider but without the baggage of a text model:

interface ContentProvider {
     provideContent(uri: Uri, token: CancellationToken): Webview;
}

function registerContentProvider(scheme: string, provider: ContentProvider): Disposable

as this would let us implement restoration.

@jrieken
Copy link
Member

jrieken commented Mar 14, 2018

do not think we agreed to support webviews other contexts

I don't think agreement is need here but projecting what will happen in the future. The request for webviews everywhere will come (if it's not already there), we can ignore it for a while, and then we'll implement it. Knowing that, we should design for it which means don't plan for deprecation nor duplication.

This gives us room to later introduce a new Webview that can be used in other UI contexts. This is the easiest

That's planning for duplication.

e still would need the concept of a WebviewEditor however for events such as onDidChangeActiveEditor (or we would need to seriously rethink those) so this ends up being very similar to the next proposal.

I might be missing something but I believe we need activeTextEditor, activeWebview, activeTreeView, activeTerminal etc. We can do that, plus all events and the enumerable types of all xyz-views. Or we come up with something more generic, like a part which is one of the pieces, e.g. type UiPart = TextEditor | WebView | TreeView | any. With that would come one event and one enumerable type, ala parts: UiPart[] and onDidChangeActivePart: Event<UiPart>. These are early ideas which need harding but I think it's the better approach.

webview editor API that is more like TextDocumentContentProvider but without the baggage of a text mode

Interesting. It would be similar to the pull model the tree view is using. I have discussed something similar with @sandy081 today because tree views started with the pull model (data provider) and are now mutating into a push model

@eamodio
Copy link
Contributor

eamodio commented Mar 14, 2018

Just for reference the request for webviews in other contexts does already exist: #43049 (there are probably other requests as well) and TBH I have a very real need for that support (a webview in at least the sidebar or panel) for a new extension I'm working on.

@mjbvz mjbvz modified the milestones: March 2018, April 2018 Mar 26, 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 _
@eamodio
Copy link
Contributor

eamodio commented Apr 19, 2018

@mjbvz Is there a way to focus a webview when its already visible? The reveal method only seems to focus if the panel isn't already in the requested column.

@mjbvz
Copy link
Collaborator Author

mjbvz commented Apr 19, 2018

This is what reveal should do. Are you on insiders?

@eamodio
Copy link
Contributor

eamodio commented Apr 19, 2018

Yeah today's. It will focus it if it changes viewcolumns but if it is already in col 3 but another tab in col 1 or 2 is focused and I reveal to col 3 it doesn't focus it

@mjbvz
Copy link
Collaborator Author

mjbvz commented Apr 19, 2018

Can you open an new issue. Also are you calling it with the view column: .reveal(webview.viewColumn)?

@eamodio
Copy link
Contributor

eamodio commented Apr 19, 2018

Out right now but will do when I get back. Basically. .reveal(ViewColumn.Three) when webview.viewColumn is also 3

@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 plan-item VS Code - planned item for upcoming
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants