From 20a59118aa9d2676f29523b9ee3ee1a8e2519a51 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sun, 12 Jun 2016 14:20:54 +0200 Subject: [PATCH 1/3] fix(slide-toggle): 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. --- src/components/slide-toggle/slide-toggle.html | 3 ++- .../slide-toggle/slide-toggle.spec.ts | 23 ++++++++++++++++++- src/components/slide-toggle/slide-toggle.ts | 1 - 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/src/components/slide-toggle/slide-toggle.html b/src/components/slide-toggle/slide-toggle.html index 595ea61e265a..a9affa00b214 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)="onTouched(); $event.stopPropagation()"> diff --git a/src/components/slide-toggle/slide-toggle.spec.ts b/src/components/slide-toggle/slide-toggle.spec.ts index cd5bf113812a..0c18c8e46c1e 100644 --- a/src/components/slide-toggle/slide-toggle.spec.ts +++ b/src/components/slide-toggle/slide-toggle.spec.ts @@ -97,6 +97,24 @@ describe('MdSlideToggle', () => { expect(slideToggle.checked).toBe(true); }); + it('should not trigger the click event multiple times', () => { + spyOn(testComponent, 'onSlideClick'); + + expect(slideToggle.checked).toBe(false); + expect(slideToggleElement.classList).not.toContain('md-checked'); + + // Mostly the users will click on the slide-toggle container which will emit in some cases the + // click event multiple times. + ( labelElement.querySelector('.md-slide-toggle-container')).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 +286,8 @@ function dispatchFocusChangeEvent(eventName: string, element: HTMLElement): void + [ariaLabelledby]="slideLabelledBy" (change)="lastEvent = $event" + (click)="onSlideClick($event)"> Test Slide Toggle `, @@ -284,4 +303,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..540304f280c7 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', From ba0fb24fe58f735ee71155232c6957f09228809a Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 14 Jun 2016 22:35:10 +0200 Subject: [PATCH 2/3] update(): move input click into component source * Moved input click event into component source file. * Added comment for the issue cause. --- src/components/slide-toggle/slide-toggle.html | 2 +- src/components/slide-toggle/slide-toggle.spec.ts | 10 ++++++---- src/components/slide-toggle/slide-toggle.ts | 14 ++++++++++++++ 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/components/slide-toggle/slide-toggle.html b/src/components/slide-toggle/slide-toggle.html index a9affa00b214..728169f0f812 100644 --- a/src/components/slide-toggle/slide-toggle.html +++ b/src/components/slide-toggle/slide-toggle.html @@ -18,7 +18,7 @@ (blur)="onInputBlur()" (focus)="onInputFocus()" (change)="onChangeEvent($event)" - (click)="onTouched(); $event.stopPropagation()"> + (click)="onInputClick($event)"> diff --git a/src/components/slide-toggle/slide-toggle.spec.ts b/src/components/slide-toggle/slide-toggle.spec.ts index 0c18c8e46c1e..cc1673e3c968 100644 --- a/src/components/slide-toggle/slide-toggle.spec.ts +++ b/src/components/slide-toggle/slide-toggle.spec.ts @@ -98,15 +98,17 @@ describe('MdSlideToggle', () => { }); 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'); - // Mostly the users will click on the slide-toggle container which will emit in some cases the - // click event multiple times. - ( labelElement.querySelector('.md-slide-toggle-container')).click(); - + labelElement.click(); fixture.detectChanges(); expect(slideToggleElement.classList).toContain('md-checked'); diff --git a/src/components/slide-toggle/slide-toggle.ts b/src/components/slide-toggle/slide-toggle.ts index 540304f280c7..7c1a8c881f36 100644 --- a/src/components/slide-toggle/slide-toggle.ts +++ b/src/components/slide-toggle/slide-toggle.ts @@ -91,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. From 6071dd281434aea536a272e66a4227400b8bfb6e Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Wed, 15 Jun 2016 14:22:55 +0200 Subject: [PATCH 3/3] 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 ++++++++++++ 3 files changed, 38 insertions(+), 2 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;