Skip to content

Commit

Permalink
fix(material/slide-toggle): remove tabindex from host node (#23891)
Browse files Browse the repository at this point in the history
The `tabindex="-1"` on the host node was causing VoicerOver to read out the slide toggle as a group and to read out the label twice. These changes remove the `tabindex` like we've done for `mat-radio` and `mat-checkbox` in the past.
  • Loading branch information
crisbeto committed Nov 9, 2021
1 parent f69cf6c commit b533e61
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 40 deletions.
14 changes: 2 additions & 12 deletions src/material-experimental/mdc-slide-toggle/slide-toggle.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -287,16 +287,6 @@ describe('MDC-based MatSlideToggle without forms', () => {
expect(document.activeElement).toBe(buttonElement);
}));

it('should focus on underlying element when the host is focused', fakeAsync(() => {
expect(document.activeElement).not.toBe(buttonElement);

slideToggleElement.focus();
fixture.detectChanges();
tick();

expect(document.activeElement).toBe(buttonElement);
}));

it('should not manually move focus to underlying when focus comes from mouse or touch', fakeAsync(
inject([FocusMonitor], (focusMonitor: FocusMonitor) => {
expect(document.activeElement).not.toBe(buttonElement);
Expand Down Expand Up @@ -397,13 +387,13 @@ describe('MDC-based MatSlideToggle without forms', () => {
expect(switchEl.classList).toContain('mdc-switch--checked');
}));

it('should set the tabindex of the host element to -1', fakeAsync(() => {
it('should remove the tabindex from the host node', fakeAsync(() => {
const fixture = TestBed.createComponent(SlideToggleWithTabindexAttr);

fixture.detectChanges();

const slideToggle = fixture.debugElement.query(By.directive(MatSlideToggle))!.nativeElement;
expect(slideToggle.getAttribute('tabindex')).toBe('-1');
expect(slideToggle.hasAttribute('tabindex')).toBe(false);
}));

it('should remove the tabindex from the host element when disabled', fakeAsync(() => {
Expand Down
9 changes: 2 additions & 7 deletions src/material-experimental/mdc-slide-toggle/slide-toggle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ export class MatSlideToggleChange {
host: {
'class': 'mat-mdc-slide-toggle',
'[id]': 'id',
// Needs to be `-1` so it can still receive programmatic focus.
'[attr.tabindex]': 'disabled ? null : -1',
// Needs to be removed since it causes some a11y issues (see #21266).
'[attr.tabindex]': 'null',
'[attr.aria-label]': 'null',
'[attr.aria-labelledby]': 'null',
'[class.mat-primary]': 'color === "primary"',
Expand Down Expand Up @@ -221,12 +221,7 @@ export class MatSlideToggle implements ControlValueAccessor, AfterViewInit, OnDe
foundation.setChecked(this.checked);

this._focusMonitor.monitor(this._elementRef, true).subscribe(focusOrigin => {
// Only forward focus manually when it was received programmatically or through the
// keyboard. We should not do this for mouse/touch focus for two reasons:
// 1. It can prevent clicks from landing in Chrome (see #18269).
// 2. They're already handled by the wrapping `label` element.
if (focusOrigin === 'keyboard' || focusOrigin === 'program') {
this._switchElement.nativeElement.focus();
this._focused = true;
} else if (!focusOrigin) {
// When a focused element becomes disabled, the browser *immediately* fires a blur event.
Expand Down
14 changes: 2 additions & 12 deletions src/material/slide-toggle/slide-toggle.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -318,16 +318,6 @@ describe('MatSlideToggle without forms', () => {
expect(document.activeElement).toBe(inputElement);
}));

it('should focus on underlying element when the host is focused', fakeAsync(() => {
expect(document.activeElement).not.toBe(inputElement);

slideToggleElement.focus();
fixture.detectChanges();
flush();

expect(document.activeElement).toBe(inputElement);
}));

it('should not manually move focus to underlying when focus comes from mouse or touch', inject(
[FocusMonitor],
(focusMonitor: FocusMonitor) => {
Expand Down Expand Up @@ -410,13 +400,13 @@ describe('MatSlideToggle without forms', () => {
.toBe(5);
}));

it('should set the tabindex of the host element to -1', fakeAsync(() => {
it('should remove the tabindex from the host node', fakeAsync(() => {
const fixture = TestBed.createComponent(SlideToggleWithTabindexAttr);

fixture.detectChanges();

const slideToggle = fixture.debugElement.query(By.directive(MatSlideToggle))!.nativeElement;
expect(slideToggle.getAttribute('tabindex')).toBe('-1');
expect(slideToggle.hasAttribute('tabindex')).toBe(false);
}));

it('should remove the tabindex from the host element when disabled', fakeAsync(() => {
Expand Down
12 changes: 3 additions & 9 deletions src/material/slide-toggle/slide-toggle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ const _MatSlideToggleBase = mixinTabIndex(
host: {
'class': 'mat-slide-toggle',
'[id]': 'id',
// Needs to be `-1` so it can still receive programmatic focus.
'[attr.tabindex]': 'disabled ? null : -1',
// Needs to be removed since it causes some a11y issues (see #21266).
'[attr.tabindex]': 'null',
'[attr.aria-label]': 'null',
'[attr.aria-labelledby]': 'null',
'[class.mat-checked]': 'checked',
Expand Down Expand Up @@ -198,13 +198,7 @@ export class MatSlideToggle

ngAfterContentInit() {
this._focusMonitor.monitor(this._elementRef, true).subscribe(focusOrigin => {
// Only forward focus manually when it was received programmatically or through the
// keyboard. We should not do this for mouse/touch focus for two reasons:
// 1. It can prevent clicks from landing in Chrome (see #18269).
// 2. They're already handled by the wrapping `label` element.
if (focusOrigin === 'keyboard' || focusOrigin === 'program') {
this._inputElement.nativeElement.focus();
} else if (!focusOrigin) {
if (!focusOrigin) {
// When a focused element becomes disabled, the browser *immediately* fires a blur event.
// Angular does not expect events to be raised during change detection, so any state
// change (such as a form control's 'ng-touched') will cause a changed-after-checked
Expand Down

0 comments on commit b533e61

Please sign in to comment.