Skip to content

Commit

Permalink
fix(dropdown): close dropdown correctly inside the OnPush component
Browse files Browse the repository at this point in the history 
Fixes a regression introduced in 2.2.1: click handler didn't mark component as dirty

Closes #2561
Fixes #2559
  • Loading branch information
@maxokorokov
maxokorokov committed Jul 31, 2018
1 parent 30cee77 commit a2dea82
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 43 deletions.
98 changes: 65 additions & 33 deletions src/dropdown/dropdown.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import {TestBed, ComponentFixture, inject} from '@angular/core/testing';
import {ComponentFixture, inject, TestBed} from '@angular/core/testing';
import {createGenericTestComponent, createKeyEvent} from '../test/common';
import {Key} from '../util/key';

import {Component} from '@angular/core';
import {By} from '@angular/platform-browser';
import {ChangeDetectionStrategy, Component} from '@angular/core';

import {NgbDropdownModule} from './dropdown.module';
import {NgbDropdown} from './dropdown';
import {NgbDropdownConfig} from './dropdown-config';

const createTestComponent = (html: string) =>
Expand Down Expand Up @@ -250,7 +248,8 @@ describe('ngb-dropdown', () => {
describe('ngb-dropdown-toggle', () => {
beforeEach(() => {
jasmine.addMatchers(jasmineMatchers);
TestBed.configureTestingModule({declarations: [TestComponent], imports: [NgbDropdownModule.forRoot()]});
TestBed.configureTestingModule(
{declarations: [TestComponent, TestClickCloseOnPush], imports: [NgbDropdownModule.forRoot()]});
});

it('should toggle dropdown on click', () => {
Expand Down Expand Up @@ -316,14 +315,46 @@ describe('ngb-dropdown-toggle', () => {
const compiled = fixture.nativeElement;
const buttonEl = compiled.querySelector('button');

fixture.detectChanges();
expect(compiled).toBeShown();

buttonEl.click();
fixture.detectChanges();
expect(compiled).not.toBeShown();
});

it('should close on inside click, outside click and escape when inside the OnPush component', () => {
const fixture = createTestComponent(`<test-click-close-on-push></test-click-close-on-push>`);
const compiled = fixture.nativeElement;
const toggleEl = compiled.querySelector('button');
const outsideEl = compiled.querySelector('.outside');
const insideEl = compiled.querySelector('.inside');

const reopen = () => {
toggleEl.click();
fixture.detectChanges();
expect(compiled).toBeShown();
};

// inside click
insideEl.click();
fixture.detectChanges();
expect(compiled).not.toBeShown();


// outside click
reopen();
outsideEl.click();
fixture.detectChanges();
expect(compiled).not.toBeShown();


// escape
reopen();
document.dispatchEvent(createFakeEscapeKeyUpEvent());
fixture.detectChanges();
expect(compiled).not.toBeShown();
});

it('should not close on outside click if right button click', () => {
const html = `
<button>Outside</button>
Expand All @@ -336,7 +367,6 @@ describe('ngb-dropdown-toggle', () => {
const compiled = fixture.nativeElement;
const buttonEl = compiled.querySelector('button');

fixture.detectChanges();
expect(compiled).toBeShown();

const evt = document.createEvent('MouseEvent');
Expand All @@ -358,7 +388,6 @@ describe('ngb-dropdown-toggle', () => {
const compiled = fixture.nativeElement;
let buttonEl = compiled.querySelector('button');

fixture.detectChanges();
expect(compiled).toBeShown();

buttonEl.click();
Expand All @@ -367,85 +396,76 @@ describe('ngb-dropdown-toggle', () => {
});

describe('escape closing', () => {

it('should close on ESC from anywhere', () => {
const fixture = createTestComponent(`
<div ngbDropdown [autoClose]="true">
<div ngbDropdown [open]="true" [autoClose]="true">
<button ngbDropdownToggle>Toggle dropdown</button>
<div ngbDropdownMenu></div>
</div>`);
const compiled = fixture.nativeElement;

compiled.querySelector('button').click();
fixture.detectChanges();
const compiled = fixture.nativeElement;
expect(compiled).toBeShown();

document.dispatchEvent(createFakeEscapeKeyUpEvent());
fixture.detectChanges();

expect(compiled).not.toBeShown();
});

it('should close on ESC from the toggling button', () => {
const fixture = createTestComponent(`
<div ngbDropdown [autoClose]="true">
<div ngbDropdown [open]="true" [autoClose]="true">
<button ngbDropdownToggle>Toggle dropdown</button>
<div ngbDropdownMenu></div>
</div>`);
const compiled = fixture.nativeElement;
const buttonElement = compiled.querySelector('button');

buttonElement.click();
fixture.detectChanges();
expect(compiled).toBeShown();

buttonElement.dispatchEvent(createFakeEscapeKeyUpEvent());
fixture.detectChanges();

expect(compiled).not.toBeShown();
});

it('should not close on ESC from the toggling button if autoClose is set to false', () => {
const fixture = createTestComponent(`
<div ngbDropdown [autoClose]="false">
<div ngbDropdown [open]="true" [autoClose]="false">
<button ngbDropdownToggle>Toggle dropdown</button>
<div ngbDropdownMenu></div>
</div>`);
const compiled = fixture.nativeElement;
const buttonElement = compiled.querySelector('button');

buttonElement.click();
fixture.detectChanges();
expect(compiled).toBeShown();

buttonElement.dispatchEvent(createFakeEscapeKeyUpEvent());
fixture.detectChanges();
expect(compiled).toBeShown();

const expectation = expect(compiled);
expectation.toBeShown();
buttonElement.click();
fixture.detectChanges();
expectation.not.toBeShown();
expect(compiled).not.toBeShown();
});

it('should not close on ESC from anywhere if autoClose is set to false', () => {
const fixture = createTestComponent(`
<div ngbDropdown [autoClose]="false">
<div ngbDropdown [open]="true" [autoClose]="false">
<button ngbDropdownToggle>Toggle dropdown</button>
<div ngbDropdownMenu></div>
</div>`);
const compiled = fixture.nativeElement;
const buttonElement = compiled.querySelector('button');

buttonElement.click();
fixture.detectChanges();
expect(compiled).toBeShown();

document.dispatchEvent(createFakeEscapeKeyUpEvent());
fixture.detectChanges();
expect(compiled).toBeShown();

const expectation = expect(compiled);
expectation.toBeShown();
buttonElement.click();
fixture.detectChanges();
expectation.not.toBeShown();
expect(compiled).not.toBeShown();
});
});

Expand All @@ -462,7 +482,6 @@ describe('ngb-dropdown-toggle', () => {
const compiled = fixture.nativeElement;
const linkEl = compiled.querySelector('a');

fixture.detectChanges();
expect(compiled).toBeShown();

linkEl.click();
Expand All @@ -483,7 +502,6 @@ describe('ngb-dropdown-toggle', () => {
const compiled = fixture.nativeElement;
const linkEl = compiled.querySelector('a');

fixture.detectChanges();
expect(compiled).toBeShown();

linkEl.click();
Expand Down Expand Up @@ -545,7 +563,6 @@ describe('ngb-dropdown-toggle', () => {
const buttonEl = compiled.querySelector('button');
const linkEl = compiled.querySelector('a');

fixture.detectChanges();
expect(compiled).toBeShown();

// remains open on item click
Expand Down Expand Up @@ -575,7 +592,6 @@ describe('ngb-dropdown-toggle', () => {
const buttonEl = compiled.querySelector('#outside');
const linkEl = compiled.querySelector('a');

fixture.detectChanges();
expect(compiled).toBeShown();

// remains open on outside click
Expand Down Expand Up @@ -660,3 +676,19 @@ class TestComponent {
this.isOpen = $event;
}
}

@Component({
selector: 'test-click-close-on-push',
changeDetection: ChangeDetectionStrategy.OnPush,
template: `
<div ngbDropdown [open]="true">
<button ngbDropdownToggle>Toggle dropdown</button>
<div ngbDropdownMenu>
<a class="dropdown-item inside">Action</a>
</div>
</div>
<button class="outside">Outside</button>
`
})
class TestClickCloseOnPush {
}
24 changes: 14 additions & 10 deletions src/dropdown/dropdown.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
ChangeDetectorRef
} from '@angular/core';
import {DOCUMENT} from '@angular/common';
import {fromEvent, race} from 'rxjs';
import {fromEvent, race, Subject, Subscription} from 'rxjs';
import {filter, takeUntil} from 'rxjs/operators';
import {NgbDropdownConfig} from './dropdown-config';
import {positionElements, PlacementArray, Placement} from '../util/positioning';
Expand Down Expand Up @@ -106,7 +106,8 @@ export class NgbDropdownToggle extends NgbDropdownAnchor {
*/
@Directive({selector: '[ngbDropdown]', exportAs: 'ngbDropdown', host: {'[class.show]': 'isOpen()'}})
export class NgbDropdown implements OnInit, OnDestroy {
private _zoneSubscription: any;
private _closed$ = new Subject<void>();
private _zoneSubscription: Subscription;

@ContentChild(NgbDropdownMenu) private _menu: NgbDropdownMenu;

Expand Down Expand Up @@ -179,16 +180,15 @@ export class NgbDropdown implements OnInit, OnDestroy {
if (this.autoClose) {
this._ngZone.runOutsideAngular(() => {
const escapes$ = fromEvent<KeyboardEvent>(this._document, 'keyup')
.pipe(
takeUntil(this.openChange.pipe(filter(opened => !opened))),
filter(event => event.which === Key.Escape));
.pipe(takeUntil(this._closed$), filter(event => event.which === Key.Escape));

const clicks$ = fromEvent<MouseEvent>(this._document, 'click')
.pipe(
takeUntil(this.openChange.pipe(filter(opened => !opened))),
filter(event => this._shouldCloseFromClick(event)));
.pipe(takeUntil(this._closed$), filter(event => this._shouldCloseFromClick(event)));

race<Event>([escapes$, clicks$]).subscribe(() => this._ngZone.run(() => this.close()));
race<Event>([escapes$, clicks$]).pipe(takeUntil(this._closed$)).subscribe(() => this._ngZone.run(() => {
this.close();
this._changeDetector.markForCheck();
}));
});
}
}
Expand All @@ -199,6 +199,7 @@ export class NgbDropdown implements OnInit, OnDestroy {
close(): void {
if (this._open) {
this._open = false;
this._closed$.next();
this.openChange.emit(false);
}
}
Expand Down Expand Up @@ -227,7 +228,10 @@ export class NgbDropdown implements OnInit, OnDestroy {
return false;
}

ngOnDestroy() { this._zoneSubscription.unsubscribe(); }
ngOnDestroy() {
this._closed$.next();
this._zoneSubscription.unsubscribe();
}

private _isEventFromToggle($event) { return this._anchor.isEventFrom($event); }

Expand Down

0 comments on commit a2dea82

Please sign in to comment.