refactor(cdk/dialog): Remove use of focusInitialElementWhenReady

Author: mmalerba

src/cdk/dialog/dialog-container.ts

@@ -31,13 +31,16 @@ import {
   ElementRef,
   EmbeddedViewRef,
   Inject,
+  Injector,
   NgZone,
   OnDestroy,
   Optional,
   ViewChild,
   ViewEncapsulation,
+  afterNextRender,
   inject,
 } from '@angular/core';
+import {take} from 'rxjs/operators';
 import {DialogConfig} from './dialog-config';
 
 export function throwDialogContentAlreadyAttachedError() {
@@ -102,6 +105,8 @@ export class CdkDialogContainer<C extends DialogConfig = DialogConfig>
 
   protected readonly _changeDetectorRef = inject(ChangeDetectorRef);
 
+  private _injector = inject(Injector);
+
   constructor(
     protected _elementRef: ElementRef,
     protected _focusTrapFactory: FocusTrapFactory,
@@ -266,13 +271,33 @@ 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.
+        const doFocus = () => {
+          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();
           }
-        });
+        };
+
+        // TODO(mmalerba): Make this behave consistently across zonefull / zoneless.
+        if (!this._ngZone.isStable) {
+          // Subscribing `onStable` has slightly different behavior than `afterNextRender`.
+          // `afterNextRender` does not wait for state changes queued up in a Promise
+          // to avoid change after checked errors. In most cases we would consider this an
+          // acceptable behavior change, the dialog at least made its best effort to focus the
+          // first element. However, this is particularly problematic when combined with the
+          // current behavior of the mat-radio-group, which adjusts the tabindex of its child
+          // radios based on the selected value of the group. When the selected value is bound
+          // via `[(ngModel)]` it hits this "state change in a promise" edge-case and can wind up
+          // putting the focus on a radio button that is not supposed to be eligible to receive
+          // focus. For now, we side-step this whole sequence of events by continuing to use
+          // `onStable` in zonefull apps, but it should be noted that zoneless apps can still
+          // suffer from this issue.
+          this._ngZone.onStable.pipe(take(1)).subscribe(doFocus);
+        } else {
+          afterNextRender(doFocus, {injector: this._injector});
+        }
         break;
       case 'first-heading':
         this._focusByCssSelector('h1, h2, h3, h4, h5, h6, [role="heading"]');

src/cdk/dialog/dialog.spec.ts

@@ -1,37 +1,37 @@
+import {Directionality} from '@angular/cdk/bidi';
+import {A, ESCAPE} from '@angular/cdk/keycodes';
+import {Overlay, OverlayContainer, ScrollDispatcher} from '@angular/cdk/overlay';
+import {_supportsShadowDom} from '@angular/cdk/platform';
 import {
-  ComponentFixture,
-  fakeAsync,
-  flushMicrotasks,
-  TestBed,
-  tick,
-  flush,
-} from '@angular/core/testing';
+  createKeyboardEvent,
+  dispatchEvent,
+  dispatchKeyboardEvent,
+} from '@angular/cdk/testing/private';
+import {Location} from '@angular/common';
+import {SpyLocation} from '@angular/common/testing';
 import {
   ChangeDetectionStrategy,
   Component,
   ComponentRef,
   Directive,
-  inject,
   Inject,
   InjectionToken,
   Injector,
   TemplateRef,
   ViewChild,
   ViewContainerRef,
   ViewEncapsulation,
+  inject,
 } from '@angular/core';
-import {By} from '@angular/platform-browser';
-import {Location} from '@angular/common';
-import {SpyLocation} from '@angular/common/testing';
-import {Directionality} from '@angular/cdk/bidi';
-import {Overlay, OverlayContainer, ScrollDispatcher} from '@angular/cdk/overlay';
-import {A, ESCAPE} from '@angular/cdk/keycodes';
-import {_supportsShadowDom} from '@angular/cdk/platform';
 import {
-  dispatchKeyboardEvent,
-  createKeyboardEvent,
-  dispatchEvent,
-} from '@angular/cdk/testing/private';
+  ComponentFixture,
+  TestBed,
+  fakeAsync,
+  flush,
+  flushMicrotasks,
+  tick,
+} from '@angular/core/testing';
+import {By} from '@angular/platform-browser';
 import {Subject} from 'rxjs';
 import {DIALOG_DATA, Dialog, DialogModule, DialogRef} from './index';
 
@@ -282,17 +282,20 @@ describe('Dialog', () => {
     const spy = jasmine.createSpy('backdropClick spy');
     dialogRef.backdropClick.subscribe(spy);
     viewContainerFixture.detectChanges();
+    flush();
 
     const backdrop = overlayContainerElement.querySelector('.cdk-overlay-backdrop') as HTMLElement;
 
     backdrop.click();
     viewContainerFixture.detectChanges();
+    flush();
     expect(spy).toHaveBeenCalledTimes(1);
 
     // Additional clicks after the dialog has closed should not be emitted
     dialogRef.disableClose = false;
     backdrop.click();
     viewContainerFixture.detectChanges();
+    flush();
     expect(spy).toHaveBeenCalledTimes(1);
   }));
 
