From 5967f6e59c90516849a45a676e49995c8073f723 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Thu, 3 Aug 2017 01:17:42 +0200 Subject: [PATCH] fix(dialog): better handling of custom ViewContainerRef with OnPush change detection (#6164) * Adds a `markForCheck` call after starting the dialog animation, in order to ensure that it animates away if it was opened from a component with OnPush change detection. * Since we can't know whether the animation will start right after the `markForCheck`, these changes switch to starting the backdrop animation together with the dialog animation. This avoids some slight timing issues where the backdrop can start its animation a little too early. * Simplifies the dialog animation streams to avoid having to subscribe to the `completed` callback in the `MdDialogRef`. * Fixes the demo app sidenav jumping to the bottom when it is opened from the homepage. * Fixes some alignment in the dialog demo that got thrown off by the new input width. The dialog changes should solve issues like #5118. --- src/demo-app/demo-app/demo-app.html | 3 +- src/demo-app/dialog/dialog-demo.scss | 6 +-- src/lib/datepicker/datepicker.ts | 3 +- src/lib/dialog/dialog-container.ts | 33 ++++++++++----- src/lib/dialog/dialog-ref.ts | 21 ++++++---- src/lib/dialog/dialog.spec.ts | 63 ++++++++++++++++++++++++---- 6 files changed, 97 insertions(+), 32 deletions(-) diff --git a/src/demo-app/demo-app/demo-app.html b/src/demo-app/demo-app/demo-app.html index dfa257be65a1..27f212ecf165 100644 --- a/src/demo-app/demo-app/demo-app.html +++ b/src/demo-app/demo-app/demo-app.html @@ -14,12 +14,13 @@
Baseline - +
diff --git a/src/demo-app/dialog/dialog-demo.scss b/src/demo-app/dialog/dialog-demo.scss index 6f7f4cac254a..0362f55fe973 100644 --- a/src/demo-app/dialog/dialog-demo.scss +++ b/src/demo-app/dialog/dialog-demo.scss @@ -1,8 +1,4 @@ -.demo-dialog { - color: rebeccapurple; -} - .demo-dialog-card { - max-width: 350px; + max-width: 405px; margin: 20px 0; } diff --git a/src/lib/datepicker/datepicker.ts b/src/lib/datepicker/datepicker.ts index 058ab377f19d..759db81cb5d9 100644 --- a/src/lib/datepicker/datepicker.ts +++ b/src/lib/datepicker/datepicker.ts @@ -288,7 +288,8 @@ export class MdDatepicker implements OnDestroy { /** Open the calendar as a dialog. */ private _openAsDialog(): void { this._dialogRef = this._dialog.open(MdDatepickerContent, { - direction: this._dir ? this._dir.value : 'ltr' + direction: this._dir ? this._dir.value : 'ltr', + viewContainerRef: this._viewContainerRef, }); this._dialogRef.afterClosed().subscribe(() => this.close()); this._dialogRef.componentInstance.datepicker = this; diff --git a/src/lib/dialog/dialog-container.ts b/src/lib/dialog/dialog-container.ts index 24cc5f96958d..61ae1338cff2 100644 --- a/src/lib/dialog/dialog-container.ts +++ b/src/lib/dialog/dialog-container.ts @@ -16,6 +16,7 @@ import { EventEmitter, Inject, Optional, + ChangeDetectorRef, } from '@angular/core'; import { animate, @@ -68,7 +69,7 @@ export function throwMdDialogContentAlreadyAttachedError() { '[attr.aria-labelledby]': '_ariaLabelledBy', '[attr.aria-describedby]': '_config?.ariaDescribedBy || null', '[@slideDialog]': '_state', - '(@slideDialog.start)': 'this._isAnimating = true', + '(@slideDialog.start)': '_onAnimationStart($event)', '(@slideDialog.done)': '_onAnimationDone($event)', }, }) @@ -82,17 +83,14 @@ export class MdDialogContainer extends BasePortalHost { /** Element that was focused before the dialog was opened. Save this to restore upon close. */ private _elementFocusedBeforeDialogWasOpened: HTMLElement | null = null; - /** Reference to the global document object. */ - private _document: Document; - /** The dialog configuration. */ _config: MdDialogConfig; /** State of the dialog animation. */ _state: 'void' | 'enter' | 'exit' = 'enter'; - /** Emits the current animation state whenever it changes. */ - _onAnimationStateChange = new EventEmitter(); + /** Emits when an animation state changes. */ + _animationStateChanged = new EventEmitter(); /** ID of the element that should be considered as the dialog's label. */ _ariaLabelledBy: string | null = null; @@ -104,10 +102,10 @@ export class MdDialogContainer extends BasePortalHost { private _ngZone: NgZone, private _elementRef: ElementRef, private _focusTrapFactory: FocusTrapFactory, - @Optional() @Inject(DOCUMENT) _document: any) { + private _changeDetectorRef: ChangeDetectorRef, + @Optional() @Inject(DOCUMENT) private _document: any) { super(); - this._document = _document; } /** @@ -171,15 +169,28 @@ export class MdDialogContainer extends BasePortalHost { /** Callback, invoked whenever an animation on the host completes. */ _onAnimationDone(event: AnimationEvent) { - this._onAnimationStateChange.emit(event); - if (event.toState === 'enter') { this._trapFocus(); } else if (event.toState === 'exit') { this._restoreFocus(); - this._onAnimationStateChange.complete(); } + this._animationStateChanged.emit(event); this._isAnimating = false; } + + /** Callback, invoked when an animation on the host starts. */ + _onAnimationStart(event: AnimationEvent) { + this._isAnimating = true; + this._animationStateChanged.emit(event); + } + + /** Starts the dialog exit animation. */ + _startExitAnimation(): void { + this._state = 'exit'; + + // Mark the container for check so it can react if the + // view container is using OnPush change detection. + this._changeDetectorRef.markForCheck(); + } } diff --git a/src/lib/dialog/dialog-ref.ts b/src/lib/dialog/dialog-ref.ts index 260afb36e45f..0bfe795dde1e 100644 --- a/src/lib/dialog/dialog-ref.ts +++ b/src/lib/dialog/dialog-ref.ts @@ -7,12 +7,11 @@ */ import {OverlayRef, GlobalPositionStrategy} from '../core'; -import {AnimationEvent} from '@angular/animations'; import {DialogPosition} from './dialog-config'; import {Observable} from 'rxjs/Observable'; import {Subject} from 'rxjs/Subject'; import {MdDialogContainer} from './dialog-container'; -import {filter} from '../core/rxjs/index'; +import {RxChain, first, filter} from '../core/rxjs/index'; // TODO(jelbourn): resizing @@ -36,9 +35,11 @@ export class MdDialogRef { private _result: any; constructor(private _overlayRef: OverlayRef, private _containerInstance: MdDialogContainer) { - filter.call(_containerInstance._onAnimationStateChange, - (event: AnimationEvent) => event.toState === 'exit') - .subscribe(() => this._overlayRef.dispose(), undefined, () => { + RxChain.from(_containerInstance._animationStateChanged) + .call(filter, event => event.phaseName === 'done' && event.toState === 'exit') + .call(first) + .subscribe(() => { + this._overlayRef.dispose(); this._afterClosed.next(this._result); this._afterClosed.complete(); this.componentInstance = null!; @@ -51,8 +52,14 @@ export class MdDialogRef { */ close(dialogResult?: any): void { this._result = dialogResult; - this._containerInstance._state = 'exit'; - this._overlayRef.detachBackdrop(); // Transition the backdrop in parallel with the dialog. + + // Transition the backdrop in parallel to the dialog. + RxChain.from(this._containerInstance._animationStateChanged) + .call(filter, event => event.phaseName === 'start') + .call(first) + .subscribe(() => this._overlayRef.detachBackdrop()); + + this._containerInstance._startExitAnimation(); } /** diff --git a/src/lib/dialog/dialog.spec.ts b/src/lib/dialog/dialog.spec.ts index fb347799cba5..c006827824a3 100644 --- a/src/lib/dialog/dialog.spec.ts +++ b/src/lib/dialog/dialog.spec.ts @@ -15,6 +15,7 @@ import { ViewContainerRef, Injector, Inject, + ChangeDetectionStrategy, } from '@angular/core'; import {By} from '@angular/platform-browser'; import {NoopAnimationsModule} from '@angular/platform-browser/animations'; @@ -153,6 +154,31 @@ describe('MdDialog', () => { }); })); + it('should close from a ViewContainerRef with OnPush change detection', fakeAsync(() => { + const onPushFixture = TestBed.createComponent(ComponentWithOnPushViewContainer); + + onPushFixture.detectChanges(); + + const dialogRef = dialog.open(PizzaMsg, { + viewContainerRef: onPushFixture.componentInstance.viewContainerRef + }); + + flushMicrotasks(); + onPushFixture.detectChanges(); + flushMicrotasks(); + + expect(overlayContainerElement.querySelectorAll('md-dialog-container').length) + .toBe(1, 'Expected one open dialog.'); + + dialogRef.close(); + flushMicrotasks(); + onPushFixture.detectChanges(); + tick(500); + + expect(overlayContainerElement.querySelectorAll('md-dialog-container').length) + .toBe(0, 'Expected no open dialogs.'); + })); + it('should close when clicking on the overlay backdrop', async(() => { dialog.open(PizzaMsg, { viewContainerRef: testViewContainerRef @@ -385,14 +411,25 @@ describe('MdDialog', () => { it('should have the componentInstance available in the afterClosed callback', fakeAsync(() => { let dialogRef = dialog.open(PizzaMsg); + let spy = jasmine.createSpy('afterClosed spy'); + + flushMicrotasks(); + viewContainerFixture.detectChanges(); + flushMicrotasks(); dialogRef.afterClosed().subscribe(() => { + spy(); expect(dialogRef.componentInstance).toBeTruthy('Expected component instance to be defined.'); }); dialogRef.close(); - tick(500); + + flushMicrotasks(); viewContainerFixture.detectChanges(); + tick(500); + + // Ensure that the callback actually fires. + expect(spy).toHaveBeenCalled(); })); describe('passing in data', () => { @@ -566,12 +603,12 @@ describe('MdDialog', () => { document.body.appendChild(button); button.focus(); - let dialogRef = dialog.open(PizzaMsg, { - viewContainerRef: testViewContainerRef - }); + let dialogRef = dialog.open(PizzaMsg, { viewContainerRef: testViewContainerRef }); + flushMicrotasks(); viewContainerFixture.detectChanges(); flushMicrotasks(); + expect(document.activeElement.id) .not.toBe('dialog-trigger', 'Expected the focus to change when dialog was opened.'); @@ -579,9 +616,9 @@ describe('MdDialog', () => { expect(document.activeElement.id).not.toBe('dialog-trigger', 'Expcted the focus not to have changed before the animation finishes.'); - tick(500); - viewContainerFixture.detectChanges(); flushMicrotasks(); + viewContainerFixture.detectChanges(); + tick(500); expect(document.activeElement.id).toBe('dialog-trigger', 'Expected that the trigger was refocused after the dialog is closed.'); @@ -603,9 +640,12 @@ describe('MdDialog', () => { let dialogRef = dialog.open(PizzaMsg, { viewContainerRef: testViewContainerRef }); - dialogRef.afterClosed().subscribe(() => input.focus()); + tick(500); + viewContainerFixture.detectChanges(); + dialogRef.afterClosed().subscribe(() => input.focus()); dialogRef.close(); + tick(500); viewContainerFixture.detectChanges(); flushMicrotasks(); @@ -775,6 +815,14 @@ class DirectiveWithViewContainer { constructor(public viewContainerRef: ViewContainerRef) { } } +@Component({ + changeDetection: ChangeDetectionStrategy.OnPush, + template: 'hello', +}) +class ComponentWithOnPushViewContainer { + constructor(public viewContainerRef: ViewContainerRef) { } +} + @Component({ selector: 'arbitrary-component', template: ``, @@ -829,6 +877,7 @@ const TEST_DIRECTIVES = [ ComponentWithChildViewContainer, PizzaMsg, DirectiveWithViewContainer, + ComponentWithOnPushViewContainer, ContentElementDialog, DialogWithInjectedData ];