angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.37k stars 6.75k forks source link

angular element with overlay rendered in overlay #16862

Open rcketscientist opened 5 years ago

rcketscientist commented 5 years ago

Reproduction

https://github.com/rcketscientist/AngularElementOverlayRepro

Steps to reproduce:

  1. Create an Angular Element that leverages a CdkOverlay Material widget (tooltip, select, etc)
  2. Place that selector in a CdkOverlay (MatDialog, for example).

As soon you interact with the "inner" CdkOverlay by hovering over the tooltip or activating the select, the host CdkOverlay will just crash without an error message I can pick up on.

This might not really be a bug, but rather a limitation of Angular Elements, but being that it can be triggered with simple widgets I wonder if there's not some way I can get the Angular Elements to play well with the host.

The Angular Elements work fine when not hosted in a CdkOverlay.

Expected Behavior

What behavior were you expecting to see? CdkOverlay within Angular Element plays well with the host.

Actual Behavior

What behavior did you actually see? Acitvation of the CdkOverlay within an Angular Element causes the host CdkOverlay to silently crash.

Environment

rcketscientist commented 5 years ago

https://github.com/rcketscientist/AngularElementOverlayRepro/tree/experiment/overlayProvider

seems like a fairly functional workaround where we dodge the overlay destruction in _createContainer()

There's probably a slightly more elegant way to do this.

devversion commented 4 years ago

@rcketscientist Thanks for this issue. I created a StackBlitz out of your reproduction: https://stackblitz.com/edit/comp-16862?file=src/main.ts.

With the latest version, I don't seem to be able to see the crash you mentioned, but I see an issue where the overlay container order is incorrect and the tooltip/select from the element are behind the actual dialog overlay. It depends on what overlay container is created first and based on my understanding we cannot combine them into a single overlay container.

rcketscientist commented 4 years ago

The crash (dead overlay) does indeed seem to be gone, that's good. I had a note in our workaround that Ivy would solve this, but I don't recall at this point what led me to that assumption.

While our app architecture changed and we no longer have this issue, what I had done was this:

export class PathwaveOverlay extends OverlayContainer {
    constructor(
        @Inject(DOCUMENT) _document: any,
        @Inject(OVERLAY_NAME_TOKEN) private pluginName: string) {
        super(_document);
    }

    _createContainer(): void {
        const containerClass = 'cdk-overlay-container';

        const container = this._document.createElement('div');
        container.classList.add(containerClass);
        container.classList.add(this.pluginName);
        this._document.body.appendChild(container);
        this._containerElement = container;
    }
}
@NgModule ({
    providers: [
        { provide: OverlayContainer, useClass: PathwaveOverlay },
        { provide: OVERLAY_NAME_TOKEN, useValue: "plugin" }
    ]
})
export class PathwaveModule {
    public static forRoot(pluginName: string): ModuleWithProviders {
        return {
            ngModule: PathwaveModule,
            providers: [ { provide: OVERLAY_NAME_TOKEN, useValue: pluginName } ]
        }
    }
}

That gave us the flexibility to target the containers and avoid collisions (when that was still a crash). Doesn't feel robust enough a solution in the main repo, however.

Arnie38 commented 4 years ago

@devversion

This is Jeff Arnett from Keysight technologies (a spinoff of Hewlett Packard). This issue related to Angular Elements was well documented by a colleage last year and it threatening to jeopardize a cross platform strategy based on an Angular implementation. Can you comment on whether its been fixed in Ng10? Maybe more importantly can you comment on the general support for mat/cdk use within an Angular element?

Anything you can help us here with would be desperately appreciated?

Arnie38 commented 4 years ago

@devversion Update. I upgraded the sample above to Ng10 and see the same index issues that you are. It happens only if a cdk-overlay-container is created by the app (say a menu is dropped on the main app) and then the dialog overlay comes up (which hosts a WC based overlay). In that case there are two cdk-overlay-containers and the full Ng stack that is hosted inside the WC is creating what is essentially an "unkown to the rest of the world" overlay container that has a root/body level z-index (net is the overlay hosted in the WC inside of the main app overlay (dialog) is behind everything). Can you comment on this use of overlays in a WC hosted in a normal Ng app?

