monaco-editor icon indicating copy to clipboard operation
monaco-editor copied to clipboard

[Bug] HoverProvider does not work inside of Webcomponent (ShadowRoot)

Open jogibear9988 opened this issue 1 year ago • 30 comments

Reproducible in vscode.dev or in VS Code Desktop?

  • [X] Not reproducible in vscode.dev or VS Code Desktop

Reproducible in the monaco editor playground?

Monaco Editor Playground Code

No response

Reproduction Steps

Host the Monacod Editor inside of a WebComponents ShadowRoot, then the hoverProvider does not work.

Actual (Problematic) Behavior

It is cause of this code: image

It will raise the mouseLeave, cause the m.target is the dom node the component is hosted in.

Expected Behavior

hoverProvider should work

I think it should be solved using smth like

  m.composedPath()[0]  instead of m.target 

for the check

Additional Context

No response


Edit by @hediet:

Playground Example with WebComponents/Shadow Dom (broken)

Playground Example without Shadow Dom (works)

jogibear9988 avatar Nov 13 '22 20:11 jogibear9988

Think is this line:

https://github.com/microsoft/vscode/blob/b9f7f85e90b4d51b38fcbf4232801ab04bd659cb/src/vs/editor/browser/controller/mouseHandler.ts#L101

jogibear9988 avatar Nov 13 '22 20:11 jogibear9988

Can you provide a minimal sample that runs in the monaco editor playground?

It would be awesome if you would file a pull request!

hediet avatar Dec 12 '22 18:12 hediet

How should I? The Playground does not use webcomponents. And I already created a pull request.

jogibear9988 avatar Dec 12 '22 21:12 jogibear9988

There is a Playground repro in https://github.com/microsoft/monaco-editor/issues/3241

romainr avatar Dec 13 '22 23:12 romainr

Hi, Here I made an example in the playground for this issue.

abdalla-rko avatar Apr 18 '23 08:04 abdalla-rko

@aiday-mar do you work on a fix? last workin version in playground was 0.34.0-20220624

Maybe this change introduced the error: https://github.com/microsoft/vscode/pull/153111

jogibear9988 avatar May 08 '23 08:05 jogibear9988

@jogibear9988 can you revert that PR and try if that fixes it? Here is how you can test it

hediet avatar May 30 '23 14:05 hediet

I've tried it in playground in the version before this fix, and this does work.

jogibear9988 avatar Jun 14 '23 05:06 jogibear9988

Can you figure out why these changes break the hover in webcomponents? Also, feel free to create a PR that reverts the changes!

hediet avatar Jun 14 '23 10:06 hediet

Problem is here: https://github.com/microsoft/vscode/blob/d88c76383f83c60a4297277c665ea8a8ce4d72f1/src/vs/editor/contrib/hover/browser/hover.ts#L144

mouseEvent.event.browserEvent.relatedTarget is null when used inside of a webcomponent, and so _hideWidgets is called

jogibear9988 avatar Jul 03 '23 10:07 jogibear9988

@hediet I could not create a PR that reverts the changes, there was changed to much since then.

Maybe we could add a check if targetEM != null? would this work for all cases?

jogibear9988 avatar Jul 03 '23 10:07 jogibear9988

Thanks for investigating! @aiday-mar can you look into this?

hediet avatar Jul 07 '23 16:07 hediet

We use the editor sometimes inside a Web component and face this issue. We use the above patch successfully, it would be awesome to get it into master

romainr avatar Aug 09 '23 15:08 romainr

@aiday-mar any news to this issue? it is really an annoying problem. I think the null check won't break anything

jogibear9988 avatar Aug 26 '23 07:08 jogibear9988

@aiday-mar @hediet I could also create a pull req. But will only do if it has a chance to be merged. Are you excepting code from outside?

jogibear9988 avatar Sep 14 '23 16:09 jogibear9988

Hi @jogibear9988, I am currently not looking at this error (efforts are turned towards copilot work). You may of course have a try at the issue and we do accept code from outside.

aiday-mar avatar Sep 15 '23 07:09 aiday-mar

Would love to have a fix for this +1

RoenLie avatar Sep 20 '23 22:09 RoenLie

It seems like this is an ongoing issue, is there any ETA on a fix?

Also it appears using m.composedPath()[0] is a suitable workaround/fix

richallanb avatar Oct 11 '23 22:10 richallanb

