Skip to content

Commit 0c3d90e

Browse files
crisbetocrisbeto
committed
fix(dialog): fire afterClosed callback after all dialog actions are done
* Fires the afterClosed callback once the dialog is removed and the previously-focused element has been refocused. Until now the order was reversed, which prevents people from being able to refocus a different element. * Cleaned up the `MdDialogContainer` logic to simplify it and to remove the need to subscribe to zone events.
1 parent f40296e commit 0c3d90e

File tree

4 files changed

+39
-74
lines changed

4 files changed

+39
-74
lines changed

‎src/demo-app/dialog/dialog-demo.html‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
<h1>Dialog demo</h1>
22

3-
<button md-raised-button color="primary" (click)="openJazz()" [disabled]="dialogRef">
3+
<button md-raised-button color="primary" (click)="openJazz()">
44
Open dialog
55
</button>
66
<button md-raised-button color="accent" (click)="openContentElement()">

‎src/lib/dialog/dialog-container.ts‎

Lines changed: 22 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,10 @@ import {
33
ComponentRef,
44
ViewChild,
55
ViewEncapsulation,
6-
NgZone,
7-
OnDestroy,
86
Renderer,
97
ElementRef,
108
EventEmitter,
9+
OnDestroy,
1110
} from '@angular/core';
1211
import {
1312
animate,
@@ -21,11 +20,6 @@ import {BasePortalHost, ComponentPortal, PortalHostDirective, TemplatePortal} fr
2120
import {MdDialogConfig} from './dialog-config';
2221
import {MdDialogContentAlreadyAttachedError} from './dialog-errors';
2322
import {FocusTrapFactory, FocusTrap} from '../core/a11y/focus-trap';
24-
import 'rxjs/add/operator/first';
25-
26-
27-
/** Possible states for the dialog container animation. */
28-
export type MdDialogContainerAnimationState = 'void' | 'enter' | 'exit' | 'exit-start';
2923

3024

3125
/**
@@ -68,13 +62,12 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy {
6862
dialogConfig: MdDialogConfig;
6963

7064
/** State of the dialog animation. */
71-
_state: MdDialogContainerAnimationState = 'enter';
65+
_state: 'void' | 'enter' | 'exit' = 'enter';
7266

7367
/** Emits the current animation state whenever it changes. */
74-
_onAnimationStateChange = new EventEmitter<MdDialogContainerAnimationState>();
68+
_onAnimationStateChange = new EventEmitter<AnimationEvent>();
7569

7670
constructor(
77-
private _ngZone: NgZone,
7871
private _renderer: Renderer,
7972
private _elementRef: ElementRef,
8073
private _focusTrapFactory: FocusTrapFactory) {
@@ -108,7 +101,6 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy {
108101

109102
/**
110103
* Moves the focus inside the focus trap.
111-
* @private
112104
*/
113105
private _trapFocus() {
114106
if (!this._focusTrap) {
@@ -123,46 +115,38 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy {
123115
}
124116

125117
/**
126-
* Kicks off the leave animation.
127-
* @docs-private
118+
* Restores focus to the element that was focused before the dialog was opened.
128119
*/
129-
_exit(): void {
130-
this._state = 'exit';
131-
this._onAnimationStateChange.emit('exit-start');
120+
private _restoreFocus() {
121+
// We need the extra check, because IE can set the `activeElement` to null in some cases.
122+
let toFocus = this._elementFocusedBeforeDialogWasOpened;
123+
124+
if (toFocus && 'focus' in toFocus) {
125+
toFocus.focus();
126+
}
127+
128+
if (this._focusTrap) {
129+
this._focusTrap.destroy();
130+
}
132131
}
133132

134133
/**
135134
* Callback, invoked whenever an animation on the host completes.
136135
* @docs-private
137136
*/
138137
_onAnimationDone(event: AnimationEvent) {
138+
this._onAnimationStateChange.emit(event);
139+
139140
if (event.toState === 'enter') {
140141
this._trapFocus();
142+
} else if (event.toState === 'exit') {
143+
this._restoreFocus();
141144
}
142-
143-
this._onAnimationStateChange.emit(event.toState as MdDialogContainerAnimationState);
144145
}
145146

146147
ngOnDestroy() {
147-
// When the dialog is destroyed, return focus to the element that originally had it before
148-
// the dialog was opened. Wait for the DOM to finish settling before changing the focus so
149-
// that it doesn't end up back on the <body>. Also note that we need the extra check, because
150-
// IE can set the `activeElement` to null in some cases.
151-
let toFocus = this._elementFocusedBeforeDialogWasOpened as HTMLElement;
152-
153-
// We shouldn't use `this` inside of the NgZone subscription, because it causes a memory leak.
154-
let animationStream = this._onAnimationStateChange;
155-
156-
this._ngZone.onMicrotaskEmpty.first().subscribe(() => {
157-
if (toFocus && 'focus' in toFocus) {
158-
toFocus.focus();
159-
}
160-
161-
animationStream.complete();
162-
});
163-
164-
if (this._focusTrap) {
165-
this._focusTrap.destroy();
166-
}
148+
// This needs to happen in OnDestroy, because the exit->void animation event
149+
// won't fire if the consumer is using the NoopAnimationsModule
150+
this._onAnimationStateChange.complete();
167151
}
168152
}

‎src/lib/dialog/dialog-ref.ts‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
import {OverlayRef, GlobalPositionStrategy} from '../core';
2+
import {AnimationEvent} from '@angular/animations';
23
import {DialogPosition} from './dialog-config';
34
import {Observable} from 'rxjs/Observable';
45
import {Subject} from 'rxjs/Subject';
5-
import {MdDialogContainer, MdDialogContainerAnimationState} from './dialog-container';
6+
import {MdDialogContainer} from './dialog-container';
7+
import 'rxjs/add/operator/filter';
68

79

810
// TODO(jelbourn): resizing
@@ -23,17 +25,14 @@ export class MdDialogRef<T> {
2325
private _result: any;
2426

2527
constructor(private _overlayRef: OverlayRef, public _containerInstance: MdDialogContainer) {
26-
_containerInstance._onAnimationStateChange.subscribe(
27-
(state: MdDialogContainerAnimationState) => {
28-
if (state === 'exit-start') {
29-
// Transition the backdrop in parallel with the dialog.
30-
this._overlayRef.detachBackdrop();
31-
} else if (state === 'exit') {
32-
this._overlayRef.dispose();
33-
this._afterClosed.next(this._result);
34-
this._afterClosed.complete();
35-
this.componentInstance = null;
36-
}
28+
_containerInstance._onAnimationStateChange
29+
.filter((event: AnimationEvent) => event.toState === 'exit')
30+
.subscribe(() => {
31+
this._overlayRef.dispose();
32+
this.componentInstance = null;
33+
}, null, () => {
34+
this._afterClosed.next(this._result);
35+
this._afterClosed.complete();
3736
});
3837
}
3938

@@ -43,7 +42,8 @@ export class MdDialogRef<T> {
4342
*/
4443
close(dialogResult?: any): void {
4544
this._result = dialogResult;
46-
this._containerInstance._exit();
45+
this._overlayRef.detachBackdrop(); // Transition the backdrop in parallel with the dialog.
46+
this._containerInstance._state = 'exit';
4747
}
4848

4949
/**

‎src/lib/dialog/dialog.spec.ts‎

Lines changed: 3 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,9 @@ describe('MdDialog', () => {
121121
dialogRef.close('Charmander');
122122
viewContainerFixture.detectChanges();
123123

124+
// Callback should not be called too early.
125+
expect(afterCloseCallback).not.toHaveBeenCalled();
126+
124127
viewContainerFixture.whenStable().then(() => {
125128
expect(afterCloseCallback).toHaveBeenCalledWith('Charmander');
126129
expect(overlayContainerElement.querySelector('md-dialog-container')).toBeNull();
@@ -335,28 +338,6 @@ describe('MdDialog', () => {
335338
expect(dialogContainer._state).toBe('exit');
336339
});
337340

338-
it('should emit an event with the proper animation state', async(() => {
339-
let dialogRef = dialog.open(PizzaMsg, { viewContainerRef: testViewContainerRef });
340-
let dialogContainer: MdDialogContainer =
341-
viewContainerFixture.debugElement.query(By.directive(MdDialogContainer)).componentInstance;
342-
let spy = jasmine.createSpy('animation state callback');
343-
344-
dialogContainer._onAnimationStateChange.subscribe(spy);
345-
viewContainerFixture.detectChanges();
346-
347-
viewContainerFixture.whenStable().then(() => {
348-
expect(spy).toHaveBeenCalledWith('enter');
349-
350-
dialogRef.close();
351-
viewContainerFixture.detectChanges();
352-
expect(spy).toHaveBeenCalledWith('exit-start');
353-
354-
viewContainerFixture.whenStable().then(() => {
355-
expect(spy).toHaveBeenCalledWith('exit');
356-
});
357-
});
358-
}));
359-
360341
describe('passing in data', () => {
361342
it('should be able to pass in data', () => {
362343
let config = {

0 commit comments

Comments
 (0)