devversion commented 4 years ago

@Arnie38 The original error reported in this issue seems to have been fixed. Though there is still the issue (as you noted) of multiple overlay containers being created. This seems expected at first glance since the WC should not communicate between each other at a global level, but at the same time it would be necessary to make stacked overlays work from WCs.

I think there are two cases. (1) Angular elements are defined in the consumer application. In that case they should share the same injector to have the same overlay container. And (2) the Angular elements are defined externally (e.g. on file load within a self-contained module). In that case it's expected that they share different injectors and will have different overlay containers.

The question for us is: Should we re-use existing overlay containers (i.e. from potential other injectors/modules) if we find one, or should the Angular element creators build their own overlay container that does this? This might work here as a workaround, or sharing the injector if there is the possibility of defining the elements in the consumer application/module.

rcketscientist commented 4 years ago

It's funny, that is almost literally the conversation I had with @Arnie38. The thing I couldn't answer was how safe it was to share the overlay. To me, that's the obvious use-case, but I figured the fact it does not do that must imply there's another use-case, or a complication to re-using the injected overlay.

One thing I wasn't sure about was, "Can an ngElement share the host root injector?" I actually thought they were separate and this was a potential use-case for providedIn: platform (haven't used this yet). Or might require a custom override that queried for an existing Overlay in the DOM, but required the host to push an overlay on load to always ensure it was "first in". Not the greatest solution. However, if the injectors are available in the ngElement that's a whole 'nother story.

Why would (2) expect a different injector? Maybe this answer is tied to my ignorance on ngElements interacting with host injectors?

devversion commented 4 years ago

but I figured the fact it does not do that must imply there's another use-case, or a complication to re-using the injected overlay.

Yeah, I don't exactly recall any reason for not sharing, but that's something we need to figure out as part of this issue.

One thing I wasn't sure about was, "Can an ngElement share the host root injector?"

Could you briefly elaborate on this? Based on my understanding an Angular custom element always receives an instance of an Angular injector. e.g. when you define the custom element:

ngDoBoostap() {
  customElements.define('example-element',
     createCustomElement(ExampleComponent, { injector: this.myModuleInjector }));
}

So if you have full control over an Angular element, you could define it as part of your module bootstrap so that all share the same injector from your AppModule. That should fix the multiple overlay container issues quite nicely. e.g.

export class AppModule {
  constructor(private injector: Injector, private appRef: ApplicationRef) {}

  ngDoBootstrap() {
    // Define all Angular elements with the same injector.
    this._defineAllElements(this.injector);
    // Bootstrap the app.
    this.appRef.bootstrap(AppComponent);
  }
}

And if you're in an AngularJS upgrade scenario, you could still have a single module for all Angular elements so they share the same injector.

Why would (2) expect a different injector? Maybe this answer is tied to my ignorance on ngElements interacting with host injectors?

Sorry, I might have expressed this in a confusing way. I meant that there are cases where a Angular custom element is loaded from an external file for example. In those cases, the file/"library" already bootstraps a separate module dedicated to define the custom element. In those cases, a temporary module and injector is used to define the custom element.

e.g. node_modules/ng-element/my_angular_element.js

@NgModule(..)
class MyElementTempModule {
  ngDoBootstrap() { /* define elements */ }
}

// Bootstraps the temp module so that the Angular element is usable
// as a custom element.
platformBrowser().bootstrapModule(MyElementTempModule);
rcketscientist commented 4 years ago

Ok, we're on the same page now. The issue was that I was only thinking of loading an external element; I didn't even think of creating and using an ngElement within the same app. I'm totally blanking on the use-case there, as opposed to simply using the component. Which also happens to be my current recommendation for the above issue. Just use an angular library in an angular app and supply the ngElement for those not in Angular.

Is the new providedIn: platform with a service that shares the host's overlay a possibility?

