Skip to content

Commit

Permalink
fix(slide-toggle): stop change event firing upon init (#713)
Browse files Browse the repository at this point in the history
* test(slide-toggle): add test to confirm that change event don't fires multiple times

* Adds a test which confirms, that the slide-toggle isn't firing the (change) event multiple times.

Closes #709

* update(): prevent change event to be fired at initialization

* update(): remove internal annotation
  • Loading branch information
devversion authored and jelbourn committed Jun 21, 2016
1 parent b407278 commit f21b2f4
Show file tree
Hide file tree
Showing 2 changed files with 117 additions and 7 deletions.
103 changes: 99 additions & 4 deletions src/components/slide-toggle/slide-toggle.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,19 @@ describe('MdSlideToggle', () => {
beforeEach(async(() => {
builder.createAsync(SlideToggleTestApp).then(f => {
fixture = f;

testComponent = fixture.debugElement.componentInstance;

// Enable jasmine spies on event functions, which may trigger at initialization
// of the slide-toggle component.
spyOn(fixture.debugElement.componentInstance, 'onSlideChange').and.callThrough();
spyOn(fixture.debugElement.componentInstance, 'onSlideClick').and.callThrough();

// Initialize the slide-toggle component, by triggering the first change detection cycle.
fixture.detectChanges();

let slideToggleDebug = fixture.debugElement.query(By.css('md-slide-toggle'));

testComponent = fixture.debugElement.componentInstance;
slideToggle = slideToggleDebug.componentInstance;
slideToggleElement = slideToggleDebug.nativeElement;
slideToggleControl = slideToggleDebug.injector.get(NgControl);
Expand Down Expand Up @@ -103,8 +111,6 @@ describe('MdSlideToggle', () => {
// Since we're using a label element and a visual hidden input, this behavior can led
// to an issue, where the click events on the slide-toggle are getting executed twice.

spyOn(testComponent, 'onSlideClick');

expect(slideToggle.checked).toBe(false);
expect(slideToggleElement.classList).not.toContain('md-checked');

Expand All @@ -117,6 +123,42 @@ describe('MdSlideToggle', () => {
expect(testComponent.onSlideClick).toHaveBeenCalledTimes(1);
});

it('should not trigger the change event multiple times', 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(() => {
expect(testComponent.onSlideChange).toHaveBeenCalledTimes(1);
});

}));

it('should not trigger the change event on initialization', 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(() => {
expect(testComponent.onSlideChange).toHaveBeenCalledTimes(1);
});

}));

it('should add a suffix to the inputs id', () => {
testComponent.slideId = 'myId';
fixture.detectChanges();
Expand Down Expand Up @@ -269,6 +311,56 @@ describe('MdSlideToggle', () => {

});

describe('custom template', () => {

let testComponent: SlideToggleTestApp;
let slideToggle: MdSlideToggle;
let slideToggleElement: HTMLElement;
let labelElement: HTMLLabelElement;
let inputElement: HTMLInputElement;

it('should not trigger the change event on initialization', async(() => {
builder
.overrideTemplate(SlideToggleTestApp, `
<md-slide-toggle checked="true" (change)="onSlideChange($event)"></md-slide-toggle>
`)
.createAsync(SlideToggleTestApp)
.then(fixture => {
// Initialize the variables for our test.
initializeTest(fixture);

// Enable jasmine spies on event functions, which may trigger at initialization
// of the slide-toggle component.
spyOn(fixture.debugElement.componentInstance, 'onSlideChange').and.callThrough();

fixture.detectChanges();

fixture.whenStable().then(() => {
expect(testComponent.onSlideChange).not.toHaveBeenCalled();
});
});
}));

/**
* Initializes the suites variables, to allow developers to easily access the several variables
* without loading / querying them always again.
* @param fixture Custom fixture, which contains the slide-toggle component.
*/
function initializeTest(fixture: ComponentFixture<any>) {
testComponent = fixture.debugElement.componentInstance;

// Initialize the slide-toggle component, by triggering the first change detection cycle.
fixture.detectChanges();

let slideToggleDebug = fixture.debugElement.query(By.css('md-slide-toggle'));

slideToggle = slideToggleDebug.componentInstance;
slideToggleElement = slideToggleDebug.nativeElement;
inputElement = fixture.debugElement.query(By.css('input')).nativeElement;
labelElement = fixture.debugElement.query(By.css('label')).nativeElement;
}
});

});

/**
Expand All @@ -288,7 +380,7 @@ function dispatchFocusChangeEvent(eventName: string, element: HTMLElement): void
<md-slide-toggle [(ngModel)]="slideModel" [disabled]="isDisabled" [color]="slideColor"
[id]="slideId" [checked]="slideChecked" [name]="slideName"
[aria-label]="slideLabel" [ariaLabel]="slideLabel"
[ariaLabelledby]="slideLabelledBy" (change)="lastEvent = $event"
[ariaLabelledby]="slideLabelledBy" (change)="onSlideChange($event)"
(click)="onSlideClick($event)">
<span>Test Slide Toggle</span>
</md-slide-toggle>
Expand All @@ -307,4 +399,7 @@ class SlideToggleTestApp {
lastEvent: MdSlideToggleChange;

onSlideClick(event: Event) {}
onSlideChange(event: MdSlideToggleChange) {
this.lastEvent = event;
}
}
21 changes: 18 additions & 3 deletions src/components/slide-toggle/slide-toggle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import {
ChangeDetectionStrategy,
Input,
Output,
EventEmitter
EventEmitter,
AfterContentInit
} from '@angular/core';
import {
ControlValueAccessor,
Expand Down Expand Up @@ -45,7 +46,7 @@ let nextId = 0;
providers: [MD_SLIDE_TOGGLE_VALUE_ACCESSOR],
changeDetection: ChangeDetectionStrategy.OnPush
})
export class MdSlideToggle implements ControlValueAccessor {
export class MdSlideToggle implements AfterContentInit, ControlValueAccessor {

private onChange = (_: any) => {};
private onTouched = () => {};
Expand All @@ -56,6 +57,7 @@ export class MdSlideToggle implements 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 @@ -74,6 +76,14 @@ export class MdSlideToggle implements 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 Down Expand Up @@ -163,7 +173,12 @@ export class MdSlideToggle implements ControlValueAccessor {
if (this.checked !== !!value) {
this._checked = value;
this.onChange(this._checked);
this._emitChangeEvent();

// 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 f21b2f4

Please sign in to comment.