From 75f09660671577cd7988e6d93332b7749f76db25 Mon Sep 17 00:00:00 2001 From: Max Okorokov Date: Fri, 27 Sep 2019 16:07:40 +0200 Subject: [PATCH] fix(modal): don't close popup on ESC when closing nested components (#3384) Fixes an issue for datepicker, typeahead, dropdown inside the modal. When these components are closed on ESC, now modal stays open. Fixes #3358 --- e2e-app/src/app/app.module.ts | 2 + e2e-app/src/app/app.routing.ts | 6 +- e2e-app/src/app/datepicker/datepicker.po.ts | 2 +- .../nesting/modal-nesting.component.html | 50 ++++++++++++ .../modal/nesting/modal-nesting.component.ts | 13 +++ .../modal/nesting/modal-nesting.e2e-spec.ts | 80 +++++++++++++++++++ .../src/app/modal/nesting/modal-nesting.po.ts | 18 +++++ e2e-app/src/app/tools.po.ts | 6 +- src/modal/modal-window.spec.ts | 4 +- src/modal/modal-window.ts | 32 +++++--- src/modal/modal.spec.ts | 80 ++++++++++--------- src/util/autoclose.ts | 6 +- 12 files changed, 242 insertions(+), 57 deletions(-) create mode 100644 e2e-app/src/app/modal/nesting/modal-nesting.component.html create mode 100644 e2e-app/src/app/modal/nesting/modal-nesting.component.ts create mode 100644 e2e-app/src/app/modal/nesting/modal-nesting.e2e-spec.ts create mode 100644 e2e-app/src/app/modal/nesting/modal-nesting.po.ts diff --git a/e2e-app/src/app/app.module.ts b/e2e-app/src/app/app.module.ts index a6b65b7568..2d34155436 100644 --- a/e2e-app/src/app/app.module.ts +++ b/e2e-app/src/app/app.module.ts @@ -14,6 +14,7 @@ import {DropdownAutoCloseComponent} from './dropdown/autoclose/dropdown-autoclos import {DropdownFocusComponent} from './dropdown/focus/dropdown-focus.component'; import {DropdownPositionComponent} from './dropdown/position/dropdown-position.component'; import {ModalFocusComponent} from './modal/focus/modal-focus.component'; +import {ModalNestingComponent} from './modal/nesting/modal-nesting.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'; @@ -34,6 +35,7 @@ import {TypeaheadValidationComponent} from './typeahead/validation/typeahead-val DropdownFocusComponent, DropdownPositionComponent, ModalFocusComponent, + ModalNestingComponent, PopoverAutocloseComponent, TooltipAutocloseComponent, TooltipFocusComponent, diff --git a/e2e-app/src/app/app.routing.ts b/e2e-app/src/app/app.routing.ts index 94f8aed82d..3a4840748d 100644 --- a/e2e-app/src/app/app.routing.ts +++ b/e2e-app/src/app/app.routing.ts @@ -15,6 +15,7 @@ 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'; export const routes: Routes = [ @@ -25,7 +26,10 @@ export const routes: Routes = [ {path: 'autoclose', component: DatepickerAutoCloseComponent} ] }, - {path: 'modal', children: [{path: 'focus', component: ModalFocusComponent}]}, + { + path: 'modal', + children: [{path: 'focus', component: ModalFocusComponent}, {path: 'nesting', component: ModalNestingComponent}] + }, { path: 'dropdown', children: [ diff --git a/e2e-app/src/app/datepicker/datepicker.po.ts b/e2e-app/src/app/datepicker/datepicker.po.ts index efb414503e..e9d48b86f8 100644 --- a/e2e-app/src/app/datepicker/datepicker.po.ts +++ b/e2e-app/src/app/datepicker/datepicker.po.ts @@ -1,6 +1,6 @@ import {$} from 'protractor'; -export abstract class DatepickerPage { +export class DatepickerPage { getDatepicker(selector = 'ngb-datepicker') { return $(selector); } getDatepickerInput(selector = 'input[ngbDatepicker]') { return $(selector); } diff --git a/e2e-app/src/app/modal/nesting/modal-nesting.component.html b/e2e-app/src/app/modal/nesting/modal-nesting.component.html new file mode 100644 index 0000000000..bc59629fed --- /dev/null +++ b/e2e-app/src/app/modal/nesting/modal-nesting.component.html @@ -0,0 +1,50 @@ +

Modal nesting tests

+ + +
+

Modal with nested popups

+ +
+
+
+ +
+ +
+ +
+ +
+
+
+ + +
+ +
+
+ +
+ + + +
+
+
+
+ + +
+ +
+ +
+
+
+
+ + + diff --git a/e2e-app/src/app/modal/nesting/modal-nesting.component.ts b/e2e-app/src/app/modal/nesting/modal-nesting.component.ts new file mode 100644 index 0000000000..cdae189ce9 --- /dev/null +++ b/e2e-app/src/app/modal/nesting/modal-nesting.component.ts @@ -0,0 +1,13 @@ +import {Component, TemplateRef} from '@angular/core'; +import {NgbModal} from '@ng-bootstrap/ng-bootstrap'; +import {Observable} from 'rxjs'; +import {map} from 'rxjs/operators'; + +@Component({templateUrl: './modal-nesting.component.html'}) +export class ModalNestingComponent { + constructor(private modalService: NgbModal) {} + + openModal(content: TemplateRef) { this.modalService.open(content); } + + search = (text$: Observable) => text$.pipe(map(() => ['one', 'two', 'three'])); +} diff --git a/e2e-app/src/app/modal/nesting/modal-nesting.e2e-spec.ts b/e2e-app/src/app/modal/nesting/modal-nesting.e2e-spec.ts new file mode 100644 index 0000000000..eccc72ea14 --- /dev/null +++ b/e2e-app/src/app/modal/nesting/modal-nesting.e2e-spec.ts @@ -0,0 +1,80 @@ +import {Key} from 'protractor'; +import {expectFocused, expectNoOpenModals, openUrl, sendKey} from '../../tools.po'; +import {ModalNestingPage} from './modal-nesting.po'; +import {DatepickerPage} from '../../datepicker/datepicker.po'; +import {DropdownPage} from '../../dropdown/dropdown.po'; +import {TypeaheadPage} from '../../typeahead/typeahead.po'; + +describe('Modal nested components', () => { + let page: ModalNestingPage; + let datepickerPage: DatepickerPage; + let dropdownPage: DropdownPage; + let typeaheadPage: TypeaheadPage; + + beforeAll(() => { + page = new ModalNestingPage(); + datepickerPage = new DatepickerPage(); + dropdownPage = new DropdownPage(); + typeaheadPage = new TypeaheadPage(); + }); + + beforeEach(async() => await openUrl('modal/nesting')); + + afterEach(async() => { await expectNoOpenModals(); }); + + it('should close only datepicker, then modal on ESC', async() => { + await page.openModal(); + + // open datepicker + await page.getDatepickerButton().click(); + expect(await datepickerPage.getDatepicker().isPresent()).toBeTruthy(`Datepicker should be opened`); + await expectFocused(await datepickerPage.getDayElement(new Date(2018, 0, 1)), `01 JAN 2018 should be focused`); + + // close datepicker + await sendKey(Key.ESCAPE); + expect(await datepickerPage.getDatepicker().isPresent()).toBeFalsy(`Datepicker should be closed`); + await expectFocused(await page.getDatepickerButton(), `Datepicker open button should be focused`); + expect(await page.getModal().isPresent()).toBeTruthy(`Modal should stay opened`); + + // close modal + await sendKey(Key.ESCAPE); + }); + + it('should close only dropdown, then modal on ESC', async() => { + await page.openModal(); + + // open dropdown + await page.getDropdownButton().click(); + const dropdown = dropdownPage.getDropdown(); + expect(await dropdownPage.isOpened(dropdown)).toBeTruthy(`Dropdown should be opened`); + await expectFocused(page.getDropdownButton(), `Dropdown button should be focused`); + + // close dropdown + await sendKey(Key.ESCAPE); + expect(await dropdownPage.isOpened(dropdown)).toBeFalsy(`Dropdown should be closed`); + await expectFocused(await page.getDropdownButton(), `Dropdown open button should be focused`); + expect(await page.getModal().isPresent()).toBeTruthy(`Modal should stay opened`); + + // close modal + await sendKey(Key.ESCAPE); + }); + + it('should close only typeahead, then modal on ESC', async() => { + await page.openModal(); + + // open typeahead + await page.getTypeaheadInput().click(); + await sendKey(Key.SPACE); + expect(await typeaheadPage.getDropdown().isPresent()).toBeTruthy(`Typeahead should be opened`); + await expectFocused(page.getTypeaheadInput(), `Typeahead input should be focused`); + + // close dropdown + await sendKey(Key.ESCAPE); + expect(await typeaheadPage.getDropdown().isPresent()).toBeFalsy(`Typeahead should be closed`); + await expectFocused(page.getTypeaheadInput(), `Typeahead input should be focused`); + expect(await page.getModal().isPresent()).toBeTruthy(`Modal should stay opened`); + + // close modal + await sendKey(Key.ESCAPE); + }); +}); diff --git a/e2e-app/src/app/modal/nesting/modal-nesting.po.ts b/e2e-app/src/app/modal/nesting/modal-nesting.po.ts new file mode 100644 index 0000000000..4c95a56e24 --- /dev/null +++ b/e2e-app/src/app/modal/nesting/modal-nesting.po.ts @@ -0,0 +1,18 @@ +import {$} from 'protractor'; + +export class ModalNestingPage { + getModal(selector = 'ngb-modal-window') { return $(selector); } + + getDatepickerButton() { return $('#datepicker-button'); } + + getDropdownButton() { return $('#dropdown'); } + + getTypeaheadInput() { return $('#typeahead'); } + + async openModal() { + await $(`#open-modal`).click(); + const modal = this.getModal(); + expect(await modal.isPresent()).toBeTruthy(`A modal should have been opened`); + return modal; + } +} diff --git a/e2e-app/src/app/tools.po.ts b/e2e-app/src/app/tools.po.ts index 3aaf1acf91..e038591ade 100644 --- a/e2e-app/src/app/tools.po.ts +++ b/e2e-app/src/app/tools.po.ts @@ -1,4 +1,4 @@ -import {browser, ElementFinder, Key, WebElement, $, $$, Button} from 'protractor'; +import {$, browser, Button, ElementFinder, Key, protractor, WebElement} from 'protractor'; /** * Sends keys to a currently focused element @@ -41,8 +41,8 @@ export const expectFocused = async(el: ElementFinder, message: string) => { /** * Checks that there are no open modal windows in the document */ -export const expectNoOpenModals = async() => { - expect(await $$('ngb-modal-window').count()).toBe(0, `There should be no open modal windows left after a modal test`); +export const expectNoOpenModals = async(error = `There should be no open modal windows left after a modal test`) => { + browser.wait(protractor.ExpectedConditions.invisibilityOf($('ngb-modal-window')), 1000, error); }; /** diff --git a/src/modal/modal-window.spec.ts b/src/modal/modal-window.spec.ts index 2f55d073b7..e859257170 100644 --- a/src/modal/modal-window.spec.ts +++ b/src/modal/modal-window.spec.ts @@ -2,6 +2,8 @@ import {TestBed, ComponentFixture} 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', () => { @@ -105,7 +107,7 @@ describe('ngb-modal-dialog', () => { done(); }); - fixture.debugElement.triggerEventHandler('keyup.esc', {}); + fixture.nativeElement.dispatchEvent(createKeyEvent(Key.Escape)); }); }); diff --git a/src/modal/modal-window.ts b/src/modal/modal-window.ts index 9ea8a5f46d..ef9bf3947f 100644 --- a/src/modal/modal-window.ts +++ b/src/modal/modal-window.ts @@ -6,13 +6,17 @@ import { EventEmitter, Inject, Input, + NgZone, OnDestroy, OnInit, Output, - ViewEncapsulation, + ViewEncapsulation } from '@angular/core'; +import {fromEvent} from 'rxjs'; +import {filter, takeUntil} from 'rxjs/operators'; import {getFocusableBoundaryElements} from '../util/focus-trap'; +import {Key} from '../util/key'; import {ModalDismissReasons} from './modal-dismiss-reasons'; @Component({ @@ -21,7 +25,6 @@ import {ModalDismissReasons} from './modal-dismiss-reasons'; '[class]': '"modal fade show d-block" + (windowClass ? " " + windowClass : "")', 'role': 'dialog', 'tabindex': '-1', - '(keyup.esc)': 'escKey($event)', '(click)': 'backdropClick($event)', '[attr.aria-modal]': 'true', '[attr.aria-labelledby]': 'ariaLabelledBy', @@ -49,17 +52,24 @@ export class NgbModalWindow implements OnInit, @Output('dismiss') dismissEvent = new EventEmitter(); - constructor(@Inject(DOCUMENT) private _document: any, private _elRef: ElementRef) {} - - backdropClick($event): void { - if (this.backdrop === true && this._elRef.nativeElement === $event.target) { - this.dismiss(ModalDismissReasons.BACKDROP_CLICK); - } + constructor(@Inject(DOCUMENT) private _document: any, private _elRef: ElementRef, zone: NgZone) { + zone.runOutsideAngular(() => { + fromEvent(this._elRef.nativeElement, 'keyup') + .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)); + } + })); + }); } - escKey($event): void { - if (this.keyboard && !$event.defaultPrevented) { - this.dismiss(ModalDismissReasons.ESC); + backdropClick(event: MouseEvent): void { + if (this.backdrop === true && this._elRef.nativeElement === event.target) { + this.dismiss(ModalDismissReasons.BACKDROP_CLICK); } } diff --git a/src/modal/modal.spec.ts b/src/modal/modal.spec.ts index 80af80cbdc..7c9f0c4388 100644 --- a/src/modal/modal.spec.ts +++ b/src/modal/modal.spec.ts @@ -9,9 +9,11 @@ import { OnDestroy, ViewChild } from '@angular/core'; -import {async, ComponentFixture, TestBed} from '@angular/core/testing'; +import {async, ComponentFixture, fakeAsync, TestBed, tick} from '@angular/core/testing'; import {NgbModalConfig} from './modal-config'; import {NgbActiveModal, NgbModal, NgbModalModule, NgbModalRef} from './modal.module'; +import {createKeyEvent} from '../test/common'; +import {Key} from '../util/key'; const NOOP = () => {}; @@ -341,25 +343,27 @@ describe('ngb-modal', () => { }); })); - it('should dismiss modals on ESC in correct order', () => { - fixture.componentInstance.open('foo').result.catch(NOOP); - fixture.componentInstance.open('bar').result.catch(NOOP); - const ngbModalWindow1 = document.querySelectorAll('ngb-modal-window')[0]; - const ngbModalWindow2 = document.querySelectorAll('ngb-modal-window')[1]; - fixture.detectChanges(); - expect(fixture.nativeElement).toHaveModal(['foo', 'bar']); - expect(document.activeElement).toBe(ngbModalWindow2); + it('should dismiss modals on ESC in correct order', fakeAsync(() => { + fixture.componentInstance.open('foo').result.catch(NOOP); + fixture.componentInstance.open('bar').result.catch(NOOP); + const ngbModalWindow1 = document.querySelectorAll('ngb-modal-window')[0]; + const ngbModalWindow2 = document.querySelectorAll('ngb-modal-window')[1]; + fixture.detectChanges(); + expect(fixture.nativeElement).toHaveModal(['foo', 'bar']); + expect(document.activeElement).toBe(ngbModalWindow2); - (getDebugNode(document.activeElement)).triggerEventHandler('keyup.esc', {}); - fixture.detectChanges(); - expect(fixture.nativeElement).toHaveModal(['foo']); - expect(document.activeElement).toBe(ngbModalWindow1); + ngbModalWindow2.dispatchEvent(createKeyEvent(Key.Escape)); + tick(16); // RAF in escape handling + fixture.detectChanges(); + expect(fixture.nativeElement).toHaveModal(['foo']); + expect(document.activeElement).toBe(ngbModalWindow1); - (getDebugNode(document.activeElement)).triggerEventHandler('keyup.esc', {}); - fixture.detectChanges(); - expect(fixture.nativeElement).not.toHaveModal(); - expect(document.activeElement).toBe(document.body); - }); + ngbModalWindow1.dispatchEvent(createKeyEvent(Key.Escape)); + tick(16); // RAF in escape handling + fixture.detectChanges(); + expect(fixture.nativeElement).not.toHaveModal(); + expect(document.activeElement).toBe(document.body); + })); }); describe('backdrop options', () => { @@ -570,29 +574,31 @@ describe('ngb-modal', () => { describe('keyboard options', () => { - it('should dismiss modals on ESC by default', () => { - fixture.componentInstance.open('foo').result.catch(NOOP); - fixture.detectChanges(); - expect(fixture.nativeElement).toHaveModal('foo'); + it('should dismiss modals on ESC by default', fakeAsync(() => { + fixture.componentInstance.open('foo').result.catch(NOOP); + fixture.detectChanges(); + expect(fixture.nativeElement).toHaveModal('foo'); - (getDebugNode(document.querySelector('ngb-modal-window'))).triggerEventHandler('keyup.esc', {}); - fixture.detectChanges(); - expect(fixture.nativeElement).not.toHaveModal(); - }); + document.querySelector('ngb-modal-window').dispatchEvent(createKeyEvent(Key.Escape)); + tick(16); // RAF in escape handling + fixture.detectChanges(); + expect(fixture.nativeElement).not.toHaveModal(); + })); - it('should not dismiss modals on ESC when keyboard option is false', () => { - const modalInstance = fixture.componentInstance.open('foo', {keyboard: false}); - fixture.detectChanges(); - expect(fixture.nativeElement).toHaveModal('foo'); + it('should not dismiss modals on ESC when keyboard option is false', fakeAsync(() => { + const modalInstance = fixture.componentInstance.open('foo', {keyboard: false}); + fixture.detectChanges(); + expect(fixture.nativeElement).toHaveModal('foo'); - (getDebugNode(document.querySelector('ngb-modal-window'))).triggerEventHandler('keyup.esc', {}); - fixture.detectChanges(); - expect(fixture.nativeElement).toHaveModal(); + document.querySelector('ngb-modal-window').dispatchEvent(createKeyEvent(Key.Escape)); + tick(16); // RAF in escape handling + fixture.detectChanges(); + expect(fixture.nativeElement).toHaveModal(); - modalInstance.close(); - fixture.detectChanges(); - expect(fixture.nativeElement).not.toHaveModal(); - }); + modalInstance.close(); + fixture.detectChanges(); + expect(fixture.nativeElement).not.toHaveModal(); + })); it('should not dismiss modals on ESC when default is prevented', () => { const modalInstance = fixture.componentInstance.open('foo', {keyboard: true}); diff --git a/src/util/autoclose.ts b/src/util/autoclose.ts index 7d72fe5f17..8020fee1cb 100644 --- a/src/util/autoclose.ts +++ b/src/util/autoclose.ts @@ -1,6 +1,6 @@ import {NgZone} from '@angular/core'; import {fromEvent, Observable, race} from 'rxjs'; -import {delay, filter, map, takeUntil, withLatestFrom} from 'rxjs/operators'; +import {delay, filter, map, takeUntil, tap, withLatestFrom} from 'rxjs/operators'; import {Key} from './key'; import {closest} from './util'; @@ -39,11 +39,11 @@ export function ngbAutoClose( } }; - const escapes$ = fromEvent(document, 'keydown') + const escapes$ = fromEvent(document, 'keyup') .pipe( takeUntil(closed$), // tslint:disable-next-line:deprecation - filter(e => e.which === Key.Escape)); + filter(e => e.which === Key.Escape), tap(e => e.preventDefault())); // we have to pre-calculate 'shouldCloseOnClick' on 'mousedown/touchstart',