From a2141c42281628c25c93e296eba73bb332e20c43 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Wed, 6 Jul 2016 20:59:20 +0200 Subject: [PATCH] fix(): slide-toggle, checkbox should only emit events if native input emitted one. --- src/components/checkbox/checkbox.spec.ts | 63 +++++++++++++++---- src/components/checkbox/checkbox.ts | 23 +++---- .../slide-toggle/slide-toggle.spec.ts | 29 ++++++++- src/components/slide-toggle/slide-toggle.ts | 25 +++----- 4 files changed, 91 insertions(+), 49 deletions(-) diff --git a/src/components/checkbox/checkbox.spec.ts b/src/components/checkbox/checkbox.spec.ts index 86c29f5e1649..cf003c3076bb 100644 --- a/src/components/checkbox/checkbox.spec.ts +++ b/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 ` @@ -504,6 +542,7 @@ class SingleCheckbox { changeCount: number = 0; 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 c1e12c4fb5c3..0a615e84c2b8 100644 --- a/src/components/checkbox/checkbox.ts +++ b/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(); } } diff --git a/src/components/slide-toggle/slide-toggle.spec.ts b/src/components/slide-toggle/slide-toggle.spec.ts index c07ce2411fae..e601cbd8e696 100644 --- a/src/components/slide-toggle/slide-toggle.spec.ts +++ b/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(); }); })); diff --git a/src/components/slide-toggle/slide-toggle.ts b/src/components/slide-toggle/slide-toggle.ts index ea66875bdfe1..dfd1e09cdac2 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, @@ -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(); - } } }