fix(modal): ignore accidental backdrop clicks

This uses a combination of 'mousedown/up' events instead of 'click'.
This aligns behavior with bootstrap and helps with closing modal accidentally (ex. when selecting text)

Some tests had to be converted to e2e

Closes #3384
Fixes #1950

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

@@ -13,6 +13,7 @@ import {DatepickerFocusComponent} from './datepicker/focus/datepicker-focus.comp
 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';
@@ -35,6 +36,7 @@ import {TypeaheadValidationComponent} from './typeahead/validation/typeahead-val
     DropdownAutoCloseComponent,
     DropdownFocusComponent,
     DropdownPositionComponent,
+    ModalAutoCloseComponent,
     ModalFocusComponent,
     ModalNestingComponent,
     ModalStackComponent,

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

@@ -5,7 +5,10 @@ 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 {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 {PopoverAutocloseComponent} from './popover/autoclose/popover-autoclose.component';
 import {TooltipAutocloseComponent} from './tooltip/autoclose/tooltip-autoclose.component';
 import {TooltipFocusComponent} from './tooltip/focus/tooltip-focus.component';
@@ -15,8 +18,6 @@ import {TypeaheadFocusComponent} from './typeahead/focus/typeahead-focus.compone
 import {TimepickerNavigationComponent} from './timepicker/navigation/timepicker-navigation.component';
 import {TypeaheadValidationComponent} from './typeahead/validation/typeahead-validation.component';
 import {DropdownPositionComponent} from './dropdown/position/dropdown-position.component';
-import {ModalNestingComponent} from './modal/nesting/modal-nesting.component';
-import {ModalStackComponent} from './modal/stack/modal-stack.component';
 
 
 export const routes: Routes = [
@@ -30,6 +31,7 @@ export const routes: Routes = [
   {
     path: 'modal',
     children: [
+      {path: 'autoclose', component: ModalAutoCloseComponent},
       {path: 'focus', component: ModalFocusComponent},
       {path: 'nesting', component: ModalNestingComponent},
       {path: 'stack', component: ModalStackComponent},

e2e-app/src/app/modal/autoclose/modal-autoclose.component.html

@@ -0,0 +1,30 @@
+<h3>
+  Modal autoclose tests
+  <span class="ml-1 badge badge-info" id="dismiss-reason">{{ reason }}</span>
+</h3>
+
+<!-- Modal content -->
+<ng-template #content>
+  <div class="modal-body">
+    <h2>Hello from modal</h2>
+    <button id="modal-close-button" class="btn btn-primary" (click)="closeModal()">Close modal</button>
+  </div>
+</ng-template>
+
+<!-- Controls -->
+<form id="default" (contextmenu)="$event.preventDefault()">
+  <div class="form-group form-inline">
+    <div class="input-group">
+      <button class="btn btn-outline-secondary" type="button" id="open-modal" (click)="openModal(content)">Open modal</button>
+    </div>
+
+    <button class="btn btn-outline-secondary ml-3" id="reset-button" (click)="reset()">Reset</button>
+    <button class="btn btn-outline-secondary ml-1" id="option-backdrop-static" (click)="options['backdrop'] = 'static'">backdrop = 'static'</button>
+    <button class="btn btn-outline-secondary ml-1" id="option-backdrop-false" (click)="options['backdrop'] = false">backdrop = false</button>
+    <button class="btn btn-outline-secondary ml-1" id="option-no-keyboard" (click)="options['keyboard'] = false">keyboard = 'false'</button>
+  </div>
+</form>
+
+<!-- Options -->
+<h4>Options</h4>
+<pre>{{ options | json }}</pre>

e2e-app/src/app/modal/autoclose/modal-autoclose.component.ts

@@ -0,0 +1,43 @@
+import {ChangeDetectionStrategy, ChangeDetectorRef, Component, TemplateRef} from '@angular/core';
+import {ModalDismissReasons, NgbModal, NgbModalRef} from '@ng-bootstrap/ng-bootstrap';
+
+@Component({templateUrl: './modal-autoclose.component.html', changeDetection: ChangeDetectionStrategy.OnPush})
+export class ModalAutoCloseComponent {
+  private modalRef: NgbModalRef = null;
+  reason = '';
+  options = {};
+
+  constructor(private modalService: NgbModal, private cd: ChangeDetectorRef) {}
+
+  openModal(content?: TemplateRef<any>) {
+    this.modalRef = this.modalService.open(content, this.options);
+    this.modalRef.result.then(
+        () => {
+          this.reason = `Closed`;
+          this.cd.markForCheck();
+        },
+        reason => {
+          if (reason === ModalDismissReasons.BACKDROP_CLICK) {
+            this.reason = 'Click';
+          } else if (reason === ModalDismissReasons.ESC) {
+            this.reason = 'Escape';
+          } else {
+            this.reason = 'Other';
+          }
+          this.cd.markForCheck();
+        });
+  }
+
+  closeModal() {
+    if (this.modalRef) {
+      this.modalRef.close();
+      this.modalRef = null;
+    }
+  }
+
+  reset() {
+    this.closeModal();
+    this.reason = '';
+    this.options = {};
+  }
+}

e2e-app/src/app/modal/autoclose/modal-autoclose.e2e-spec.ts

@@ -0,0 +1,70 @@
+import {ModalAutoClosePage} from './modal-autoclose.po';
+import {expectNoOpenModals, openUrl, sendKey} from '../../tools.po';
+import {Key} from 'protractor';
+
+describe('Modal', () => {
+  let page: ModalAutoClosePage;
+
+  beforeAll(() => page = new ModalAutoClosePage());
+
+  beforeEach(async() => {
+    await openUrl('modal/autoclose');
+    await page.getResetButton().click();
+  });
+
+  afterEach(async() => { await expectNoOpenModals(); });
+
+  it('should close modal from the inside', async() => {
+    const modal = await page.openModal();
+
+    // close
+    await page.getModalCloseButton().click();
+    expect(await modal.isPresent()).toBeFalsy('The modal should be closed imperatively');
+    expect(await page.getDismissReason()).toBe('Closed', `Modal should have been closed`);
+  });
+
+  it('should close modal on ESC', async() => {
+    await page.openModal();
+
+    // close
+    await sendKey(Key.ESCAPE);
+    expect(await page.getModal().isPresent()).toBeFalsy('The modal should be closed on ESC');
+    expect(await page.getDismissReason()).toBe('Escape', `Modal should have been dismissed with 'Escape' reason`);
+  });
+
+  it(`should NOT close modal on ESC when keyboard === 'false'`, async() => {
+    const modal = await page.openModal('no-keyboard');
+
+    // close
+    await sendKey(Key.ESCAPE);
+    expect(await modal.isPresent()).toBeTruthy('The modal should stay opened on ESC');
+    await page.getModalCloseButton().click();
+  });
+
+  it('should close modal on backdrop click', async() => {
+    const modal = await page.openModal();
+
+    // close
+    await modal.click();
+    expect(await modal.isPresent()).toBeFalsy('The modal should be closed on backdrop click');
+    expect(await page.getDismissReason()).toBe('Click', `Modal should have been dismissed with 'Click' reason`);
+  });
+
+  it(`should NOT close modal on 'static' backdrop click`, async() => {
+    const modal = await page.openModal('backdrop-static');
+
+    // close
+    await modal.click();
+    expect(await modal.isPresent()).toBeTruthy('The modal should stay opened on backdrop click');
+    await page.getModalCloseButton().click();
+  });
+
+  it(`should NOT close modal on click with no backdrop`, async() => {
+    const modal = await page.openModal('backdrop-false');
+
+    // close
+    await modal.click();
+    expect(await modal.isPresent()).toBeTruthy('The modal should stay opened on backdrop click');
+    await page.getModalCloseButton().click();
+  });
+});

e2e-app/src/app/modal/autoclose/modal-autoclose.po.ts

@@ -0,0 +1,23 @@
+import {$} from 'protractor';
+
+export class ModalAutoClosePage {
+  getModal(selector = 'ngb-modal-window') { return $(selector); }
+
+  getModalCloseButton() { return $('#modal-close-button'); }
+
+  getDismissReason() { return $('#dismiss-reason').getText(); }
+
+  getResetButton() { return $('#reset-button'); }
+
+  async openModal(option = '') {
+    if (option !== '') {
+      await $(`#option-${option}`).click();
+    }
+
+    await $('#open-modal').click();
+
+    const modal = this.getModal();
+    expect(await modal.isPresent()).toBeTruthy(`A modal should have been opened`);
+    return modal;
+  }
+}

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

@@ -1,7 +1,9 @@
-import {Key} from 'protractor';
+import {$, Key} from 'protractor';
 import {expectFocused, expectNoOpenModals, openUrl, sendKey} from '../../tools.po';
 import {ModalStackPage} from './modal-stack.po';
 
+const bodyClass = () => $('body').getAttribute('class');
+
 describe('Modal stacked', () => {
   let page: ModalStackPage;
 
@@ -14,9 +16,11 @@ describe('Modal stacked', () => {
   it('should keep tab on the first modal after the second modal has closed', async() => {
     await page.openModal();
     await page.openStackModal();
+    expect(await bodyClass()).toContain('modal-open', `body should have 'modal-open' class`);
 
     // close the stack modal
     await sendKey(Key.ESCAPE);
+    expect(await bodyClass()).toContain('modal-open', `body should have 'modal-open' class`);
 
     // Check that the button is focused again
     await expectFocused(page.getStackModalButton(), 'Button element not focused');
@@ -26,7 +30,7 @@ describe('Modal stacked', () => {
 
     // close the main modal
     await sendKey(Key.ESCAPE);
-
+    expect(await bodyClass()).not.toContain('modal-open', `body should have 'modal-open' class`);
   });
 
 });

src/modal/modal-window.spec.ts

@@ -1,9 +1,6 @@
-import {TestBed, ComponentFixture} from '@angular/core/testing';
+import {ComponentFixture, TestBed} from '@angular/core/testing';
 
 import {NgbModalWindow} from './modal-window';
-import {ModalDismissReasons} from './modal-dismiss-reasons';
-import {createKeyEvent} from '../test/common';
-import {Key} from '../util/key';
 
 describe('ngb-modal-dialog', () => {
 
@@ -50,65 +47,4 @@ describe('ngb-modal-dialog', () => {
       expect(dialogEl.getAttribute('role')).toBe('document');
     });
   });
-
-  describe('dismiss', () => {
-
-    it('should dismiss on backdrop click by default', (done) => {
-      fixture.detectChanges();
-
-      fixture.componentInstance.dismissEvent.subscribe(($event) => {
-        expect($event).toBe(ModalDismissReasons.BACKDROP_CLICK);
-        done();
-      });
-
-      fixture.nativeElement.click();
-    });
-
-    it('should not dismiss on modal content click when there is active backdrop', (done) => {
-      fixture.detectChanges();
-      fixture.componentInstance.dismissEvent.subscribe(
-          () => { done.fail(new Error('Should not trigger dismiss event')); });
-
-      fixture.nativeElement.querySelector('.modal-content').click();
-      setTimeout(done, 200);
-    });
-
-    it('should ignore backdrop clicks when there is no backdrop', (done) => {
-      fixture.componentInstance.backdrop = false;
-      fixture.detectChanges();
-
-      fixture.componentInstance.dismissEvent.subscribe(($event) => {
-        expect($event).toBe(ModalDismissReasons.BACKDROP_CLICK);
-        done.fail(new Error('Should not trigger dismiss event'));
-      });
-
-      fixture.nativeElement.querySelector('.modal-dialog').click();
-      setTimeout(done, 200);
-    });
-
-    it('should ignore backdrop clicks when backdrop is "static"', (done) => {
-      fixture.componentInstance.backdrop = 'static';
-      fixture.detectChanges();
-
-      fixture.componentInstance.dismissEvent.subscribe(($event) => {
-        expect($event).toBe(ModalDismissReasons.BACKDROP_CLICK);
-        done.fail(new Error('Should not trigger dismiss event'));
-      });
-
-      fixture.nativeElement.querySelector('.modal-dialog').click();
-      setTimeout(done, 200);
-    });
-
-    it('should dismiss on esc press by default', (done) => {
-      fixture.detectChanges();
-
-      fixture.componentInstance.dismissEvent.subscribe(($event) => {
-        expect($event).toBe(ModalDismissReasons.ESC);
-        done();
-      });
-
-      fixture.nativeElement.dispatchEvent(createKeyEvent(Key.Escape, {type: 'keydown'}));
-    });
-  });
-
 });

src/modal/modal-window.ts

@@ -13,7 +13,7 @@ import {
   ViewEncapsulation
 } from '@angular/core';
 import {fromEvent} from 'rxjs';
-import {filter, takeUntil} from 'rxjs/operators';
+import {filter, map, takeUntil, withLatestFrom} from 'rxjs/operators';
 
 import {getFocusableBoundaryElements} from '../util/focus-trap';
 import {Key} from '../util/key';
@@ -25,7 +25,6 @@ import {ModalDismissReasons} from './modal-dismiss-reasons';
     '[class]': '"modal fade show d-block" + (windowClass ? " " + windowClass : "")',
     'role': 'dialog',
     'tabindex': '-1',
-    '(click)': 'backdropClick($event)',
     '[attr.aria-modal]': 'true',
     '[attr.aria-labelledby]': 'ariaLabelledBy',
   },
@@ -65,13 +64,16 @@ export class NgbModalWindow implements OnInit,
                          _zone.run(() => this.dismiss(ModalDismissReasons.ESC));
                        }
                      }));
-    });
-  }
 
-  backdropClick(event: MouseEvent): void {
-    if (this.backdrop === true && this._elRef.nativeElement === event.target) {
-      this.dismiss(ModalDismissReasons.BACKDROP_CLICK);
-    }
+      const mouseDowns$ = fromEvent<MouseEvent>(this._elRef.nativeElement, 'mousedown')
+                              .pipe(
+                                  takeUntil(this.dismissEvent),
+                                  map(e => this.backdrop === true && this._elRef.nativeElement === e.target));
+
+      fromEvent<MouseEvent>(this._elRef.nativeElement, 'mouseup')
+          .pipe(takeUntil(this.dismissEvent), withLatestFrom(mouseDowns$), filter(([_, shouldClose]) => shouldClose))
+          .subscribe(() => this._zone.run(() => this.dismiss(ModalDismissReasons.BACKDROP_CLICK)));
+    });
   }
 
   dismiss(reason): void { this.dismissEvent.emit(reason); }