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

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

@@ -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,
@@ -97,6 +87,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();
     }
   }
 
@@ -168,12 +163,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();
-      }
     }
   }