Dialog triggers change detection on keyboard events

[OmriSoudry]

Bug, feature request, or proposal:

Opening a dialog using this.dialog.open(...) [where dialog is MatDialog from the constructor] causes my app to run a change detection for every key press - even after closing the dialog. After opening 10 dialogs, each key press triggers 10 change detections. The event triggers are only removed when I refresh the page and reload the app.

This happens even with a dialog that only has a mat-dialog-close button.

What is the expected behavior?

When a dialog is closed (no matter the reason), all listeners should be removed.

What is the current behavior?

The listeners remains, triggering a change detection event after every key press. Trying to hold down a key after a while causes hundreds of change detections every second, and low-end devices freeze.

What are the steps to reproduce?

I Couldn't reproduce it on StackBlitz. I'm using Angular v2.0.0.beta-12.

[crisbeto]

You're using a very old Angular version which might have the dialog at a point where it was attaching its keyboard events to the document. A lot has changed since then which would've resolved the issue. Can you reproduce it with the latest version of Material and Angular?

[OmriSoudry]

Sorry, I'm actually using a newer version than the one used in StackBlitz.
Here's my ng -v output:

Angular CLI: 1.7.1
Node: 8.9.0
OS: win32 x64
Angular: 5.2.6
... animations, common, compiler, compiler-cli, core, forms
... http, language-service, platform-browser
... platform-browser-dynamic, router

@angular/cdk: 5.2.2
@angular/cli: 1.7.1
@angular/material: 5.2.2
@angular-devkit/build-optimizer: 0.3.2
@angular-devkit/core: 0.3.2
@angular-devkit/schematics: 0.3.2
@ngtools/json-schema: 1.2.0
@ngtools/webpack: 1.10.1
@schematics/angular: 0.3.2
@schematics/package-update: 0.3.2
typescript: 2.6.2
webpack: 3.11.0

[OmriSoudry]

I've tried using Chrome's debug tools to see what happens.
Phase 1
Before a dialog opens. Each keydown event triggers 2 calls for ZoneDelegate.prototype.invokeTask:

1. a call with the #document element (as applyThis variable) where the active element is "body" with the keydown event (as applyArgs variable).
2. a call with the "window" element (as applyThis) with no arguments in applyArgs.
   None of the calls triggers a change detection.

Phase 2
The dialog is open. Now, each keydown event triggers the following:

1. a call with the #document element (as applyThis variable) where the active element is "body" with the keydown event (as applyArgs variable) - same as before. No change detection is triggered.
2. a call with the "body" element and the keydown event - This triggers a change detection.
3. another 4 calls where both variables (applyThis & applyArgs) are null.
4. a call with the "window" element (as applyThis) with no arguments in applyArgs.

Phase 3
The dialog is closed. Now, each keydown event triggers the following:

1. a call with the #document element (as applyThis variable) where the active element is "body" with the keydown event (as applyArgs variable) - same as before. No change detection is triggered.
2. a call with the "body" element and the keydown event - This triggers a change detection.
3. another 3 calls where both variables (applyThis & applyArgs) are null.
4. a call with the "window" element (as applyThis) with no arguments in applyArgs.

After another 5 dialogs opens and closes, the calls are like so:

1. a call with the #document element (as applyThis variable) where the active element is "body" with the keydown event (as applyArgs variable) - same as before. No change detection is triggered.
2. For each dialog that was open, in my case 5 times: - a call with the "body" element and the keydown event - This triggers a change detection, followed by another 3 calls where both variables (applyThis & applyArgs) are null.
3. a call with the "window" element (as applyThis) with no arguments in applyArgs.

[OmriSoudry]

The callback that causes the change detection is triggered by FromEventObservable, but I'm not sure who's calling it.

[OmriSoudry]

Could it be that the dialog's keydownEvents observable is causing this?

[crisbeto]

We have a global way of handling keyboard events for all overlays which is being used by the dialog. That being said, the global keyboard handler will be removed once there are no more open overlays.

[OmriSoudry]

The "cdk-overlay-container" div remains even after the dialog is closed. Is that the overlay you're talking about?

[crisbeto]

No, I'm talking about the OverlayKeyboardDispatcher.

[OmriSoudry]

So what is an open overlay? does the cdk overlay container div is an open overlay? in my case, each dialog adds another event listener... 100 dialogs causes 100 event callbacks for every keydown on tge body element.

[crisbeto]

Each dialog is an overlay, the overlay container isn't. I'll take a better look later to see if there's anything that could introduce the extra listeners.

[crisbeto]

Alright, I could confirm that the event listener wasn't being removed. I've submitted #10160 to address it. As for it running change detection, I think it should still do it while there's an open overlay, because the overlays need to be able to react to changes caused by key presses.

[OmriSoudry]

I agree - change detection should be triggered while the dialog is open and receiving events - but the listeners should be removed when the dialog closes. Thank you Kristiyan!

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}