Skip to content

Commit

Permalink
fix(material/button-toggle): unable to tab into ngModel-based group o…
Browse files Browse the repository at this point in the history 
…n first render (angular#30103)

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 angular#30097.
  • Loading branch information
@crisbeto
crisbeto authored Dec 2, 2024
1 parent de6c491 commit f0a767c
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 45 deletions.
73 changes: 46 additions & 27 deletions src/material/button-toggle/button-toggle.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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;
Expand All @@ -360,34 +370,28 @@ describe('MatButtonToggle without forms', () => {

buttonToggleNativeElements = buttonToggleDebugElements.map(debugEl => debugEl.nativeElement);

buttonToggleLabelElements = fixture.debugElement
innerButtons = fixture.debugElement
.queryAll(By.css('button'))
.map(debugEl => debugEl.nativeElement);

buttonToggleInstances = buttonToggleDebugElements.map(debugEl => debugEl.componentInstance);
});

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);
}
});
Expand All @@ -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);
Expand Down Expand Up @@ -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');
Expand All @@ -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');
Expand All @@ -474,15 +478,15 @@ 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');
expect(groupInstance.selected).toBe(buttonToggleInstances[0]);
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');
Expand All @@ -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);
Expand All @@ -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();

Expand All @@ -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);
Expand Down Expand Up @@ -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');
Expand All @@ -595,7 +599,7 @@ describe('MatButtonToggle without forms', () => {
}));

it('should show checkmark indicator by default', () => {
buttonToggleLabelElements[0].click();
innerButtons[0].click();
fixture.detectChanges();

expect(
Expand Down Expand Up @@ -1310,3 +1314,18 @@ class ButtonToggleGroupWithFormControlAndDynamicButtons {
control = new FormControl('');
values = ['a', 'b', 'c'];
}

@Component({
template: `
<mat-button-toggle-group [(ngModel)]="value">
<mat-button-toggle value="1">One</mat-button-toggle>
<mat-button-toggle value="2">Two</mat-button-toggle>
<mat-button-toggle value="3">Three</mat-button-toggle>
</mat-button-toggle-group>
`,
standalone: true,
imports: [MatButtonToggleModule, FormsModule],
})
class ButtonToggleGroupWithNgModelAndStaticOptions {
value = '';
}
48 changes: 30 additions & 18 deletions src/material/button-toggle/button-toggle.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}
}

Expand All @@ -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;
}
}
}
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -668,14 +681,13 @@ export class MatButtonToggle implements OnInit, AfterViewInit, OnDestroy {
constructor() {
inject(_CdkPrivateStyleLoader).load(_StructuralStylesLoader);
const toggleGroup = inject<MatButtonToggleGroup>(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<MatButtonToggleDefaultOptions>(
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';
Expand Down

0 comments on commit f0a767c

Please sign in to comment.