Skip to content

Commit

Permalink
fix(material/core): remove tabindex from mat-option (#26917)
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 #26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.

Unblocks #26861.

(cherry picked from commit 97410fa)
  • Loading branch information
zarend committed Apr 14, 2023
1 parent 01d920f commit 9147660
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 10 deletions.
4 changes: 3 additions & 1 deletion src/material/core/option/option.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,9 @@ export class _MatOptionBase<T = any> implements FocusableOption, AfterViewChecke
}

/** Returns the correct tabindex for the option depending on disabled state. */
// This method is only used by `MatLegacyOption`. Keeping it here to avoid breaking the types.
// That's because `MatLegacyOption` use `MatOption` type in a few places such as
// `MatOptionSelectionChange`. It is safe to delete this when `MatLegacyOption` is deleted.
_getTabIndex(): string {
return this.disabled ? '-1' : '0';
}
Expand Down Expand Up @@ -251,7 +254,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
28 changes: 19 additions & 9 deletions src/material/select/select.spec.ts
Original file line number Diff line number Diff line change
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

0 comments on commit 9147660

Please sign in to comment.