microsoft / monaco-editor

A browser based code editor
https://microsoft.github.io/monaco-editor/
MIT License
40.03k stars 3.57k forks source link

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

Open jogibear9988 opened 1 year ago

jogibear9988 commented 1 year ago

Reproducible in vscode.dev or in 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 commented 1 year ago

Think is this line:

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

hediet commented 1 year ago

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

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

jogibear9988 commented 1 year ago

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

romainr commented 1 year ago

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

abdalla-rko commented 1 year ago

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

jogibear9988 commented 1 year ago

@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

hediet commented 1 year ago

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

jogibear9988 commented 1 year ago

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

hediet commented 1 year ago

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

jogibear9988 commented 1 year ago

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 commented 1 year ago

@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?

hediet commented 1 year ago

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

romainr commented 1 year ago

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

jogibear9988 commented 1 year ago

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

jogibear9988 commented 1 year ago

@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?

aiday-mar commented 1 year ago

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.

RoenLie commented 1 year ago

Would love to have a fix for this +1

richallanb commented 11 months ago

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

DrNiels commented 11 months ago

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

jogibear9988 commented 11 months ago

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 commented 11 months ago

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

jogibear9988 commented 10 months ago

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

aiday-mar commented 10 months ago

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.

forrany commented 9 months ago

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 commented 9 months ago

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.

jogibear9988 commented 9 months ago

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);
        });
    }
});
forrany commented 9 months ago

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...

JulianCataldo commented 5 months ago

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

MuhammedKalkan commented 5 months ago

@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

jogibear9988 commented 5 months ago

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