Skip to content

Commit 23a61ab

Browse files
devversionjelbourn
authored andcommitted
fix(radio): only emit change event if native input does. (#911)
* 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 * Add todo for internal. * update: button-toggle should emit change event when native input does. * tests: add tests for button toggle change events
1 parent ae5717c commit 23a61ab

File tree

6 files changed

+145
-64
lines changed

6 files changed

+145
-64
lines changed

src/components/button-toggle/button-toggle.html

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@
55
[checked]="checked"
66
[disabled]="disabled"
77
[name]="name"
8-
(change)="_onInputChange($event)">
8+
(change)="_onInputChange($event)"
9+
(click)="_onInputClick($event)">
910

1011
<div class="md-button-toggle-label-content">
1112
<ng-content></ng-content>

src/components/button-toggle/button-toggle.spec.ts

Lines changed: 70 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -41,11 +41,13 @@ describe('MdButtonToggle', () => {
4141
}));
4242

4343
describe('inside of an exclusive selection group', () => {
44+
4445
let fixture: ComponentFixture<ButtonTogglesInsideButtonToggleGroup>;
4546
let groupDebugElement: DebugElement;
4647
let groupNativeElement: HTMLElement;
4748
let buttonToggleDebugElements: DebugElement[];
4849
let buttonToggleNativeElements: HTMLElement[];
50+
let buttonToggleLabelElements: HTMLLabelElement[];
4951
let groupInstance: MdButtonToggleGroup;
5052
let buttonToggleInstances: MdButtonToggle[];
5153
let testComponent: ButtonTogglesInsideButtonToggleGroup;
@@ -62,8 +64,13 @@ describe('MdButtonToggle', () => {
6264
groupInstance = groupDebugElement.injector.get(MdButtonToggleGroup);
6365

6466
buttonToggleDebugElements = fixture.debugElement.queryAll(By.directive(MdButtonToggle));
65-
buttonToggleNativeElements =
66-
buttonToggleDebugElements.map(debugEl => debugEl.nativeElement);
67+
68+
buttonToggleNativeElements = buttonToggleDebugElements
69+
.map(debugEl => debugEl.nativeElement);
70+
71+
buttonToggleLabelElements = fixture.debugElement.queryAll(By.css('label'))
72+
.map(debugEl => debugEl.nativeElement);
73+
6774
buttonToggleInstances = buttonToggleDebugElements.map(debugEl => debugEl.componentInstance);
6875
});
6976
}));
@@ -133,15 +140,19 @@ describe('MdButtonToggle', () => {
133140
let changeSpy = jasmine.createSpy('button-toggle change listener');
134141
buttonToggleInstances[0].change.subscribe(changeSpy);
135142

136-
buttonToggleInstances[0].checked = true;
143+
144+
buttonToggleLabelElements[0].click();
137145
fixture.detectChanges();
138146
tick();
139147
expect(changeSpy).toHaveBeenCalled();
140148

141-
buttonToggleInstances[0].checked = false;
149+
buttonToggleLabelElements[0].click();
142150
fixture.detectChanges();
143151
tick();
144-
expect(changeSpy).toHaveBeenCalledTimes(2);
152+
153+
// The default browser behavior is to not emit a change event, when the value was set
154+
// to false. That's because the current input type is set to `radio`
155+
expect(changeSpy).toHaveBeenCalledTimes(1);
145156
}));
146157

