From d52e6e05f11a8be2ce605172114057a8ec211723 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 15 Jul 2016 20:37:24 +0200 Subject: [PATCH] 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 --- 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 | 17 ++--- src/demo-app/checkbox/checkbox-demo.html | 5 +- src/demo-app/checkbox/nested-checklist.html | 2 +- 6 files changed, 92 insertions(+), 47 deletions(-) diff --git a/src/components/checkbox/checkbox.spec.ts b/src/components/checkbox/checkbox.spec.ts index 86c29f5e1649..3796203ad405 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 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 ` @@ -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..828c77ab52a1 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 bfe4829f168b..999cc5f69ec0 100644 --- a/src/components/slide-toggle/slide-toggle.ts +++ b/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(); - } } } diff --git a/src/demo-app/checkbox/checkbox-demo.html b/src/demo-app/checkbox/checkbox-demo.html index aae781f98d58..9fd9c922cd8e 100644 --- a/src/demo-app/checkbox/checkbox-demo.html +++ b/src/demo-app/checkbox/checkbox-demo.html @@ -40,6 +40,5 @@

md-checkbox: Basic Example

-

Application Example: Nested Checklist

-

Caution: WIP!

- +

Nested Checklist

+ \ No newline at end of file diff --git a/src/demo-app/checkbox/nested-checklist.html b/src/demo-app/checkbox/nested-checklist.html index 4b721ab99759..71c77d83f2e5 100644 --- a/src/demo-app/checkbox/nested-checklist.html +++ b/src/demo-app/checkbox/nested-checklist.html @@ -4,7 +4,7 @@

Tasks

+ (change)="setAllCompleted(task.subtasks, $event.checked)">

{{task.name}}