zefoy / ngx-color-picker

Color picker widget for the Angular (version 2 and newer)
MIT License
457 stars 143 forks source link

ShadowRoot not expected #230

Open wilianwrech opened 4 years ago

wilianwrech commented 4 years ago

Description:

Every time I click to open the dialog an error occurs.

I don't really know how to reproduce, I think there is something to do with view encapsulation in my component, not sure.

@Component({
  ...
  encapsulation: myEncapsulation
})
export class MyComponent {
    ...
}

Error:

ERROR TypeError: "Window.getComputedStyle: Argument 1 does not implement interface Element."
    setDialogPosition ngx-color-picker.es5.js:1838
    openColorPicker ngx-color-picker.es5.js:1681
    Angular 16
    openColorPicker ngx-color-picker.es5.js:1676
    openDialog ngx-color-picker.es5.js:862
    openDialog ngx-color-picker.es5.js:2160
    inputFocus ngx-color-picker.es5.js:2257
    handleFocus ngx-color-picker.es5.js:2066

Solution:

Just fixing the error line will stop the current exception, but there is a lot of other problems using ShadowRoot, the best thing I can think of is to treat the dialog as an inline absolute element and just change its visibility, as it will become part of the Shadow, the problem is gone.

Workaround:

If someone needs a quick solution to the specific error, here is, but i recomend changing the color picker to inline display cpDialogDisplay="inline".

//Hook to support ShadowRoot in getComputedStyle
(function () {
    let originalGetComputedStyle = window.getComputedStyle;
    Object.defineProperty(window, 'getComputedStyle', {
      value: function (...args) {
        try {
          return originalGetComputedStyle.apply(this, args);
        } catch (e) {
          if (e instanceof TypeError && args.length && args[0] instanceof ShadowRoot)
            return window.getComputedStyle(args[0].activeElement, null);
          throw e;
        }
      }
    });
  })();

Version:

I am using v8.2.0, I tried v9.1.0 and the problem persist, I am not using v9.1.0 because of another open issue.

Angular CLI: 7.3.1
Node: 12.14.1
OS: win32 x64
Angular: 7.2.4
... animations, common, compiler, compiler-cli, core, elements
... forms, http, language-service, platform-browser
... platform-browser-dynamic, router

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.13.1
@angular-devkit/build-angular     0.13.9
@angular-devkit/build-optimizer   0.13.9
@angular-devkit/build-webpack     0.13.9
@angular-devkit/core              7.3.1
@angular-devkit/schematics        7.3.1
@angular/cli                      7.3.1
@ngtools/webpack                  7.3.9
@schematics/angular               7.3.1
@schematics/update                0.13.1
rxjs                              6.5.5
typescript                        3.2.4
webpack                           4.29.0
pigeonvictor commented 4 years ago

I Have the exact same problem using encapsulation: ViewEncapsulation.ShadowDom.

This is a problem for the setDialogPosition() function. It contains this loop :

while (node !== null && node.tagName !== 'HTML') {
                style = window.getComputedStyle(node);

When using shadowDom, there is no html tag, so the function fails. Any workaround possible ?

dimamarksman commented 2 years ago

Why is it closed?

It is still the issue for me.

wilianwrech commented 2 years ago

Why is it closed?

It is still the issue for me.

I think this will be always an issue, nobody implements things to work across Shadow DOMs, in fact, the purpose of using Shadow DOM is to not mix codes (ref). You could do a workaround to make this work, but if you are thinking of doing a workaround it's probably better to remove the encapsulation. In my case, due to bad decisions when creating the project (not mine though), I had to refactor the whole project because everything was using encapsulation, it took some time to do it, but it was the best decision.

dimamarksman commented 2 years ago

@sconix are you saying that popover creation inside a shadow-dom is a bad idea? What is wrong to have a popover encapsulated within shadow dom?

wilianwrech commented 2 years ago

@sconix are you saying that popover creation inside a shadow-dom is a bad idea? What is wrong to have a popover encapsulated within shadow dom?

The thing is, the color picker is being loaded in the DOM that has the Shadow DOM inside it, the purpose of using Shadow DOM is to not have any code from outside running in it. If you create a workaround, you are actually creating a bypass to the Shadow DOM feature, a hole in the encapsulation.

Conclusion: Yes, it's wrong to have a popover working inside a Shadow DOM.

dimamarksman commented 2 years ago

Ok, thank you for the explanation. Does it mean if the popover script is loaded directly inside the shadow dom the popover should work within this dom correctly? Or it's not allowed to use any external scripts inside the shadow?

wilianwrech commented 2 years ago

Ok, thank you for the explanation. Does it mean if the popover script is loaded directly inside the shadow dom the popover should work within this dom correctly? Or it's not allowed to use any external scripts inside the shadow?

Yes, exactly, everything you want to run inside the encapsulation should be loaded inside it, no matter if it's a style sheet or a script, but keep in mind, if you have hundreds of encapsulated components you will end up loading hundreds of times the same thing, breaking one of the advantages of using a single-page application framework like Angular.

dimamarksman commented 2 years ago

Even if this package will be loaded directly inside a shadow dom I don't see how the current code will work, the issue will remain the same. Please correct me if I'm wrong

wilianwrech commented 2 years ago

Even if this package will be loaded directly inside a shadow dom I don't see how the current code will work, the issue will remain the same. Please correct me if I'm wrong

I didn't try to load it inside the Shadow DOM, if it's not working that way, so maybe it could have some space for improvements. I will reopen this issue so some code maintainer can see if something can be done. This issue is from 2 years ago, and I am not using it anymore, so I don't want to invest time to fix this.