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

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>

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);
+    }));
+
   });
 });
 

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();
   }
 }
 

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'">

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,7 +122,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 +130,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 +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;
-      fixture.detectChanges();
-      tick();
-      expect(changeSpy).toHaveBeenCalled();
+      spies.forEach((spy, index) => radioInstances[index].change.subscribe(spy));
 
-      radioInstances[0].checked = false;
+      radioLabelElements[0].click();
       fixture.detectChanges();
-      tick();
-      expect(changeSpy).toHaveBeenCalledTimes(2);
+
+      expect(spies[0]).toHaveBeenCalled();
+
+      radioLabelElements[1].click();
+      fixture.detectChanges();
+
+      // 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,7 +306,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 +313,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 +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>

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,31 @@ 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 <input type="radio">
+   * 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.
@@ -417,6 +410,8 @@ export class MdRadioButton implements OnInit {
     event.stopPropagation();
 
     this.checked = true;
+    this._emitChangeEvent();
+
     if (this.radioGroup) {
       this.radioGroup._touch();
     }