devversion commented 4 years ago

I'm totally blanking on the use-case there, as opposed to simply using the component.

Yeah, for sure. I can't think of a good use-case from the top of my head either. The pattern remains the same though. The Angular elements would need to be defined as part of a single module w/ same injector. Could you share your concrete use-case for this issue? (so that we are on the same page; that is what I'm somehow missing still).

rcketscientist commented 4 years ago

The original use-case was a host desktop application with a window manager (custom GoldenLayout), that would dynamically load entire applications via ngElements into individual panes. The original hack above was developed to ensure that the "last in" app didn't eat the overlay, which I think went away in ng9 anyway. There were also a host of hacks hoisting overlays at times among other things that led to code that was more comment than code, lol. Thankfully, that design was abandoned in the end.

I believe @Arnie38 is attempting to distribute universal components for a cloud deployment, so consumers can BYOF(ramework). I'll let him fill in more.

Arnie38 commented 4 years ago

@devversion Thanks for all the helpful info. Here is my specific scenario... 1) Have an Ng app with overlays. Same Ng app includes a WC with its own overlays. The overlays of the main Ng app also host yet another/different WC with yet another set of overlays. This "stacked" scenario is tickling the z-index issue we have discussed. Details... 1) Ng app includes a WC implemented top/nav bar proffered from a lib. This WC has overlays (tooltips, menus, selects etc.) 2) If user of Ng app invokes any overlay based control from the WC navbar ...I see a cdk-overlay-container created 3) If Ng app also has a normal overlay based control (in this case a dialog) and user invokes it I see a 2nd overlay container to host that dialog 4) User then invokes an overlay based control found in a WC hosted in the dialog (main app) overlay 5) The content of the main app -> dialog/overlay -> WC overlay is "submarined" behind the dialog. It chooses the wrong (or creates its own) cdk-overlay container that doesn't know about the world around it and therefore seems to be a child of the root and therefore is a lower z-index than the dialog *In this scenario we would control all hosting apps and WC pieces/parts. The WC would be delivered as .js elements and host apps would really only use them if they were non angular or if using a different incompatible major version of Angular (our primary use case for using AE). Our apps range from Ng6 - Ng10 (even som Ng4 apps).

Arnie38 commented 4 years ago

@devversion So since we control everything seem like your suggested answer from above is the way to go. A bit of coding on the part of each adopter. PS This is really only an issue with AE being used in Ng apps. IOW Say react adopters would have no such headache to deal with.

export class AppModule {
  constructor(private injector: Injector, private appRef: ApplicationRef) {}

  ngDoBootstrap() {
    // Define all Angular elements with the same injector.
    this._defineAllElements(this.injector);
    // Bootstrap the app.
    this.appRef.bootstrap(AppComponent);
  }
}
Arnie38 commented 4 years ago

@devversion I tried the above with no success. Obviously that this._defineAllElements() is pseudo code for defining any all Angular components as Angular Elements here in the context of the using/consuming app. So they have a shot of all sharing the same injector. When I do that the Angular components have all kinds of dependencies and setup that the consuming app has to now know about. I was hoping to keep a very strict architectural boundary there with the host app just importing a .js of the web component. That said if this is the only way then for those adopters that have this issue (overlay that hosts a WC that has an overlay) then they'll have to make do. Any way that providedIn: Platform new feature can help rendezvous all with the same injector?

angular-robot[bot] commented 2 years ago

Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.

Find more details about Angular's feature request process in our documentation.

GitStorageOne commented 2 years ago

Everyone! We need more thumbs-up at first post. Or feature request will be doomed.
Ask friends to add thumb up.

angular-robot[bot] commented 2 years ago

Thank you for submitting your feature request! Looks like during the polling process it didn't collect a sufficient number of votes to move to the next stage.

We want to keep Angular rich and ergonomic and at the same time be mindful about its scope and learning journey. If you think your request could live outside Angular's scope, we'd encourage you to collaborate with the community on publishing it as an open source package.

You can find more details about the feature request process in our documentation.