Skip to content

Commit

Permalink
fix(): slide-toggle, checkbox should only emit events if native input…
Browse files Browse the repository at this point in the history 
… emitted one.
  • Loading branch information
@devversion
devversion committed Jul 6, 2016
1 parent 63c7d9c commit c79221c
Show file tree
Hide file tree
Showing 4 changed files with 92 additions and 58 deletions.
73 changes: 52 additions & 21 deletions src/components/checkbox/checkbox.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ import {
inject,
async,
fakeAsync,
flushMicrotasks,
tick
flushMicrotasks
} from '@angular/core/testing';
import {
FORM_DIRECTIVES,
Expand All @@ -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';



Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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>`
Expand All @@ -511,6 +541,7 @@ class SingleCheckbox {
lastKeydownEvent: Event = null;

onCheckboxClick(event: Event) {}
onCheckboxChange(event: MdCheckboxChange) {}
}

/** Simple component for testing an MdCheckbox with ngModel. */
Expand Down
23 changes: 7 additions & 16 deletions src/components/checkbox/checkbox.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ import {
Provider,
Renderer,
ViewEncapsulation,
forwardRef,
AfterContentInit
forwardRef
} from '@angular/core';
import {NG_VALUE_ACCESSOR, ControlValueAccessor} from '@angular/forms';

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
}
}

Expand Down
29 changes: 26 additions & 3 deletions src/components/slide-toggle/slide-toggle.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,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);
Expand All @@ -142,11 +142,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');
Expand All @@ -160,7 +182,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();
});

}));
Expand Down
25 changes: 7 additions & 18 deletions src/components/slide-toggle/slide-toggle.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ import {
ChangeDetectionStrategy,
Input,
Output,
EventEmitter,
AfterContentInit
EventEmitter
} from '@angular/core';
import {
ControlValueAccessor,
Expand Down Expand Up @@ -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 = () => {};
Expand All @@ -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;
Expand All @@ -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,
Expand All @@ -98,6 +88,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();
}
}

Expand Down Expand Up @@ -173,12 +168,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();
}
}
}

Expand Down

0 comments on commit c79221c

Please sign in to comment.