fix(checkbox, radio): setting id to null causes invalid id for input

* 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 id's are auto-generated they should be reflected in the `id` property of the component (after setting the id to `null` for example)
* 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

Author: devversion

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]'));
       let checked: string;
 
       screenshot('start');
@@ -33,7 +33,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');

src/lib/checkbox/checkbox.spec.ts

@@ -257,6 +257,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.id}-input`);
     });
 
     it('should project the checkbox content into the label element', () => {
@@ -666,8 +675,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);
     });
   });
@@ -824,7 +833,7 @@ describe('MdCheckbox', () => {
   template: `
   <div (click)="parentElementClicked = true" (keyup)="parentElementKeyedUp = true">
     <md-checkbox
-        id="simple-check"
+        [id]="checkboxId"
         [required]="isRequired"
         [labelPosition]="labelPos"
         [checked]="isChecked"
@@ -848,6 +857,7 @@ class SingleCheckbox {
   disableRipple: boolean = false;
   parentElementClicked: boolean = false;
   parentElementKeyedUp: boolean = false;
+  checkboxId: string | null = 'simple-check';
   checkboxColor: string = 'primary';
   checkboxValue: string = 'single_checkbox';
 

src/lib/checkbox/checkbox.ts

@@ -26,10 +26,7 @@ import {coerceBooleanProperty} from '@angular/cdk';
 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;
+import {CanId, mixinId} from '../core/common-behaviors/id';
 
 /**
  * Provider Expression that allows md-checkbox to register as a ControlValueAccessor.
@@ -70,7 +67,8 @@ export class MdCheckboxChange {
 export class MdCheckboxBase {
   constructor(public _renderer: Renderer2, public _elementRef: ElementRef) {}
 }
-export const _MdCheckboxMixinBase = mixinColor(mixinDisabled(MdCheckboxBase), 'accent');
+export const _MdCheckboxMixinBase =
+  mixinId(mixinColor(mixinDisabled(MdCheckboxBase), 'accent'), 'md-checkbox');
 
 
 /**
@@ -88,18 +86,19 @@ 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',
     '[class.mat-checkbox-label-before]': 'labelPosition == "before"',
   },
   providers: [MD_CHECKBOX_CONTROL_VALUE_ACCESSOR],
-  inputs: ['disabled', 'color'],
+  inputs: ['disabled', 'color', 'id'],
   encapsulation: ViewEncapsulation.None,
   changeDetection: ChangeDetectionStrategy.OnPush
 })
 export class MdCheckbox extends _MdCheckboxMixinBase
-    implements ControlValueAccessor, AfterViewInit, OnDestroy, CanColor, CanDisable {
+    implements ControlValueAccessor, AfterViewInit, OnDestroy, CanColor, CanDisable, CanId {
   /**
    * Attached to the aria-label attribute of the host element. In most cases, arial-labelledby will
    * take precedence so this may be omitted.
@@ -111,8 +110,8 @@ 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}`;
+  /** Returns the unique id for the visual hidden input. */
+  get inputId(): string { return `${this.id}-input`; }
 
   /** Whether the ripple effect on click should be disabled. */
   private _disableRipple: boolean;
@@ -122,11 +121,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. */

src/lib/core/common-behaviors/id.spec.ts

@@ -0,0 +1,33 @@
+import {mixinId} from './id';
+
+describe('MixinId', () => {
+
+  it('should augment an existing class with an id property', () => {
+    const classWithId = mixinId(BaseClass, 'mat-checkbox');
+    const instance = new classWithId();
+
+    expect(instance.id)
+      .toMatch(/mat-checkbox-\d+/, 'Expected the mixin to auto-generate a unique id by default.');
+
+    instance.id = 'custom-name';
+
+    expect(instance.id).toBe('custom-name', 'Expected the mixin to allow setting the id property');
+  });
+
+  it('should not generate the same unique id multiple times', () => {
+    const firstClassWithId = mixinId(BaseClass, 'mat-checkbox');
+    const secondClassWithId = mixinId(BaseClass, 'mat-checkbox');
+
+    const firstClassInstance = new firstClassWithId();
+    const secondClassInstance = new secondClassWithId();
+
+    expect(firstClassInstance.id).toBeTruthy();
+    expect(secondClassInstance.id).toBeTruthy();
+
+    expect(firstClassInstance.id).not.toBe(secondClassInstance.id,
+        'Expected the mixin to not generate the unique id multiple times');
+  });
+
+});
+
+export class BaseClass {}

src/lib/core/common-behaviors/id.ts

@@ -0,0 +1,35 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {Constructor} from './constructor';
+
+/** Monotonically increasing integer used to auto-generate unique ids. */
+let nextUniqueId = 0;
+
+/** @docs-private */
+export interface CanId {
+  id: string;
+}
+
+/**
+ * Mixin to augment a directive with an `id` property.
+ * The id property will fall back to auto-generated unique ids if developers don't specify one.
+ */
+export function mixinId<T extends Constructor<{}>>(base: T, componentName: string)
+    : Constructor<CanId> & T {
+  return class extends base {
+    private _id: string;
+    private _uniqueId = `${componentName}-${++nextUniqueId}`;
+
+    /** Unique id for the component. If one is not supplied, it is auto-generated. */
+    get id(): string { return this._id || this._uniqueId; }
+    set id(value: string) { this._id = value; }
+
+    constructor(...args: any[]) { super(...args); }
+  };
+}

src/lib/radio/radio.ts

@@ -38,6 +38,7 @@ import {
 import {coerceBooleanProperty} from '@angular/cdk';
 import {mixinDisabled, CanDisable} from '../core/common-behaviors/disabled';
 import {CanColor, mixinColor} from '../core/common-behaviors/color';
+import {CanId, mixinId} from '../core/common-behaviors/id';
 
 
 /**
@@ -51,8 +52,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. */
@@ -65,7 +64,7 @@ export class MdRadioChange {
 // Boilerplate for applying mixins to MdRadioGroup.
 /** @docs-private */
 export class MdRadioGroupBase { }
-export const _MdRadioGroupMixinBase = mixinDisabled(MdRadioGroupBase);
+export const _MdRadioGroupMixinBase = mixinId(mixinDisabled(MdRadioGroupBase), 'md-radio-group');
 
 /**
  * A group of radio buttons. May contain one or more `<md-radio-button>` elements.
@@ -76,11 +75,12 @@ export const _MdRadioGroupMixinBase = mixinDisabled(MdRadioGroupBase);
   host: {
     'role': 'radiogroup',
     'class': 'mat-radio-group',
+    '[id]': 'id',
   },
   inputs: ['disabled'],
 })
 export class MdRadioGroup extends _MdRadioGroupMixinBase
-    implements AfterContentInit, ControlValueAccessor, CanDisable {
+    implements AfterContentInit, ControlValueAccessor, CanDisable, CanId {
   /**
    * Selected value for group. Should equal the value of the selected radio button if there *is*
    * a corresponding radio button with a matching value. If there is *not* such a corresponding
@@ -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 = this.id;
 
   /** The currently selected radio button. Should match value. */
   private _selected: MdRadioButton | null = null;
@@ -303,7 +303,8 @@ export class MdRadioButtonBase {
 }
 // As per Material design specifications the selection control radio should use the accent color
 // palette by default. https://material.io/guidelines/components/selection-controls.html
-export const _MdRadioButtonMixinBase = mixinColor(MdRadioButtonBase, 'accent');
+export const _MdRadioButtonMixinBase =
+    mixinId(mixinColor(MdRadioButtonBase, 'accent'), 'md-radio-button');
 
 /**
  * A radio-button. May be inside of
@@ -313,7 +314,7 @@ export const _MdRadioButtonMixinBase = mixinColor(MdRadioButtonBase, 'accent');
   selector: 'md-radio-button, mat-radio-button',
   templateUrl: 'radio.html',
   styleUrls: ['radio.css'],
-  inputs: ['color'],
+  inputs: ['color', 'id'],
   encapsulation: ViewEncapsulation.None,
   host: {
     'class': 'mat-radio-button',
@@ -324,13 +325,10 @@ export const _MdRadioButtonMixinBase = mixinColor(MdRadioButtonBase, 'accent');
   changeDetection: ChangeDetectionStrategy.OnPush,
 })
 export class MdRadioButton extends _MdRadioButtonMixinBase
-    implements OnInit, AfterViewInit, OnDestroy, CanColor {
-
-  /** The unique ID for the radio button. */
-  @Input() id: string = `md-radio-${_uniqueIdCounter++}`;
+    implements OnInit, AfterViewInit, OnDestroy, CanColor, CanId {
 
   /** Analog to HTML 'name' attribute used to group radios for unique selection. */
-  @Input() name: string;
+  @Input() name: string = this.id;
 
   /** Used to set the 'aria-label' attribute on the underlying input element. */
   @Input('aria-label') ariaLabel: string;
@@ -437,9 +435,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}-input`; }
 
   /** Whether this radio is checked. */
   private _checked: boolean = false;

src/lib/slide-toggle/slide-toggle.spec.ts

@@ -177,18 +177,21 @@ describe('MdSlideToggle', () => {
       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(slideToggleElement.id).toMatch(/md-slide-toggle-\d+/);
+      expect(inputElement.id).toBe(`${slideToggleElement.id}-input`);
     });
 
     it('should forward the tabIndex to the underlying input', () => {

src/lib/slide-toggle/slide-toggle.ts

@@ -34,6 +34,7 @@ import {
 import {ControlValueAccessor, NG_VALUE_ACCESSOR} from '@angular/forms';
 import {mixinDisabled, CanDisable} from '../core/common-behaviors/disabled';
 import {CanColor, mixinColor} from '../core/common-behaviors/color';
+import {mixinId, CanId} from '../core/common-behaviors/id';
 
 
 export const MD_SLIDE_TOGGLE_VALUE_ACCESSOR: any = {
@@ -48,42 +49,37 @@ 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 {
   constructor(public _renderer: Renderer2, public _elementRef: ElementRef) {}
 }
-export const _MdSlideToggleMixinBase = mixinColor(mixinDisabled(MdSlideToggleBase), 'accent');
+export const _MdSlideToggleMixinBase =
+    mixinId(mixinColor(mixinDisabled(MdSlideToggleBase), 'accent'), 'md-slide-toggle');
 
 /** Represents a slidable "switch" toggle that can be moved between on and off. */
 @Component({
   moduleId: module.id,
   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"',
   },
   templateUrl: 'slide-toggle.html',
   styleUrls: ['slide-toggle.css'],
   providers: [MD_SLIDE_TOGGLE_VALUE_ACCESSOR],
-  inputs: ['disabled', 'color'],
+  inputs: ['disabled', 'color', 'id'],
   encapsulation: ViewEncapsulation.None,
   changeDetection: ChangeDetectionStrategy.OnPush
 })
 export class MdSlideToggle extends _MdSlideToggleMixinBase
-    implements OnDestroy, AfterContentInit, ControlValueAccessor, CanDisable, CanColor {
+    implements OnDestroy, AfterContentInit, ControlValueAccessor, CanDisable, CanColor, CanId {
   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 _checked: boolean = false;
   private _slideRenderer: SlideToggleRenderer;
   private _required: boolean = false;
@@ -95,8 +91,6 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
   /** Name value will be applied to the input element if present */
   @Input() name: string | null = null;
 
-  /** A unique id for the slide-toggle input. If none is supplied, it will be auto-generated. */
-  @Input() id: string = this._uniqueId;
 
   /** Used to specify the tabIndex value for the underlying input element. */
   @Input() tabIndex: number = 0;
@@ -124,7 +118,7 @@ export class MdSlideToggle extends _MdSlideToggleMixinBase
   @Output() change: EventEmitter<MdSlideToggleChange> = new EventEmitter<MdSlideToggleChange>();
 
   /** Returns the unique id for the visual hidden input. */
-  get inputId(): string { return `${this.id || this._uniqueId}-input`; }
+  get inputId(): string { return `${this.id}-input`; }
 
   /** Reference to the underlying input element. */
   @ViewChild('input') _inputElement: ElementRef;