Skip to content

Commit

Permalink
fix(material/chips): focus escape not working consistently
Browse files Browse the repository at this point in the history
Fixes that the chip grid was using change detection to allow focus to escape on tab presses. That's unreliable, because change detection might not be executed immediately. These changes fix the issue by changing the DOM node directly.

(cherry picked from commit 8e17112)
  • Loading branch information
crisbeto committed Sep 30, 2024
1 parent 9280ad3 commit 0fabf52
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 19 deletions.
12 changes: 6 additions & 6 deletions src/material/chips/chip-grid.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -396,37 +396,37 @@ describe('MatChipGrid', () => {

dispatchKeyboardEvent(firstNativeChip, 'keydown', TAB);

expect(chipGridInstance.tabIndex)
expect(chipGridNativeElement.tabIndex)
.withContext('Expected tabIndex to be set to -1 temporarily.')
.toBe(-1);

flush();

expect(chipGridInstance.tabIndex)
expect(chipGridNativeElement.tabIndex)
.withContext('Expected tabIndex to be reset back to 0')
.toBe(0);
}));

it(`should use user defined tabIndex`, fakeAsync(() => {
it('should use user defined tabIndex', fakeAsync(() => {
chipGridInstance.tabIndex = 4;
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

expect(chipGridInstance.tabIndex)
expect(chipGridNativeElement.tabIndex)
.withContext('Expected tabIndex to be set to user defined value 4.')
.toBe(4);

let nativeChips = chipGridNativeElement.querySelectorAll('mat-chip-row');
let firstNativeChip = nativeChips[0] as HTMLElement;

dispatchKeyboardEvent(firstNativeChip, 'keydown', TAB);
expect(chipGridInstance.tabIndex)
expect(chipGridNativeElement.tabIndex)
.withContext('Expected tabIndex to be set to -1 temporarily.')
.toBe(-1);

flush();

expect(chipGridInstance.tabIndex)
expect(chipGridNativeElement.tabIndex)
.withContext('Expected tabIndex to be reset back to 4')
.toBe(4);
}));
Expand Down
10 changes: 5 additions & 5 deletions src/material/chips/chip-listbox.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -367,13 +367,13 @@ describe('MatChipListbox', () => {
it('should allow focus to escape when tabbing away', fakeAsync(() => {
dispatchKeyboardEvent(chipListboxNativeElement, 'keydown', TAB);

expect(chipListboxInstance.tabIndex)
expect(chipListboxNativeElement.tabIndex)
.withContext('Expected tabIndex to be set to -1 temporarily.')
.toBe(-1);

flush();

expect(chipListboxInstance.tabIndex)
expect(chipListboxNativeElement.tabIndex)
.withContext('Expected tabIndex to be reset back to 0')
.toBe(0);
}));
Expand All @@ -384,19 +384,19 @@ describe('MatChipListbox', () => {

fixture.detectChanges();

expect(chipListboxInstance.tabIndex)
expect(chipListboxNativeElement.tabIndex)
.withContext('Expected tabIndex to be set to user defined value 4.')
.toBe(4);

dispatchKeyboardEvent(chipListboxNativeElement, 'keydown', TAB);

expect(chipListboxInstance.tabIndex)
expect(chipListboxNativeElement.tabIndex)
.withContext('Expected tabIndex to be set to -1 temporarily.')
.toBe(-1);

flush();

expect(chipListboxInstance.tabIndex)
expect(chipListboxNativeElement.tabIndex)
.withContext('Expected tabIndex to be reset back to 4')
.toBe(4);
}));
Expand Down
16 changes: 8 additions & 8 deletions src/material/chips/chip-set.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,17 +191,17 @@ export class MatChipSet implements AfterViewInit, OnDestroy {
* it back to the first chip, creating a focus trap, if it user tries to tab away.
*/
protected _allowFocusEscape() {
if (this.tabIndex !== -1) {
const previousTabIndex = this.tabIndex;
this.tabIndex = -1;
this._changeDetectorRef.markForCheck();
const previous = this._elementRef.nativeElement.tabIndex;

if (previous !== -1) {
// Set the tabindex directly on the element, instead of going through
// the data binding, because we aren't guaranteed that change detection
// will run quickly enough to allow focus to escape.
this._elementRef.nativeElement.tabIndex = -1;

// Note that this needs to be a `setTimeout`, because a `Promise.resolve`
// doesn't allow enough time for the focus to escape.
setTimeout(() => {
this.tabIndex = previousTabIndex;
this._changeDetectorRef.markForCheck();
});
setTimeout(() => (this._elementRef.nativeElement.tabIndex = previous));
}
}

Expand Down

0 comments on commit 0fabf52

Please sign in to comment.