From c1d9323afff5d05e3eaeb2386ecdf2611360931b Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Sun, 27 Mar 2022 14:36:41 +0200 Subject: [PATCH] fix(material/radio): set tabindex based on selected state Currently all radio buttons inside a radio group have a `tabindex` of 0, unless they're disabled, and we leave it up to the browser to focus the proper button based on its selected state when the user is tabbing. This works for the most part, but it breaks down with something like our focus trap which determines which element to focus based on its `tabindex` since all buttons have the same `tabindex`. These changes fix the issue by setting a `tabindex` of 0 only on the selected radio button. Fixes #17876. --- .../mdc-radio/radio.html | 1 - .../mdc-radio/radio.spec.ts | 49 +++++++++++++++++++ src/material/radio/radio.html | 1 - src/material/radio/radio.spec.ts | 49 +++++++++++++++++++ src/material/radio/radio.ts | 38 +++++++++++++- tools/public_api_guard/material/radio.md | 5 +- 6 files changed, 139 insertions(+), 4 deletions(-) diff --git a/src/material-experimental/mdc-radio/radio.html b/src/material-experimental/mdc-radio/radio.html index e81ed6778d94..f6f3b116600a 100644 --- a/src/material-experimental/mdc-radio/radio.html +++ b/src/material-experimental/mdc-radio/radio.html @@ -7,7 +7,6 @@ [id]="inputId" [checked]="checked" [disabled]="disabled" - [tabIndex]="tabIndex" [attr.name]="name" [attr.value]="value" [required]="required" diff --git a/src/material-experimental/mdc-radio/radio.spec.ts b/src/material-experimental/mdc-radio/radio.spec.ts index 26f578c2a553..39c40f84d35b 100644 --- a/src/material-experimental/mdc-radio/radio.spec.ts +++ b/src/material-experimental/mdc-radio/radio.spec.ts @@ -26,6 +26,7 @@ describe('MDC-based MatRadio', () => { TranscludingWrapper, RadioButtonWithPredefinedTabindex, RadioButtonWithPredefinedAriaAttributes, + RadiosInsidePreCheckedRadioGroup, ], }); @@ -431,6 +432,43 @@ describe('MDC-based MatRadio', () => { ), ).toBe(true); }); + + it('should set the input tabindex based on the selected radio button', () => { + const getTabIndexes = () => { + return radioInputElements.map(element => parseInt(element.getAttribute('tabindex') || '')); + }; + + expect(getTabIndexes()).toEqual([0, 0, 0]); + + radioLabelElements[0].click(); + fixture.detectChanges(); + + expect(getTabIndexes()).toEqual([0, -1, -1]); + + radioLabelElements[1].click(); + fixture.detectChanges(); + + expect(getTabIndexes()).toEqual([-1, 0, -1]); + + radioLabelElements[2].click(); + fixture.detectChanges(); + + expect(getTabIndexes()).toEqual([-1, -1, 0]); + }); + + it('should set the input tabindex correctly with a pre-checked radio button', () => { + const precheckedFixture = TestBed.createComponent(RadiosInsidePreCheckedRadioGroup); + precheckedFixture.detectChanges(); + + const radios: NodeListOf = + precheckedFixture.nativeElement.querySelectorAll('mat-radio-button input'); + + expect( + Array.from(radios).map(radio => { + return radio.getAttribute('tabindex'); + }), + ).toEqual(['-1', '-1', '0']); + }); }); describe('group with ngModel', () => { @@ -960,6 +998,17 @@ class RadiosInsideRadioGroup { color: string | null; } +@Component({ + template: ` + + Charmander + Squirtle + Bulbasaur + + `, +}) +class RadiosInsidePreCheckedRadioGroup {} + @Component({ template: ` Spring diff --git a/src/material/radio/radio.html b/src/material/radio/radio.html index c497ead4a2fd..15e839e9e12a 100644 --- a/src/material/radio/radio.html +++ b/src/material/radio/radio.html @@ -9,7 +9,6 @@ [id]="inputId" [checked]="checked" [disabled]="disabled" - [tabIndex]="tabIndex" [attr.name]="name" [attr.value]="value" [required]="required" diff --git a/src/material/radio/radio.spec.ts b/src/material/radio/radio.spec.ts index 7a25723534ce..f46f24361f14 100644 --- a/src/material/radio/radio.spec.ts +++ b/src/material/radio/radio.spec.ts @@ -22,6 +22,7 @@ describe('MatRadio', () => { TranscludingWrapper, RadioButtonWithPredefinedTabindex, RadioButtonWithPredefinedAriaAttributes, + RadiosInsidePreCheckedRadioGroup, ], }); @@ -423,6 +424,43 @@ describe('MatRadio', () => { ), ).toBe(true); }); + + it('should set the input tabindex based on the selected radio button', () => { + const getTabIndexes = () => { + return radioInputElements.map(element => parseInt(element.getAttribute('tabindex') || '')); + }; + + expect(getTabIndexes()).toEqual([0, 0, 0]); + + radioLabelElements[0].click(); + fixture.detectChanges(); + + expect(getTabIndexes()).toEqual([0, -1, -1]); + + radioLabelElements[1].click(); + fixture.detectChanges(); + + expect(getTabIndexes()).toEqual([-1, 0, -1]); + + radioLabelElements[2].click(); + fixture.detectChanges(); + + expect(getTabIndexes()).toEqual([-1, -1, 0]); + }); + + it('should set the input tabindex correctly with a pre-checked radio button', () => { + const precheckedFixture = TestBed.createComponent(RadiosInsidePreCheckedRadioGroup); + precheckedFixture.detectChanges(); + + const radios: NodeListOf = + precheckedFixture.nativeElement.querySelectorAll('mat-radio-button input'); + + expect( + Array.from(radios).map(radio => { + return radio.getAttribute('tabindex'); + }), + ).toEqual(['-1', '-1', '0']); + }); }); describe('group with ngModel', () => { @@ -943,6 +981,17 @@ class RadiosInsideRadioGroup { color: string | null; } +@Component({ + template: ` + + Charmander + Squirtle + Bulbasaur + + `, +}) +class RadiosInsidePreCheckedRadioGroup {} + @Component({ template: ` Spring diff --git a/src/material/radio/radio.ts b/src/material/radio/radio.ts index 5a8b8e91b0ca..2052a915af07 100644 --- a/src/material/radio/radio.ts +++ b/src/material/radio/radio.ts @@ -18,6 +18,7 @@ import { Component, ContentChildren, Directive, + DoCheck, ElementRef, EventEmitter, forwardRef, @@ -361,7 +362,7 @@ const _MatRadioButtonMixinBase = mixinDisableRipple(mixinTabIndex(MatRadioButton @Directive() export abstract class _MatRadioButtonBase extends _MatRadioButtonMixinBase - implements OnInit, AfterViewInit, OnDestroy, CanDisableRipple, HasTabIndex + implements OnInit, AfterViewInit, DoCheck, OnDestroy, CanDisableRipple, HasTabIndex { private _uniqueId: string = `mat-radio-${++nextUniqueId}`; @@ -500,6 +501,9 @@ export abstract class _MatRadioButtonBase /** Unregister function for _radioDispatcher */ private _removeUniqueSelectionListener: () => void = () => {}; + /** Previous value of the input's tabindex. */ + private _previousTabIndex: number | undefined; + /** The native `` element */ @ViewChild('input') _inputElement: ElementRef; @@ -568,7 +572,12 @@ export abstract class _MatRadioButtonBase } } + ngDoCheck(): void { + this._updateTabIndex(); + } + ngAfterViewInit() { + this._updateTabIndex(); this._focusMonitor.monitor(this._elementRef, true).subscribe(focusOrigin => { if (!focusOrigin && this.radioGroup) { this.radioGroup._touch(); @@ -629,6 +638,33 @@ export abstract class _MatRadioButtonBase this._changeDetector.markForCheck(); } } + + /** Gets the tabindex for the underlying input element. */ + private _updateTabIndex() { + const group = this.radioGroup; + let value: number; + + // Implement a roving tabindex if the button is inside a group. For most cases this isn't + // necessary, because the browser handles the tab order for inputs inside a group automatically, + // but we need an explicitly higher tabindex for the selected button in order for things like + // the focus trap to pick it up correctly. + if (!group || !group.selected || this.disabled) { + value = this.tabIndex; + } else { + value = group.selected === this ? this.tabIndex : -1; + } + + if (value !== this._previousTabIndex) { + // We have to set the tabindex directly on the DOM node, because it depends on + // the selected state which is prone to "changed after checked errors". + const input: HTMLInputElement | undefined = this._inputElement?.nativeElement; + + if (input) { + input.setAttribute('tabindex', value + ''); + this._previousTabIndex = value; + } + } + } } /** diff --git a/tools/public_api_guard/material/radio.md b/tools/public_api_guard/material/radio.md index c727b1ae98af..57c7757cec77 100644 --- a/tools/public_api_guard/material/radio.md +++ b/tools/public_api_guard/material/radio.md @@ -12,6 +12,7 @@ import { CanDisableRipple } from '@angular/material/core'; import { ChangeDetectorRef } from '@angular/core'; import { _Constructor } from '@angular/material/core'; import { ControlValueAccessor } from '@angular/forms'; +import { DoCheck } from '@angular/core'; import { ElementRef } from '@angular/core'; import { EventEmitter } from '@angular/core'; import { FocusMonitor } from '@angular/cdk/a11y'; @@ -48,7 +49,7 @@ export class MatRadioButton extends _MatRadioButtonBase { } // @public -export abstract class _MatRadioButtonBase extends _MatRadioButtonMixinBase implements OnInit, AfterViewInit, OnDestroy, CanDisableRipple, HasTabIndex { +export abstract class _MatRadioButtonBase extends _MatRadioButtonMixinBase implements OnInit, AfterViewInit, DoCheck, OnDestroy, CanDisableRipple, HasTabIndex { constructor(radioGroup: _MatRadioGroupBase<_MatRadioButtonBase>, elementRef: ElementRef, _changeDetector: ChangeDetectorRef, _focusMonitor: FocusMonitor, _radioDispatcher: UniqueSelectionDispatcher, animationMode?: string, _providerOverride?: MatRadioDefaultOptions | undefined, tabIndex?: string); ariaDescribedby: string; ariaLabel: string; @@ -75,6 +76,8 @@ export abstract class _MatRadioButtonBase extends _MatRadioButtonMixinBase imple // (undocumented) ngAfterViewInit(): void; // (undocumented) + ngDoCheck(): void; + // (undocumented) ngOnDestroy(): void; // (undocumented) ngOnInit(): void;