fix(checkbox, slide-toggle): only emit change event if native input emitted one. (#820)

* fix(): slide-toggle, checkbox should only emit events if native input emitted one.

* Fix nested checkbox demo and update test name

Fixes #575.

* Remove whitespace

Author: devversion

src/components/checkbox/checkbox.spec.ts

@@ -3,8 +3,7 @@ import {
     inject,
     async,
     fakeAsync,
-    flushMicrotasks,
-    tick
+    flushMicrotasks
 } from '@angular/core/testing';
 import {
     FORM_DIRECTIVES,
@@ -214,13 +213,47 @@ describe('MdCheckbox', () => {
       expect(testComponent.onCheckboxClick).toHaveBeenCalledTimes(1);
     });
 
-    it('should emit a change event when the `checked` value changes', async(() => {
+    it('should trigger a change event when the native input does', 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();
 
+      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(() => {
-        expect(fixture.componentInstance.changeCount).toBe(1);
+        // 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', () => {
@@ -300,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();
@@ -488,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>`
@@ -504,6 +542,7 @@ class SingleCheckbox {
   changeCount: number = 0;
 
   onCheckboxClick(event: Event) {}
+  onCheckboxChange(event: MdCheckboxChange) {}
 }
 
 /** Simple component for testing an MdCheckbox with ngModel. */

src/components/checkbox/checkbox.ts

@@ -7,8 +7,7 @@ import {
     Output,
     Renderer,
     ViewEncapsulation,
-    forwardRef,
-    AfterContentInit
+    forwardRef
 } from '@angular/core';
 import {NG_VALUE_ACCESSOR, ControlValueAccessor} from '@angular/forms';
 
@@ -71,7 +70,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.
@@ -115,9 +114,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;
@@ -146,19 +142,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
@@ -267,6 +253,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();
     }
   }
 

src/components/slide-toggle/slide-toggle.spec.ts

@@ -127,11 +127,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);
@@ -140,11 +140,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');
@@ -158,7 +180,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();
       });
 
     }));

src/components/slide-toggle/slide-toggle.ts

@@ -58,7 +58,6 @@ export class MdSlideToggle implements AfterContentInit, ControlValueAccessor {
   private _color: string;
   private _hasFocus: boolean = false;
   private _isMousedown: boolean = false;
-  private _isInitialized: boolean = false;
   private _slideRenderer: SlideToggleRenderer = null;
 
   @Input() @BooleanFieldValue() disabled: boolean = false;
@@ -79,11 +78,6 @@ export class MdSlideToggle implements AfterContentInit, ControlValueAccessor {
   /** TODO: internal */
   ngAfterContentInit() {
     this._slideRenderer = new SlideToggleRenderer(this._elementRef);
-
-    // 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;
   }
 
   /**
@@ -100,6 +94,11 @@ export class MdSlideToggle implements AfterContentInit, ControlValueAccessor {
     // Once a drag is currently in progress, we do not want to toggle the slide-toggle on a click.
     if (!this.disabled && !this._slideRenderer.isDragging()) {
       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();
     }
   }
 
@@ -171,12 +170,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();
-      }
     }
   }
 

src/demo-app/checkbox/checkbox-demo.html

@@ -40,6 +40,5 @@ <h1>md-checkbox: Basic Example</h1>
   </div>
 </div>
 
-<h1>Application Example: Nested Checklist</h1>
-<h2><em>Caution: WIP!</em></h2>
-<md-checkbox-demo-nested-checklist></md-checkbox-demo-nested-checklist>
+<h1>Nested Checklist</h1>
+<md-checkbox-demo-nested-checklist></md-checkbox-demo-nested-checklist>

src/demo-app/checkbox/nested-checklist.html

@@ -4,7 +4,7 @@ <h2>Tasks</h2>
     <md-checkbox [(ngModel)]="task.completed"
                  [checked]="allComplete(task)"
                  [indeterminate]="someComplete(task.subtasks)"
-                 (change)="setAllCompleted(task.subtasks, $event)">
+                 (change)="setAllCompleted(task.subtasks, $event.checked)">
       <h3>{{task.name}}</h3>
     </md-checkbox>
     <ul>