Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions e2e/components/checkbox-e2e.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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');
Expand Down
16 changes: 13 additions & 3 deletions src/lib/checkbox/checkbox.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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);
});
});
Expand Down Expand Up @@ -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"
Expand All @@ -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';

Expand Down
20 changes: 10 additions & 10 deletions src/lib/checkbox/checkbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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',
Expand All @@ -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;
Expand All @@ -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. */
Expand Down
14 changes: 7 additions & 7 deletions src/lib/radio/radio.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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. */
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
10 changes: 6 additions & 4 deletions src/lib/slide-toggle/slide-toggle.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,18 +180,20 @@ 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(inputElement.id).toMatch(/md-slide-toggle-\d+-input/);
});

it('should forward the tabIndex to the underlying input', () => {
Expand Down
11 changes: 4 additions & 7 deletions src/lib/slide-toggle/slide-toggle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 {
Expand All @@ -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"',
Expand All @@ -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;
Expand Down