Skip to content

Commit

Permalink
fix(): visual hidden inputs should not bubble change event
Browse files Browse the repository at this point in the history
* 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 angular/angular#4059

To prevent the events from bubbling up, we have to stop propagation on
change.

Fixes angular#544.
  • Loading branch information
devversion committed May 27, 2016
1 parent 3b527e8 commit c40893b
Show file tree
Hide file tree
Showing 7 changed files with 57 additions and 18 deletions.
26 changes: 23 additions & 3 deletions src/components/checkbox/checkbox.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

}));
});

Expand Down Expand Up @@ -521,8 +541,8 @@ class CheckboxWithNameAttribute {}
/** Simple test component with change event */
@Component({
directives: [MdCheckbox],
template: `<md-checkbox (change)="handleChange()"></md-checkbox>`
template: `<md-checkbox (change)="handleChange($event)"></md-checkbox>`
})
class CheckboxWithChangeEvent {
handleChange(): void {}
handleChange(event: any): void {}
}
12 changes: 8 additions & 4 deletions src/components/checkbox/checkbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/components/radio/radio.html
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
[checked]="checked"
[disabled]="disabled"
[name]="name"
(change)="onInputChange()"
(change)="onInputChange($event)"
(focus)="onInputFocus()"
(blur)="onInputBlur()" />

Expand Down
7 changes: 6 additions & 1 deletion src/components/radio/radio.ts
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,12 @@ export class MdRadioButton implements OnInit {
* Checks the radio due to an interaction with the underlying native <input type="radio">
* @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();
Expand Down
2 changes: 1 addition & 1 deletion src/components/slide-toggle/slide-toggle.html
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
[attr.aria-labelledby]="ariaLabelledby"
(blur)="onInputBlur()"
(focus)="onInputFocus()"
(change)="onChangeEvent()">
(change)="onChangeEvent($event)">
</div>
<span class="md-slide-toggle-content">
<ng-content></ng-content>
Expand Down
19 changes: 12 additions & 7 deletions src/components/slide-toggle/slide-toggle.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 => {
Expand Down Expand Up @@ -265,7 +269,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)="changeCount = changeCount + 1">
[ariaLabelledby]="slideLabelledBy" (change)="onValueChange($event)">
<span>Test Slide Toggle</span>
</md-slide-toggle>
`,
Expand All @@ -280,5 +284,6 @@ class SlideToggleTestApp {
slideName: string;
slideLabel: string;
slideLabelledBy: string;
changeCount: number = 0;

onValueChange(event: any): void {};
}
7 changes: 6 additions & 1 deletion src/components/slide-toggle/slide-toggle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down

0 comments on commit c40893b

Please sign in to comment.