Skip to content

Commit 9d883ff

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 9d883ff

File tree

4 files changed

+91
-95
lines changed

4 files changed

+91
-95
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: 28 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,6 @@ import {BasePortalHost, ComponentPortal, PortalHostDirective, TemplatePortal} fr
2121
import {MdDialogConfig} from './dialog-config';
2222
import {MdDialogContentAlreadyAttachedError} from './dialog-errors';
2323
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';
2924

3025

3126
/**
@@ -68,10 +63,10 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy {
6863
dialogConfig: MdDialogConfig;
6964

7065
/** State of the dialog animation. */
71-
_state: MdDialogContainerAnimationState = 'enter';
66+
_state: 'void' | 'enter' | 'exit' = 'enter';
7267

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

7671
constructor(
7772
private _ngZone: NgZone,
@@ -108,7 +103,6 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy {
108103

109104
/**
110105
* Moves the focus inside the focus trap.
111-
* @private
112106
*/
113107
private _trapFocus() {
114108
if (!this._focusTrap) {
@@ -123,46 +117,46 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy {
123117
}
124118

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

134135
/**
135136
* Callback, invoked whenever an animation on the host completes.
136137
* @docs-private
137138
*/
138139
_onAnimationDone(event: AnimationEvent) {
140+
this._onAnimationStateChange.emit(event);
141+
139142
if (event.toState === 'enter') {
140143
this._trapFocus();
144+
} else if (event.toState === 'exit') {
145+
this._restoreFocus();
146+
} else if (event.toState === 'void' && event.fromState === 'exit') {
147+
this._onAnimationStateChange.complete();
141148
}
142-
143-
this._onAnimationStateChange.emit(event.toState as MdDialogContainerAnimationState);
144149
}
145150

146151
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();
152+
// We need to do this manually in OnDestroy again, because the exit->void callback
153+
// won't fire if the consumer is using the NoopAnimationsModule.
154+
this._ngZone.runOutsideAngular(() => {
155+
setTimeout(() => {
156+
if (!this._onAnimationStateChange.closed) {
157+
this._onAnimationStateChange.complete();
158+
}
159+
});
162160
});
163-
164-
if (this._focusTrap) {
165-
this._focusTrap.destroy();
166-
}
167161
}
168162
}

‎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: 49 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -113,18 +113,19 @@ describe('MdDialog', () => {
113113
expect(dialogContainerElement.getAttribute('role')).toBe('alertdialog');
114114
});
115115

116-
it('should close a dialog and get back a result', async(() => {
116+
it('should close a dialog and get back a result', fakeAsync(() => {
117117
let dialogRef = dialog.open(PizzaMsg, { viewContainerRef: testViewContainerRef });
118118
let afterCloseCallback = jasmine.createSpy('afterClose callback');
119119

120120
dialogRef.afterClosed().subscribe(afterCloseCallback);
121121
dialogRef.close('Charmander');
122+
123+
tick(500);
122124
viewContainerFixture.detectChanges();
125+
tick();
123126

124-
viewContainerFixture.whenStable().then(() => {
125-
expect(afterCloseCallback).toHaveBeenCalledWith('Charmander');
126-
expect(overlayContainerElement.querySelector('md-dialog-container')).toBeNull();
127-
});
127+
expect(afterCloseCallback).toHaveBeenCalledWith('Charmander');
128+
expect(overlayContainerElement.querySelector('md-dialog-container')).toBeNull();
128129
}));
129130

130131

@@ -168,32 +169,26 @@ describe('MdDialog', () => {
168169
})).toBe(ref);
169170
});
170171

171-
it('should notify the observers if all open dialogs have finished closing', async(() => {
172-
const ref1 = dialog.open(PizzaMsg, {
173-
viewContainerRef: testViewContainerRef
174-
});
175-
const ref2 = dialog.open(ContentElementDialog, {
176-
viewContainerRef: testViewContainerRef
177-
});
178-
let allClosed = false;
172+
it('should notify the observers if all open dialogs have finished closing', fakeAsync(() => {
173+
const ref1 = dialog.open(PizzaMsg, { viewContainerRef: testViewContainerRef });
174+
const ref2 = dialog.open(ContentElementDialog, { viewContainerRef: testViewContainerRef });
175+
const allClosedCallback = jasmine.createSpy('afterAllClosed callback');
179176

180-
dialog.afterAllClosed.subscribe(() => {
181-
allClosed = true;
182-
});
177+
dialog.afterAllClosed.subscribe(allClosedCallback);
183178

184179
ref1.close();
180+
tick(500);
185181
viewContainerFixture.detectChanges();
182+
tick();
186183

187-
viewContainerFixture.whenStable().then(() => {
188-
expect(allClosed).toBeFalsy();
184+
expect(allClosedCallback).not.toHaveBeenCalled();
189185

190-
ref2.close();
191-
viewContainerFixture.detectChanges();
186+
ref2.close();
187+
tick(500);
188+
viewContainerFixture.detectChanges();
189+
tick();
192190

193-
viewContainerFixture.whenStable().then(() => {
194-
expect(allClosed).toBeTruthy();
195-
});
196-
});
191+
expect(allClosedCallback).toHaveBeenCalled();
197192
}));
198193

199194
it('should should override the width of the overlay pane', () => {
@@ -335,28 +330,6 @@ describe('MdDialog', () => {
335330
expect(dialogContainer._state).toBe('exit');
336331
});
337332

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-
360333
describe('passing in data', () => {
361334
it('should be able to pass in data', () => {
362335
let config = {
@@ -463,13 +436,42 @@ describe('MdDialog', () => {
463436
dialogRef.close();
464437
tick(500);
465438
viewContainerFixture.detectChanges();
466-
flushMicrotasks();
439+
tick();
467440

468441
expect(document.activeElement.id)
469442
.toBe('dialog-trigger', 'Expected that the trigger was refocused after dialog close');
470443

471444
document.body.removeChild(button);
472445
}));
446+
447+
it('should allow the consumer to shift focus in afterClosed', fakeAsync(() => {
448+
// Create a element that has focus before the dialog is opened.
449+
let button = document.createElement('button');
450+
let input = document.createElement('input');
451+
452+
button.id = 'dialog-trigger';
453+
input.id = 'input-to-be-focused';
454+
455+
document.body.appendChild(button);
456+
document.body.appendChild(input);
457+
button.focus();
458+
459+
let dialogRef = dialog.open(PizzaMsg, { viewContainerRef: testViewContainerRef });
460+
461+
dialogRef.afterClosed().subscribe(() => input.focus());
462+
463+
dialogRef.close();
464+
tick(500);
465+
viewContainerFixture.detectChanges();
466+
tick();
467+
468+
expect(document.activeElement.id).toBe('input-to-be-focused',
469+
'Expected that the trigger was refocused after dialog close');
470+
471+
document.body.removeChild(button);
472+
document.body.removeChild(input);
473+
}));
474+
473475
});
474476

475477
describe('dialog content elements', () => {

0 commit comments

Comments
 (0)