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 #26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.

Unblocks #26861.
  • Loading branch information
zarend committed Apr 12, 2023
1 parent df839e6 commit 0ecd962
Show file tree
Hide file tree
Showing 6 changed files with 26 additions and 17 deletions.
6 changes: 0 additions & 6 deletions src/material/core/option/option.ts
Original file line number Diff line number Diff line change
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
5 changes: 5 additions & 0 deletions src/material/legacy-core/option/option.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,9 @@ export class MatLegacyOption<T = any> extends _MatOptionBase<T> {
) {
super(element, changeDetectorRef, parent, group);
}

/** Returns the correct tabindex for the option depending on disabled state. */
_getTabIndex(): string {
return this.disabled ? '-1' : '0';
}
}
2 changes: 1 addition & 1 deletion src/material/legacy-select/select.ts
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ export class MatLegacySelect extends _MatSelectBase<MatLegacySelectChange> imple
selectedOptionOffset = 0;
} else {
selectedOptionOffset = Math.max(
this.options.toArray().indexOf(this._selectionModel.selected[0]),
this.options.toArray().indexOf(this._selectionModel.selected[0] as any),
0,
);
}
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
1 change: 0 additions & 1 deletion tools/public_api_guard/material/core.md
Original file line number Diff line number Diff line change
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
1 change: 1 addition & 0 deletions tools/public_api_guard/material/legacy-core.md
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ export { _MatLegacyOptgroupBase }
// @public @deprecated
export class MatLegacyOption<T = any> extends _MatLegacyOptionBase<T> {
constructor(element: ElementRef<HTMLElement>, changeDetectorRef: ChangeDetectorRef, parent: MatLegacyOptionParentComponent, group: MatLegacyOptgroup);
_getTabIndex(): string;
// (undocumented)
static ɵcmp: i0.ɵɵComponentDeclaration<MatLegacyOption<any>, "mat-option", ["matOption"], {}, {}, never, ["*"], false, never>;
// (undocumented)
Expand Down

0 comments on commit 0ecd962

Please sign in to comment.