From 1ff9cf028d4072399c8d11f03d38108be81cbf75 Mon Sep 17 00:00:00 2001
From: tinayuangao <tinagao@google.com>
Date: Wed, 20 Jun 2018 10:53:48 -0700
Subject: [PATCH] fix(button-toggle): use native button and aria-pressed for
 button-toggle (#10990)

---
 src/lib/button-toggle/button-toggle.html    | 27 +++++-----
 src/lib/button-toggle/button-toggle.scss    | 10 ++++
 src/lib/button-toggle/button-toggle.spec.ts | 57 +++++++++++++--------
 src/lib/button-toggle/button-toggle.ts      | 42 ++++++---------
 4 files changed, 75 insertions(+), 61 deletions(-)

diff --git a/src/lib/button-toggle/button-toggle.html b/src/lib/button-toggle/button-toggle.html
index f487d13e4f9..f0389997704 100644
--- a/src/lib/button-toggle/button-toggle.html
+++ b/src/lib/button-toggle/button-toggle.html
@@ -1,22 +1,19 @@
-<label [attr.for]="inputId" class="mat-button-toggle-label" #label>
-  <input #input class="mat-button-toggle-input cdk-visually-hidden"
-         [type]="_type"
-         [id]="inputId"
-         [checked]="checked"
-         [disabled]="disabled || null"
-         [attr.name]="name"
-         [attr.aria-label]="ariaLabel"
-         [attr.aria-labelledby]="ariaLabelledby"
-         (change)="_onInputChange($event)"
-         (click)="_onInputClick($event)">
-
+<button #button class="mat-button-toggle-button"
+        type="button"
+        [id]="buttonId"
+        [attr.aria-pressed]="checked"
+        [disabled]="disabled || null"
+        [attr.name]="name || null"
+        [attr.aria-label]="ariaLabel"
+        [attr.aria-labelledby]="ariaLabelledby"
+        (click)="_onButtonClick($event)">
   <div class="mat-button-toggle-label-content">
     <ng-content></ng-content>
   </div>
-</label>
-<div class="mat-button-toggle-focus-overlay"></div>
+</button>
 
+<div class="mat-button-toggle-focus-overlay"></div>
 <div class="mat-button-toggle-ripple" matRipple
-     [matRippleTrigger]="label"
+     [matRippleTrigger]="button"
      [matRippleDisabled]="this.disableRipple || this.disabled">
 </div>
diff --git a/src/lib/button-toggle/button-toggle.scss b/src/lib/button-toggle/button-toggle.scss
index 97b5bd45618..05a2edc6cfb 100644
--- a/src/lib/button-toggle/button-toggle.scss
+++ b/src/lib/button-toggle/button-toggle.scss
@@ -100,3 +100,13 @@ $mat-button-toggle-border-radius: 2px !default;
   // Pointer events can be safely disabled because the ripple trigger element is the label element.
   pointer-events: none;
 }
+
+.mat-button-toggle-button {
+  border: 0;
+  background: none;
+  color: inherit;
+  padding: inherit;
+  margin: inherit;
+  font: inherit;
+  outline: none;
+}
diff --git a/src/lib/button-toggle/button-toggle.spec.ts b/src/lib/button-toggle/button-toggle.spec.ts
index 1bb11660d71..76c025410f6 100644
--- a/src/lib/button-toggle/button-toggle.spec.ts
+++ b/src/lib/button-toggle/button-toggle.spec.ts
@@ -93,7 +93,7 @@ describe('MatButtonToggle with forms', () => {
       buttonToggleDebugElements = fixture.debugElement.queryAll(By.directive(MatButtonToggle));
       buttonToggleInstances = buttonToggleDebugElements.map(debugEl => debugEl.componentInstance);
       buttonToggleLabels = buttonToggleDebugElements.map(
-        debugEl => debugEl.query(By.css('label')).nativeElement);
+        debugEl => debugEl.query(By.css('button')).nativeElement);
 
       fixture.detectChanges();
     }));
