Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(material/dialog): autofocus should wait for rendering to complete #28330

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 22 additions & 9 deletions src/cdk/dialog/dialog-container.ts
Expand Up @@ -30,11 +30,14 @@ import {
ElementRef,
EmbeddedViewRef,
Inject,
Injector,
NgZone,
OnDestroy,
Optional,
ViewChild,
ViewEncapsulation,
afterNextRender,
inject,
} from '@angular/core';
import {DialogConfig} from './dialog-config';

Expand Down Expand Up @@ -96,6 +99,7 @@ export class CdkDialogContainer<C extends DialogConfig = DialogConfig>
* the rest are present.
*/
_ariaLabelledByQueue: string[] = [];
protected readonly _injector = inject(Injector);

constructor(
protected _elementRef: ElementRef,
Expand Down Expand Up @@ -127,7 +131,12 @@ export class CdkDialogContainer<C extends DialogConfig = DialogConfig>
* capturing behavior (e.g. if it's tied to an animation).
*/
protected _captureInitialFocus() {
this._trapFocus();
afterNextRender(
() => {
this._trapFocus();
},
{injector: this._injector},
);
}

ngOnDestroy() {
Expand Down Expand Up @@ -247,13 +256,12 @@ export class CdkDialogContainer<C extends DialogConfig = DialogConfig>
break;
case true:
case 'first-tabbable':
this._focusTrap.focusInitialElementWhenReady().then(focusedSuccessfully => {
// If we weren't able to find a focusable element in the dialog, then focus the dialog
// container instead.
if (!focusedSuccessfully) {
this._focusDialogContainer();
}
});
const focusedSuccessfully = this._focusTrap.focusInitialElement();
// If we weren't able to find a focusable element in the dialog, then focus the dialog
// container instead.
if (!focusedSuccessfully) {
this._focusDialogContainer();
}
break;
case 'first-heading':
this._focusByCssSelector('h1, h2, h3, h4, h5, h6, [role="heading"]');
Expand Down Expand Up @@ -342,7 +350,12 @@ export class CdkDialogContainer<C extends DialogConfig = DialogConfig>
// Recapture it if closing via the backdrop is disabled.
this._overlayRef.backdropClick().subscribe(() => {
if (this._config.disableClose) {
this._recaptureFocus();
afterNextRender(
() => {
this._recaptureFocus();
},
{injector: this._injector},
);
}
});
}
Expand Down
17 changes: 15 additions & 2 deletions src/material/dialog/dialog-container.ts
Expand Up @@ -16,10 +16,13 @@ import {
ElementRef,
EventEmitter,
Inject,
Injector,
NgZone,
OnDestroy,
Optional,
ViewEncapsulation,
afterNextRender,
inject,
} from '@angular/core';
import {MatDialogConfig} from './dialog-config';
import {ANIMATION_MODULE_TYPE} from '@angular/platform-browser/animations';
Expand Down Expand Up @@ -148,10 +151,20 @@ export class MatDialogContainer extends CdkDialogContainer<MatDialogConfig> impl
} else {
this._hostElement.classList.add(OPEN_CLASS);
// Note: We could immediately finish the dialog opening here with noop animations,
// but we defer until next tick so that consumers can subscribe to `afterOpened`.
// but we defer until after render so that consumers can subscribe to `afterOpened`.
// Executing this immediately would mean that `afterOpened` emits synchronously
// on `dialog.open` before the consumer had a change to subscribe to `afterOpened`.
Promise.resolve().then(() => this._finishDialogOpen());
//
// Waiting until afterNextRender is required because we need the dialog content to be
// rendered before attempting to set focus.
//
// We use `queueMicrotask` in `afterNextRender` because at the moment, `afterNextRender`
// runs after every ChangeDetectorRef.detectChanges, which would still trigger before a
// dialog component gets rendered. Instead, `afterNextRender` needs to run at the end of
// `ApplicationRef.tick` (https://github.com/angular/angular/issues/53232).
afterNextRender(() => queueMicrotask(() => this._finishDialogOpen()), {
injector: this._injector,
});
}
}

Expand Down
33 changes: 23 additions & 10 deletions src/material/dialog/dialog.spec.ts
Expand Up @@ -30,6 +30,7 @@ import {
ViewContainerRef,
ViewEncapsulation,
forwardRef,
ApplicationRef,
} from '@angular/core';
import {
ComponentFixture,
Expand Down Expand Up @@ -1090,15 +1091,25 @@ describe('MDC-based MatDialog', () => {
it(
'should recapture focus to the first element that matches the css selector when ' +
'clicking on the backdrop with autoFocus set to a css selector',
fakeAsync(() => {
dialog.open(ContentElementDialog, {
disableClose: true,
viewContainerRef: testViewContainerRef,
autoFocus: 'button',
async () => {
viewContainerFixture.autoDetectChanges();
TestBed.inject(NgZone).run(() => {
// Queueing a microtask prevents synchronous change detection after zone.run
// This test wants to verify that the focus waits for change detection.
// Causing a synchronous change detection does not effectively test the
// async wait code. This behavior is more similar to what happens in
// an application where a dialog is opened after async work or in response
// to an interaction.
queueMicrotask(() => {
dialog.open(ContentElementDialog, {
disableClose: true,
viewContainerRef: testViewContainerRef,
autoFocus: 'button',
});
});
});

viewContainerFixture.detectChanges();
flushMicrotasks();
await viewContainerFixture.whenStable();

let backdrop = overlayContainerElement.querySelector('.cdk-overlay-backdrop') as HTMLElement;
let firstButton = overlayContainerElement.querySelector(
Expand All @@ -1111,13 +1122,12 @@ describe('MDC-based MatDialog', () => {

firstButton.blur(); // Programmatic clicks might not move focus so we simulate it.
backdrop.click();
viewContainerFixture.detectChanges();
flush();
await viewContainerFixture.whenStable();

expect(document.activeElement)
.withContext('Expected first button to stay focused after click')
.toBe(firstButton);
}),
},
);

describe('hasBackdrop option', () => {
Expand Down Expand Up @@ -2193,6 +2203,8 @@ class PizzaMsg {
<h1 mat-dialog-title>This is the third title</h1>
}
<mat-dialog-content>Lorem ipsum dolor sit amet.</mat-dialog-content>
<!-- Added control flow to render during update pass instead of on create -->
@if (true) {
<mat-dialog-actions align="end">
<button mat-dialog-close>Close</button>
<button class="close-with-true" [mat-dialog-close]="true">Close and return true</button>
Expand All @@ -2203,6 +2215,7 @@ class PizzaMsg {
<div mat-dialog-close>Should not close</div>
<button class="with-submit" type="submit" mat-dialog-close>Should have submit</button>
</mat-dialog-actions>
}
`,
standalone: true,
imports: [MatDialogTitle, MatDialogContent, MatDialogActions, MatDialogClose],
Expand Down