diff --git a/src/components/checkbox/checkbox.spec.ts b/src/components/checkbox/checkbox.spec.ts index 131ffd63a565..508d0eca9176 100644 --- a/src/components/checkbox/checkbox.spec.ts +++ b/src/components/checkbox/checkbox.spec.ts @@ -5,8 +5,7 @@ import { inject, async, fakeAsync, - flushMicrotasks, - tick + flushMicrotasks } from '@angular/core/testing'; import { FORM_DIRECTIVES, @@ -19,7 +18,6 @@ import {TestComponentBuilder, ComponentFixture} from '@angular/compiler/testing' import {Component, DebugElement} from '@angular/core'; import {By} from '@angular/platform-browser'; import {MdCheckbox, MdCheckboxChange} from './checkbox'; -import {PromiseCompleter} from '@angular2-material/core/async/promise-completer'; @@ -215,21 +213,48 @@ describe('MdCheckbox', () => { expect(testComponent.onCheckboxClick).toHaveBeenCalledTimes(1); }); - it('should emit a change event when the `checked` value changes', () => { - // TODO(jelbourn): this *should* work with async(), but fixture.whenStable currently doesn't - // know to look at pending macro tasks. - // See https://github.com/angular/angular/issues/8389 - // As a short-term solution, use a promise (which jasmine knows how to understand). - let promiseCompleter = new PromiseCompleter(); - checkboxInstance.change.subscribe(() => { - promiseCompleter.resolve(); + it('should trigger the change event properly', async(() => { + spyOn(testComponent, 'onCheckboxChange'); + + expect(inputElement.checked).toBe(false); + expect(checkboxNativeElement.classList).not.toContain('md-checkbox-checked'); + + labelElement.click(); + fixture.detectChanges(); + + expect(inputElement.checked).toBe(true); + expect(checkboxNativeElement.classList).toContain('md-checkbox-checked'); + + // Wait for the fixture to become stable, because the EventEmitter for the change event, + // will only fire after the zone async change detection has finished. + fixture.whenStable().then(() => { + // The change event shouldn't fire, because the value change was not caused + // by any interaction. + expect(testComponent.onCheckboxChange).toHaveBeenCalledTimes(1); }); + })); + + it('should not trigger the change event by changing the native value', async(() => { + spyOn(testComponent, 'onCheckboxChange'); + + expect(inputElement.checked).toBe(false); + expect(checkboxNativeElement.classList).not.toContain('md-checkbox-checked'); testComponent.isChecked = true; fixture.detectChanges(); - return promiseCompleter.promise; - }); + expect(inputElement.checked).toBe(true); + expect(checkboxNativeElement.classList).toContain('md-checkbox-checked'); + + // Wait for the fixture to become stable, because the EventEmitter for the change event, + // will only fire after the zone async change detection has finished. + fixture.whenStable().then(() => { + // The change event shouldn't fire, because the value change was not caused + // by any interaction. + expect(testComponent.onCheckboxChange).not.toHaveBeenCalled(); + }); + + })); describe('state transition css classes', () => { it('should transition unchecked -> checked -> unchecked', () => { @@ -308,17 +333,21 @@ describe('MdCheckbox', () => { }); })); - it('should call the change event on first change after initialization', fakeAsync(() => { - fixture.detectChanges(); - expect(testComponent.lastEvent).toBeUndefined(); + it('should emit the event to the change observable', () => { + let changeSpy = jasmine.createSpy('onChangeObservable'); + + checkboxInstance.change.subscribe(changeSpy); - checkboxInstance.checked = true; fixture.detectChanges(); + expect(changeSpy).not.toHaveBeenCalled(); - tick(); + // When changing the native `checked` property the checkbox will not fire a change event, + // because the element is not focused and it's not the native behavior of the input element. + labelElement.click(); + fixture.detectChanges(); - expect(testComponent.lastEvent.checked).toBe(true); - })); + expect(changeSpy).toHaveBeenCalledTimes(1); + }); it('should not emit a DOM event to the change output', async(() => { fixture.detectChanges(); @@ -496,7 +525,8 @@ describe('MdCheckbox', () => { [indeterminate]="isIndeterminate" [disabled]="isDisabled" (change)="changeCount = changeCount + 1" - (click)="onCheckboxClick($event)"> + (click)="onCheckboxClick($event)" + (change)="onCheckboxChange($event)"> Simple checkbox ` @@ -511,6 +541,7 @@ class SingleCheckbox { lastKeydownEvent: Event = null; onCheckboxClick(event: Event) {} + onCheckboxChange(event: MdCheckboxChange) {} } /** Simple component for testing an MdCheckbox with ngModel. */ diff --git a/src/components/checkbox/checkbox.ts b/src/components/checkbox/checkbox.ts index 34dbd4ba8c92..3e57b62b2fd3 100644 --- a/src/components/checkbox/checkbox.ts +++ b/src/components/checkbox/checkbox.ts @@ -8,8 +8,7 @@ import { Provider, Renderer, ViewEncapsulation, - forwardRef, - AfterContentInit + forwardRef } from '@angular/core'; import {NG_VALUE_ACCESSOR, ControlValueAccessor} from '@angular/forms'; @@ -72,7 +71,7 @@ export class MdCheckboxChange { encapsulation: ViewEncapsulation.None, changeDetection: ChangeDetectionStrategy.OnPush }) -export class MdCheckbox implements AfterContentInit, ControlValueAccessor { +export class MdCheckbox implements ControlValueAccessor { /** * Attached to the aria-label attribute of the host element. In most cases, arial-labelledby will * take precedence so this may be omitted. @@ -116,9 +115,6 @@ export class MdCheckbox implements AfterContentInit, ControlValueAccessor { /** Called when the checkbox is blurred. Needed to properly implement ControlValueAccessor. */ onTouched: () => any = () => {}; - /** Whether the `checked` state has been set to its initial value. */ - private _isInitialized: boolean = false; - private _currentAnimationClass: string = ''; private _currentCheckState: TransitionCheckState = TransitionCheckState.Init; @@ -147,19 +143,9 @@ export class MdCheckbox implements AfterContentInit, ControlValueAccessor { this._checked = checked; this._transitionCheckState( this._checked ? TransitionCheckState.Checked : TransitionCheckState.Unchecked); - - // Only fire a change event if this isn't the first time the checked property is ever set. - if (this._isInitialized) { - this._emitChangeEvent(); - } } } - /** TODO: internal */ - ngAfterContentInit() { - this._isInitialized = true; - } - /** * Whether the checkbox is indeterminate. This is also known as "mixed" mode and can be used to * represent a checkbox with three states, e.g. a checkbox that represents a nested list of @@ -275,6 +261,11 @@ export class MdCheckbox implements AfterContentInit, ControlValueAccessor { if (!this.disabled) { this.toggle(); + + // Emit our custom change event if the native input emitted one. + // It is important to only emit it, if the native input triggered one, because + // we don't want to trigger a change event, when the `checked` variable changes for example. + this._emitChangeEvent(); } } diff --git a/src/components/slide-toggle/slide-toggle.spec.ts b/src/components/slide-toggle/slide-toggle.spec.ts index 305be953ed70..5559c1a22c46 100644 --- a/src/components/slide-toggle/slide-toggle.spec.ts +++ b/src/components/slide-toggle/slide-toggle.spec.ts @@ -129,11 +129,11 @@ describe('MdSlideToggle', () => { expect(testComponent.onSlideClick).toHaveBeenCalledTimes(1); }); - it('should not trigger the change event multiple times', async(() => { + it('should trigger the change event properly', async(() => { expect(inputElement.checked).toBe(false); expect(slideToggleElement.classList).not.toContain('md-checked'); - testComponent.slideChecked = true; + labelElement.click(); fixture.detectChanges(); expect(inputElement.checked).toBe(true); @@ -142,11 +142,33 @@ describe('MdSlideToggle', () => { // Wait for the fixture to become stable, because the EventEmitter for the change event, // will only fire after the zone async change detection has finished. fixture.whenStable().then(() => { + // The change event shouldn't fire, because the value change was not caused + // by any interaction. expect(testComponent.onSlideChange).toHaveBeenCalledTimes(1); }); })); + it('should not trigger the change event by changing the native value', async(() => { + expect(inputElement.checked).toBe(false); + expect(slideToggleElement.classList).not.toContain('md-checked'); + + testComponent.slideChecked = true; + fixture.detectChanges(); + + expect(inputElement.checked).toBe(true); + expect(slideToggleElement.classList).toContain('md-checked'); + + // Wait for the fixture to become stable, because the EventEmitter for the change event, + // will only fire after the zone async change detection has finished. + fixture.whenStable().then(() => { + // The change event shouldn't fire, because the value change was not caused + // by any interaction. + expect(testComponent.onSlideChange).not.toHaveBeenCalled(); + }); + + })); + it('should not trigger the change event on initialization', async(() => { expect(inputElement.checked).toBe(false); expect(slideToggleElement.classList).not.toContain('md-checked'); @@ -160,7 +182,8 @@ describe('MdSlideToggle', () => { // Wait for the fixture to become stable, because the EventEmitter for the change event, // will only fire after the zone async change detection has finished. fixture.whenStable().then(() => { - expect(testComponent.onSlideChange).toHaveBeenCalledTimes(1); + // The change event shouldn't fire, because the native input element is not focused. + expect(testComponent.onSlideChange).not.toHaveBeenCalled(); }); })); diff --git a/src/components/slide-toggle/slide-toggle.ts b/src/components/slide-toggle/slide-toggle.ts index fe9afc129171..237256c28efa 100644 --- a/src/components/slide-toggle/slide-toggle.ts +++ b/src/components/slide-toggle/slide-toggle.ts @@ -6,8 +6,7 @@ import { ChangeDetectionStrategy, Input, Output, - EventEmitter, - AfterContentInit + EventEmitter } from '@angular/core'; import { ControlValueAccessor, @@ -46,7 +45,7 @@ let nextId = 0; providers: [MD_SLIDE_TOGGLE_VALUE_ACCESSOR], changeDetection: ChangeDetectionStrategy.OnPush }) -export class MdSlideToggle implements AfterContentInit, ControlValueAccessor { +export class MdSlideToggle implements ControlValueAccessor { private onChange = (_: any) => {}; private onTouched = () => {}; @@ -57,7 +56,6 @@ export class MdSlideToggle implements AfterContentInit, ControlValueAccessor { private _color: string; private _hasFocus: boolean = false; private _isMousedown: boolean = false; - private _isInitialized: boolean = false; @Input() @BooleanFieldValue() disabled: boolean = false; @Input() name: string = null; @@ -76,14 +74,6 @@ export class MdSlideToggle implements AfterContentInit, ControlValueAccessor { private _renderer: Renderer) { } - /** TODO: internal */ - ngAfterContentInit() { - // Mark this component as initialized in AfterContentInit because the initial checked value can - // possibly be set by NgModel or the checked attribute. This would cause the change event to - // be emitted, before the component is actually initialized. - this._isInitialized = true; - } - /** * The onChangeEvent method will be also called on click. * This is because everything for the slide-toggle is wrapped inside of a label, @@ -98,6 +88,11 @@ export class MdSlideToggle implements AfterContentInit, ControlValueAccessor { if (!this.disabled) { this.toggle(); + + // Emit our custom change event if the native input emitted one. + // It is important to only emit it, if the native input triggered one, because + // we don't want to trigger a change event, when the `checked` variable changes for example. + this._emitChangeEvent(); } } @@ -173,12 +168,6 @@ export class MdSlideToggle implements AfterContentInit, ControlValueAccessor { if (this.checked !== !!value) { this._checked = value; this.onChange(this._checked); - - // Only fire a change event if the `slide-toggle` is completely initialized and - // all attributes / inputs are properly loaded. - if (this._isInitialized) { - this._emitChangeEvent(); - } } }