Skip to content

Commit

Permalink
fix(material/dialog): autofocus should wait for rendering to complete
Browse files Browse the repository at this point in the history
The current implementation of the dialog's `finishDialogOpen`, which
includes autofocus, only waits for a microtask before executing. This
only works in the tests because of the synchronous call to
`detectChanges` after opening the dialog, ensuring that change detection
runs before the microtask. In an application, this is not guaranteed
because `NgZone` will only run change detection when the microtask queue
empties, which includes the microtask created by the dialog container.

This commit updates the implementation to wait for the next render to
coplete, _then_ queues a microtask that will finish the open process and
perform autofocus.
  • Loading branch information
atscott committed Dec 26, 2023
1 parent a8b8e62 commit 97c0416
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 8 deletions.
18 changes: 16 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 @@ -89,6 +92,7 @@ export class MatDialogContainer extends CdkDialogContainer<MatDialogConfig> impl
: 0;
/** Current timer for dialog animations. */
private _animationTimer: ReturnType<typeof setTimeout> | null = null;
private readonly injector = inject(Injector);

constructor(
elementRef: ElementRef,
Expand Down Expand Up @@ -148,10 +152,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
24 changes: 18 additions & 6 deletions src/material/dialog/dialog.spec.ts
Expand Up @@ -1091,13 +1091,23 @@ describe('MDC-based MatDialog', () => {
'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',
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();

let backdrop = overlayContainerElement.querySelector('.cdk-overlay-backdrop') as HTMLElement;
Expand All @@ -1111,7 +1121,6 @@ describe('MDC-based MatDialog', () => {

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

expect(document.activeElement)
Expand Down Expand Up @@ -2193,6 +2202,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 +2214,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

0 comments on commit 97c0416

Please sign in to comment.