From 0ecd962f2cc19d9eac3f96d19fd91ab501d71635 Mon Sep 17 00:00:00 2001 From: Zach Arend Date: Wed, 12 Apr 2023 20:15:28 +0000 Subject: [PATCH] fix(material/core): remove tabindex from mat-option 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. --- src/material/core/option/option.ts | 6 ---- src/material/legacy-core/option/option.ts | 5 ++++ src/material/legacy-select/select.ts | 2 +- src/material/select/select.spec.ts | 28 +++++++++++++------ tools/public_api_guard/material/core.md | 1 - .../public_api_guard/material/legacy-core.md | 1 + 6 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/material/core/option/option.ts b/src/material/core/option/option.ts index d9a455f9bc26..56238bba5a9a 100644 --- a/src/material/core/option/option.ts +++ b/src/material/core/option/option.ts @@ -204,11 +204,6 @@ export class _MatOptionBase 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; @@ -251,7 +246,6 @@ export class _MatOptionBase 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', diff --git a/src/material/legacy-core/option/option.ts b/src/material/legacy-core/option/option.ts index c2ab6d03f9f9..4efafe45bce3 100644 --- a/src/material/legacy-core/option/option.ts +++ b/src/material/legacy-core/option/option.ts @@ -59,4 +59,9 @@ export class MatLegacyOption extends _MatOptionBase { ) { super(element, changeDetectorRef, parent, group); } + + /** Returns the correct tabindex for the option depending on disabled state. */ + _getTabIndex(): string { + return this.disabled ? '-1' : '0'; + } } diff --git a/src/material/legacy-select/select.ts b/src/material/legacy-select/select.ts index a63fde7061f6..7e8eb7c86b79 100644 --- a/src/material/legacy-select/select.ts +++ b/src/material/legacy-select/select.ts @@ -489,7 +489,7 @@ export class MatLegacySelect extends _MatSelectBase 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, ); } diff --git a/src/material/select/select.spec.ts b/src/material/select/select.spec.ts index 9eb724ebe315..d47d8dda7ee9 100644 --- a/src/material/select/select.spec.ts +++ b/src/material/select/select.spec.ts @@ -824,17 +824,27 @@ describe('MDC-based MatSelect', () => { 'mat-option', ) as NodeListOf; - 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); }), ); @@ -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(() => { diff --git a/tools/public_api_guard/material/core.md b/tools/public_api_guard/material/core.md index d65db4fd6a82..da21c8c32b83 100644 --- a/tools/public_api_guard/material/core.md +++ b/tools/public_api_guard/material/core.md @@ -281,7 +281,6 @@ export class _MatOptionBase implements FocusableOption, AfterViewChecke focus(_origin?: FocusOrigin, options?: FocusOptions): void; _getHostElement(): HTMLElement; getLabel(): string; - _getTabIndex(): string; // (undocumented) readonly group: _MatOptgroupBase; _handleKeydown(event: KeyboardEvent): void; diff --git a/tools/public_api_guard/material/legacy-core.md b/tools/public_api_guard/material/legacy-core.md index 39fa98a39ce6..f21f09e3f0fb 100644 --- a/tools/public_api_guard/material/legacy-core.md +++ b/tools/public_api_guard/material/legacy-core.md @@ -173,6 +173,7 @@ export { _MatLegacyOptgroupBase } // @public @deprecated export class MatLegacyOption extends _MatLegacyOptionBase { constructor(element: ElementRef, changeDetectorRef: ChangeDetectorRef, parent: MatLegacyOptionParentComponent, group: MatLegacyOptgroup); + _getTabIndex(): string; // (undocumented) static ɵcmp: i0.ɵɵComponentDeclaration, "mat-option", ["matOption"], {}, {}, never, ["*"], false, never>; // (undocumented)