147158
it('should emit a change event from the button toggle group', fakeAsync(() => {
@@ -330,6 +341,7 @@ describe('MdButtonToggle', () => {
330341
let groupNativeElement: HTMLElement;
331342
let buttonToggleDebugElements: DebugElement[];
332343
let buttonToggleNativeElements: HTMLElement[];
344+
let buttonToggleLabelElements: HTMLLabelElement[];
333345
let groupInstance: MdButtonToggleGroupMultiple;
334346
let buttonToggleInstances: MdButtonToggle[];
335347
let testComponent: ButtonTogglesInsideButtonToggleGroupMultiple;
@@ -346,8 +358,10 @@ describe('MdButtonToggle', () => {
346358
groupInstance = groupDebugElement.injector.get(MdButtonToggleGroupMultiple);
347359

348360
buttonToggleDebugElements = fixture.debugElement.queryAll(By.directive(MdButtonToggle));
349-
buttonToggleNativeElements =
350-
buttonToggleDebugElements.map(debugEl => debugEl.nativeElement);
361+
buttonToggleNativeElements = buttonToggleDebugElements
362+
.map(debugEl => debugEl.nativeElement);
363+
buttonToggleLabelElements = fixture.debugElement.queryAll(By.css('label'))
364+
.map(debugEl => debugEl.nativeElement);
351365
buttonToggleInstances = buttonToggleDebugElements.map(debugEl => debugEl.componentInstance);
352366
});
353367
}));
@@ -398,12 +412,36 @@ describe('MdButtonToggle', () => {
398412

399413
expect(buttonToggleInstances[0].checked).toBe(false);
400414
});
415+
416+
it('should emit a change event for state changes', fakeAsync(() => {
417+
418+
expect(buttonToggleInstances[0].checked).toBe(false);
419+
420+
let changeSpy = jasmine.createSpy('button-toggle change listener');
421+
buttonToggleInstances[0].change.subscribe(changeSpy);
422+
423+
buttonToggleLabelElements[0].click();
424+
fixture.detectChanges();
425+
tick();
426+
expect(changeSpy).toHaveBeenCalled();
427+
428+
buttonToggleLabelElements[0].click();
429+
fixture.detectChanges();
430+
tick();
431+
432+
// The default browser behavior is to emit an event, when the value was set
433+
// to false. That's because the current input type is set to `checkbox` when
434+
// using the multiple mode.
435+
expect(changeSpy).toHaveBeenCalledTimes(2);
436+
}));
437+
401438
});
402439

403440
describe('as standalone', () => {
404441
let fixture: ComponentFixture<StandaloneButtonToggle>;
405442
let buttonToggleDebugElement: DebugElement;
406443
let buttonToggleNativeElement: HTMLElement;
444+
let buttonToggleLabelElement: HTMLLabelElement;
407445
let buttonToggleInstance: MdButtonToggle;
408446
let testComponent: StandaloneButtonToggle;
409447

@@ -416,24 +454,45 @@ describe('MdButtonToggle', () => {
416454

417455
buttonToggleDebugElement = fixture.debugElement.query(By.directive(MdButtonToggle));
418456
buttonToggleNativeElement = buttonToggleDebugElement.nativeElement;
457+
buttonToggleLabelElement = fixture.debugElement.query(By.css('label')).nativeElement;
419458
buttonToggleInstance = buttonToggleDebugElement.componentInstance;
420459
});
421460
}));
422461

423462
it('should toggle when clicked', () => {
424-
let nativeCheckboxLabel = buttonToggleDebugElement.query(By.css('label')).nativeElement;
425-
426-
nativeCheckboxLabel.click();
463+
buttonToggleLabelElement.click();
427464

428465
fixture.detectChanges();
429466

430467
expect(buttonToggleInstance.checked).toBe(true);
431468

432-
nativeCheckboxLabel.click();
469+
buttonToggleLabelElement.click();
433470
fixture.detectChanges();
434471

435472
expect(buttonToggleInstance.checked).toBe(false);
436473
});
474+
475+
it('should emit a change event for state changes', fakeAsync(() => {
476+
477+
expect(buttonToggleInstance.checked).toBe(false);
478+
479+
let changeSpy = jasmine.createSpy('button-toggle change listener');
480+
buttonToggleInstance.change.subscribe(changeSpy);
481+
482+
buttonToggleLabelElement.click();
483+
fixture.detectChanges();
484+
tick();
485+
expect(changeSpy).toHaveBeenCalled();
486+
487+
buttonToggleLabelElement.click();
488+
fixture.detectChanges();
489+
tick();
490+
491+
// The default browser behavior is to emit an event, when the value was set
492+
// to false. That's because the current input type is set to `checkbox`.
493+
expect(changeSpy).toHaveBeenCalledTimes(2);
494+
}));
495+
437496
});
438497
});
439498

src/components/button-toggle/button-toggle.ts

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -303,10 +303,6 @@ export class MdButtonToggle implements OnInit {
303303
// Notify all button toggles with the same name (in the same group) to un-check.
304304
this.buttonToggleDispatcher.notify(this.id, this.name);
305305
}
306-
307-
if (newCheckedState != this._checked) {
308-
this._emitChangeEvent();
309-
}
310306
}
311307

