From 93b4c79a1073464d1c7f99bd2c23ad3e12b6df2c Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 29 Nov 2024 12:42:17 +0100 Subject: [PATCH] fix(material/button-toggle): unable to tab into ngModel-based group on first render Fixes an issue where the button toggle group would reset all the buttons back to `tabindex="-1"` if they're all static (e.g. not in a loop) and none of them match the value. Fixes #30097. --- .../button-toggle/button-toggle.spec.ts | 73 ++++++++++++------- src/material/button-toggle/button-toggle.ts | 48 +++++++----- 2 files changed, 76 insertions(+), 45 deletions(-) diff --git a/src/material/button-toggle/button-toggle.spec.ts b/src/material/button-toggle/button-toggle.spec.ts index 90c3776028cd..f223feaaa33d 100644 --- a/src/material/button-toggle/button-toggle.spec.ts +++ b/src/material/button-toggle/button-toggle.spec.ts @@ -96,8 +96,6 @@ describe('MatButtonToggle with forms', () => { innerButtons = buttonToggleDebugElements.map( debugEl => debugEl.query(By.css('button'))!.nativeElement, ); - - fixture.detectChanges(); }); it('should update the model before firing change event', fakeAsync(() => { @@ -312,6 +310,18 @@ describe('MatButtonToggle with forms', () => { expect(instance.toggles.map(t => t.checked)).toEqual([true, false, false]); }); + + it('should set the initial tabindex when using ngModel with a static list of options where none match the value', fakeAsync(() => { + const fixture = TestBed.createComponent(ButtonToggleGroupWithNgModelAndStaticOptions); + fixture.detectChanges(); + tick(); + const indexes = Array.from( + fixture.nativeElement.querySelectorAll('button'), + (button: HTMLElement) => button.getAttribute('tabindex'), + ); + + expect(indexes).toEqual(['0', '-1', '-1']); + })); }); describe('MatButtonToggle without forms', () => { @@ -341,7 +351,7 @@ describe('MatButtonToggle without forms', () => { let groupNativeElement: HTMLElement; let buttonToggleDebugElements: DebugElement[]; let buttonToggleNativeElements: HTMLElement[]; - let buttonToggleLabelElements: HTMLLabelElement[]; + let innerButtons: HTMLLabelElement[]; let groupInstance: MatButtonToggleGroup; let buttonToggleInstances: MatButtonToggle[]; let testComponent: ButtonTogglesInsideButtonToggleGroup; @@ -360,7 +370,7 @@ describe('MatButtonToggle without forms', () => { buttonToggleNativeElements = buttonToggleDebugElements.map(debugEl => debugEl.nativeElement); - buttonToggleLabelElements = fixture.debugElement + innerButtons = fixture.debugElement .queryAll(By.css('button')) .map(debugEl => debugEl.nativeElement); @@ -368,26 +378,20 @@ describe('MatButtonToggle without forms', () => { }); it('should initialize the tab index correctly', () => { - buttonToggleLabelElements.forEach((buttonToggle, index) => { - if (index === 0) { - expect(buttonToggle.getAttribute('tabindex')).toBe('0'); - } else { - expect(buttonToggle.getAttribute('tabindex')).toBe('-1'); - } - }); + expect(innerButtons.map(b => b.getAttribute('tabindex'))).toEqual(['0', '-1', '-1']); }); it('should update the tab index correctly', () => { - buttonToggleLabelElements[1].click(); + innerButtons[1].click(); fixture.detectChanges(); - expect(buttonToggleLabelElements[0].getAttribute('tabindex')).toBe('-1'); - expect(buttonToggleLabelElements[1].getAttribute('tabindex')).toBe('0'); + expect(innerButtons[0].getAttribute('tabindex')).toBe('-1'); + expect(innerButtons[1].getAttribute('tabindex')).toBe('0'); }); it('should set individual button toggle names based on the group name', () => { expect(groupInstance.name).toBeTruthy(); - for (let buttonToggle of buttonToggleLabelElements) { + for (let buttonToggle of innerButtons) { expect(buttonToggle.getAttribute('name')).toBe(groupInstance.name); } }); @@ -407,7 +411,7 @@ describe('MatButtonToggle without forms', () => { expect(buttonToggleInstances[0].disabled).toBe(false); - buttonToggleLabelElements[0].click(); + innerButtons[0].click(); fixture.detectChanges(); expect(buttonToggleInstances[0].checked).toBe(true); @@ -456,7 +460,7 @@ describe('MatButtonToggle without forms', () => { it('should update the group value when one of the toggles changes', () => { expect(groupInstance.value).toBeFalsy(); - buttonToggleLabelElements[0].click(); + innerButtons[0].click(); fixture.detectChanges(); expect(groupInstance.value).toBe('test1'); @@ -465,7 +469,7 @@ describe('MatButtonToggle without forms', () => { it('should propagate the value change back up via a two-way binding', () => { expect(groupInstance.value).toBeFalsy(); - buttonToggleLabelElements[0].click(); + innerButtons[0].click(); fixture.detectChanges(); expect(groupInstance.value).toBe('test1'); @@ -474,7 +478,7 @@ describe('MatButtonToggle without forms', () => { it('should update the group and toggles when one of the button toggles is clicked', () => { expect(groupInstance.value).toBeFalsy(); - buttonToggleLabelElements[0].click(); + innerButtons[0].click(); fixture.detectChanges(); expect(groupInstance.value).toBe('test1'); @@ -482,7 +486,7 @@ describe('MatButtonToggle without forms', () => { expect(buttonToggleInstances[0].checked).toBe(true); expect(buttonToggleInstances[1].checked).toBe(false); - buttonToggleLabelElements[1].click(); + innerButtons[1].click(); fixture.detectChanges(); expect(groupInstance.value).toBe('test2'); @@ -492,7 +496,7 @@ describe('MatButtonToggle without forms', () => { }); it('should check a button toggle upon interaction with underlying native radio button', () => { - buttonToggleLabelElements[0].click(); + innerButtons[0].click(); fixture.detectChanges(); expect(buttonToggleInstances[0].checked).toBe(true); @@ -515,12 +519,12 @@ describe('MatButtonToggle without forms', () => { const changeSpy = jasmine.createSpy('button-toggle change listener'); buttonToggleInstances[0].change.subscribe(changeSpy); - buttonToggleLabelElements[0].click(); + innerButtons[0].click(); fixture.detectChanges(); tick(); expect(changeSpy).toHaveBeenCalledTimes(1); - buttonToggleLabelElements[0].click(); + innerButtons[0].click(); fixture.detectChanges(); tick(); @@ -534,12 +538,12 @@ describe('MatButtonToggle without forms', () => { const changeSpy = jasmine.createSpy('button-toggle-group change listener'); groupInstance.change.subscribe(changeSpy); - buttonToggleLabelElements[0].click(); + innerButtons[0].click(); fixture.detectChanges(); tick(); expect(changeSpy).toHaveBeenCalled(); - buttonToggleLabelElements[1].click(); + innerButtons[1].click(); fixture.detectChanges(); tick(); expect(changeSpy).toHaveBeenCalledTimes(2); @@ -579,7 +583,7 @@ describe('MatButtonToggle without forms', () => { it('should update the model if a selected toggle is removed', fakeAsync(() => { expect(groupInstance.value).toBeFalsy(); - buttonToggleLabelElements[0].click(); + innerButtons[0].click(); fixture.detectChanges(); expect(groupInstance.value).toBe('test1'); @@ -595,7 +599,7 @@ describe('MatButtonToggle without forms', () => { })); it('should show checkmark indicator by default', () => { - buttonToggleLabelElements[0].click(); + innerButtons[0].click(); fixture.detectChanges(); expect( @@ -1310,3 +1314,18 @@ class ButtonToggleGroupWithFormControlAndDynamicButtons { control = new FormControl(''); values = ['a', 'b', 'c']; } + +@Component({ + template: ` + + One + Two + Three + + `, + standalone: true, + imports: [MatButtonToggleModule, FormsModule], +}) +class ButtonToggleGroupWithNgModelAndStaticOptions { + value = ''; +} diff --git a/src/material/button-toggle/button-toggle.ts b/src/material/button-toggle/button-toggle.ts index 6f3b618dd5f4..2838742faa19 100644 --- a/src/material/button-toggle/button-toggle.ts +++ b/src/material/button-toggle/button-toggle.ts @@ -473,16 +473,28 @@ export class MatButtonToggleGroup implements ControlValueAccessor, OnInit, After return; } + const toggles = this._buttonToggles.toArray(); + if (this.multiple && value) { if (!Array.isArray(value) && (typeof ngDevMode === 'undefined' || ngDevMode)) { throw Error('Value must be an array in multiple-selection mode.'); } this._clearSelection(); - value.forEach((currentValue: any) => this._selectValue(currentValue)); + value.forEach((currentValue: any) => this._selectValue(currentValue, toggles)); } else { this._clearSelection(); - this._selectValue(value); + this._selectValue(value, toggles); + } + + // In single selection mode we need at least one enabled toggle to always be focusable. + if (!this.multiple && toggles.every(toggle => toggle.tabIndex === -1)) { + for (const toggle of toggles) { + if (!toggle.disabled) { + toggle.tabIndex = 0; + break; + } + } } } @@ -499,17 +511,16 @@ export class MatButtonToggleGroup implements ControlValueAccessor, OnInit, After } /** Selects a value if there's a toggle that corresponds to it. */ - private _selectValue(value: any) { - const correspondingOption = this._buttonToggles.find(toggle => { - return toggle.value != null && toggle.value === value; - }); - - if (correspondingOption) { - correspondingOption.checked = true; - this._selectionModel.select(correspondingOption); - if (!this.multiple) { - // If the button toggle is in single select mode, reset the tabIndex. - correspondingOption.tabIndex = 0; + private _selectValue(value: any, toggles: MatButtonToggle[]) { + for (const toggle of toggles) { + if (toggle.value != null && toggle.value === value) { + toggle.checked = true; + this._selectionModel.select(toggle); + if (!this.multiple) { + // If the button toggle is in single select mode, reset the tabIndex. + toggle.tabIndex = 0; + } + break; } } } @@ -601,8 +612,10 @@ export class MatButtonToggle implements OnInit, AfterViewInit, OnDestroy { return this._tabIndex; } set tabIndex(value: number | null) { - this._tabIndex = value; - this._markForCheck(); + if (value !== this._tabIndex) { + this._tabIndex = value; + this._markForCheck(); + } } private _tabIndex: number | null; @@ -668,14 +681,13 @@ export class MatButtonToggle implements OnInit, AfterViewInit, OnDestroy { constructor() { inject(_CdkPrivateStyleLoader).load(_StructuralStylesLoader); const toggleGroup = inject(MAT_BUTTON_TOGGLE_GROUP, {optional: true})!; - const defaultTabIndex = inject(new HostAttributeToken('tabindex'), {optional: true}); + const defaultTabIndex = inject(new HostAttributeToken('tabindex'), {optional: true}) || ''; const defaultOptions = inject( MAT_BUTTON_TOGGLE_DEFAULT_OPTIONS, {optional: true}, ); - const parsedTabIndex = Number(defaultTabIndex); - this.tabIndex = parsedTabIndex || parsedTabIndex === 0 ? parsedTabIndex : null; + this._tabIndex = parseInt(defaultTabIndex) || 0; this.buttonToggleGroup = toggleGroup; this.appearance = defaultOptions && defaultOptions.appearance ? defaultOptions.appearance : 'standard';