fix(modal): remove key/mouse handlers only when modal is destroyed (#…

…3532)

At the moment key/mouse handlers are removed when modal triggers dismiss event. The problem is that modal closure could be cancelled with `beforeDismiss` and modal will stay opened, but handlers will be removed.

This commit introduces `closed$` event triggered inside `onDestroy` to clean up handlers.

Fixes #3515

e2e-app/src/app/app.module.ts

@@ -17,6 +17,7 @@ import {ModalAutoCloseComponent} from './modal/autoclose/modal-autoclose.compone
 import {ModalFocusComponent} from './modal/focus/modal-focus.component';
 import {ModalNestingComponent} from './modal/nesting/modal-nesting.component';
 import {ModalStackComponent} from './modal/stack/modal-stack.component';
+import {ModalStackConfirmationComponent} from './modal/stack-confirmation/modal-stack-confirmation.component';
 import {PopoverAutocloseComponent} from './popover/autoclose/popover-autoclose.component';
 import {TooltipAutocloseComponent} from './tooltip/autoclose/tooltip-autoclose.component';
 import {TooltipFocusComponent} from './tooltip/focus/tooltip-focus.component';
@@ -40,6 +41,7 @@ import {TypeaheadValidationComponent} from './typeahead/validation/typeahead-val
     ModalFocusComponent,
     ModalNestingComponent,
     ModalStackComponent,
+    ModalStackConfirmationComponent,
     PopoverAutocloseComponent,
     TooltipAutocloseComponent,
     TooltipFocusComponent,

e2e-app/src/app/app.routing.ts

@@ -5,10 +5,12 @@ import {DatepickerAutoCloseComponent} from './datepicker/autoclose/datepicker-au
 import {DatepickerFocusComponent} from './datepicker/focus/datepicker-focus.component';
 import {DropdownAutoCloseComponent} from './dropdown/autoclose/dropdown-autoclose.component';
 import {DropdownFocusComponent} from './dropdown/focus/dropdown-focus.component';
+import {DropdownPositionComponent} from './dropdown/position/dropdown-position.component';
 import {ModalAutoCloseComponent} from './modal/autoclose/modal-autoclose.component';
 import {ModalFocusComponent} from './modal/focus/modal-focus.component';
 import {ModalNestingComponent} from './modal/nesting/modal-nesting.component';
 import {ModalStackComponent} from './modal/stack/modal-stack.component';
+import {ModalStackConfirmationComponent} from './modal/stack-confirmation/modal-stack-confirmation.component';
 import {PopoverAutocloseComponent} from './popover/autoclose/popover-autoclose.component';
 import {TooltipAutocloseComponent} from './tooltip/autoclose/tooltip-autoclose.component';
 import {TooltipFocusComponent} from './tooltip/focus/tooltip-focus.component';
@@ -17,7 +19,6 @@ import {TypeaheadAutoCloseComponent} from './typeahead/autoclose/typeahead-autoc
 import {TypeaheadFocusComponent} from './typeahead/focus/typeahead-focus.component';
 import {TimepickerNavigationComponent} from './timepicker/navigation/timepicker-navigation.component';
 import {TypeaheadValidationComponent} from './typeahead/validation/typeahead-validation.component';
