fix(overlay): fix click trigger

Closes #907
akveo · Oct 16, 2018 · 3a89b69 · 3a89b69
1 parent a85eaf2
commit 3a89b69
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 88 deletions.
diff --git a/src/framework/theme/components/cdk/overlay/overlay-trigger.spec.ts b/src/framework/theme/components/cdk/overlay/overlay-trigger.spec.ts
@@ -7,17 +7,29 @@ import { NB_DOCUMENT } from '../../../theme.options';
 import createSpy = jasmine.createSpy;
 
 
+const withContainer = el => () => ({ location: { nativeElement: el } }) as ComponentRef<any>;
+const createElement = (name = 'div') => {
+  const el = document.createElement(name);
+  document.body.appendChild(el);
+  return el;
+};
+const click = el => el.dispatchEvent(new Event('click', { bubbles: true }));
+const mouseMove = el => el.dispatchEvent(new Event('mousemove'));
+const mouseEnter = el => el.dispatchEvent(new Event('mouseenter'));
+const mouseLeave = el => el.dispatchEvent(new Event('mouseleave'));
+const focus = el => el.dispatchEvent(new Event('focusin', { bubbles: true }));
+const blur = el => el.dispatchEvent(new Event('focusout', { bubbles: true }));
+const tab = (el) => el.dispatchEvent(new KeyboardEvent('keydown', <any> {
+  bubbles: true,
+  keyCode: 9,
+}));
+
 describe('click-trigger-strategy', () => {
   let triggerStrategyBuilder: NbTriggerStrategyBuilder;
   let document: Document;
   let host: HTMLElement;
   let container: HTMLElement;
 
-  const withContainer = el => () => ({ location: { nativeElement: el } }) as ComponentRef<any>;
-
-  const click = el => el.dispatchEvent(new Event('click'));
-
-  const createElement = () => document.createElement('div');
 
   beforeEach(() => {
     const bed = TestBed.configureTestingModule({ providers: [{ provide: NB_DOCUMENT, useExisting: DOCUMENT }] });
@@ -26,7 +38,7 @@ describe('click-trigger-strategy', () => {
 
   beforeEach(() => {
     host = createElement();
-    container = createElement();
+    container = createElement('li');
     triggerStrategyBuilder = new NbTriggerStrategyBuilder()
       .document(document)
       .trigger(NbTrigger.CLICK)
@@ -73,16 +85,6 @@ describe('hover-trigger-strategy', () => {
   let host: HTMLElement;
   let container: HTMLElement;
 
-  const withContainer = el => () => ({ location: { nativeElement: el } }) as ComponentRef<any>;
-
-  const mouseMove = el => el.dispatchEvent(new Event('mousemove'));
-
-  const mouseEnter = el => el.dispatchEvent(new Event('mouseenter'));
-
-  const mouseLeave = el => el.dispatchEvent(new Event('mouseleave'));
-
-  const createElement = () => document.createElement('div');
-
   beforeEach(() => {
     const bed = TestBed.configureTestingModule({ providers: [{ provide: NB_DOCUMENT, useExisting: DOCUMENT }] });
     document = bed.get(NB_DOCUMENT);
@@ -131,14 +133,6 @@ describe('hint-trigger-strategy', () => {
   let host: HTMLElement;
   let container: HTMLElement;
 
-  const withContainer = el => () => ({ location: { nativeElement: el } }) as ComponentRef<any>;
-
-  const mouseEnter = el => el.dispatchEvent(new Event('mouseenter'));
-
-  const mouseLeave = el => el.dispatchEvent(new Event('mouseleave'));
-
-  const createElement = () => document.createElement('div');
-
   beforeEach(() => {
     const bed = TestBed.configureTestingModule({ providers: [{ provide: NB_DOCUMENT, useExisting: DOCUMENT }] });
     document = bed.get(NB_DOCUMENT);
@@ -173,21 +167,6 @@ describe('focus-trigger-strategy', () => {
   let host: HTMLElement;
   let container: HTMLElement;
 
-  const withContainer = el => () => ({ location: { nativeElement: el } }) as ComponentRef<any>;
-
-  const focus = el => el.dispatchEvent(new Event('focusin', { bubbles: true }));
-
-  const blur = el => el.dispatchEvent(new Event('focusout', { bubbles: true }));
-
-  const click = el => el.dispatchEvent(new Event('click', { bubbles: true }));
-
-  const tab = (el, target) => el.dispatchEvent(new KeyboardEvent('keydown', <any> {
-    target: target,
-    keyCode: 9,
-  }));
-
-  const createElement = () => document.createElement('div');
-
   beforeEach(() => {
     const bed = TestBed.configureTestingModule({ providers: [{ provide: NB_DOCUMENT, useExisting: DOCUMENT }] });
     document = bed.get(NB_DOCUMENT);
@@ -228,7 +207,7 @@ describe('focus-trigger-strategy', () => {
   it('should fire hide$ when tab pressed', done => {
     const triggerStrategy = triggerStrategyBuilder.build();
     triggerStrategy.hide$.subscribe(done);
-    tab(document, host);
+    tab(host);
   });
 
   it('should fire hide$ when focusout', done => {

diff --git a/src/framework/theme/components/cdk/overlay/overlay-trigger.ts b/src/framework/theme/components/cdk/overlay/overlay-trigger.ts
@@ -1,6 +1,7 @@
-import { fromEvent as observableFromEvent, merge as observableMerge, Observable, Subject } from 'rxjs';
-import { debounceTime, delay, filter, repeat, switchMap, takeUntil, takeWhile } from 'rxjs/operators';
+import { fromEvent as observableFromEvent, merge as observableMerge, Observable } from 'rxjs';
+import { debounceTime, delay, filter, repeat, share, switchMap, takeUntil, takeWhile } from 'rxjs/operators';
 import { ComponentRef } from '@angular/core';
+import { map } from 'rxjs/operators';
 
 
 export enum NbTrigger {
@@ -20,6 +21,23 @@ export enum NbTrigger {
  * Renderer provides capability use it in service worker, ssr and so on.
  * */
 export abstract class NbTriggerStrategy {
+
+  protected isNotOnHostOrContainer(event: Event): boolean {
+    return !this.isOnHost(event) && !this.isOnContainer(event);
+  }
+
+  protected isOnHostOrContainer(event: Event): boolean {
+    return this.isOnHost(event) || this.isOnContainer(event);
+  }
+
+  protected isOnHost({ target }: Event): boolean {
+    return this.host.contains(target as Node);
+  }
+
+  protected isOnContainer({ target }: Event): boolean {
+    return this.container() && this.container().location.nativeElement.contains(target);
+  }
+
   abstract show$: Observable<Event>;
   abstract hide$: Observable<Event>;
 
@@ -34,41 +52,29 @@ export abstract class NbTriggerStrategy {
  * not on the host or container.
  * */
 export class NbClickTriggerStrategy extends NbTriggerStrategy {
-  protected show: Subject<Event> = new Subject();
-  readonly show$: Observable<Event> = this.show.asObservable();
 
-  protected hide: Subject<Event> = new Subject();
-  readonly hide$: Observable<Event> = observableMerge(
-    this.hide.asObservable(),
-    observableFromEvent<Event>(this.document, 'click')
-      .pipe(filter((event: Event) => this.isNotHostOrContainer(event))),
-  );
-
-  constructor(protected document: Document, protected host: HTMLElement, protected container: () => ComponentRef<any>) {
-    super(document, host, container);
-    this.subscribeOnHostClick();
-  }
-
-  protected subscribeOnHostClick() {
-    observableFromEvent(this.host, 'click')
-      .subscribe((event: Event) => {
-        if (this.isContainerExists()) {
-          this.hide.next(event);
-        } else {
-          this.show.next(event);
-        }
-      })
-  }
+  // since we should track click for both SHOW and HIDE event we firstly need to track the click and the state
+  // of the container and then later on decide should we hide it or show
+  // if we track the click & state separately this will case a behavior when the container is getting shown
+  // and then hidden right away
+  protected click$: Observable<any[]> = observableFromEvent<Event>(this.document, 'click')
+    .pipe(
+      map(event => [!this.container() && this.isOnHost(event), event]),
+      share(),
+    );
 
-  protected isContainerExists(): boolean {
-    return !!this.container();
-  }
+  readonly show$: Observable<Event> = this.click$
+    .pipe(
+      filter(([shouldShow]) => shouldShow),
+      map(([, event]) => event),
+    );
 
-  protected isNotHostOrContainer(event: Event): boolean {
-    return !this.host.contains(event.target as Node)
-      && this.isContainerExists()
-      && !this.container().location.nativeElement.contains(event.target as Node);
-  }
+  readonly hide$: Observable<Event> = this.click$
+    .pipe(
+      filter(([shouldShow]) => !shouldShow),
+      map(([, event]) => event),
+      filter((event: Event) => !this.isOnContainer(event)),
+    );
 }
 
 /**
@@ -91,8 +97,7 @@ export class NbHoverTriggerStrategy extends NbTriggerStrategy {
         .pipe(
           debounceTime(100),
           takeWhile(() => !!this.container()),
-          filter(event => !this.host.contains(event.target as Node)
-            && !this.container().location.nativeElement.contains(event.target as Node),
+          filter(event => !this.isOnHostOrContainer(event),
           ),
         ),
       ),
@@ -109,6 +114,8 @@ export class NbHintTriggerStrategy extends NbTriggerStrategy {
     .pipe(
       delay(100),
       takeUntil(observableFromEvent(this.host, 'mouseleave')),
+      // this `delay & takeUntil & repeat` operators combination is a synonym for `conditional debounce`
+      // meaning that if one event occurs in some time afther the initial one we won't react to it
       repeat(),
     );
 
@@ -123,18 +130,6 @@ export class NbHintTriggerStrategy extends NbTriggerStrategy {
  * */
 export class NbFocusTriggerStrategy extends NbTriggerStrategy {
 
-  protected isNotOnHostOrContainer(event: Event): boolean {
-    return !this.isOnHost(event) && !this.isOnContainer(event);
-  }
-
-  protected isOnHost({ target }: Event): boolean {
-    return this.host.contains(target as Node);
-  }
-
-  protected isOnContainer({ target }: Event): boolean {
-    return this.container() && this.container().location.nativeElement.contains(target);
-  }
-
   protected focusOut$: Observable<Event> = observableFromEvent<Event>(this.host, 'focusout')
     .pipe(
       switchMap(() => observableFromEvent<Event>(this.document, 'focusin')