Skip to content

Commit

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

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

Fixes #544.
  • Loading branch information
devversion authored and jelbourn committed May 27, 2016
1 parent 6b86df0 commit d037ed3
Show file tree
Hide file tree
Showing 8 changed files with 58 additions and 19 deletions.
26 changes: 23 additions & 3 deletions src/components/checkbox/checkbox.spec.ts
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', async(() => {
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(value: boolean): void {}
}
12 changes: 8 additions & 4 deletions src/components/checkbox/checkbox.ts
Expand Up @@ -250,14 +250,18 @@ export class MdCheckbox implements AfterContentInit, 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
Expand Up @@ -13,7 +13,7 @@
[checked]="checked"
[disabled]="disabled"
[name]="name"
(change)="onInputChange()"
(change)="onInputChange($event)"
(focus)="onInputFocus()"
(blur)="onInputBlur()" />

Expand Down
4 changes: 2 additions & 2 deletions src/components/radio/radio.spec.ts
Expand Up @@ -12,7 +12,7 @@ import {FORM_DIRECTIVES, NgControl} from '@angular/common';
import {TestComponentBuilder, ComponentFixture} from '@angular/compiler/testing';
import {Component, DebugElement, provide} from '@angular/core';
import {By} from '@angular/platform-browser';
import {MD_RADIO_DIRECTIVES, MdRadioGroup, MdRadioButton} from './radio';
import {MD_RADIO_DIRECTIVES, MdRadioGroup, MdRadioButton, MdRadioChange} from './radio';
import {MdRadioDispatcher} from './radio_dispatcher';


Expand Down Expand Up @@ -446,7 +446,7 @@ class RadioGroupWithNgModel {
{label: 'Chocolate', value: 'chocolate'},
{label: 'Strawberry', value: 'strawberry'},
];
onChange(value: string) {}
onChange(value: MdRadioChange) {}
}

// TODO(jelbourn): remove eveything below when Angular supports faking events.
Expand Down
7 changes: 6 additions & 1 deletion src/components/radio/radio.ts
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
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
17 changes: 11 additions & 6 deletions src/components/slide-toggle/slide-toggle.spec.ts
Expand Up @@ -5,6 +5,7 @@ import {
beforeEach,
inject,
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(value: boolean): void {};
}
7 changes: 6 additions & 1 deletion src/components/slide-toggle/slide-toggle.ts
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 d037ed3

Please sign in to comment.