Skip to content

fix(button-toggle): use native button and aria-pressed for button-toggle #10990

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.

Sign up for GitHub

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 1 commit into from
Jun 20, 2018
Merged

fix(button-toggle): use native button and aria-pressed for button-toggle #10990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 12 additions & 15 deletions src/lib/button-toggle/button-toggle.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs type="button", otherwise it'll submit any parent form elements when it's toggled.

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>
10 changes: 10 additions & 0 deletions src/lib/button-toggle/button-toggle.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
57 changes: 36 additions & 21 deletions src/lib/button-toggle/button-toggle.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}));
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}));
Expand All @@ -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();

Expand All @@ -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']);
Expand Down Expand Up @@ -562,15 +562,19 @@ describe('MatButtonToggle without forms', () => {
let buttonToggleNativeElement: HTMLElement;
let buttonToggleLabelElement: HTMLLabelElement;
let buttonToggleInstance: MatButtonToggle;
let buttonToggleButtonElement: HTMLButtonElement;

beforeEach(fakeAsync(() => {
fixture = TestBed.createComponent(StandaloneButtonToggle);
fixture.detectChanges();

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(() => {
Expand Down Expand Up @@ -609,66 +613,77 @@ 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 ', () => {
it('should not set the aria-label attribute if none is provided', () => {
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);
});
});

Expand Down
42 changes: 17 additions & 25 deletions src/lib/button-toggle/button-toggle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
},
Expand Down Expand Up @@ -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`; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change to the public API and needs to be aliased and marked deprecated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I missed that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Updated and marked deprecated.


/** The unique ID for this button toggle. */
@Input() id: string;
Expand Down Expand Up @@ -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();
}

// Emit a change event when the native input does.
this.change.emit(new MatButtonToggleChange(this, this.value));
}
if (newChecked !== this._checked) {
this._checked = newChecked;
if (this.buttonToggleGroup) {
this.buttonToggleGroup._syncButtonToggle(this, this._checked, true);
this.buttonToggleGroup._onTouched();
}

_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();
// Emit a change event when the native button does.
this.change.emit(new MatButtonToggleChange(this, this.value));
}
}

/**
Expand Down