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

Notebook: Error in dev tools when moving editor #99255

Closed
aeschli opened this issue Jun 3, 2020 · 5 comments
Closed

Notebook: Error in dev tools when moving editor #99255

aeschli opened this issue Jun 3, 2020 · 5 comments
Assignees
@rebornix @mjbvz
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders notebook webview Webview issues
Milestone
June 2020

Comments

@aeschli
Copy link
Contributor

aeschli commented Jun 3, 2020 •
edited

Testing #98985

  • create a Github notebook, open it besides an other editor
  • drag the notebook below the other editor
  • drag seems to have worked, but dev tools console says:
baseWebviewElement.ts:186 Error: The WebView must be attached to the DOM and the dom-ready event emitted before this method can be called.
    at HTMLElement.WebViewElement.getWebContentsId (web-view-impl.ts:222)
    at HTMLElement.send (web-view-impl.ts:259)
    at t.postMessage (webviewElement.ts:443)
@rebornix
Copy link
Member

rebornix commented Jun 3, 2020

@deepak1556 @mjbvz I often see this error and since I'm just using the norma webview element, I don't know what I did wrong.

@rebornix rebornix added webview Webview issues notebook labels Jun 3, 2020
@deepak1556
Copy link
Contributor

deepak1556 commented Jun 3, 2020 •
edited

The error indicates a webview method is being called before the webview was attached to the DOM or either after webview was destroyed. The stack trace indicates its happening for <webview>.send. The check is to ensure methods are not processed on the renderer side when the backing webContents are either not created/destroyed in the browser process.

We seem to anchor webview methods on presence of the webview element this.element?.send instead we should have a state that is updated when dom-ready, close, destroyed events are fired to use the webview methods safely.

@rebornix rebornix added this to the June 2020 milestone Jun 5, 2020
@rebornix
Copy link
Member

rebornix commented Jun 5, 2020 •
edited

Found a stable reproduce with a notebook containing fancy html outputs. After the notebook is opened, click into the notebook, if you click on the webview, the preload main.js will emit did-focus message

vscode/src/vs/workbench/contrib/webview/browser/pre/main.js

Line 596 in 6fef595

onFocus: () => host.postMessage('did-focus'),

and then it will try to ignore menu shortcut on all webviews, which will lead to the error

vscode/src/vs/workbench/contrib/webview/electron-browser/webviewElement.ts

Lines 234 to 237 in 6fef595

case 'did-focus':
this.setIgnoreMenuShortcuts(true);
break;

In this WebviewKeyboardHandler I don't see an existing way of checking dom-ready so need @mjbvz 's help here.

One thing I'm still not clear about the webview is actually attached to the DOM, otherwise how did I click on it?

@rebornix rebornix added the debt Code quality issues label Jun 7, 2020
@rebornix
Copy link
Member

rebornix commented Jun 8, 2020

It's actually a bug in Notebook. When we create webview we mount it to a detached DOM node which will be attached to DOM later on. Also we didn't dispose the webview properly after editor widget is closed. Both of which will contribute to this error message.

@deepak1556
Copy link
Contributor

Awesome, thanks for addressing it 👍

@github-actions github-actions bot locked and limited conversation to collaborators Jul 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@rebornix rebornix

@mjbvz mjbvz

Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders notebook webview Webview issues
Projects
None yet
Milestone
June 2020
Development

No branches or pull requests
5 participants
@rebornix @deepak1556 @aeschli @mjbvz and others