Skip to content

Add closePredicate option to dialog #25174

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Add closePredicate option to dialog #25174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a70315
feat(cdk/dialog): add closePredicate option
crisbeto Jun 28, 2022
98cf6b9
feat(material/dialog): add closePredicate option
crisbeto Jun 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/cdk/dialog/dialog-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@ export class DialogConfig<D = unknown, R = unknown, C extends BasePortalOutlet =
/** Whether the dialog closes with the escape key or pointer events outside the panel element. */
disableClose?: boolean = false;

/** Function used to determine whether the dialog is allowed to close. */
closePredicate?: <Result = unknown, Component = unknown>(
Copy link
Member Author

@crisbeto crisbeto Jun 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somewhat on the fence about this API, because technically you can do the same by combining existing APIs and being careful with your event handlers, but I decided to implement it since there's clearly demand for it in #14292 and it can be easy for users to mess up if they do it themselves.

There's an overlap with disableClose and initially my thought was to change disableClose to also accept a function, but the problem is that it only applies to attempts at closing the dialog using the backdrop or the escape key, not programmatic closes or by something like the matDialogClose directive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also iffy about this API.

  • It's typically an a11y issue to prevent dialogs from closing on escape/outside click; this might encourage more of that
  • The name could be misleading, because what we're really doing is something like disableCloseFromUserInteractionPredicate. That is, it doesn't prevent closing via API call . This is especially confusing with matDialogClose.

I know the current disableClose also has these issues- I'd be more inclined to rename that to simplify this (something like disableCloseOnInteraction or disableDefaultCloseActions)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea with the new API is that it blocks all the close calls, even the programmatic ones, whereas disableClose was only for the backdrop and escape keys. The primary use case for this is if you have a form and you want to prevent the user from accidentally closing it based on a certain condition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can be on board with this if we rename disableClose to avoid confusion between the two (deprecating the current name for the appropriate time)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm interested in this feature. Would this new API block the modal from being closed if matDialog.closeAll() was called?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any update on this?

result: Result | undefined,
config: this,
componentInstance: Component | null,
) => boolean;

/** Width of the dialog. */
width?: string = '';

Expand Down
14 changes: 1 addition & 13 deletions src/cdk/dialog/dialog-container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export class CdkDialogContainer<C extends DialogConfig = DialogConfig>
@Inject(DialogConfig) readonly _config: C,
private _interactivityChecker: InteractivityChecker,
private _ngZone: NgZone,
private _overlayRef: OverlayRef,
protected _overlayRef: OverlayRef,
private _focusMonitor?: FocusMonitor,
) {
super();
Expand All @@ -107,7 +107,6 @@ export class CdkDialogContainer<C extends DialogConfig = DialogConfig>

protected _contentAttached() {
this._initializeFocusTrap();
this._handleBackdropClicks();
this._captureInitialFocus();
}

Expand Down Expand Up @@ -324,15 +323,4 @@ export class CdkDialogContainer<C extends DialogConfig = DialogConfig>
this._elementFocusedBeforeDialogWasOpened = _getFocusedElementPierceShadowDom();
}
}

/** Sets up the listener that handles clicks on the dialog backdrop. */
private _handleBackdropClicks() {
// Clicking on the backdrop will move focus out of dialog.
// Recapture it if closing via the backdrop is disabled.
this._overlayRef.backdropClick().subscribe(() => {
if (this._config.disableClose) {
this._recaptureFocus();
}
});
}
}
22 changes: 19 additions & 3 deletions src/cdk/dialog/dialog-ref.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ export class DialogRef<R = unknown, C = unknown> {
readonly componentInstance: C | null;

/** Instance of the container that is rendering out the dialog content. */
readonly containerInstance: BasePortalOutlet & {_closeInteractionType?: FocusOrigin};
readonly containerInstance: BasePortalOutlet & {
_closeInteractionType?: FocusOrigin;
_recaptureFocus?: () => void;
};

/** Whether the user is allowed to close the dialog. */
disableClose: boolean | undefined;
Expand Down Expand Up @@ -68,8 +71,12 @@ export class DialogRef<R = unknown, C = unknown> {
});

this.backdropClick.subscribe(() => {
if (!this.disableClose) {
if (!this.disableClose && this._canClose()) {
this.close(undefined, {focusOrigin: 'mouse'});
} else {
// Clicking on the backdrop will move focus out of dialog.
// Recapture it if closing via the backdrop is disabled.
this.containerInstance._recaptureFocus?.();
}
});
}
Expand All @@ -80,7 +87,7 @@ export class DialogRef<R = unknown, C = unknown> {
* @param options Additional options to customize the closing behavior.
*/
close(result?: R, options?: DialogCloseOptions): void {
if (this.containerInstance) {
if (this._canClose(result)) {
const closedSubject = this.closed as Subject<R | undefined>;
this.containerInstance._closeInteractionType = options?.focusOrigin || 'program';
this.overlayRef.dispose();
Expand Down Expand Up @@ -119,4 +126,13 @@ export class DialogRef<R = unknown, C = unknown> {
this.overlayRef.removePanelClass(classes);
return this;
}

/** Whether the dialog is allowed to close. */
private _canClose(result?: R): boolean {
return (
!!this.containerInstance &&
(!this.config.closePredicate ||
this.config.closePredicate(result, this.config, this.componentInstance))
);
}
}
196 changes: 196 additions & 0 deletions src/cdk/dialog/dialog.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,202 @@ describe('Dialog', () => {

expect(overlayContainerElement.querySelector('cdk-dialog-container')).toBeTruthy();
}));

it('should recapture focus to the first tabbable element when clicking on the backdrop', fakeAsync(() => {
// When testing focus, all of the elements must be in the DOM.
document.body.appendChild(overlayContainerElement);

dialog.open(PizzaMsg, {
disableClose: true,
viewContainerRef: testViewContainerRef,
});

viewContainerFixture.detectChanges();
flushMicrotasks();

const backdrop = overlayContainerElement.querySelector(
'.cdk-overlay-backdrop',
) as HTMLElement;
const input = overlayContainerElement.querySelector('input') as HTMLInputElement;

expect(document.activeElement)
.withContext('Expected input to be focused on open')
.toBe(input);

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

expect(document.activeElement)
.withContext('Expected input to stay focused after click')
.toBe(input);

overlayContainerElement.remove();
}));
});

