fix(popover): run change detection only when the Esc is clicked

vmware-clarity · Mar 21, 2022 · 144947b · 144947b
1 parent 4414c17
commit 144947b
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 9 deletions.
diff --git a/projects/angular/src/popover/common/abstract-popover.spec.ts b/projects/angular/src/popover/common/abstract-popover.spec.ts
@@ -4,7 +4,7 @@
  * The full license information can be found in LICENSE in the root directory of this project.
  */
 
-import { Component, ElementRef, Injector, Optional, ViewChild } from '@angular/core';
+import { ApplicationRef, Component, ElementRef, Injector, Optional, ViewChild } from '@angular/core';
 import { ComponentFixture, TestBed } from '@angular/core/testing';
 import { By } from '@angular/platform-browser';
 
@@ -71,7 +71,7 @@ describe('Abstract Popover', function () {
   describe('Keyboard Events', () => {
     beforeEach(() => {
       TestBed.configureTestingModule({ declarations: [TestPopover], providers: [ClrPopoverToggleService] });
-      toggleService = TestBed.get(ClrPopoverToggleService);
+      toggleService = TestBed.inject(ClrPopoverToggleService);
       toggleService.open = true;
       fixture = TestBed.createComponent(TestPopover);
       fixture.detectChanges();
@@ -84,6 +84,20 @@ describe('Abstract Popover', function () {
 
       expect(toggleService.open).toBe(false);
     });
+
+    it('should not run change detection when any button is pressed except ESC', () => {
+      const appRef = TestBed.inject(ApplicationRef);
+      spyOn(appRef, 'tick').and.callThrough();
+
+      document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Shift' }));
+      expect(appRef.tick).not.toHaveBeenCalled();
+
+      document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Tab' }));
+      expect(appRef.tick).not.toHaveBeenCalled();
+
+      document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' }));
+      expect(appRef.tick).toHaveBeenCalled();
+    });
   });
 
   describe('Popover with clrIfOpen Directive', () => {
@@ -93,7 +107,7 @@ describe('Abstract Popover', function () {
         imports: [ClrConditionalModule],
         providers: [ClrPopoverToggleService],
       });
-      toggleService = TestBed.get(ClrPopoverToggleService);
+      toggleService = TestBed.inject(ClrPopoverToggleService);
       fixture = TestBed.createComponent(TestPopoverWithIfOpenDirective);
       fixture.detectChanges();
     });
@@ -144,7 +158,7 @@ describe('Abstract Popover', function () {
   describe('Open behavior', () => {
     beforeEach(() => {
       TestBed.configureTestingModule({ declarations: [TestPopover], providers: [ClrPopoverToggleService] });
-      toggleService = TestBed.get(ClrPopoverToggleService);
+      toggleService = TestBed.inject(ClrPopoverToggleService);
       toggleService.open = true;
       fixture = TestBed.createComponent(TestPopover);
       fixture.detectChanges();

diff --git a/projects/angular/src/popover/common/abstract-popover.ts b/projects/angular/src/popover/common/abstract-popover.ts
@@ -13,6 +13,8 @@ import {
   Renderer2,
   SkipSelf,
   Directive,
+  NgZone,
+  ChangeDetectorRef,
 } from '@angular/core';
 import { Subscription } from 'rxjs';
 
@@ -28,6 +30,8 @@ export abstract class AbstractPopover implements AfterViewChecked, OnDestroy {
     this.el = injector.get(ElementRef);
     this.toggleService = injector.get(ClrPopoverToggleService);
     this.renderer = injector.get(Renderer2);
+    this.ngZone = injector.get(NgZone);
+    this.ref = injector.get(ChangeDetectorRef);
     // Default anchor is the parent host
     this.anchorElem = parentHost.nativeElement;
 
@@ -50,6 +54,8 @@ export abstract class AbstractPopover implements AfterViewChecked, OnDestroy {
   protected el: ElementRef;
   protected toggleService: ClrPopoverToggleService;
   protected renderer: Renderer2;
+  protected ngZone: NgZone;
+  protected ref: ChangeDetectorRef;
 
   private popoverInstance: Popover;
   private subscription: Subscription;
@@ -105,24 +111,31 @@ export abstract class AbstractPopover implements AfterViewChecked, OnDestroy {
    * a separate directive on the host. So let's do dirty but performant for now.
    */
   public closeOnOutsideClick = false;
-  private documentESCListener: () => void;
+  private documentESCListener: VoidFunction | null = null;
 
   private attachESCListener(): void {
-    if (!this.popoverOptions.ignoreGlobalESCListener) {
+    if (this.popoverOptions.ignoreGlobalESCListener) {
+      return;
+    }
+
+    this.ngZone.runOutsideAngular(() => {
       this.documentESCListener = this.renderer.listen('document', 'keydown', event => {
         if (event && event.key) {
           if (event.key === 'Escape' || event.key === 'Esc') {
-            this.toggleService.open = false;
+            this.ngZone.run(() => {
+              this.toggleService.open = false;
+              this.ref.markForCheck();
+            });
           }
         }
       });
-    }
+    });
   }
 
   private detachESCListener(): void {
     if (this.documentESCListener) {
       this.documentESCListener();
-      delete this.documentESCListener;
+      this.documentESCListener = null;
     }
   }