fix(cdk/overlay): don't crash when a dropdown's trigger element disappears - #33734
fix(cdk/overlay): don't crash when a dropdown's trigger element disappears#33734arturovt wants to merge 1 commit into
Conversation
…pears What was happening: if you had a dropdown/menu/tooltip open, and the button (or whatever) that it was pointing at got removed from the page (for example, because of `*ngIf` or an `@if` going false, or the page navigating away) while the dropdown was still open, the app would crash with a confusing error like: ``` TypeError: Cannot read properties of undefined (reading 'width') ``` This could happen because Angular still tried to figure out where to draw the dropdown, but the thing it was supposed to point at was gone. Now: if the trigger element is gone, the code just skips repositioning instead of crashing. Nothing visible breaks, it just quietly does nothing since there's nothing left to point at anyway. Added a test that recreates this exact situation (a button behind an `@if` that disappears while the dropdown is still open) to make sure this doesn't crash again in the future.
| // We shouldn't do anything if the strategy was disposed, we're on the server, or the | ||
| // origin is no longer usable (e.g. it was removed from the view while the overlay | ||
| // hasn't been disposed of yet). There's nothing meaningful to position against in that case. | ||
| if (this._isDisposed || !this._platform.isBrowser || !this._isOriginUsable()) { |
There was a problem hiding this comment.
The expectation is that the consumer should call dispose when the trigger is destroyed.
There was a problem hiding this comment.
Yeah, I thought that too at first, but I don't think it actually works here.
The problem is CdkConnectedOverlay calls apply() itself, right when its own origin input changes to undefined (e.g. the trigger disappears behind an *ngIf), and open is still true at that point:
if (changes['origin'] && this.open) {
this._position.apply();
}It all happens in the same change detection cycle, so there's no real point where the consumer could've called dispose() in time — CDK gets there first.
Also, open being true isn't really a mistake on the consumer's side — the overlay is supposed to still be open, its trigger just happened to not exist for a moment. Forcing people to dispose every time that happens seems like it'd cause more harm than good (overlays closing themselves any time their trigger re-renders).
Let me know if you'd rather I move the guard into CdkConnectedOverlay instead of apply(), but I don't think "should've disposed" is something the consumer could actually act on here.
What was happening: if you had a dropdown/menu/tooltip open, and the
button (or whatever) that it was pointing at got removed from the page
(for example, because of
*ngIfor an@ifgoing false, or the pagenavigating away) while the dropdown was still open, the app would
crash with a confusing error like:
This could happen because Angular still tried to figure out where to
draw the dropdown, but the thing it was supposed to point at was gone.
Now: if the trigger element is gone, the code just skips repositioning
instead of crashing. Nothing visible breaks, it just quietly does
nothing since there's nothing left to point at anyway.
Added a test that recreates this exact situation (a button behind an
@ifthat disappears while the dropdown is still open) to make surethis doesn't crash again in the future.