describe('closePredicate option', () => {
function getDialogs() {
return overlayContainerElement.querySelectorAll('cdk-dialog-container');
}

it('should determine whether closing via the backdrop is allowed', fakeAsync(() => {
let canClose = false;
const closedSpy = jasmine.createSpy('closed spy');
const ref = dialog.open(PizzaMsg, {
closePredicate: () => canClose,
viewContainerRef: testViewContainerRef,
});

ref.closed.subscribe(closedSpy);
viewContainerFixture.detectChanges();

expect(getDialogs().length).toBe(1);

let backdrop = overlayContainerElement.querySelector('.cdk-overlay-backdrop') as HTMLElement;
backdrop.click();
viewContainerFixture.detectChanges();
flush();

expect(getDialogs().length).toBe(1);
expect(closedSpy).not.toHaveBeenCalled();

canClose = true;
backdrop.click();
viewContainerFixture.detectChanges();
flush();

expect(getDialogs().length).toBe(0);
expect(closedSpy).toHaveBeenCalledTimes(1);
}));

it('should determine whether closing via the escape key is allowed', fakeAsync(() => {
let canClose = false;
const closedSpy = jasmine.createSpy('closed spy');
const ref = dialog.open(PizzaMsg, {
closePredicate: () => canClose,
viewContainerRef: testViewContainerRef,
});

ref.closed.subscribe(closedSpy);
viewContainerFixture.detectChanges();

expect(getDialogs().length).toBe(1);

dispatchKeyboardEvent(document.body, 'keydown', ESCAPE);
viewContainerFixture.detectChanges();
flush();

expect(getDialogs().length).toBe(1);
expect(closedSpy).not.toHaveBeenCalled();

canClose = true;
dispatchKeyboardEvent(document.body, 'keydown', ESCAPE);
viewContainerFixture.detectChanges();
flush();

expect(getDialogs().length).toBe(0);
expect(closedSpy).toHaveBeenCalledTimes(1);
}));

it('should determine whether closing via the `close` method is allowed', fakeAsync(() => {
let canClose = false;
const closedSpy = jasmine.createSpy('closed spy');
const ref = dialog.open(PizzaMsg, {
closePredicate: () => canClose,
viewContainerRef: testViewContainerRef,
});

ref.closed.subscribe(closedSpy);
viewContainerFixture.detectChanges();

expect(getDialogs().length).toBe(1);

ref.close();
viewContainerFixture.detectChanges();
flush();

expect(getDialogs().length).toBe(1);
expect(closedSpy).not.toHaveBeenCalled();

canClose = true;
ref.close('hello');
viewContainerFixture.detectChanges();
flush();

expect(getDialogs().length).toBe(0);
expect(closedSpy).toHaveBeenCalledTimes(1);
expect(closedSpy).toHaveBeenCalledWith('hello');
}));

it('should not be closed by `closeAll` if not allowed by the predicate', fakeAsync(() => {
let canClose = false;
const config = {closePredicate: () => canClose};
const spy = jasmine.createSpy('afterAllClosed spy');
dialog.open(PizzaMsg, config);
viewContainerFixture.detectChanges();
dialog.open(PizzaMsg, config);
viewContainerFixture.detectChanges();
dialog.open(PizzaMsg, config);
viewContainerFixture.detectChanges();

const subscription = dialog.afterAllClosed.subscribe(spy);
expect(getDialogs().length).toBe(3);
expect(dialog.openDialogs.length).toBe(3);

dialog.closeAll();
viewContainerFixture.detectChanges();
flush();

expect(getDialogs().length).toBe(3);
expect(dialog.openDialogs.length).toBe(3);
expect(spy).not.toHaveBeenCalled();

canClose = true;
dialog.closeAll();
viewContainerFixture.detectChanges();
flush();

expect(getDialogs().length).toBe(0);
expect(dialog.openDialogs.length).toBe(0);
expect(spy).toHaveBeenCalledTimes(1);

subscription.unsubscribe();
}));

it('should recapture focus to the first tabbable element when clicking on the backdrop while the `closePredicate` is blocking the close sequence', fakeAsync(() => {
// When testing focus, all of the elements must be in the DOM.
document.body.appendChild(overlayContainerElement);

dialog.open(PizzaMsg, {
closePredicate: () => false,
viewContainerRef: testViewContainerRef,
});

viewContainerFixture.detectChanges();
flushMicrotasks();

const backdrop = overlayContainerElement.querySelector(
'.cdk-overlay-backdrop',
) as HTMLElement;
const input = overlayContainerElement.querySelector('input') as HTMLInputElement;

expect(document.activeElement)
.withContext('Expected input to be focused on open')
.toBe(input);

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

expect(document.activeElement)
.withContext('Expected input to stay focused after click')
.toBe(input);

overlayContainerElement.remove();
}));
});

describe('hasBackdrop option', () => {
Expand Down
Loading