Skip to content

Commit

Permalink
fix(material/dialog): mat-dialog-title should work under OnPush `…
Browse files Browse the repository at this point in the history
…viewContainerRef`

The `mat-dialog-title` directive updates state in a microtask and should
call `ChangeDetectorRef.markForCheck`. Failing to do this will cause the
component tree to not be checked if it lives under an `OnPush` component
that has not otherwise been marked for check. This commit updates the
queue to be a signal so we don't have to think about calling
`markForCheck` at the right times.
  • Loading branch information
atscott committed Dec 26, 2023
1 parent a8b8e62 commit 1180bdc
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 7 deletions.
7 changes: 4 additions & 3 deletions src/cdk/dialog/dialog-container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
Optional,
ViewChild,
ViewEncapsulation,
signal,
} from '@angular/core';
import {DialogConfig} from './dialog-config';

Expand Down Expand Up @@ -62,7 +63,7 @@ export function throwDialogContentAlreadyAttachedError() {
'[attr.id]': '_config.id || null',
'[attr.role]': '_config.role',
'[attr.aria-modal]': '_config.ariaModal',
'[attr.aria-labelledby]': '_config.ariaLabel ? null : _ariaLabelledByQueue[0]',
'[attr.aria-labelledby]': '_config.ariaLabel ? null : _ariaLabelledByQueue()[0]',
'[attr.aria-label]': '_config.ariaLabel',
'[attr.aria-describedby]': '_config.ariaDescribedBy || null',
},
Expand Down Expand Up @@ -95,7 +96,7 @@ export class CdkDialogContainer<C extends DialogConfig = DialogConfig>
* where there are two or more titles in the DOM at a time and the first one is destroyed while
* the rest are present.
*/
_ariaLabelledByQueue: string[] = [];
_ariaLabelledByQueue = signal([] as string[]);

constructor(
protected _elementRef: ElementRef,
Expand All @@ -112,7 +113,7 @@ export class CdkDialogContainer<C extends DialogConfig = DialogConfig>
this._document = _document;

if (this._config.ariaLabelledBy) {
this._ariaLabelledByQueue.push(this._config.ariaLabelledBy);
this._ariaLabelledByQueue.update(queue => [...queue, this._config.ariaLabelledBy!]);
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/material/dialog/dialog-container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {OverlayRef} from '@angular/cdk/overlay';
import {DOCUMENT} from '@angular/common';
import {
ChangeDetectionStrategy,
ChangeDetectorRef,
Component,
ComponentRef,
ElementRef,
Expand All @@ -20,6 +21,7 @@ import {
OnDestroy,
Optional,
ViewEncapsulation,
inject,
} from '@angular/core';
import {MatDialogConfig} from './dialog-config';
import {ANIMATION_MODULE_TYPE} from '@angular/platform-browser/animations';
Expand Down Expand Up @@ -64,7 +66,7 @@ export const CLOSE_ANIMATION_DURATION = 75;
'[attr.aria-modal]': '_config.ariaModal',
'[id]': '_config.id',
'[attr.role]': '_config.role',
'[attr.aria-labelledby]': '_config.ariaLabel ? null : _ariaLabelledByQueue[0]',
'[attr.aria-labelledby]': '_config.ariaLabel ? null : _ariaLabelledByQueue()[0]',
'[attr.aria-label]': '_config.ariaLabel',
'[attr.aria-describedby]': '_config.ariaDescribedBy || null',
'[class._mat-animation-noopable]': '!_animationsEnabled',
Expand Down
10 changes: 7 additions & 3 deletions src/material/dialog/dialog-content-directives.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
OnInit,
Optional,
SimpleChanges,
untracked,
} from '@angular/core';

import {MatDialog} from './dialog';
Expand Down Expand Up @@ -120,7 +121,10 @@ export class MatDialogTitle implements OnInit, OnDestroy {
Promise.resolve().then(() => {
// Note: we null check the queue, because there are some internal
// tests that are mocking out `MatDialogRef` incorrectly.
this._dialogRef._containerInstance?._ariaLabelledByQueue?.push(this.id);
this._dialogRef._containerInstance?._ariaLabelledByQueue.update(queue => [
...queue,
this.id,
]);
});
}
}
Expand All @@ -132,10 +136,10 @@ export class MatDialogTitle implements OnInit, OnDestroy {

if (queue) {
Promise.resolve().then(() => {
const index = queue.indexOf(this.id);
const index = untracked(queue).indexOf(this.id);

if (index > -1) {
queue.splice(index, 1);
queue.update(value => [...value.slice(0, index), ...value.slice(index + 1)]);
}
});
}
Expand Down
59 changes: 59 additions & 0 deletions src/material/dialog/dialog.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
ViewContainerRef,
ViewEncapsulation,
forwardRef,
signal,
} from '@angular/core';
import {
ComponentFixture,
Expand Down Expand Up @@ -1623,6 +1624,64 @@ describe('MDC-based MatDialog', () => {
runContentElementTests();
});

it('should set the aria-labelledby attribute to the id of the title under OnPush host', fakeAsync(() => {
@Component({
standalone: true,
imports: [MatDialogTitle],
template: `@if (showTitle()) { <h1 mat-dialog-title>This is the first title</h1> }`,
})
class DialogCmp {
showTitle = signal(true);
}

@Component({
template: '',
selector: 'child',
standalone: true,
})
class Child {
dialogRef?: MatDialogRef<DialogCmp>;

constructor(
readonly viewContainerRef: ViewContainerRef,
readonly dialog: MatDialog,
) {}

open() {
this.dialogRef = this.dialog.open(DialogCmp, {viewContainerRef: this.viewContainerRef});
}
}

@Component({
standalone: true,
imports: [Child],
template: `<child></child>`,
changeDetection: ChangeDetectionStrategy.OnPush,
})
class OnPushHost {
@ViewChild(Child, {static: true}) child: Child;
}

const hostFixture = TestBed.createComponent(OnPushHost);
hostFixture.componentInstance.child.open();
hostFixture.autoDetectChanges();
flush();

const overlayContainer = TestBed.inject(OverlayContainer);
const title = overlayContainer.getContainerElement().querySelector('[mat-dialog-title]')!;
const container = overlayContainerElement.querySelector('mat-dialog-container')!;

expect(title.id).withContext('Expected title element to have an id.').toBeTruthy();
expect(container.getAttribute('aria-labelledby'))
.withContext('Expected the aria-labelledby to match the title id.')
.toBe(title.id);

hostFixture.componentInstance.child.dialogRef?.componentInstance.showTitle.set(false);
hostFixture.detectChanges();
flush();
expect(container.getAttribute('aria-labelledby')).toBe(null);
}));

function runContentElementTests() {
it('should close the dialog when clicking on the close button', fakeAsync(() => {
expect(overlayContainerElement.querySelectorAll('.mat-mdc-dialog-container').length).toBe(
Expand Down

0 comments on commit 1180bdc

Please sign in to comment.