From e8d27f468ce978b40ee93fd19b9b7dcf53c25887 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sat, 23 Jul 2016 15:27:55 +0200 Subject: [PATCH 1/4] fix(radio): radio-button should only emit change event if native input does. * The radio-button should only emit a change event, when the native input does. This ensures that the radio-button matches its behavior with the native radio buttons. Breaking Change: radio-button will no longer emit change event on de-select. Fixes #791 --- src/components/radio/radio.html | 3 +- src/components/radio/radio.spec.ts | 54 +++++++++++++++++++----------- src/components/radio/radio.ts | 46 +++++++++++-------------- 3 files changed, 56 insertions(+), 47 deletions(-) diff --git a/src/components/radio/radio.html b/src/components/radio/radio.html index 63c153d948ef..fd1aa756c2b7 100644 --- a/src/components/radio/radio.html +++ b/src/components/radio/radio.html @@ -17,7 +17,8 @@ [attr.aria-labelledby]="ariaLabelledby" (change)="_onInputChange($event)" (focus)="_onInputFocus()" - (blur)="_onInputBlur()"> + (blur)="_onInputBlur()" + (click)="_onInputClick($event)">
diff --git a/src/components/radio/radio.spec.ts b/src/components/radio/radio.spec.ts index b502d99d700a..42b16e43aed3 100644 --- a/src/components/radio/radio.spec.ts +++ b/src/components/radio/radio.spec.ts @@ -45,6 +45,7 @@ describe('MdRadio', () => { let groupNativeElement: HTMLElement; let radioDebugElements: DebugElement[]; let radioNativeElements: HTMLElement[]; + let radioLabelElements: HTMLLabelElement[]; let groupInstance: MdRadioGroup; let radioInstances: MdRadioButton[]; let testComponent: RadiosInsideRadioGroup; @@ -63,6 +64,9 @@ describe('MdRadio', () => { radioDebugElements = fixture.debugElement.queryAll(By.directive(MdRadioButton)); radioNativeElements = radioDebugElements.map(debugEl => debugEl.nativeElement); radioInstances = radioDebugElements.map(debugEl => debugEl.componentInstance); + + radioLabelElements = radioDebugElements + .map(debugEl => debugEl.query(By.css('label')).nativeElement); }); })); @@ -77,7 +81,7 @@ describe('MdRadio', () => { testComponent.isGroupDisabled = true; fixture.detectChanges(); - radioNativeElements[0].click(); + radioLabelElements[0].click(); expect(radioInstances[0].checked).toBe(false); }); @@ -119,7 +123,7 @@ describe('MdRadio', () => { it('should update the group and radios when one of the radios is clicked', () => { expect(groupInstance.value).toBeFalsy(); - radioNativeElements[0].click(); + radioLabelElements[0].click(); fixture.detectChanges(); expect(groupInstance.value).toBe('fire'); @@ -127,7 +131,7 @@ describe('MdRadio', () => { expect(radioInstances[0].checked).toBe(true); expect(radioInstances[1].checked).toBe(false); - radioNativeElements[1].click(); + radioLabelElements[1].click(); fixture.detectChanges(); expect(groupInstance.value).toBe('water'); @@ -150,18 +154,23 @@ describe('MdRadio', () => { it('should emit a change event from radio buttons', fakeAsync(() => { expect(radioInstances[0].checked).toBe(false); - let changeSpy = jasmine.createSpy('radio change listener'); - radioInstances[0].change.subscribe(changeSpy); + let spies = radioInstances + .map((value, index) => jasmine.createSpy(`onChangeSpy ${index}`)); - radioInstances[0].checked = true; + spies.forEach((spy, index) => radioInstances[index].change.subscribe(spy)); + + radioLabelElements[0].click(); fixture.detectChanges(); - tick(); - expect(changeSpy).toHaveBeenCalled(); - radioInstances[0].checked = false; + expect(spies[0]).toHaveBeenCalled(); + + radioLabelElements[1].click(); fixture.detectChanges(); - tick(); - expect(changeSpy).toHaveBeenCalledTimes(2); + + // To match the native radio button behavior, the change event shouldn't + // be triggered when the radio got unselected. + expect(spies[0]).toHaveBeenCalledTimes(1); + expect(spies[1]).toHaveBeenCalledTimes(1); })); it('should emit a change event from the radio group', fakeAsync(() => { @@ -172,12 +181,12 @@ describe('MdRadio', () => { groupInstance.value = 'fire'; fixture.detectChanges(); - tick(); + expect(changeSpy).toHaveBeenCalled(); groupInstance.value = 'water'; fixture.detectChanges(); - tick(); + expect(changeSpy).toHaveBeenCalledTimes(2); })); @@ -236,6 +245,7 @@ describe('MdRadio', () => { let groupNativeElement: HTMLElement; let radioDebugElements: DebugElement[]; let radioNativeElements: HTMLElement[]; + let radioLabelElements: HTMLLabelElement[]; let groupInstance: MdRadioGroup; let radioInstances: MdRadioButton[]; let testComponent: RadioGroupWithNgModel; @@ -256,6 +266,9 @@ describe('MdRadio', () => { radioDebugElements = fixture.debugElement.queryAll(By.directive(MdRadioButton)); radioNativeElements = radioDebugElements.map(debugEl => debugEl.nativeElement); radioInstances = radioDebugElements.map(debugEl => debugEl.componentInstance); + + radioLabelElements = radioDebugElements + .map(debugEl => debugEl.query(By.css('label')).nativeElement); }); })); @@ -294,7 +307,6 @@ describe('MdRadio', () => { // but remain untouched. radioInstances[1].checked = true; fixture.detectChanges(); - tick(); expect(groupNgControl.valid).toBe(true); expect(groupNgControl.pristine).toBe(false); @@ -302,9 +314,8 @@ describe('MdRadio', () => { // After a user interaction occurs (such as a click), the control should remain dirty and // now also be touched. - radioNativeElements[2].click(); + radioLabelElements[2].click(); fixture.detectChanges(); - tick(); expect(groupNgControl.valid).toBe(true); expect(groupNgControl.pristine).toBe(false); @@ -315,8 +326,6 @@ describe('MdRadio', () => { radioInstances[1].checked = true; fixture.detectChanges(); - tick(); - expect(testComponent.modelValue).toBe('chocolate'); })); }); @@ -358,9 +367,14 @@ describe('MdRadio', () => { groupInstance.value = 'chocolate'; fixture.detectChanges(); - tick(); expect(testComponent.modelValue).toBe('chocolate'); expect(testComponent.lastEvent.value).toBe('chocolate'); + + groupInstance.value = 'vanilla'; + fixture.detectChanges(); + + expect(testComponent.modelValue).toBe('vanilla'); + expect(testComponent.lastEvent.value).toBe('vanilla'); })); }); @@ -500,7 +514,7 @@ class RadiosInsideRadioGroup { Summer Autumn - Baby Banana + Baby Banana Raspberry diff --git a/src/components/radio/radio.ts b/src/components/radio/radio.ts index eee7e82fc11f..4bc1e3c64e5a 100644 --- a/src/components/radio/radio.ts +++ b/src/components/radio/radio.ts @@ -238,12 +238,10 @@ export class MdRadioGroup implements AfterContentInit, ControlValueAccessor { selector: 'md-radio-button', templateUrl: 'radio.html', styleUrls: ['radio.css'], - encapsulation: ViewEncapsulation.None, - host: { - '(click)': '_onClick($event)' - } + encapsulation: ViewEncapsulation.None }) export class MdRadioButton implements OnInit { + @HostBinding('class.md-radio-focused') _isFocused: boolean; @@ -308,10 +306,6 @@ export class MdRadioButton implements OnInit { this.radioDispatcher.notify(this.id, this.name); } - if (newCheckedState != this._checked) { - this._emitChangeEvent(); - } - this._checked = newCheckedState; if (newCheckedState && this.radioGroup && this.radioGroup.value != this.value) { @@ -374,23 +368,6 @@ export class MdRadioButton implements OnInit { this.change.emit(event); } - _onClick(event: Event) { - if (this.disabled) { - event.preventDefault(); - event.stopPropagation(); - return; - } - - if (this.radioGroup != null) { - // Propagate the change one-way via the group, which will in turn mark this - // button as checked. - this.radioGroup.selected = this; - this.radioGroup._touch(); - } else { - this.checked = true; - } - } - /** * We use a hidden native input field to handle changes to focus state via keyboard navigation, * with visual rendering done separately. The native element is kept in sync with the overall @@ -400,15 +377,30 @@ export class MdRadioButton implements OnInit { this._isFocused = true; } + /** TODO: internal */ _onInputBlur() { this._isFocused = false; + if (this.radioGroup) { this.radioGroup._touch(); } } + /** TODO: 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 `radio-button` 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(); + } + /** - * Checks the radio due to an interaction with the underlying native + * Triggered when the radio button received a click or the input recognized any change. + * Clicking on a label element, will trigger a change event on the associated input. */ _onInputChange(event: Event) { // We always have to stop propagation on the change event. @@ -417,6 +409,8 @@ export class MdRadioButton implements OnInit { event.stopPropagation(); this.checked = true; + this._emitChangeEvent(); + if (this.radioGroup) { this.radioGroup._touch(); } From 9e3fbd64cab8d760c8b0015afb5326a902f80996 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sat, 23 Jul 2016 15:29:14 +0200 Subject: [PATCH 2/4] Add todo for internal. --- src/components/radio/radio.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/radio/radio.ts b/src/components/radio/radio.ts index 4bc1e3c64e5a..b915a8acf758 100644 --- a/src/components/radio/radio.ts +++ b/src/components/radio/radio.ts @@ -401,6 +401,7 @@ export class MdRadioButton implements OnInit { /** * Triggered when the radio button received a click or the input recognized any change. * Clicking on a label element, will trigger a change event on the associated input. + * TODO: internal */ _onInputChange(event: Event) { // We always have to stop propagation on the change event. From 6f21d103c35bd3a0d4a1c81ab96aa031872fae91 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Mon, 25 Jul 2016 20:19:06 +0200 Subject: [PATCH 3/4] update: button-toggle should emit change event when native input does. --- .../button-toggle/button-toggle.html | 3 ++- .../button-toggle/button-toggle.spec.ts | 21 ++++++++++++++----- src/components/button-toggle/button-toggle.ts | 19 +++++++++++++---- 3 files changed, 33 insertions(+), 10 deletions(-) diff --git a/src/components/button-toggle/button-toggle.html b/src/components/button-toggle/button-toggle.html index 36c77811e6ea..c08e5370c55d 100644 --- a/src/components/button-toggle/button-toggle.html +++ b/src/components/button-toggle/button-toggle.html @@ -5,7 +5,8 @@ [checked]="checked" [disabled]="disabled" [name]="name" - (change)="_onInputChange($event)"> + (change)="_onInputChange($event)" + (click)="_onInputClick($event)">
diff --git a/src/components/button-toggle/button-toggle.spec.ts b/src/components/button-toggle/button-toggle.spec.ts index 16b5e1140045..b8a51ec91068 100644 --- a/src/components/button-toggle/button-toggle.spec.ts +++ b/src/components/button-toggle/button-toggle.spec.ts @@ -41,11 +41,13 @@ describe('MdButtonToggle', () => { })); describe('inside of an exclusive selection group', () => { + let fixture: ComponentFixture; let groupDebugElement: DebugElement; let groupNativeElement: HTMLElement; let buttonToggleDebugElements: DebugElement[]; let buttonToggleNativeElements: HTMLElement[]; + let buttonToggleLabelElements: HTMLLabelElement[]; let groupInstance: MdButtonToggleGroup; let buttonToggleInstances: MdButtonToggle[]; let testComponent: ButtonTogglesInsideButtonToggleGroup; @@ -62,8 +64,13 @@ describe('MdButtonToggle', () => { groupInstance = groupDebugElement.injector.get(MdButtonToggleGroup); buttonToggleDebugElements = fixture.debugElement.queryAll(By.directive(MdButtonToggle)); - buttonToggleNativeElements = - buttonToggleDebugElements.map(debugEl => debugEl.nativeElement); + + buttonToggleNativeElements = buttonToggleDebugElements + .map(debugEl => debugEl.nativeElement); + + buttonToggleLabelElements = fixture.debugElement.queryAll(By.css('label')) + .map(debugEl => debugEl.nativeElement); + buttonToggleInstances = buttonToggleDebugElements.map(debugEl => debugEl.componentInstance); }); })); @@ -133,15 +140,19 @@ describe('MdButtonToggle', () => { let changeSpy = jasmine.createSpy('button-toggle change listener'); buttonToggleInstances[0].change.subscribe(changeSpy); - buttonToggleInstances[0].checked = true; + + buttonToggleLabelElements[0].click(); fixture.detectChanges(); tick(); expect(changeSpy).toHaveBeenCalled(); - buttonToggleInstances[0].checked = false; + buttonToggleLabelElements[0].click(); fixture.detectChanges(); tick(); - expect(changeSpy).toHaveBeenCalledTimes(2); + + // The default browser behavior is to not emit a change event, when the value was set + // to false. That's why the change event was only triggered once. + expect(changeSpy).toHaveBeenCalledTimes(1); })); it('should emit a change event from the button toggle group', fakeAsync(() => { diff --git a/src/components/button-toggle/button-toggle.ts b/src/components/button-toggle/button-toggle.ts index ffe9da869d94..001b34528fc7 100644 --- a/src/components/button-toggle/button-toggle.ts +++ b/src/components/button-toggle/button-toggle.ts @@ -303,10 +303,6 @@ export class MdButtonToggle implements OnInit { // Notify all button toggles with the same name (in the same group) to un-check. this.buttonToggleDispatcher.notify(this.id, this.name); } - - if (newCheckedState != this._checked) { - this._emitChangeEvent(); - } } this._checked = newCheckedState; @@ -368,6 +364,21 @@ export class MdButtonToggle implements OnInit { } else { this._toggle(); } + + // Emit a change event when the native input does. + this._emitChangeEvent(); + } + + _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 `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(); } } From 461ecc98ae22e5ac518b29a26ebe3ddc896f0ca6 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 9 Aug 2016 00:13:05 +0200 Subject: [PATCH 4/4] tests: add tests for button toggle change events --- .../button-toggle/button-toggle.spec.ts | 62 ++++++++++++++++--- src/components/button-toggle/button-toggle.ts | 1 + src/components/radio/radio.spec.ts | 1 - 3 files changed, 56 insertions(+), 8 deletions(-) diff --git a/src/components/button-toggle/button-toggle.spec.ts b/src/components/button-toggle/button-toggle.spec.ts index b8a51ec91068..53b7b65b59fa 100644 --- a/src/components/button-toggle/button-toggle.spec.ts +++ b/src/components/button-toggle/button-toggle.spec.ts @@ -151,7 +151,7 @@ describe('MdButtonToggle', () => { tick(); // The default browser behavior is to not emit a change event, when the value was set - // to false. That's why the change event was only triggered once. + // to false. That's because the current input type is set to `radio` expect(changeSpy).toHaveBeenCalledTimes(1); })); @@ -341,6 +341,7 @@ describe('MdButtonToggle', () => { let groupNativeElement: HTMLElement; let buttonToggleDebugElements: DebugElement[]; let buttonToggleNativeElements: HTMLElement[]; + let buttonToggleLabelElements: HTMLLabelElement[]; let groupInstance: MdButtonToggleGroupMultiple; let buttonToggleInstances: MdButtonToggle[]; let testComponent: ButtonTogglesInsideButtonToggleGroupMultiple; @@ -357,8 +358,10 @@ describe('MdButtonToggle', () => { groupInstance = groupDebugElement.injector.get(MdButtonToggleGroupMultiple); buttonToggleDebugElements = fixture.debugElement.queryAll(By.directive(MdButtonToggle)); - buttonToggleNativeElements = - buttonToggleDebugElements.map(debugEl => debugEl.nativeElement); + buttonToggleNativeElements = buttonToggleDebugElements + .map(debugEl => debugEl.nativeElement); + buttonToggleLabelElements = fixture.debugElement.queryAll(By.css('label')) + .map(debugEl => debugEl.nativeElement); buttonToggleInstances = buttonToggleDebugElements.map(debugEl => debugEl.componentInstance); }); })); @@ -409,12 +412,36 @@ describe('MdButtonToggle', () => { expect(buttonToggleInstances[0].checked).toBe(false); }); + + it('should emit a change event for state changes', fakeAsync(() => { + + expect(buttonToggleInstances[0].checked).toBe(false); + + let changeSpy = jasmine.createSpy('button-toggle change listener'); + buttonToggleInstances[0].change.subscribe(changeSpy); + + buttonToggleLabelElements[0].click(); + fixture.detectChanges(); + tick(); + expect(changeSpy).toHaveBeenCalled(); + + buttonToggleLabelElements[0].click(); + fixture.detectChanges(); + tick(); + + // The default browser behavior is to emit an event, when the value was set + // to false. That's because the current input type is set to `checkbox` when + // using the multiple mode. + expect(changeSpy).toHaveBeenCalledTimes(2); + })); + }); describe('as standalone', () => { let fixture: ComponentFixture; let buttonToggleDebugElement: DebugElement; let buttonToggleNativeElement: HTMLElement; + let buttonToggleLabelElement: HTMLLabelElement; let buttonToggleInstance: MdButtonToggle; let testComponent: StandaloneButtonToggle; @@ -427,24 +454,45 @@ describe('MdButtonToggle', () => { buttonToggleDebugElement = fixture.debugElement.query(By.directive(MdButtonToggle)); buttonToggleNativeElement = buttonToggleDebugElement.nativeElement; + buttonToggleLabelElement = fixture.debugElement.query(By.css('label')).nativeElement; buttonToggleInstance = buttonToggleDebugElement.componentInstance; }); })); it('should toggle when clicked', () => { - let nativeCheckboxLabel = buttonToggleDebugElement.query(By.css('label')).nativeElement; - - nativeCheckboxLabel.click(); + buttonToggleLabelElement.click(); fixture.detectChanges(); expect(buttonToggleInstance.checked).toBe(true); - nativeCheckboxLabel.click(); + buttonToggleLabelElement.click(); fixture.detectChanges(); expect(buttonToggleInstance.checked).toBe(false); }); + + it('should emit a change event for state changes', fakeAsync(() => { + + expect(buttonToggleInstance.checked).toBe(false); + + let changeSpy = jasmine.createSpy('button-toggle change listener'); + buttonToggleInstance.change.subscribe(changeSpy); + + buttonToggleLabelElement.click(); + fixture.detectChanges(); + tick(); + expect(changeSpy).toHaveBeenCalled(); + + buttonToggleLabelElement.click(); + fixture.detectChanges(); + tick(); + + // The default browser behavior is to emit an event, when the value was set + // to false. That's because the current input type is set to `checkbox`. + expect(changeSpy).toHaveBeenCalledTimes(2); + })); + }); }); diff --git a/src/components/button-toggle/button-toggle.ts b/src/components/button-toggle/button-toggle.ts index 001b34528fc7..d5fc30168196 100644 --- a/src/components/button-toggle/button-toggle.ts +++ b/src/components/button-toggle/button-toggle.ts @@ -369,6 +369,7 @@ export class MdButtonToggle implements OnInit { this._emitChangeEvent(); } + /** TODO: internal */ _onInputClick(event: Event) { // We have to stop propagation for click events on the visual hidden input element. diff --git a/src/components/radio/radio.spec.ts b/src/components/radio/radio.spec.ts index 42b16e43aed3..be838b525dd3 100644 --- a/src/components/radio/radio.spec.ts +++ b/src/components/radio/radio.spec.ts @@ -2,7 +2,6 @@ import { inject, async, fakeAsync, - tick, TestComponentBuilder, ComponentFixture, TestBed,