312308
this._checked = newCheckedState;
@@ -368,6 +364,22 @@ export class MdButtonToggle implements OnInit {
368364
} else {
369365
this._toggle();
370366
}
367+
368+
// Emit a change event when the native input does.
369+
this._emitChangeEvent();
370+
}
371+
372+
/** TODO: internal */
373+
_onInputClick(event: Event) {
374+
375+
// We have to stop propagation for click events on the visual hidden input element.
376+
// By default, when a user clicks on a label element, a generated click event will be
377+
// dispatched on the associated input element. Since we are using a label element as our
378+
// root container, the click event on the `slide-toggle` will be executed twice.
379+
// The real click event will bubble up, and the generated click event also tries to bubble up.
380+
// This will lead to multiple click events.
381+
// Preventing bubbling for the second event will solve that issue.
382+
event.stopPropagation();
371383
}
372384
}
373385

src/components/radio/radio.html

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@
1717
[attr.aria-labelledby]="ariaLabelledby"
1818
(change)="_onInputChange($event)"
1919
(focus)="_onInputFocus()"
20-
(blur)="_onInputBlur()">
20+
(blur)="_onInputBlur()"
21+
(click)="_onInputClick($event)">
2122

2223
<!-- The label content for radio control. -->
2324
<div class="md-radio-label-content" [class.md-radio-align-end]="align == 'end'">

src/components/radio/radio.spec.ts

Lines changed: 34 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ import {
22
inject,
33
async,
44
fakeAsync,
5-
tick,
65
TestComponentBuilder,
76
ComponentFixture,
87
TestBed,
@@ -45,6 +44,7 @@ describe('MdRadio', () => {
4544
let groupNativeElement: HTMLElement;
4645
let radioDebugElements: DebugElement[];
4746
let radioNativeElements: HTMLElement[];
47+
let radioLabelElements: HTMLLabelElement[];
4848
let groupInstance: MdRadioGroup;
4949
let radioInstances: MdRadioButton[];
5050
let testComponent: RadiosInsideRadioGroup;
@@ -63,6 +63,9 @@ describe('MdRadio', () => {
6363
radioDebugElements = fixture.debugElement.queryAll(By.directive(MdRadioButton));
6464
radioNativeElements = radioDebugElements.map(debugEl => debugEl.nativeElement);
6565
radioInstances = radioDebugElements.map(debugEl => debugEl.componentInstance);
66+
67+
radioLabelElements = radioDebugElements
68+
.map(debugEl => debugEl.query(By.css('label')).nativeElement);
6669
});
6770
}));
6871

@@ -77,7 +80,7 @@ describe('MdRadio', () => {
7780
testComponent.isGroupDisabled = true;
7881
fixture.detectChanges();
7982

80-
radioNativeElements[0].click();
83+
radioLabelElements[0].click();
8184
expect(radioInstances[0].checked).toBe(false);
8285
});
8386

