From c40893b53ecb44d4251006832923df6a2e753e2e Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 27 May 2016 19:12:37 +0200 Subject: [PATCH] fix(): visual hidden inputs should not bubble change event * Currently the change event of the visual hidden inputs will bubble up and emit its event object to the component's `change` output. This is currently an issue of Angular 2 - See https://github.com/angular/angular/issues/4059 To prevent the events from bubbling up, we have to stop propagation on change. Fixes #544. --- src/components/checkbox/checkbox.spec.ts | 26 ++++++++++++++++--- src/components/checkbox/checkbox.ts | 12 ++++++--- src/components/radio/radio.html | 2 +- src/components/radio/radio.ts | 7 ++++- src/components/slide-toggle/slide-toggle.html | 2 +- .../slide-toggle/slide-toggle.spec.ts | 19 +++++++++----- src/components/slide-toggle/slide-toggle.ts | 7 ++++- 7 files changed, 57 insertions(+), 18 deletions(-) diff --git a/src/components/checkbox/checkbox.spec.ts b/src/components/checkbox/checkbox.spec.ts index 90ce7e8ee014..0a9745dc8278 100644 --- a/src/components/checkbox/checkbox.spec.ts +++ b/src/components/checkbox/checkbox.spec.ts @@ -286,7 +286,27 @@ describe('MdCheckbox', () => { fixture.detectChanges(); tick(); - expect(testComponent.handleChange).toHaveBeenCalled(); + + expect(testComponent.handleChange).toHaveBeenCalledTimes(1); + expect(testComponent.handleChange).toHaveBeenCalledWith(true); + })); + + it('should not emit a DOM event to the change output', fakeAsync(() => { + expect(testComponent.handleChange).not.toHaveBeenCalled(); + + // Trigger the click on the inputElement, because the input will probably + // emit a DOM event to the change output. + inputElement.click(); + fixture.detectChanges(); + + fixture.whenStable().then(() => { + // We're checking the arguments type / emitted value to be a boolean, because sometimes the + // emitted value can be a DOM Event, which is not valid. + // See angular/angular#4059 + expect(testComponent.handleChange).toHaveBeenCalledTimes(1); + expect(testComponent.handleChange).toHaveBeenCalledWith(true); + }); + })); }); @@ -521,8 +541,8 @@ class CheckboxWithNameAttribute {} /** Simple test component with change event */ @Component({ directives: [MdCheckbox], - template: `` + template: `` }) class CheckboxWithChangeEvent { - handleChange(): void {} + handleChange(event: any): void {} } diff --git a/src/components/checkbox/checkbox.ts b/src/components/checkbox/checkbox.ts index 4754b99ec617..4c1799bab7c6 100644 --- a/src/components/checkbox/checkbox.ts +++ b/src/components/checkbox/checkbox.ts @@ -242,14 +242,18 @@ export class MdCheckbox implements ControlValueAccessor { } /** - * Event handler for checkbox input element. Toggles checked state if element is not disabled. + * Event handler for checkbox input element. + * Toggles checked state if element is not disabled. * @param event * @internal */ onInteractionEvent(event: Event) { - if (this.disabled) { - event.stopPropagation(); - } else { + // We always have to stop propagation on the change event. + // Otherwise the change event, from the input element, will bubble up and + // emit its event object to the `change` output. + event.stopPropagation(); + + if (!this.disabled) { this.toggle(); } } diff --git a/src/components/radio/radio.html b/src/components/radio/radio.html index 9edffea69521..e094b07f4842 100644 --- a/src/components/radio/radio.html +++ b/src/components/radio/radio.html @@ -13,7 +13,7 @@ [checked]="checked" [disabled]="disabled" [name]="name" - (change)="onInputChange()" + (change)="onInputChange($event)" (focus)="onInputFocus()" (blur)="onInputBlur()" /> diff --git a/src/components/radio/radio.ts b/src/components/radio/radio.ts index 7ca5fe887a2c..16c68ce7111c 100644 --- a/src/components/radio/radio.ts +++ b/src/components/radio/radio.ts @@ -391,7 +391,12 @@ export class MdRadioButton implements OnInit { * Checks the radio due to an interaction with the underlying native * @internal */ - onInputChange() { + onInputChange(event: Event) { + // We always have to stop propagation on the change event. + // Otherwise the change event, from the input element, will bubble up and + // emit its event object to the `change` output. + event.stopPropagation(); + this.checked = true; if (this.radioGroup) { this.radioGroup.touch(); diff --git a/src/components/slide-toggle/slide-toggle.html b/src/components/slide-toggle/slide-toggle.html index 6a3af979532f..595ea61e265a 100644 --- a/src/components/slide-toggle/slide-toggle.html +++ b/src/components/slide-toggle/slide-toggle.html @@ -17,7 +17,7 @@ [attr.aria-labelledby]="ariaLabelledby" (blur)="onInputBlur()" (focus)="onInputFocus()" - (change)="onChangeEvent()"> + (change)="onChangeEvent($event)"> diff --git a/src/components/slide-toggle/slide-toggle.spec.ts b/src/components/slide-toggle/slide-toggle.spec.ts index 15cca9a781d5..5d2075b8b32f 100644 --- a/src/components/slide-toggle/slide-toggle.spec.ts +++ b/src/components/slide-toggle/slide-toggle.spec.ts @@ -4,7 +4,8 @@ import { expect, beforeEach, inject, - async, + async, + fakeAsync, } from '@angular/core/testing'; import {TestComponentBuilder, ComponentFixture} from '@angular/compiler/testing'; import {By} from '@angular/platform-browser'; @@ -161,14 +162,17 @@ describe('MdSlideToggle', () => { expect(slideToggleElement.classList).not.toContain('ng-dirty'); }); - it('should emit the new values', () => { - expect(testComponent.changeCount).toBe(0); + it('should emit the new values properly', fakeAsync(() => { + spyOn(testComponent, 'onValueChange'); labelElement.click(); fixture.detectChanges(); - expect(testComponent.changeCount).toBe(1); - }); + fixture.whenStable().then(() => { + expect(testComponent.onValueChange).toHaveBeenCalledTimes(1); + expect(testComponent.onValueChange).toHaveBeenCalledWith(true); + }); + })); it('should support subscription on the change observable', () => { slideToggle.change.subscribe(value => { @@ -265,7 +269,7 @@ function dispatchFocusChangeEvent(eventName: string, element: HTMLElement): void + [ariaLabelledby]="slideLabelledBy" (change)="onValueChange($event)"> Test Slide Toggle `, @@ -280,5 +284,6 @@ class SlideToggleTestApp { slideName: string; slideLabel: string; slideLabelledBy: string; - changeCount: number = 0; + + onValueChange(event: any): void {}; } diff --git a/src/components/slide-toggle/slide-toggle.ts b/src/components/slide-toggle/slide-toggle.ts index df1684231312..08b2cf1a71f0 100644 --- a/src/components/slide-toggle/slide-toggle.ts +++ b/src/components/slide-toggle/slide-toggle.ts @@ -75,7 +75,12 @@ export class MdSlideToggle implements ControlValueAccessor { * which triggers a onChange event on click. * @internal */ - onChangeEvent() { + onChangeEvent(event: Event) { + // We always have to stop propagation on the change event. + // Otherwise the change event, from the input element, will bubble up and + // emit its event object to the component's `change` output. + event.stopPropagation(); + if (!this.disabled) { this.toggle(); }