Skip to content

Commit

Permalink
fix(cdk): FocusTrap has race condition problems (#9759)
Browse files Browse the repository at this point in the history
  • Loading branch information
nsbarsukov authored Nov 19, 2024
1 parent 8602e4e commit d84b437
Show file tree
Hide file tree
Showing 5 changed files with 142 additions and 6 deletions.
38 changes: 37 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 14 additions & 4 deletions projects/cdk/directives/focus-trap/focus-trap.directive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,32 @@ import {
selector: '[tuiFocusTrap]',
host: {
tabIndex: '0',
'(window:focusin.silent)': 'onFocusIn($event.target)',
'(window:focusin.silent)': 'initialized && onFocusIn($event.target)',
},
})
export class TuiFocusTrap implements OnDestroy {
private readonly doc = inject(DOCUMENT);
private readonly el = tuiInjectElement();
private readonly activeElement = tuiGetNativeFocused(this.doc);
private activeElement: Element | null = null;
protected initialized = false;

constructor() {
/**
* This would cause currently focused element to lose focus,
* but it might cause ExpressionChanged error due to potential HostBinding.
* Microtask keeps it in the same frame but allows change detection to run
*/
void Promise.resolve().then(() => this.el.focus());
Promise.resolve().then(() => {
/**
* The same event can synchronously close already opened focus trap and open another one.
* All focus traps have microtask inside its `ngOnDestroy` –
* they should be resolved before enabling of new focus trap.
* Don't enable any new event listeners before `initialized` equals to `true`!
*/
this.initialized = true;
this.activeElement = tuiGetNativeFocused(this.doc);
this.el.focus();
});
}

public ngOnDestroy(): void {
Expand All @@ -43,7 +54,6 @@ export class TuiFocusTrap implements OnDestroy {
* so we need to delay it but stay in the same sync cycle,
* therefore using Promise instead of setTimeout
*/
// eslint-disable-next-line
Promise.resolve().then(() => {
if (tuiIsHTMLElement(this.activeElement)) {
this.activeElement.focus();
Expand Down
3 changes: 2 additions & 1 deletion projects/demo-cypress/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"devDependencies": {
"@nx/cypress": "20.1.1",
"cypress": "13.15.2",
"cypress-image-diff-js": "2.3.0"
"cypress-image-diff-js": "2.3.0",
"cypress-plugin-tab": "1.0.5"
}
}
2 changes: 2 additions & 0 deletions projects/demo-cypress/src/support/component.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import 'cypress-plugin-tab';

import {NG_EVENT_PLUGINS} from '@taiga-ui/event-plugins';
import {mount} from 'cypress/angular';
import addCompareSnapshotCommand from 'cypress-image-diff-js/command';
Expand Down
87 changes: 87 additions & 0 deletions projects/demo-cypress/src/tests/focus-trap.cy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import {NgIf} from '@angular/common';
import {ChangeDetectionStrategy, Component} from '@angular/core';
import {TuiFocusTrap} from '@taiga-ui/cdk';

const TOUCHED = '_touched';

describe('FocusTrap', () => {
@Component({
standalone: true,
imports: [NgIf, TuiFocusTrap],
template: `
<button
id="increase"
(click)="increase()"
>
Increase counter
</button>
<div
*ngIf="i % 2 === 0"
tuiFocusTrap
class="even"
>
<input />
<button (click)="i = -1">Close</button>
</div>
<div
*ngIf="i % 2 === 1"
tuiFocusTrap
class="odd"
>
<input (focus)="markTouched($event.target)" />
<button (click)="i = -1">Close</button>
</div>
`,
changeDetection: ChangeDetectionStrategy.OnPush,
})
class Test {
protected i = -1;
protected increase = (): number => this.i++;
protected markTouched = (el: HTMLElement): void => el.classList.add(TOUCHED);
}

beforeEach(() => {
cy.mount(Test);
cy.get('#increase').focus().click();
});

it('move focus inside trap on mounting', () => {
cy.get('.even').should('be.visible').should('be.focused');
});

it('moving focus on another element inside focus trap works', () => {
cy.tab();
cy.get('.even input').first().should('be.focused');
cy.tab();
cy.get('.even button').should('be.focused');
});

it('focus cannot be moved outside active focus trap', () => {
cy.tab();
cy.get('.even input').first().should('be.focused');
cy.tab();
cy.get('.even button').should('be.focused');
cy.tab();
cy.get('.even input').first().should('be.focused');
cy.tab();
cy.get('.even button').should('be.focused');
cy.tab();
cy.get('.even input').first().should('be.focused');
});

it('focus should be returned to the element (focused before opening trap) on closing focus trap', () => {
cy.get('.even button').click();
cy.get('.even').should('not.exist');
cy.get('#increase').should('be.focused');
});

it('synchronous closing already opened focus trap and opening another one dont cause race condition', () => {
cy.get('#increase').click();
cy.get('.odd').should('be.visible').should('be.focused');
cy.get('.odd input').should('not.be.focused').should('not.have.class', TOUCHED);
cy.get('.odd button').click();
cy.get('#increase').should('be.focused');
});
});

0 comments on commit d84b437

Please sign in to comment.