-import {DropdownPositionComponent} from './dropdown/position/dropdown-position.component';
 
 
 export const routes: Routes = [
@@ -35,6 +36,7 @@ export const routes: Routes = [
       {path: 'focus', component: ModalFocusComponent},
       {path: 'nesting', component: ModalNestingComponent},
       {path: 'stack', component: ModalStackComponent},
+      {path: 'stack-confirmation', component: ModalStackConfirmationComponent},
     ]
   },
   {

e2e-app/src/app/modal/stack-confirmation/modal-stack-confirmation.component.html

@@ -0,0 +1,27 @@
+<h3>Modal closure confirmation test</h3>
+
+<ng-template #modal let-modal>
+  <div class="modal-header">
+    <h4 class="modal-title">Modal with closure confirmation</h4>
+    <button id="close" type="button" class="close" aria-label="Close" (click)="modal.dismiss()">
+      <span aria-hidden="true">&times;</span>
+    </button>
+  </div>
+  <div class="modal-body">
+    <p>This modal will trigger confirmation</p>
+  </div>
+</ng-template>
+
+<ng-template #confirmation let-modal>
+  <div class="modal-header">
+    <h4 class="modal-title">Are you sure you want to close the modal?</h4>
+    <button id="dismiss" type="button" class="close" aria-label="Close" (click)="modal.dismiss()">
+      <span aria-hidden="true">&times;</span>
+    </button>
+  </div>
+  <div class="modal-body">
+    <button id="confirm" class="btn btn-primary" (click)="modal.close()">Yep</button>
+  </div>
+</ng-template>
+
+<button class="btn btn-outline-secondary ml-2" type="button" id="open-modal" (click)="openModal(modal)">Open Modal</button>

e2e-app/src/app/modal/stack-confirmation/modal-stack-confirmation.component.ts

@@ -0,0 +1,13 @@
+import {Component, TemplateRef, ViewChild} from '@angular/core';
+import {NgbModal} from '@ng-bootstrap/ng-bootstrap';
+
+@Component({templateUrl: './modal-stack-confirmation.component.html'})
+export class ModalStackConfirmationComponent {
+  @ViewChild('confirmation', {static: true, read: TemplateRef}) confirmationTpl: TemplateRef<any>;
+
+  constructor(private modalService: NgbModal) {}
+
+  openModal(content: TemplateRef<any>) {
+    this.modalService.open(content, {beforeDismiss: () => this.modalService.open(this.confirmationTpl).result});
+  }
+}

e2e-app/src/app/modal/stack-confirmation/modal-stack-confirmation.e2e-spec.ts

@@ -0,0 +1,71 @@
+import {expectNoOpenModals, openUrl, sendKey} from '../../tools.po';
+import {ModalStackConfirmationPage} from './modal-stack-confirmation.po';
+import {Key} from 'protractor';
+
+describe('Modal stacked with confirmation', () => {
+  let page: ModalStackConfirmationPage;
+
+  beforeAll(() => { page = new ModalStackConfirmationPage(); });
+
+  beforeEach(async() => await openUrl('modal/stack-confirmation'));
+
+  afterEach(async() => { await expectNoOpenModals(); });
+
+  it('should close modals correctly using close button', async() => {
+    await page.openModal();
+
+    // close with button
+    await page.getModalClose().click();
+    expect(await page.getOpenModals().count()).toBe(2, 'Confirmation modal should be opened');
+
+    // cancel closure with button
+    await page.getDismissalButton().click();
+    expect(await page.getOpenModals().count()).toBe(1, 'Confirmation modal should be dismissed');
+
+    // close again
+    await page.getModalClose().click();
+    expect(await page.getOpenModals().count()).toBe(2, 'Confirmation modal should be re-opened');
+
+    // close all modals
+    await page.getConfirmationButton().click();
+  });
+
+  it('should close modals correctly using ESC', async() => {
+    await page.openModal();
+
+    // close with Escape
+    await sendKey(Key.ESCAPE);
+    expect(await page.getOpenModals().count()).toBe(2, 'Confirmation modal should be opened');
+
+    // cancel closure with Escape
+    await sendKey(Key.ESCAPE);
+    expect(await page.getOpenModals().count()).toBe(1, 'Confirmation modal should be dismissed');
+
+    // close again
+    await sendKey(Key.ESCAPE);
+    expect(await page.getOpenModals().count()).toBe(2, 'Confirmation modal should be re-opened');
+
+    // close all modals
+    await page.getConfirmationButton().click();
+  });
+
+  it('should close modals correctly using backdrop click', async() => {
+    await page.openModal();
+
+    // close with click
+    await page.getModal(0).click();
+    expect(await page.getOpenModals().count()).toBe(2, 'Confirmation modal should be opened');
+
+    // cancel closure with click
+    await page.getModal(1).click();
+    expect(await page.getOpenModals().count()).toBe(1, 'Confirmation modal should be dismissed');
+
+    // close again
+    await page.getModal(0).click();
+    expect(await page.getOpenModals().count()).toBe(2, 'Confirmation modal should be re-opened');
+
+    // close all modals
+    await page.getConfirmationButton().click();
+  });
+
+});

e2e-app/src/app/modal/stack-confirmation/modal-stack-confirmation.po.ts

@@ -0,0 +1,22 @@
+import {$, $$} from 'protractor';
+
+export class ModalStackConfirmationPage {
+  getOpenModals() { return $$('ngb-modal-window'); }
+
+  getModal(index) { return this.getOpenModals().get(index); }
+
+  getModalButton() { return $('#open-modal'); }
+
+  getModalClose() { return $('#close'); }
+
+  getConfirmationButton() { return $('#confirm'); }
+
+  getDismissalButton() { return $('#dismiss'); }
+
+  async openModal() {
+    await this.getModalButton().click();
+    const modal = this.getModal(0);
+    expect(await modal.isPresent()).toBeTruthy(`A modal should have been opened`);
+    return modal;
+  }
+}

src/modal/modal-window.ts

@@ -13,7 +13,7 @@ import {
   ViewChild,
   ViewEncapsulation
 } from '@angular/core';
-import {fromEvent} from 'rxjs';
+import {fromEvent, Subject} from 'rxjs';
 import {filter, switchMap, take, takeUntil, tap} from 'rxjs/operators';
 
 import {getFocusableBoundaryElements} from '../util/focus-trap';
@@ -40,6 +40,7 @@ import {ModalDismissReasons} from './modal-dismiss-reasons';
 })
 export class NgbModalWindow implements OnInit,
     AfterViewInit, OnDestroy {
+  private _closed$ = new Subject<void>();
   private _elWithFocus: Element;  // element that is focused prior to modal opening
 
   @ViewChild('dialog', {static: true}) private _dialogEl: ElementRef<HTMLElement>;
@@ -67,7 +68,7 @@ export class NgbModalWindow implements OnInit,
 
       fromEvent<KeyboardEvent>(nativeElement, 'keydown')
           .pipe(
-              takeUntil(this.dismissEvent),
+              takeUntil(this._closed$),
               // tslint:disable-next-line:deprecation
               filter(e => e.which === Key.Escape && this.keyboard))
           .subscribe(event => requestAnimationFrame(() => {
@@ -81,17 +82,16 @@ export class NgbModalWindow implements OnInit,
       let preventClose = false;
       fromEvent<MouseEvent>(this._dialogEl.nativeElement, 'mousedown')
           .pipe(
-              takeUntil(this.dismissEvent), tap(() => preventClose = false),
-              switchMap(
-                  () => fromEvent<MouseEvent>(nativeElement, 'mouseup').pipe(takeUntil(this.dismissEvent), take(1))),
+              takeUntil(this._closed$), tap(() => preventClose = false),
+              switchMap(() => fromEvent<MouseEvent>(nativeElement, 'mouseup').pipe(takeUntil(this._closed$), take(1))),
               filter(({target}) => nativeElement === target))
           .subscribe(() => { preventClose = true; });
 
       // We're listening to 'click' to dismiss modal on modal window click, except when:
       // 1. clicking on modal dialog itself
       // 2. closing was prevented by mousedown/up handlers
       // 3. clicking on scrollbar when the viewport is too small and modal doesn't fit (click is not triggered at all)
-      fromEvent<MouseEvent>(nativeElement, 'click').pipe(takeUntil(this.dismissEvent)).subscribe(({target}) => {
+      fromEvent<MouseEvent>(nativeElement, 'click').pipe(takeUntil(this._closed$)).subscribe(({target}) => {
         if (this.backdrop === true && nativeElement === target && !preventClose) {
           this._zone.run(() => this.dismiss(ModalDismissReasons.BACKDROP_CLICK));
         }
@@ -122,5 +122,7 @@ export class NgbModalWindow implements OnInit,
       setTimeout(() => elementToFocus.focus());
       this._elWithFocus = null;
     });
+
+    this._closed$.next();
   }
 }