@richallanb Could you explain your workaround via m.composedPath? I would really like this to work again :(

DrNiels avatar Oct 27 '23 11:10 DrNiels

I run this after a update:

//fix for monaco editor (issue https://github.com/microsoft/monaco-editor/issues/3409)
console.log("fix ./node_modules/monaco-editor/min/vs/editor/editor.main.js");
fs.readFile("./node_modules/monaco-editor/min/vs/editor/editor.main.js", function (err, buf) {
    let code = buf.toString();
    code = code.replaceAll('contains(m.target)', 'contains(m.composedPath()[0])');
    fs.writeFile("./node_modules/monaco-editor/min/vs/editor/editor.main.js", code, (err) => {
        if (err) console.log(err);
    });
});

jogibear9988 avatar Oct 27 '23 12:10 jogibear9988

but I'll look to create a pull for this in the next week

jogibear9988 avatar Oct 27 '23 12:10 jogibear9988

couldn't you merge my pull? https://github.com/microsoft/vscode/pull/166240

jogibear9988 avatar Dec 05 '23 22:12 jogibear9988

Hi @jogibear9988 your PR has some review comments which are not resolved yet with the reviewer on that PR. Once the reviewer on that PR approves the PR, it will go in.

aiday-mar avatar Dec 06 '23 09:12 aiday-mar

Thanks to @jogibear9988, the project has been greatly affected by this problem, for so long, someone helped to show the problem and even gave a solution, it is really incomprehensible that it has not been solved for so long

forrany avatar Dec 08 '23 08:12 forrany

Hi @jogibear9988 your PR has some review comments which are not resolved yet with the reviewer on that PR. Once the reviewer on that PR approves the PR, it will go in.

Please fix this problem as soon as possible, the product manager is going to kill me.

forrany avatar Dec 08 '23 08:12 forrany

At the moment I run this script after node modules update...

import fs from 'fs';

//fix for monaco editor (issue https://github.com/microsoft/monaco-editor/issues/3409)
console.log("fix ./node_modules/monaco-editor/min/vs/editor/editor.main.js");
fs.readFile("./node_modules/monaco-editor/min/vs/editor/editor.main.js", function (err, buf) {
    let code = buf.toString();
    let rgx = /(.)=>{this\.viewHelper\.viewDomNode\.contains\((.)\.target\)/;
    let newcode = code.replace(rgx, '$1=>{this.viewHelper.viewDomNode.contains($1.composedPath()[0])');
    if (code != newcode) {
        console.log("patched monaco editor");
        fs.writeFile("./node_modules/monaco-editor/min/vs/editor/editor.main.js", newcode, (err) => {
            if (err) console.log(err);
        });
    }
});

jogibear9988 avatar Dec 08 '23 10:12 jogibear9988

At the moment I run this script after node modules update...

import fs from 'fs';

//fix for monaco editor (issue https:github.com/microsoft/monaco-editor/issues/3409)
console.log("fix ./node_modules/monaco-editor/min/vs/editor/editor.main.js");
fs.readFile("./node_modules/monaco-editor/min/vs/editor/editor.main.js", function (err, buf) {
    let code = buf.toString();
    let rgx = /(.) =>{this\.viewHelper\.viewDomNode\.contains\((.) \.target\)/;
    let newcode = code.replace(rgx, '$1=>{this.viewHelper.viewDomNode.contains($1.composedPath()[0])');
    if (code != newcode) {
        console.log("patched monaco editor");
        fs.writeFile("./node_modules/monaco-editor/min/vs/editor/editor.main.js", newcode, (err) => {
            if (err) console.log(err);
        });
    }
});

It's been another two weeks and the officials still haven't merged
This solution is not a lone-term plan, which may hidden dangers in huge projects...

forrany avatar Dec 19 '23 04:12 forrany

Just chiming in this issue, it looks like in the official Monaco Editor, the 2nd demo is a Web Component with a Shadow Root attached:

https://microsoft.github.io/monaco-editor/playground.html?source=v0.47.0#example-creating-the-editor-web-component

The hover popup for the <div> tag isn't showing up.

Just replacing shadowRoot.* with this.* can fix it, but it's totally non-obvious for neophytes.

It can be confusing for people trying the playground to see that it's impossible to make a shadowy Web Component with Monaco, with the expected, essential features like IntelliSense.

So, either the demo should be removed/adapted to light DOM, or the Shadow issue fixed.

Personally, I've patched Monaco with patch-package (or PNPM patch) in the meanwhile, thanks to the neat fix provided by @jogibear9988.

Cheers

JulianCataldo avatar Apr 09 '24 21:04 JulianCataldo

@jogibear9988 looks like code is changed since then. Maybe you can update your fix and try again.

@aiday-mar what is the status on this ? We are impacted by this as well

MuhammedKalkan avatar Apr 22 '24 11:04 MuhammedKalkan

Problem here is, I already created a pull request to fix this (https://github.com/microsoft/vscode/pull/166240). It was also allready reviewed. It was complained that it wont work with closed shadowRoot (wich most of the people don't use, and also the previous code does not work with shadowRoots at all). I also would change it, but I can not get VSCode to build, and also then I've no Idea how to build the monaco package. This would be a 5 minutes task for someone of the monaco team, wich already has the build pipline set up. Also to review the suggested code from @alexdima I don't know why this could not be done from someone of the monaco team

jogibear9988 avatar Apr 23 '24 19:04 jogibear9988