From afed81884f969af2f09f2293b1c71b3342b3201e Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Wed, 15 Jun 2016 17:37:57 +0200 Subject: [PATCH] fix(checkbox): prevent double click events (#672) * fix(slide-toggle and checkbox): visual hidden input should not bubble click * Currently the (click) event gets called twice. This is caused by the bubbling of the (click) event on the input. Fixes #671. * update(): move input click into component source * Moved input click event into component source file. * Added comment for the issue cause. * fix(checkbox): do not trigger the click event twice --- src/components/checkbox/checkbox.html | 3 ++- src/components/checkbox/checkbox.spec.ts | 25 ++++++++++++++++++- src/components/checkbox/checkbox.ts | 12 +++++++++ src/components/slide-toggle/slide-toggle.html | 3 ++- .../slide-toggle/slide-toggle.spec.ts | 25 ++++++++++++++++++- src/components/slide-toggle/slide-toggle.ts | 15 ++++++++++- 6 files changed, 78 insertions(+), 5 deletions(-) diff --git a/src/components/checkbox/checkbox.html b/src/components/checkbox/checkbox.html index c1fff754de90..15a62654afb3 100644 --- a/src/components/checkbox/checkbox.html +++ b/src/components/checkbox/checkbox.html @@ -11,7 +11,8 @@ [attr.aria-labelledby]="ariaLabelledby" (focus)="onInputFocus()" (blur)="onInputBlur()" - (change)="onInteractionEvent($event)"> + (change)="onInteractionEvent($event)" + (click)="onInputClick($event)">
diff --git a/src/components/checkbox/checkbox.spec.ts b/src/components/checkbox/checkbox.spec.ts index 0364f34f8698..35f61163d433 100644 --- a/src/components/checkbox/checkbox.spec.ts +++ b/src/components/checkbox/checkbox.spec.ts @@ -183,6 +183,26 @@ describe('MdCheckbox', () => { expect(checkboxNativeElement.classList).toContain('md-checkbox-align-end'); }); + it('should not trigger the click event multiple times', () => { + // By default, when clicking on a label element, a generated click will be dispatched + // on the associated input element. + // 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 checkbox are getting executed twice. + + spyOn(testComponent, 'onCheckboxClick'); + + expect(inputElement.checked).toBe(false); + expect(checkboxNativeElement.classList).not.toContain('md-checkbox-checked'); + + labelElement.click(); + fixture.detectChanges(); + + expect(checkboxNativeElement.classList).toContain('md-checkbox-checked'); + expect(inputElement.checked).toBe(true); + + expect(testComponent.onCheckboxClick).toHaveBeenCalledTimes(1); + }); + it('should emit a change event when the `checked` value changes', () => { // TODO(jelbourn): this *should* work with async(), but fixture.whenStable currently doesn't // know to look at pending macro tasks. @@ -463,7 +483,8 @@ describe('MdCheckbox', () => { [checked]="isChecked" [indeterminate]="isIndeterminate" [disabled]="isDisabled" - (change)="changeCount = changeCount + 1"> + (change)="changeCount = changeCount + 1" + (click)="onCheckboxClick($event)"> Simple checkbox
` @@ -476,6 +497,8 @@ class SingleCheckbox { parentElementClicked: boolean = false; parentElementKeyedUp: boolean = false; lastKeydownEvent: Event = null; + + onCheckboxClick(event: Event) {} } /** Simple component for testing an MdCheckbox with ngModel and ngControl. */ diff --git a/src/components/checkbox/checkbox.ts b/src/components/checkbox/checkbox.ts index 374b1ff9d444..76e2efabe6bd 100644 --- a/src/components/checkbox/checkbox.ts +++ b/src/components/checkbox/checkbox.ts @@ -278,6 +278,18 @@ export class MdCheckbox implements AfterContentInit, ControlValueAccessor { } } + /** @internal */ + onInputClick(event: Event) { + // We have to stop propagation for click events on the visual hidden input element. + // By default, when a user clicks on a label element, a generated click event will be + // dispatched on the associated input element. Since we are using a label element as our + // root container, the click event on the `checkbox` will be executed twice. + // The real click event will bubble up, and the generated click event also tries to bubble up. + // This will lead to multiple click events. + // Preventing bubbling for the second event will solve that issue. + event.stopPropagation(); + } + private _getAnimationClassForCheckStateTransition( oldState: TransitionCheckState, newState: TransitionCheckState): string { var animSuffix: string; diff --git a/src/components/slide-toggle/slide-toggle.html b/src/components/slide-toggle/slide-toggle.html index 595ea61e265a..728169f0f812 100644 --- a/src/components/slide-toggle/slide-toggle.html +++ b/src/components/slide-toggle/slide-toggle.html @@ -17,7 +17,8 @@ [attr.aria-labelledby]="ariaLabelledby" (blur)="onInputBlur()" (focus)="onInputFocus()" - (change)="onChangeEvent($event)"> + (change)="onChangeEvent($event)" + (click)="onInputClick($event)"> diff --git a/src/components/slide-toggle/slide-toggle.spec.ts b/src/components/slide-toggle/slide-toggle.spec.ts index cd5bf113812a..cc1673e3c968 100644 --- a/src/components/slide-toggle/slide-toggle.spec.ts +++ b/src/components/slide-toggle/slide-toggle.spec.ts @@ -97,6 +97,26 @@ describe('MdSlideToggle', () => { expect(slideToggle.checked).toBe(true); }); + it('should not trigger the click event multiple times', () => { + // By default, when clicking on a label element, a generated click will be dispatched + // on the associated input element. + // 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'); + + labelElement.click(); + fixture.detectChanges(); + + expect(slideToggleElement.classList).toContain('md-checked'); + expect(slideToggle.checked).toBe(true); + + expect(testComponent.onSlideClick).toHaveBeenCalledTimes(1); + }); + it('should add a suffix to the inputs id', () => { testComponent.slideId = 'myId'; fixture.detectChanges(); @@ -268,7 +288,8 @@ function dispatchFocusChangeEvent(eventName: string, element: HTMLElement): void + [ariaLabelledby]="slideLabelledBy" (change)="lastEvent = $event" + (click)="onSlideClick($event)"> Test Slide Toggle `, @@ -284,4 +305,6 @@ class SlideToggleTestApp { slideLabel: string; slideLabelledBy: string; lastEvent: MdSlideToggleChange; + + onSlideClick(event: Event) {} } diff --git a/src/components/slide-toggle/slide-toggle.ts b/src/components/slide-toggle/slide-toggle.ts index ade15efcf82c..7c1a8c881f36 100644 --- a/src/components/slide-toggle/slide-toggle.ts +++ b/src/components/slide-toggle/slide-toggle.ts @@ -38,7 +38,6 @@ let nextId = 0; '[class.md-disabled]': 'disabled', // This md-slide-toggle prefix will change, once the temporary ripple is removed. '[class.md-slide-toggle-focused]': '_hasFocus', - '(click)': 'onTouched()', '(mousedown)': 'setMousedown()' }, templateUrl: 'slide-toggle.html', @@ -92,6 +91,20 @@ export class MdSlideToggle implements ControlValueAccessor { } } + /** @internal */ + onInputClick(event: Event) { + this.onTouched(); + + // We have to stop propagation for click events on the visual hidden input element. + // By default, when a user clicks on a label element, a generated click event will be + // dispatched on the associated input element. Since we are using a label element as our + // root container, the click event on the `slide-toggle` will be executed twice. + // The real click event will bubble up, and the generated click event also tries to bubble up. + // This will lead to multiple click events. + // Preventing bubbling for the second event will solve that issue. + event.stopPropagation(); + } + /** @internal */ setMousedown() { // We only *show* the focus style when focus has come to the button via the keyboard.