Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cdk/overlay): backdrop timeouts not being cleared in some cases #23972

Merged
merged 1 commit into from
Mar 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/cdk/overlay/_index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,18 @@ $backdrop-animation-timing-function: cubic-bezier(0.25, 0.8, 0.25, 1) !default;
}

.cdk-overlay-transparent-backdrop {
// Define a transition on the visibility so that the `transitionend` event can fire immediately.
transition: visibility 1ms linear, opacity 1ms linear;
visibility: hidden;
opacity: 1;

// Note: as of Firefox 57, having the backdrop be `background: none` will prevent it from
// capturing the user's mouse scroll events. Since we also can't use something like
// `rgba(0, 0, 0, 0)`, we work around the inconsistency by not setting the background at
// all and using `opacity` to make the element transparent.
&, &.cdk-overlay-backdrop-showing {
&.cdk-overlay-backdrop-showing {
opacity: 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was the opacity: 0 removed? this seems to be breaking screenshots in some apps

Copy link
Member Author

@crisbeto crisbeto Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while since I opened the PR, but I think it was because the opacity would actually cause the transition to run, even though it's animating from transparent to transparent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there may be something not quite right about it. When I presubmitted this I wound up with some apps that had opaque backdrops covering everything

visibility: visible;
}
}

Expand Down
36 changes: 18 additions & 18 deletions src/cdk/overlay/overlay-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,17 @@ export type ImmutableObject<T> = {
*/
export class OverlayRef implements PortalOutlet, OverlayReference {
private _backdropElement: HTMLElement | null = null;
private _backdropTimeout: number | undefined;
private readonly _backdropClick = new Subject<MouseEvent>();
private readonly _attachments = new Subject<void>();
private readonly _detachments = new Subject<void>();
private _positionStrategy: PositionStrategy | undefined;
private _scrollStrategy: ScrollStrategy | undefined;
private _locationChanges: SubscriptionLike = Subscription.EMPTY;
private _backdropClickHandler = (event: MouseEvent) => this._backdropClick.next(event);
private _backdropTransitionendHandler = (event: TransitionEvent) => {
this._disposeBackdrop(event.target as HTMLElement | null);
};

/**
* Reference to the parent of the `_host` at the time it was detached. Used to restore
Expand Down Expand Up @@ -418,26 +422,10 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
return;
}

let timeoutId: number;
const finishDetach = () => {
// It may not be attached to anything in certain cases (e.g. unit tests).
if (backdropToDetach) {
backdropToDetach.removeEventListener('click', this._backdropClickHandler);
backdropToDetach.removeEventListener('transitionend', finishDetach);
this._disposeBackdrop(backdropToDetach);
}

if (this._config.backdropClass) {
this._toggleClasses(backdropToDetach!, this._config.backdropClass, false);
}

clearTimeout(timeoutId);
};

backdropToDetach.classList.remove('cdk-overlay-backdrop-showing');

this._ngZone.runOutsideAngular(() => {
backdropToDetach!.addEventListener('transitionend', finishDetach);
backdropToDetach!.addEventListener('transitionend', this._backdropTransitionendHandler);
});

// If the backdrop doesn't have a transition, the `transitionend` event won't fire.
Expand All @@ -447,7 +435,12 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
// Run this outside the Angular zone because there's nothing that Angular cares about.
// If it were to run inside the Angular zone, every test that used Overlay would have to be
// either async or fakeAsync.
timeoutId = this._ngZone.runOutsideAngular(() => setTimeout(finishDetach, 500));
this._backdropTimeout = this._ngZone.runOutsideAngular(() =>
setTimeout(() => {
console.log('fallback');
this._disposeBackdrop(backdropToDetach);
}, 500),
);
}

/** Toggles a single CSS class or an array of classes on an element. */
Expand Down Expand Up @@ -505,6 +498,8 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
/** Removes a backdrop element from the DOM. */
private _disposeBackdrop(backdrop: HTMLElement | null) {
if (backdrop) {
backdrop.removeEventListener('click', this._backdropClickHandler);
backdrop.removeEventListener('transitionend', this._backdropTransitionendHandler);
backdrop.remove();

// It is possible that a new portal has been attached to this overlay since we started
Expand All @@ -514,6 +509,11 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
this._backdropElement = null;
}
}

if (this._backdropTimeout) {
clearTimeout(this._backdropTimeout);
this._backdropTimeout = undefined;
}
}
}

Expand Down
10 changes: 10 additions & 0 deletions src/cdk/overlay/overlay.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,16 @@ describe('Overlay', () => {
// `fakeAsync` will throw if we have an unflushed timer.
}));

it('should clear the backdrop timeout if the overlay is disposed', fakeAsync(() => {
const overlayRef = overlay.create({hasBackdrop: true});
overlayRef.attach(componentPortal);
overlayRef.detach();
overlayRef.dispose();

// Note: we don't `tick` or `flush` here. The assertion is that
// `fakeAsync` will throw if we have an unflushed timer.
}));

it('should be able to use the `Overlay` provider during app initialization', () => {
/** Dummy provider that depends on `Overlay`. */
@Injectable()
Expand Down