@@ -244,7 +244,7 @@ describe('MatButtonToggle without forms', () => {
       buttonToggleNativeElements = buttonToggleDebugElements
         .map(debugEl => debugEl.nativeElement);
 
-      buttonToggleLabelElements = fixture.debugElement.queryAll(By.css('label'))
+      buttonToggleLabelElements = fixture.debugElement.queryAll(By.css('button'))
         .map(debugEl => debugEl.nativeElement);
 
       buttonToggleInstances = buttonToggleDebugElements.map(debugEl => debugEl.componentInstance);
@@ -339,7 +339,7 @@ describe('MatButtonToggle without forms', () => {
       buttonToggleLabelElements[0].click();
       fixture.detectChanges();
       tick();
-      expect(changeSpy).toHaveBeenCalled();
+      expect(changeSpy).toHaveBeenCalledTimes(1);
 
       buttonToggleLabelElements[0].click();
       fixture.detectChanges();
@@ -447,7 +447,7 @@ describe('MatButtonToggle without forms', () => {
       buttonToggleDebugElements = fixture.debugElement.queryAll(By.directive(MatButtonToggle));
       buttonToggleNativeElements = buttonToggleDebugElements
         .map(debugEl => debugEl.nativeElement);
-      buttonToggleLabelElements = fixture.debugElement.queryAll(By.css('label'))
+      buttonToggleLabelElements = fixture.debugElement.queryAll(By.css('button'))
         .map(debugEl => debugEl.nativeElement);
       buttonToggleInstances = buttonToggleDebugElements.map(debugEl => debugEl.componentInstance);
     }));
@@ -463,7 +463,7 @@ describe('MatButtonToggle without forms', () => {
     it('should check a button toggle when clicked', () => {
       expect(buttonToggleInstances.every(buttonToggle => !buttonToggle.checked)).toBe(true);
 
-      let nativeCheckboxLabel = buttonToggleDebugElements[0].query(By.css('label')).nativeElement;
+      let nativeCheckboxLabel = buttonToggleDebugElements[0].query(By.css('button')).nativeElement;
 
       nativeCheckboxLabel.click();
 
@@ -487,9 +487,9 @@ describe('MatButtonToggle without forms', () => {
     });
 
     it('should check a button toggle upon interaction with underlying native checkbox', () => {
-      let nativeCheckboxInput = buttonToggleDebugElements[0].query(By.css('input')).nativeElement;
+      let nativeCheckboxButton = buttonToggleDebugElements[0].query(By.css('button')).nativeElement;
 
-      nativeCheckboxInput.click();
+      nativeCheckboxButton.click();
       fixture.detectChanges();
 
       expect(groupInstance.value).toEqual(['eggs']);
@@ -562,6 +562,7 @@ describe('MatButtonToggle without forms', () => {
     let buttonToggleNativeElement: HTMLElement;
     let buttonToggleLabelElement: HTMLLabelElement;
     let buttonToggleInstance: MatButtonToggle;
+    let buttonToggleButtonElement: HTMLButtonElement;
 
     beforeEach(fakeAsync(() => {
       fixture = TestBed.createComponent(StandaloneButtonToggle);
@@ -569,8 +570,11 @@ describe('MatButtonToggle without forms', () => {
 
       buttonToggleDebugElement = fixture.debugElement.query(By.directive(MatButtonToggle));
       buttonToggleNativeElement = buttonToggleDebugElement.nativeElement;
-      buttonToggleLabelElement = fixture.debugElement.query(By.css('label')).nativeElement;
+      buttonToggleLabelElement = fixture.debugElement
+          .query(By.css('.mat-button-toggle-label-content')).nativeElement;
       buttonToggleInstance = buttonToggleDebugElement.componentInstance;
+      buttonToggleButtonElement =
+          buttonToggleNativeElement.querySelector('button')! as HTMLButtonElement;
     }));
 
     it('should toggle when clicked', fakeAsync(() => {
@@ -609,19 +613,30 @@ describe('MatButtonToggle without forms', () => {
     }));
 
     it('should focus on underlying input element when focus() is called', () => {
-      let nativeRadioInput = buttonToggleDebugElement.query(By.css('input')).nativeElement;
-      expect(document.activeElement).not.toBe(nativeRadioInput);
+      let nativeButton = buttonToggleDebugElement.query(By.css('button')).nativeElement;
+      expect(document.activeElement).not.toBe(nativeButton);
 
       buttonToggleInstance.focus();
       fixture.detectChanges();
 
-      expect(document.activeElement).toBe(nativeRadioInput);
+      expect(document.activeElement).toBe(nativeButton);
     });
 
     it('should not assign a name to the underlying input if one is not passed in', () => {
-      expect(buttonToggleNativeElement.querySelector('input')!.getAttribute('name')).toBeFalsy();
+      expect(buttonToggleButtonElement.getAttribute('name')).toBeFalsy();
     });
 
+    it('should have correct aria-pressed attribute', () => {
+      expect(buttonToggleButtonElement.getAttribute('aria-pressed'))
+          .toBe('false');
+
+      buttonToggleLabelElement.click();
+
+      fixture.detectChanges();
+
+      expect(buttonToggleButtonElement.getAttribute('aria-pressed'))
+        .toBe('true');
+    });
   });
 
   describe('aria-label handling ', () => {
@@ -629,46 +644,46 @@ describe('MatButtonToggle without forms', () => {
       let fixture = TestBed.createComponent(StandaloneButtonToggle);
       let checkboxDebugElement = fixture.debugElement.query(By.directive(MatButtonToggle));
       let checkboxNativeElement = checkboxDebugElement.nativeElement;
-      let inputElement = checkboxNativeElement.querySelector('input') as HTMLInputElement;
+      let buttonElement = checkboxNativeElement.querySelector('button') as HTMLButtonElement;
 
       fixture.detectChanges();
-      expect(inputElement.hasAttribute('aria-label')).toBe(false);
+      expect(buttonElement.hasAttribute('aria-label')).toBe(false);
     });
 
     it('should use the provided aria-label', () => {
       let fixture = TestBed.createComponent(ButtonToggleWithAriaLabel);
       let checkboxDebugElement = fixture.debugElement.query(By.directive(MatButtonToggle));
       let checkboxNativeElement = checkboxDebugElement.nativeElement;
-      let inputElement = checkboxNativeElement.querySelector('input') as HTMLInputElement;
+      let buttonElement = checkboxNativeElement.querySelector('button') as HTMLButtonElement;
 
       fixture.detectChanges();
-      expect(inputElement.getAttribute('aria-label')).toBe('Super effective');
+      expect(buttonElement.getAttribute('aria-label')).toBe('Super effective');
     });
   });
 
   describe('with provided aria-labelledby ', () => {
     let checkboxDebugElement: DebugElement;
     let checkboxNativeElement: HTMLElement;
-    let inputElement: HTMLInputElement;
+    let buttonElement: HTMLButtonElement;
 
     it('should use the provided aria-labelledby', () => {
       let fixture = TestBed.createComponent(ButtonToggleWithAriaLabelledby);
       checkboxDebugElement = fixture.debugElement.query(By.directive(MatButtonToggle));
       checkboxNativeElement = checkboxDebugElement.nativeElement;
-      inputElement = checkboxNativeElement.querySelector('input') as HTMLInputElement;
+      buttonElement = checkboxNativeElement.querySelector('button') as HTMLButtonElement;
 
       fixture.detectChanges();
-      expect(inputElement.getAttribute('aria-labelledby')).toBe('some-id');
+      expect(buttonElement.getAttribute('aria-labelledby')).toBe('some-id');
     });
 
     it('should not assign aria-labelledby if none is provided', () => {
       let fixture = TestBed.createComponent(StandaloneButtonToggle);
       checkboxDebugElement = fixture.debugElement.query(By.directive(MatButtonToggle));
       checkboxNativeElement = checkboxDebugElement.nativeElement;
-      inputElement = checkboxNativeElement.querySelector('input') as HTMLInputElement;
+      buttonElement = checkboxNativeElement.querySelector('button') as HTMLButtonElement;
 
       fixture.detectChanges();
-      expect(inputElement.getAttribute('aria-labelledby')).toBe(null);
+      expect(buttonElement.getAttribute('aria-labelledby')).toBe(null);
     });
   });
 
diff --git a/src/lib/button-toggle/button-toggle.ts b/src/lib/button-toggle/button-toggle.ts
index db15195cb4e..b1b4ab0f097 100644
--- a/src/lib/button-toggle/button-toggle.ts
+++ b/src/lib/button-toggle/button-toggle.ts
@@ -82,7 +82,7 @@ export class MatButtonToggleChange {
   ],
   inputs: ['disabled'],
   host: {
-    '[attr.role]': 'multiple ? "group" : "radiogroup"',
+    'role': 'group',
     'class': 'mat-button-toggle-group',
     '[class.mat-button-toggle-vertical]': 'vertical'
   },
@@ -353,13 +353,13 @@ export class MatButtonToggle extends _MatButtonToggleMixinBase implements OnInit
   /** Type of the button toggle. Either 'radio' or 'checkbox'. */
   _type: ToggleType;
 
-  @ViewChild('input') _inputElement: ElementRef<HTMLInputElement>;
+  @ViewChild('button') _buttonElement: ElementRef<HTMLButtonElement>;
 
   /** The parent button toggle group (exclusive selection). Optional. */
   buttonToggleGroup: MatButtonToggleGroup;
 
-  /** Unique ID for the underlying `input` element. */
-  get inputId(): string { return `${this.id}-input`; }
+  /** Unique ID for the underlying `button` element. */
+  get buttonId(): string { return `${this.id}-button`; }
 
   /** The unique ID for this button toggle. */
   @Input() id: string;
@@ -432,33 +432,25 @@ export class MatButtonToggle extends _MatButtonToggleMixinBase implements OnInit
 
   /** Focuses the button. */
   focus(): void {
-    this._inputElement.nativeElement.focus();
+    this._buttonElement.nativeElement.focus();
   }
 
-  /** Checks the button toggle due to an interaction with the underlying native input. */
-  _onInputChange(event: Event) {
+  /** Checks the button toggle due to an interaction with the underlying native button. */
+  _onButtonClick(event: Event) {
     event.stopPropagation();
 
-    this._checked = this._isSingleSelector ? true : !this._checked;
+    const newChecked = this._isSingleSelector ? true : !this._checked;
 
-    if (this.buttonToggleGroup) {
-      this.buttonToggleGroup._syncButtonToggle(this, this._checked, true);
-      this.buttonToggleGroup._onTouched();
+    if (newChecked !== this._checked) {
+      this._checked = newChecked;
+      if (this.buttonToggleGroup) {
+        this.buttonToggleGroup._syncButtonToggle(this, this._checked, true);
+        this.buttonToggleGroup._onTouched();
+      }
+
+      // Emit a change event when the native button does.
+      this.change.emit(new MatButtonToggleChange(this, this.value));
     }
-
-    // Emit a change event when the native input does.
-    this.change.emit(new MatButtonToggleChange(this, this.value));
-  }
-
-  _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();
   }
 
   /**