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
angular · Aug 8, 2016 · 23a61ab · 23a61ab
1 parent ae5717c
commit 23a61ab
Show file tree

Hide file tree

Showing 6 changed files with 145 additions and 64 deletions.
diff --git 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)">
 
   <div class="md-button-toggle-label-content">
     <ng-content></ng-content>

diff --git 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<ButtonTogglesInsideButtonToggleGroup>;
     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 because the current input type is set to `radio`
+      expect(changeSpy).toHaveBeenCalledTimes(1);
     }));
 
     it('should emit a change event from the button toggle group', fakeAsync(() => {
@@ -330,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;
@@ -346,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);
       });
     }));
@@ -398,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<StandaloneButtonToggle>;
     let buttonToggleDebugElement: DebugElement;
     let buttonToggleNativeElement: HTMLElement;
+    let buttonToggleLabelElement: HTMLLabelElement;
     let buttonToggleInstance: MdButtonToggle;
     let testComponent: StandaloneButtonToggle;
 
@@ -416,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
@@ -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,22 @@ export class MdButtonToggle implements OnInit {
     } else {
       this._toggle();
     }
+
+    // Emit a change event when the native input does.
+    this._emitChangeEvent();
+  }
+
+  /** 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 `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();
   }
 }
 

diff --git 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)">
 
   <!-- The label content for radio control. -->
   <div class="md-radio-label-content" [class.md-radio-align-end]="align == 'end'">

diff --git 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,
@@ -45,6 +44,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 +63,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 +80,7 @@ describe('MdRadio', () => {
       testComponent.isGroupDisabled = true;
       fixture.detectChanges();
 
-      radioNativeElements[0].click();
+      radioLabelElements[0].click();
       expect(radioInstances[0].checked).toBe(false);
     });
 
@@ -119,15 +122,15 @@ 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');
       expect(groupInstance.selected).toBe(radioInstances[0]);
       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 +153,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 +180,12 @@ describe('MdRadio', () => {
 
       groupInstance.value = 'fire';
       fixture.detectChanges();
-      tick();
+
       expect(changeSpy).toHaveBeenCalled();
 
       groupInstance.value = 'water';
       fixture.detectChanges();
-      tick();
+
       expect(changeSpy).toHaveBeenCalledTimes(2);
     }));
 
@@ -236,6 +244,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 +265,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,17 +306,15 @@ describe('MdRadio', () => {
       // but remain untouched.
       radioInstances[1].checked = true;
       fixture.detectChanges();
-      tick();
 
       expect(groupNgControl.valid).toBe(true);
       expect(groupNgControl.pristine).toBe(false);
       expect(groupNgControl.touched).toBe(false);
 
       // 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 +325,6 @@ describe('MdRadio', () => {
       radioInstances[1].checked = true;
       fixture.detectChanges();
 
-      tick();
-
       expect(testComponent.modelValue).toBe('chocolate');
     }));
   });
@@ -358,9 +366,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 +513,7 @@ class RadiosInsideRadioGroup {
     <md-radio-button name="weather" value="hot">Summer</md-radio-button>
     <md-radio-button name="weather" value="cool">Autumn</md-radio-button>
     
-    <span id="xyz">Baby Banana<span>
+    <span id="xyz">Baby Banana</span>
     <md-radio-button name="fruit" value="banana" aria-label="Banana" aria-labelledby="xyz">
     </md-radio-button>
     <md-radio-button name="fruit" value="raspberry">Raspberry</md-radio-button>