From fb8bb11e13a07409a732251dc68bc832fa7fec9f Mon Sep 17 00:00:00 2001 From: Max Okorokov Date: Tue, 7 Jan 2020 11:19:03 +0100 Subject: [PATCH] fix(modal): prevent modal from closing when using scrollbar (#3531) Using `mousedown` - `mouseup` - `click` combination to handle closing correctly. Fixes #3518 --- .../autoclose/modal-autoclose.e2e-spec.ts | 35 +++++++++- .../app/modal/autoclose/modal-autoclose.po.ts | 2 + src/modal/modal-window.ts | 64 ++++++++++++------- 3 files changed, 77 insertions(+), 24 deletions(-) diff --git a/e2e-app/src/app/modal/autoclose/modal-autoclose.e2e-spec.ts b/e2e-app/src/app/modal/autoclose/modal-autoclose.e2e-spec.ts index d9ff4fc004..634d35480d 100644 --- a/e2e-app/src/app/modal/autoclose/modal-autoclose.e2e-spec.ts +++ b/e2e-app/src/app/modal/autoclose/modal-autoclose.e2e-spec.ts @@ -1,6 +1,6 @@ import {ModalAutoClosePage} from './modal-autoclose.po'; import {expectNoOpenModals, openUrl, sendKey} from '../../tools.po'; -import {Key} from 'protractor'; +import {browser, Key} from 'protractor'; describe('Modal', () => { let page: ModalAutoClosePage; @@ -44,15 +44,44 @@ describe('Modal', () => { it('should close modal on backdrop click', async() => { const modal = await page.openModal(); + // dialog click + await page.getModalDialog().click(); + expect(await modal.isPresent()).toBeTruthy('The modal should stay opened on dialog click'); + // 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 close modal when dragging from backdrop -> dialog', async() => { + const modal = await page.openModal(); + + // close + const dialog = await page.getModalDialog(); + await browser.actions().dragAndDrop(modal, dialog).mouseUp().perform(); + expect(await modal.isPresent()).toBeFalsy('The modal should be closed on drag from backdrop -> dialog'); + expect(await page.getDismissReason()).toBe('Click', `Modal should have been dismissed with 'Click' reason`); + }); + + it('should NOT close modal when dragging from dialog -> backdrop', async() => { + const modal = await page.openModal(); + + // close + const dialog = await page.getModalDialog(); + await browser.actions().dragAndDrop(dialog, modal).mouseUp().perform(); + expect(await modal.isPresent()).toBeTruthy('The modal should stay opened on drag from dialog -> backdrop'); + await page.getModalCloseButton().click(); + }); + + it(`should NOT close modal on 'static' backdrop click`, async() => { const modal = await page.openModal('backdrop-static'); + // dialog click + await page.getModalDialog().click(); + expect(await modal.isPresent()).toBeTruthy('The modal should stay opened on dialog click'); + // close await modal.click(); expect(await modal.isPresent()).toBeTruthy('The modal should stay opened on backdrop click'); @@ -62,6 +91,10 @@ describe('Modal', () => { it(`should NOT close modal on click with no backdrop`, async() => { const modal = await page.openModal('backdrop-false'); + // dialog click + await page.getModalDialog().click(); + expect(await modal.isPresent()).toBeTruthy('The modal should stay opened on dialog click'); + // close await modal.click(); expect(await modal.isPresent()).toBeTruthy('The modal should stay opened on backdrop click'); diff --git a/e2e-app/src/app/modal/autoclose/modal-autoclose.po.ts b/e2e-app/src/app/modal/autoclose/modal-autoclose.po.ts index 89029044ca..942c61b5fa 100644 --- a/e2e-app/src/app/modal/autoclose/modal-autoclose.po.ts +++ b/e2e-app/src/app/modal/autoclose/modal-autoclose.po.ts @@ -3,6 +3,8 @@ import {$} from 'protractor'; export class ModalAutoClosePage { getModal(selector = 'ngb-modal-window') { return $(selector); } + getModalDialog() { return $('.modal-dialog'); } + getModalCloseButton() { return $('#modal-close-button'); } getDismissReason() { return $('#dismiss-reason').getText(); } diff --git a/src/modal/modal-window.ts b/src/modal/modal-window.ts index 3554383d0a..345b2cc0a8 100644 --- a/src/modal/modal-window.ts +++ b/src/modal/modal-window.ts @@ -10,10 +10,11 @@ import { OnDestroy, OnInit, Output, + ViewChild, ViewEncapsulation } from '@angular/core'; import {fromEvent} from 'rxjs'; -import {filter, map, takeUntil, withLatestFrom} from 'rxjs/operators'; +import {filter, switchMap, take, takeUntil, tap} from 'rxjs/operators'; import {getFocusableBoundaryElements} from '../util/focus-trap'; import {Key} from '../util/key'; @@ -29,7 +30,7 @@ import {ModalDismissReasons} from './modal-dismiss-reasons'; '[attr.aria-labelledby]': 'ariaLabelledBy', }, template: ` -
@@ -41,6 +42,8 @@ export class NgbModalWindow implements OnInit, AfterViewInit, OnDestroy { private _elWithFocus: Element; // element that is focused prior to modal opening + @ViewChild('dialog', {static: true}) private _dialogEl: ElementRef; + @Input() ariaLabelledBy: string; @Input() backdrop: boolean | string = true; @Input() centered: string; @@ -52,40 +55,55 @@ export class NgbModalWindow implements OnInit, @Output('dismiss') dismissEvent = new EventEmitter(); constructor( - @Inject(DOCUMENT) private _document: any, private _elRef: ElementRef, private _zone: NgZone) { - _zone.runOutsideAngular(() => { - fromEvent(this._elRef.nativeElement, 'keydown') + @Inject(DOCUMENT) private _document: any, private _elRef: ElementRef, private _zone: NgZone) {} + + dismiss(reason): void { this.dismissEvent.emit(reason); } + + ngOnInit() { this._elWithFocus = this._document.activeElement; } + + ngAfterViewInit() { + const {nativeElement} = this._elRef; + this._zone.runOutsideAngular(() => { + + fromEvent(nativeElement, 'keydown') .pipe( takeUntil(this.dismissEvent), // tslint:disable-next-line:deprecation filter(e => e.which === Key.Escape && this.keyboard)) .subscribe(event => requestAnimationFrame(() => { if (!event.defaultPrevented) { - _zone.run(() => this.dismiss(ModalDismissReasons.ESC)); + this._zone.run(() => this.dismiss(ModalDismissReasons.ESC)); } })); - const mouseDowns$ = fromEvent(this._elRef.nativeElement, 'mousedown') - .pipe( - takeUntil(this.dismissEvent), - map(e => this.backdrop === true && this._elRef.nativeElement === e.target)); + // We're listening to 'mousedown' and 'mouseup' to prevent modal from closing when pressing the mouse + // inside the modal dialog and releasing it outside + let preventClose = false; + fromEvent(this._dialogEl.nativeElement, 'mousedown') + .pipe( + takeUntil(this.dismissEvent), tap(() => preventClose = false), + switchMap( + () => fromEvent(nativeElement, 'mouseup').pipe(takeUntil(this.dismissEvent), take(1))), + filter(({target}) => nativeElement === target)) + .subscribe(() => { preventClose = true; }); - fromEvent(this._elRef.nativeElement, 'mouseup') - .pipe(takeUntil(this.dismissEvent), withLatestFrom(mouseDowns$), filter(([_, shouldClose]) => shouldClose)) - .subscribe(() => this._zone.run(() => this.dismiss(ModalDismissReasons.BACKDROP_CLICK))); + // 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(nativeElement, 'click').pipe(takeUntil(this.dismissEvent)).subscribe(({target}) => { + if (this.backdrop === true && nativeElement === target && !preventClose) { + this._zone.run(() => this.dismiss(ModalDismissReasons.BACKDROP_CLICK)); + } + preventClose = false; + }); }); - } - - dismiss(reason): void { this.dismissEvent.emit(reason); } - - ngOnInit() { this._elWithFocus = this._document.activeElement; } - ngAfterViewInit() { - if (!this._elRef.nativeElement.contains(document.activeElement)) { - const autoFocusable = this._elRef.nativeElement.querySelector(`[ngbAutofocus]`) as HTMLElement; - const firstFocusable = getFocusableBoundaryElements(this._elRef.nativeElement)[0]; + if (!nativeElement.contains(document.activeElement)) { + const autoFocusable = nativeElement.querySelector(`[ngbAutofocus]`) as HTMLElement; + const firstFocusable = getFocusableBoundaryElements(nativeElement)[0]; - const elementToFocus = autoFocusable || firstFocusable || this._elRef.nativeElement; + const elementToFocus = autoFocusable || firstFocusable || nativeElement; elementToFocus.focus(); } }