@@ -303,6 +306,7 @@ describe('Dialog', () => {
     dialogRef.keydownEvents.subscribe(spy);
 
     viewContainerFixture.detectChanges();
+    flush();
 
     let backdrop = overlayContainerElement.querySelector('.cdk-overlay-backdrop') as HTMLElement;
     let container = overlayContainerElement.querySelector('cdk-dialog-container') as HTMLElement;
@@ -757,9 +761,11 @@ describe('Dialog', () => {
       fakeAsync(() => {
         const templateInjectFixture = TestBed.createComponent(TemplateInjectorParentComponent);
         templateInjectFixture.detectChanges();
+        flush();
 
         dialog.open(templateInjectFixture.componentInstance.templateRef);
         templateInjectFixture.detectChanges();
+        flush();
 
         expect(templateInjectFixture.componentInstance.innerComponentValue).toBe(
           'hello from parent component',
@@ -840,7 +846,7 @@ describe('Dialog', () => {
       });
 
       viewContainerFixture.detectChanges();
-      flushMicrotasks();
+      flush();
 
       expect(document.activeElement!.tagName)
         .withContext('Expected first tabbable element (input) in the dialog to be focused.')
@@ -910,17 +916,15 @@ describe('Dialog', () => {
 
       let dialogRef = dialog.open(PizzaMsg, {viewContainerRef: testViewContainerRef});
 
-      flushMicrotasks();
       viewContainerFixture.detectChanges();
-      flushMicrotasks();
+      flush();
 
       expect(document.activeElement!.id).not.toBe(
         'dialog-trigger',
         'Expected the focus to change when dialog was opened.',
       );
 
       dialogRef.close();
-      flushMicrotasks();
       viewContainerFixture.detectChanges();
       flush();
 
@@ -939,18 +943,18 @@ describe('Dialog', () => {
       viewContainerFixture.destroy();
       const fixture = TestBed.createComponent(ShadowDomComponent);
       fixture.detectChanges();
+      flush();
       const button = fixture.debugElement.query(By.css('button'))!.nativeElement;
 
       button.focus();
 
       const dialogRef = dialog.open(PizzaMsg);
-      flushMicrotasks();
       fixture.detectChanges();
-      flushMicrotasks();
+      flush();
 
       const spy = spyOn(button, 'focus').and.callThrough();
       dialogRef.close();
-      flushMicrotasks();
+      flush();
       fixture.detectChanges();
       tick(500);
 
@@ -994,7 +998,7 @@ describe('Dialog', () => {
       dialog.open(DialogWithoutFocusableElements);
 
       viewContainerFixture.detectChanges();
-      flushMicrotasks();
+      flush();
 
       expect(document.activeElement!.tagName.toLowerCase())
         .withContext('Expected dialog container to be focused.')
@@ -1013,14 +1017,12 @@ describe('Dialog', () => {
         restoreFocus: false,
       });
 
-      flushMicrotasks();
       viewContainerFixture.detectChanges();
-      flushMicrotasks();
+      flush();
 
       expect(document.activeElement!.id).not.toBe('dialog-trigger');
 
       dialogRef.close();
-      flushMicrotasks();
       viewContainerFixture.detectChanges();
       flush();
 

src/material/bottom-sheet/bottom-sheet.spec.ts

@@ -4,9 +4,9 @@ import {OverlayContainer, ScrollStrategy} from '@angular/cdk/overlay';
 import {_supportsShadowDom} from '@angular/cdk/platform';
 import {ViewportRuler} from '@angular/cdk/scrolling';
 import {
-  dispatchKeyboardEvent,
   createKeyboardEvent,
   dispatchEvent,
+  dispatchKeyboardEvent,
 } from '@angular/cdk/testing/private';
 import {Location} from '@angular/common';
 import {SpyLocation} from '@angular/common/testing';
@@ -23,11 +23,11 @@ import {
 } from '@angular/core';
 import {
   ComponentFixture,
+  TestBed,
   fakeAsync,
   flush,
   flushMicrotasks,
   inject,
-  TestBed,
   tick,
 } from '@angular/core/testing';
 import {By} from '@angular/platform-browser';
@@ -640,7 +640,7 @@ describe('MatBottomSheet', () => {
         });
 
         viewContainerFixture.detectChanges();
-        flushMicrotasks();
+        flush();
 
         expect(document.activeElement!.tagName)
           .withContext('Expected first tabbable element (input) in the dialog to be focused.')

src/material/dialog/dialog-container.ts

@@ -94,6 +94,8 @@ export class MatDialogContainer extends CdkDialogContainer<MatDialogConfig> impl
   /** Current timer for dialog animations. */
   private _animationTimer: ReturnType<typeof setTimeout> | null = null;
 
+  private _isDestroyed = false;
+
   constructor(
     elementRef: ElementRef,
     focusTrapFactory: FocusTrapFactory,
@@ -262,6 +264,10 @@ export class MatDialogContainer extends CdkDialogContainer<MatDialogConfig> impl
    * be called by sub-classes that use different animation implementations.
    */
   protected _openAnimationDone(totalTime: number) {
+    if (this._isDestroyed) {
+      return;
+    }
+
     if (this._config.delayFocusTrap) {
       this._trapFocus();
     }
@@ -275,6 +281,8 @@ export class MatDialogContainer extends CdkDialogContainer<MatDialogConfig> impl
     if (this._animationTimer !== null) {
       clearTimeout(this._animationTimer);
     }
+
+    this._isDestroyed = true;
   }
 
   override attachComponentPortal<T>(portal: ComponentPortal<T>): ComponentRef<T> {