Skip to content

Commit

Permalink
fix(material/core): remove tabindex from mat-option
Browse files Browse the repository at this point in the history
Remove the tabindex attribute added to MatOption components. MatOption
does not need tabindex because the parent component manages focus by
setting `aria-activedescendant` attribute. Previously, MatOption set
tabindex but was also a referenced by aria-activedescendant. This was
not the correct ARIA semantics. Align closer to ARIA spec by using only
aria-activedescendant rather than both.

Tabindex="-1" seems to be causing a problem in angular#26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.

Unblocks angular#26861.
  • Loading branch information
zarend committed Apr 11, 2023
1 parent d7eeb2c commit 43585f8
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 26 deletions.
6 changes: 0 additions & 6 deletions src/material/core/option/option.ts
Expand Up @@ -204,11 +204,6 @@ export class _MatOptionBase<T = any> implements FocusableOption, AfterViewChecke
}
}

/** Returns the correct tabindex for the option depending on disabled state. */
_getTabIndex(): string {
return this.disabled ? '-1' : '0';
}

/** Gets the host DOM element. */
_getHostElement(): HTMLElement {
return this._element.nativeElement;
Expand Down Expand Up @@ -251,7 +246,6 @@ export class _MatOptionBase<T = any> implements FocusableOption, AfterViewChecke
exportAs: 'matOption',
host: {
'role': 'option',
'[attr.tabindex]': '_getTabIndex()',
'[class.mdc-list-item--selected]': 'selected',
'[class.mat-mdc-option-multiple]': 'multiple',
'[class.mat-mdc-option-active]': 'active',
Expand Down
1 change: 0 additions & 1 deletion src/material/legacy-core/option/option.ts
Expand Up @@ -33,7 +33,6 @@ import {MatLegacyOptgroup} from './optgroup';
exportAs: 'matOption',
host: {
'role': 'option',
'[attr.tabindex]': '_getTabIndex()',
'[class.mat-selected]': 'selected',
'[class.mat-option-multiple]': 'multiple',
'[class.mat-active]': 'active',
Expand Down
28 changes: 19 additions & 9 deletions src/material/legacy-select/select.spec.ts
Expand Up @@ -790,17 +790,27 @@ describe('MatSelect', () => {
'mat-option',
) as NodeListOf<HTMLElement>;

options[3].focus();
select.focus();
multiFixture.detectChanges();
multiFixture.componentInstance.select._keyManager.setActiveItem(3);
multiFixture.detectChanges();

expect(document.activeElement)
.withContext('Expected fourth option to be focused.')
.toBe(options[3]);
.withContext('Expected select to have DOM focus.')
.toBe(select);
expect(select.getAttribute('aria-activedescendant'))
.withContext('Expected fourth option to be activated.')
.toBe(options[3].id);

multiFixture.componentInstance.control.setValue(['steak-0', 'sushi-7']);
multiFixture.detectChanges();

expect(document.activeElement)
.withContext('Expected fourth option to remain focused.')
.toBe(options[3]);
.withContext('Expected select to have DOM focus.')
.toBe(select);
expect(select.getAttribute('aria-activedescendant'))
.withContext('Expected fourth optino to remain activated.')
.toBe(options[3].id);
}),
);

Expand Down Expand Up @@ -1223,10 +1233,10 @@ describe('MatSelect', () => {
.toBe(true);
}));

it('should set the tabindex of each option according to disabled state', fakeAsync(() => {
expect(options[0].getAttribute('tabindex')).toEqual('0');
expect(options[1].getAttribute('tabindex')).toEqual('0');
expect(options[2].getAttribute('tabindex')).toEqual('-1');
it('should omit the tabindex attribute on each option', fakeAsync(() => {
expect(options[0].hasAttribute('tabindex')).toBeFalse();
expect(options[1].hasAttribute('tabindex')).toBeFalse();
expect(options[2].hasAttribute('tabindex')).toBeFalse();
}));

it('should set aria-disabled for disabled options', fakeAsync(() => {
Expand Down
28 changes: 19 additions & 9 deletions src/material/select/select.spec.ts
Expand Up @@ -824,17 +824,27 @@ describe('MDC-based MatSelect', () => {
'mat-option',
) as NodeListOf<HTMLElement>;

options[3].focus();
select.focus();
multiFixture.detectChanges();
multiFixture.componentInstance.select._keyManager.setActiveItem(3);
multiFixture.detectChanges();

expect(document.activeElement)
.withContext('Expected fourth option to be focused.')
.toBe(options[3]);
.withContext('Expected select to have DOM focus.')
.toBe(select);
expect(select.getAttribute('aria-activedescendant'))
.withContext('Expected fourth option to be activated.')
.toBe(options[3].id);

multiFixture.componentInstance.control.setValue(['steak-0', 'sushi-7']);
multiFixture.detectChanges();

expect(document.activeElement)
.withContext('Expected fourth option to remain focused.')
.toBe(options[3]);
.withContext('Expected select to have DOM focus.')
.toBe(select);
expect(select.getAttribute('aria-activedescendant'))
.withContext('Expected fourth optino to remain activated.')
.toBe(options[3].id);
}),
);

Expand Down Expand Up @@ -1260,10 +1270,10 @@ describe('MDC-based MatSelect', () => {
.toBe(true);
}));

it('should set the tabindex of each option according to disabled state', fakeAsync(() => {
expect(options[0].getAttribute('tabindex')).toEqual('0');
expect(options[1].getAttribute('tabindex')).toEqual('0');
expect(options[2].getAttribute('tabindex')).toEqual('-1');
it('should omit the tabindex attribute on each option', fakeAsync(() => {
expect(options[0].hasAttribute('tabindex')).toBeFalse();
expect(options[1].hasAttribute('tabindex')).toBeFalse();
expect(options[2].hasAttribute('tabindex')).toBeFalse();
}));

it('should set aria-disabled for disabled options', fakeAsync(() => {
Expand Down
1 change: 0 additions & 1 deletion tools/public_api_guard/material/core.md
Expand Up @@ -281,7 +281,6 @@ export class _MatOptionBase<T = any> implements FocusableOption, AfterViewChecke
focus(_origin?: FocusOrigin, options?: FocusOptions): void;
_getHostElement(): HTMLElement;
getLabel(): string;
_getTabIndex(): string;
// (undocumented)
readonly group: _MatOptgroupBase;
_handleKeydown(event: KeyboardEvent): void;
Expand Down

0 comments on commit 43585f8

Please sign in to comment.