Skip to content

Commit

Permalink
fix(drawer): infinite loop when two-way opened binding is toggled mid…
Browse files Browse the repository at this point in the history
…-animation

As of 004e0fe the drawer supports a two-way binding on the `opened` property, however because the `openedChange` emits at the end of the animation and two-way bindings invoke their setter which then trigger an animation which in turn invokes `openedChange`, there is the potential for an infinite loop if the user changes the `opened` state mid-animation. These changes avoid the issue by turning the `openedChanged` into an async stream and emitting the current value at the end of the animation, rather than determining the value based on the animation state.

Fixes #8869.
  • Loading branch information
crisbeto committed Dec 7, 2017
1 parent 0c39219 commit 17917a5
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 6 deletions.
35 changes: 34 additions & 1 deletion src/lib/sidenav/drawer.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
} from '@angular/core/testing';
import {Component, ElementRef, ViewChild} from '@angular/core';
import {By} from '@angular/platform-browser';
import {NoopAnimationsModule} from '@angular/platform-browser/animations';
import {BrowserAnimationsModule, NoopAnimationsModule} from '@angular/platform-browser/animations';
import {MatDrawer, MatSidenavModule, MatDrawerContainer} from './index';
import {A11yModule} from '@angular/cdk/a11y';
import {PlatformModule} from '@angular/cdk/platform';
Expand Down Expand Up @@ -138,6 +138,7 @@ describe('MatDrawer', () => {
expect(testComponent.openCount).toBe(0);
expect(testComponent.closeCount).toBe(0);

tick();
fixture.debugElement.query(By.css('.close')).nativeElement.click();
fixture.detectChanges();

Expand Down Expand Up @@ -367,6 +368,37 @@ describe('MatDrawer', () => {

expect(fixture.componentInstance.isOpen).toBe(true);
}));

it('should not throw when a two-way binding is toggled quickly while animating',
fakeAsync(() => {
TestBed
.resetTestingModule()
.configureTestingModule({
imports: [MatSidenavModule, BrowserAnimationsModule],
declarations: [DrawerOpenBinding],
})
.compileComponents();

const fixture = TestBed.createComponent(DrawerOpenBinding);
fixture.detectChanges();

// Note that we need actual timeouts and the `BrowserAnimationsModule`
// in order to test it correctly.
setTimeout(() => {
fixture.componentInstance.isOpen = !fixture.componentInstance.isOpen;
expect(() => fixture.detectChanges()).not.toThrow();

setTimeout(() => {
fixture.componentInstance.isOpen = !fixture.componentInstance.isOpen;
expect(() => fixture.detectChanges()).not.toThrow();
}, 1);

tick(1);
}, 1);

tick(1);
}));

});

describe('focus trapping behavior', () => {
Expand Down Expand Up @@ -496,6 +528,7 @@ describe('MatDrawerContainer', () => {

fixture.componentInstance.renderDrawer = false;
fixture.detectChanges();
tick();

expect(parseInt(contentElement.style.marginLeft)).toBeLessThan(initialMargin);
}));
Expand Down
9 changes: 4 additions & 5 deletions src/lib/sidenav/drawer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
_animationState: 'open-instant' | 'open' | 'void' = 'void';

/** Event emitted when the drawer open state is changed. */
@Output() openedChange: EventEmitter<boolean> = new EventEmitter<boolean>();
@Output() openedChange: EventEmitter<boolean> = new EventEmitter<boolean>(true);

/** Event emitted when the drawer has been opened. */
@Output('opened')
Expand Down Expand Up @@ -390,10 +390,9 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
_onAnimationEnd(event: AnimationEvent) {
const {fromState, toState} = event;

if (toState.indexOf('open') === 0 && fromState === 'void') {
this.openedChange.emit(true);
} else if (toState === 'void' && fromState.indexOf('open') === 0) {
this.openedChange.emit(false);
if ((toState.indexOf('open') === 0 && fromState === 'void') ||
(toState === 'void' && fromState.indexOf('open') === 0)) {
this.openedChange.emit(this._opened);
}
}

Expand Down

0 comments on commit 17917a5

Please sign in to comment.