@@ -119,15 +122,15 @@ describe('MdRadio', () => {
119122
it('should update the group and radios when one of the radios is clicked', () => {
120123
expect(groupInstance.value).toBeFalsy();
121124

122-
radioNativeElements[0].click();
125+
radioLabelElements[0].click();
123126
fixture.detectChanges();
124127

125128
expect(groupInstance.value).toBe('fire');
126129
expect(groupInstance.selected).toBe(radioInstances[0]);
127130
expect(radioInstances[0].checked).toBe(true);
128131
expect(radioInstances[1].checked).toBe(false);
129132

130-
radioNativeElements[1].click();
133+
radioLabelElements[1].click();
131134
fixture.detectChanges();
132135

133136
expect(groupInstance.value).toBe('water');
@@ -150,18 +153,23 @@ describe('MdRadio', () => {
150153
it('should emit a change event from radio buttons', fakeAsync(() => {
151154
expect(radioInstances[0].checked).toBe(false);
152155

153-
let changeSpy = jasmine.createSpy('radio change listener');
154-
radioInstances[0].change.subscribe(changeSpy);
156+
let spies = radioInstances
157+
.map((value, index) => jasmine.createSpy(`onChangeSpy ${index}`));
155158

156-
radioInstances[0].checked = true;
159+
spies.forEach((spy, index) => radioInstances[index].change.subscribe(spy));
160+
161+
radioLabelElements[0].click();
157162
fixture.detectChanges();
158-
tick();
159-
expect(changeSpy).toHaveBeenCalled();
160163

161-
radioInstances[0].checked = false;
164+
expect(spies[0]).toHaveBeenCalled();
165+
166+
radioLabelElements[1].click();
162167
fixture.detectChanges();
163-
tick();
164-
expect(changeSpy).toHaveBeenCalledTimes(2);
168+
169+
// To match the native radio button behavior, the change event shouldn't
170+
// be triggered when the radio got unselected.
171+
expect(spies[0]).toHaveBeenCalledTimes(1);
172+
expect(spies[1]).toHaveBeenCalledTimes(1);
165173
}));
166174

167175
it('should emit a change event from the radio group', fakeAsync(() => {
@@ -172,12 +180,12 @@ describe('MdRadio', () => {
172180

173181
groupInstance.value = 'fire';
174182
fixture.detectChanges();
175-
tick();
183+
176184
expect(changeSpy).toHaveBeenCalled();
177185

178186
groupInstance.value = 'water';
179187
fixture.detectChanges();
180-
tick();
188+
181189
expect(changeSpy).toHaveBeenCalledTimes(2);
182190
}));
183191

@@ -236,6 +244,7 @@ describe('MdRadio', () => {
236244
let groupNativeElement: HTMLElement;
237245
let radioDebugElements: DebugElement[];
238246
let radioNativeElements: HTMLElement[];
247+
let radioLabelElements: HTMLLabelElement[];
239248
let groupInstance: MdRadioGroup;
240249
let radioInstances: MdRadioButton[];
241250
let testComponent: RadioGroupWithNgModel;
@@ -256,6 +265,9 @@ describe('MdRadio', () => {
256265
radioDebugElements = fixture.debugElement.queryAll(By.directive(MdRadioButton));
257266
radioNativeElements = radioDebugElements.map(debugEl => debugEl.nativeElement);
258267
radioInstances = radioDebugElements.map(debugEl => debugEl.componentInstance);
268+
269+
radioLabelElements = radioDebugElements
270+
.map(debugEl => debugEl.query(By.css('label')).nativeElement);
259271
});
260272
}));
261273

@@ -294,17 +306,15 @@ describe('MdRadio', () => {
294306
// but remain untouched.
295307
radioInstances[1].checked = true;
296308
fixture.detectChanges();
297-
tick();
298309

299310
expect(groupNgControl.valid).toBe(true);
300311
expect(groupNgControl.pristine).toBe(false);
301312
expect(groupNgControl.touched).toBe(false);
302313

303314
// After a user interaction occurs (such as a click), the control should remain dirty and
304315
// now also be touched.
305-
radioNativeElements[2].click();
316+
radioLabelElements[2].click();
306317
fixture.detectChanges();
307-
tick();
308318

309319
expect(groupNgControl.valid).toBe(true);
310320
expect(groupNgControl.pristine).toBe(false);
@@ -315,8 +325,6 @@ describe('MdRadio', () => {
315325
radioInstances[1].checked = true;
316326
fixture.detectChanges();
317327

318-
tick();
319-
320328
expect(testComponent.modelValue).toBe('chocolate');
321329
}));
322330
});
@@ -358,9 +366,14 @@ describe('MdRadio', () => {
358366
groupInstance.value = 'chocolate';
359367
fixture.detectChanges();
360368

361-
tick();
362369
expect(testComponent.modelValue).toBe('chocolate');
363370
expect(testComponent.lastEvent.value).toBe('chocolate');
371+
372+
groupInstance.value = 'vanilla';
373+
fixture.detectChanges();
374+
375+
expect(testComponent.modelValue).toBe('vanilla');
376+
expect(testComponent.lastEvent.value).toBe('vanilla');
364377
}));
365378
});
366379

@@ -500,7 +513,7 @@ class RadiosInsideRadioGroup {
500513
<md-radio-button name="weather" value="hot">Summer</md-radio-button>
501514
<md-radio-button name="weather" value="cool">Autumn</md-radio-button>
502515
503-
<span id="xyz">Baby Banana<span>
516+
<span id="xyz">Baby Banana</span>
504517
<md-radio-button name="fruit" value="banana" aria-label="Banana" aria-labelledby="xyz">
505518
</md-radio-button>
506519
<md-radio-button name="fruit" value="raspberry">Raspberry</md-radio-button>

0 commit comments

Comments
 (0)