Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jul 15, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 51 additions & 12 deletions src/components/checkbox/checkbox.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ import {
inject,
async,
fakeAsync,
flushMicrotasks,
tick
flushMicrotasks
} from '@angular/core/testing';
import {
FORM_DIRECTIVES,
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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>`
Expand All @@ -504,6 +542,7 @@ class SingleCheckbox {
changeCount: number = 0;

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
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ import {
Output,
Renderer,
ViewEncapsulation,
forwardRef,
AfterContentInit
forwardRef
} from '@angular/core';
import {NG_VALUE_ACCESSOR, ControlValueAccessor} from '@angular/forms';

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

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

}));
Expand Down
17 changes: 5 additions & 12 deletions src/components/slide-toggle/slide-toggle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
}

/**
Expand All @@ -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();
}
}

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

Expand Down
5 changes: 2 additions & 3 deletions src/demo-app/checkbox/checkbox-demo.html
Original file line number Diff line number Diff line change
Expand Up @@ -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>
2 changes: 1 addition & 1 deletion src/demo-app/checkbox/nested-checklist.html
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down