fix(): checkbox should only emit a change event when input element emitted one.

Author: devversion

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
     </md-checkbox>
   </div>`
@@ -511,6 +541,7 @@ class SingleCheckbox {
   lastKeydownEvent: Event = null;
 
   onCheckboxClick(event: Event) {}
+  onCheckboxChange(event: MdCheckboxChange) {}
 }
 
 /** Simple component for testing an MdCheckbox with ngModel. */

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();
     }
   }