From bcf48261f4bd50b1c2b8e6f060a5f5ecb701c341 Mon Sep 17 00:00:00 2001
From: Paul Gschwendtner <paulgschwendtner@gmail.com>
Date: Tue, 11 Jul 2017 19:09:02 +0200
Subject: [PATCH] fix(checkbox, radio): setting id to null causes invalid id
 for input (#5398)

* In some situations, developers will change the `id` of a component dynamically. At some point if they set the `[id]` to `null` the input id will look like the following: `null-input`.
* If developers are setting the `id` using a binding then the DOM Id for the host component won't be set at all (because the @Input is triggered instead of the DOM property)

Fixes #5394
---
 e2e/components/checkbox-e2e.spec.ts       |  4 ++--
 src/lib/checkbox/checkbox.spec.ts         | 16 +++++++++++++---
 src/lib/checkbox/checkbox.ts              | 20 ++++++++++----------
 src/lib/radio/radio.ts                    | 14 +++++++-------
 src/lib/slide-toggle/slide-toggle.spec.ts | 10 ++++++----
 src/lib/slide-toggle/slide-toggle.ts      | 11 ++++-------
 6 files changed, 42 insertions(+), 33 deletions(-)

diff --git a/e2e/components/checkbox-e2e.spec.ts b/e2e/components/checkbox-e2e.spec.ts
index 2256bf712043..4d4988f04a76 100644
--- a/e2e/components/checkbox-e2e.spec.ts
+++ b/e2e/components/checkbox-e2e.spec.ts
@@ -9,7 +9,7 @@ describe('checkbox', () => {
 
     it('should be checked when clicked, and unchecked when clicked again', async () => {
       let checkboxEl = element(by.id('test-checkbox'));
-      let inputEl = element(by.css('input[id=input-test-checkbox]'));
+      let inputEl = element(by.css('input[id=test-checkbox-input]'));
 
       screenshot('start');
       checkboxEl.click();
@@ -32,7 +32,7 @@ describe('checkbox', () => {
     });
 
     it('should toggle the checkbox when pressing space', () => {
-      let inputEl = element(by.css('input[id=input-test-checkbox]'));
+      let inputEl = element(by.css('input[id=test-checkbox-input]'));
 
       expect(inputEl.getAttribute('checked'))
           .toBeFalsy('Expect checkbox "checked" property to be false');
diff --git a/src/lib/checkbox/checkbox.spec.ts b/src/lib/checkbox/checkbox.spec.ts
index 5ccecf52af5f..0fb39a761746 100644
--- a/src/lib/checkbox/checkbox.spec.ts
+++ b/src/lib/checkbox/checkbox.spec.ts
@@ -266,6 +266,15 @@ describe('MdCheckbox', () => {
 
     it('should preserve the user-provided id', () => {
       expect(checkboxNativeElement.id).toBe('simple-check');
+      expect(inputElement.id).toBe('simple-check-input');
+    });
+
+    it('should generate a unique id for the checkbox input if no id is set', () => {
+      testComponent.checkboxId = null;
+      fixture.detectChanges();
+
+      expect(checkboxInstance.inputId).toMatch(/md-checkbox-\d+/);
+      expect(inputElement.id).toBe(checkboxInstance.inputId);
     });
 
     it('should project the checkbox content into the label element', () => {
@@ -675,8 +684,8 @@ describe('MdCheckbox', () => {
           fixture.debugElement.queryAll(By.directive(MdCheckbox))
           .map(debugElement => debugElement.nativeElement.querySelector('input').id);
 
-      expect(firstId).toBeTruthy();
-      expect(secondId).toBeTruthy();
+      expect(firstId).toMatch(/md-checkbox-\d+-input/);
+      expect(secondId).toMatch(/md-checkbox-\d+-input/);
       expect(firstId).not.toEqual(secondId);
     });
   });
@@ -833,7 +842,7 @@ describe('MdCheckbox', () => {
   template: `
   <div (click)="parentElementClicked = true" (keyup)="parentElementKeyedUp = true">
     <md-checkbox
-        id="simple-check"
+        [id]="checkboxId"
         [required]="isRequired"
         [labelPosition]="labelPos"
         [checked]="isChecked"
@@ -857,6 +866,7 @@ class SingleCheckbox {
   disableRipple: boolean = false;
   parentElementClicked: boolean = false;
   parentElementKeyedUp: boolean = false;
+  checkboxId: string | null = 'simple-check';
   checkboxColor: string = 'primary';
   checkboxValue: string = 'single_checkbox';
 
diff --git a/src/lib/checkbox/checkbox.ts b/src/lib/checkbox/checkbox.ts
index 83d7df83cc27..a1c228008e6d 100644
--- a/src/lib/checkbox/checkbox.ts
+++ b/src/lib/checkbox/checkbox.ts
@@ -27,9 +27,8 @@ import {FocusOrigin, FocusOriginMonitor, MdRipple, RippleRef} from '../core';
 import {mixinDisabled, CanDisable} from '../core/common-behaviors/disabled';
 import {CanColor, mixinColor} from '../core/common-behaviors/color';
 
-
-/** Monotonically increasing integer used to auto-generate unique ids for checkbox components. */
-let nextId = 0;
+// Increasing integer for generating unique ids for checkbox components.
+let nextUniqueId = 0;
 
 /**
  * Provider Expression that allows md-checkbox to register as a ControlValueAccessor.
@@ -88,6 +87,7 @@ export const _MdCheckboxMixinBase = mixinColor(mixinDisabled(MdCheckboxBase), 'a
   styleUrls: ['checkbox.css'],
   host: {
     'class': 'mat-checkbox',
+    '[id]': 'id',
     '[class.mat-checkbox-indeterminate]': 'indeterminate',
     '[class.mat-checkbox-checked]': 'checked',
     '[class.mat-checkbox-disabled]': 'disabled',
@@ -111,8 +111,13 @@ export class MdCheckbox extends _MdCheckboxMixinBase
    */
   @Input('aria-labelledby') ariaLabelledby: string | null = null;
 
-  /** A unique id for the checkbox. If one is not supplied, it is auto-generated. */
-  @Input() id: string = `md-checkbox-${++nextId}`;
+  private _uniqueId: string = `md-checkbox-${++nextUniqueId}`;
+
+  /** A unique id for the checkbox input. If none is supplied, it will be auto-generated. */
+  @Input() id: string = this._uniqueId;
+
+  /** Returns the unique id for the visual hidden input. */
+  get inputId(): string { return `${this.id || this._uniqueId}-input`; }
 
   /** Whether the ripple effect on click should be disabled. */
   private _disableRipple: boolean;
@@ -122,11 +127,6 @@ export class MdCheckbox extends _MdCheckboxMixinBase
   get disableRipple(): boolean { return this._disableRipple; }
   set disableRipple(value) { this._disableRipple = coerceBooleanProperty(value); }
 
-  /** ID of the native input element inside `<md-checkbox>` */
-  get inputId(): string {
-    return `input-${this.id}`;
-  }
-
   private _required: boolean;
 
   /** Whether the checkbox is required. */
diff --git a/src/lib/radio/radio.ts b/src/lib/radio/radio.ts
index febc665e636a..4ece23549821 100644
--- a/src/lib/radio/radio.ts
+++ b/src/lib/radio/radio.ts
@@ -39,6 +39,8 @@ import {coerceBooleanProperty} from '@angular/cdk';
 import {mixinDisabled, CanDisable} from '../core/common-behaviors/disabled';
 import {CanColor, mixinColor} from '../core/common-behaviors/color';
 
+// Increasing integer for generating unique ids for radio components.
+let nextUniqueId = 0;
 
 /**
  * Provider Expression that allows md-radio-group to register as a ControlValueAccessor. This
@@ -51,8 +53,6 @@ export const MD_RADIO_GROUP_CONTROL_VALUE_ACCESSOR: any = {
   multi: true
 };
 
-let _uniqueIdCounter = 0;
-
 /** Change event object emitted by MdRadio and MdRadioGroup. */
 export class MdRadioChange {
   /** The MdRadioButton that emits the change event. */
@@ -90,7 +90,7 @@ export class MdRadioGroup extends _MdRadioGroupMixinBase
   private _value: any = null;
 
   /** The HTML name attribute applied to radio buttons in this group. */
-  private _name: string = `md-radio-group-${_uniqueIdCounter++}`;
+  private _name: string = `md-radio-group-${nextUniqueId++}`;
 
   /** The currently selected radio button. Should match value. */
   private _selected: MdRadioButton | null = null;
@@ -326,8 +326,10 @@ export const _MdRadioButtonMixinBase = mixinColor(MdRadioButtonBase, 'accent');
 export class MdRadioButton extends _MdRadioButtonMixinBase
     implements OnInit, AfterViewInit, OnDestroy, CanColor {
 
+  private _uniqueId: string = `md-radio-${++nextUniqueId}`;
+
   /** The unique ID for the radio button. */
-  @Input() id: string = `md-radio-${_uniqueIdCounter++}`;
+  @Input() id: string = this._uniqueId;
 
   /** Analog to HTML 'name' attribute used to group radios for unique selection. */
   @Input() name: string;
@@ -437,9 +439,7 @@ export class MdRadioButton extends _MdRadioButtonMixinBase
   radioGroup: MdRadioGroup;
 
   /** ID of the native input element inside `<md-radio-button>` */
-  get inputId(): string {
-    return `${this.id}-input`;
-  }
+  get inputId(): string { return `${this.id || this._uniqueId}-input`; }
 
   /** Whether this radio is checked. */
   private _checked: boolean = false;
diff --git a/src/lib/slide-toggle/slide-toggle.spec.ts b/src/lib/slide-toggle/slide-toggle.spec.ts
index 4e9b606efbdc..7a0f009ce51c 100644
--- a/src/lib/slide-toggle/slide-toggle.spec.ts
+++ b/src/lib/slide-toggle/slide-toggle.spec.ts
@@ -163,18 +163,20 @@ describe('MdSlideToggle without forms', () => {
       testComponent.slideId = 'myId';
       fixture.detectChanges();
 
-      expect(inputElement.id).toBe('myId-input');
+      expect(slideToggleElement.id).toBe('myId');
+      expect(inputElement.id).toBe(`${slideToggleElement.id}-input`);
 
       testComponent.slideId = 'nextId';
       fixture.detectChanges();
 
-      expect(inputElement.id).toBe('nextId-input');
+      expect(slideToggleElement.id).toBe('nextId');
+      expect(inputElement.id).toBe(`${slideToggleElement.id}-input`);
 
       testComponent.slideId = null;
       fixture.detectChanges();
 
-      // Once the id input is falsy, we use a default prefix with a incrementing unique number.
-      expect(inputElement.id).toMatch(/md-slide-toggle-[0-9]+-input/g);
+      // Once the id binding is set to null, the id property should auto-generate a unique id.
+      expect(inputElement.id).toMatch(/md-slide-toggle-\d+-input/);
     });
 
     it('should forward the tabIndex to the underlying input', () => {
diff --git a/src/lib/slide-toggle/slide-toggle.ts b/src/lib/slide-toggle/slide-toggle.ts
index fb6646080609..b09995da3095 100644
--- a/src/lib/slide-toggle/slide-toggle.ts
+++ b/src/lib/slide-toggle/slide-toggle.ts
@@ -35,6 +35,8 @@ import {ControlValueAccessor, NG_VALUE_ACCESSOR} from '@angular/forms';
 import {mixinDisabled, CanDisable} from '../core/common-behaviors/disabled';
 import {CanColor, mixinColor} from '../core/common-behaviors/color';
 
+// Increasing integer for generating unique ids for slide-toggle components.
+let nextUniqueId = 0;
 
 export const MD_SLIDE_TOGGLE_VALUE_ACCESSOR: any = {
   provide: NG_VALUE_ACCESSOR,
@@ -48,11 +50,6 @@ export class MdSlideToggleChange {
   checked: boolean;
 }
 
-// Increasing integer for generating unique ids for slide-toggle components.
-let nextId = 0;
-
-
-
 // Boilerplate for applying mixins to MdSlideToggle.
 /** @docs-private */
 export class MdSlideToggleBase {
@@ -66,6 +63,7 @@ export const _MdSlideToggleMixinBase = mixinColor(mixinDisabled(MdSlideToggleBas
   selector: 'md-slide-toggle, mat-slide-toggle',
   host: {
     'class': 'mat-slide-toggle',
+    '[id]': 'id',
     '[class.mat-checked]': 'checked',
     '[class.mat-disabled]': 'disabled',
     '[class.mat-slide-toggle-label-before]': 'labelPosition == "before"',
@@ -82,8 +80,7 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
   private onChange = (_: any) => {};
   private onTouched = () => {};
 
-  // A unique id for the slide-toggle. By default the id is auto-generated.
-  private _uniqueId = `md-slide-toggle-${++nextId}`;
+  private _uniqueId: string = `md-slide-toggle-${++nextUniqueId}`;
   private _checked: boolean = false;
   private _slideRenderer: SlideToggleRenderer;
   private _required: boolean = false;