fix(overlay): overlay directives not emitting when detached externally

[crisbeto]

Currently the ConnectedOverlayDirective only emits the detach event when it thinks that the overlay is detached (escape press, backdrop click etc.), but this won't necessarily be correct (e.g. when it was closed by a scroll strategy). These changes refactor the outputs to always be one-to-one with the OverlayRef detachments.

Fixes #16779

[jelbourn]

LGTM, can add merge-ready when ready

[jelbourn]

We should stick with the string access here; Closure Compiler may attempt to rename these otherwise

[ngbot]

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

[ngbot]

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

[skoropadas]

any news?

[crisbeto]

Rebased and bumping to a P2, because we're getting new reports about this.

[DMezhenskyi]

are there any plans to merge this branch?

[andrewseguin]

After a full day's deep dive into the test failures, I think I figured out how we can get this merged in.

When the OverlayRef is being destroyed, it emits a detachment which now causes the OverlayContainer to emit on detach.

Can you add a check so that it will not emit a detach from the OverlayRef's destroy?

[crisbeto]

@andrewseguin emitting the detached event on destruction is by design. It happens since we're detaching an overlay first, before we destroy it, and it's been this way for a long time so I suspect that we might break somebody if we did it at the OverlayRef level. We could have an exception in CdkConnectedOverlay specifically, but that feels like it would be inconsistent. How many apps is the current behavior breaking and is it some kind of runtime error or does the app's logic break down?

[andrewseguin]

The issue boils down to really one component that essentially wraps our overlay directive, so if we can make it work there then I think we're good.

What's happening is that this component's view contains an <ng-template cdkConnectedOverlay>. They subscribe to the (detach) output and then emit an event that their overlay's open state has changed. Listeners to that event are then using that as a hint that they should (for whatever reason) call detectChanges().

It's there that we seem to have an issue, because that detectChanges() fails for reasons that I haven't been able to totally nail down. It seems to me that the reason is because the view contains some components that are being destroyed or have been destroyed.

Does it make sense that calling detectChanges() during an onDestroy would be dangerous? Or perhaps I'm on the wrong track and Angular handles this gracefully

Oh and something really important - it seems this is fine for client code. I'm seeing this fail during test code, and it seems that it's specifically only failing during the teardown and only when the overlay is left open. Therefore I suspect what we're seeing is that the teardown tries to start destroying everything, but the detach causes a call to detectChanges()

[crisbeto]

I believe that the framework will throw an error if you try to call detectChanges on a destroyed ChangeDetectorRef, although assuming that the view is destroyed, the detach listener should've been removed from the view as well. We've dealt with something similar in the autocomplete trigger so maybe the same approach can be applied to their project? It boils down to setting a flag in ngOnDestroy and using it to guard all of the detectChanges calls. Here's the relevant code: https://github.com/angular/components/blob/master/src/material/autocomplete/autocomplete-trigger.ts#L291

[andrewseguin]

That was my initial thinking too, but adding that across the affected components didn't fix anything. For some reason the teardown is destroying in a strange order, probably due to these cases having a weird view tree with all the amount of content projection and view insertions.

Since you are sure this logic is sound and what we want, I'll focus on making changes to these tests. While there are a dozen different failing targets, they all use the same wrapper for our overlay, so I suspect I can make adjustments in that wrapper to fix this.

[andrewseguin]

@crisbeto Please check out crisbeto#10

I managed to distill our internal tests into a single simple case that I can put into our repo. It's a little contrived, but it's because I'm trying to reduce it down as much as possible. This one test represents multiple components with tons of code.

Let me know what you think on moving forward to get this to pass.

[crisbeto]

Wow that's one convoluted bug @andrewseguin! What seems to be happening is that the automatic change detection triggers another run when the output emits, even though the related component was destroyed. It looks like the only way forward is to stop emitting the event ourselves once we hit ngOnDestroy. I've updated the code so it shouldn't emit anymore and I've added a link to your repro so we don't regress in the future. Thank you for investigating it!

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}