fix(animations): don't reflow in 'ngbRunTransition' (#3962)

Replace a generic `reflow()` call in `ngbRunTransition`: - introduce granular per-component reflows only when necessary - take into account `animation` flag for reflows Fixes #3954 Fixes #3952 (cherry picked from commit ad3c9c3)
ng-bootstrap · Jan 21, 2021 · f699999 · f699999
1 parent a006a62
commit f699999
Show file tree

Hide file tree

Showing 10 changed files with 106 additions and 78 deletions.
diff --git a/src/accordion/accordion.ts b/src/accordion/accordion.ts
@@ -348,11 +348,9 @@ export class NgbAccordion implements AfterContentChecked {
         if (panelElement) {
           if (!panel.initClassDone) {
             panel.initClassDone = true;
-            const {classList} = panelElement;
-            classList.add('collapse');
-            if (panel.isOpen) {
-              classList.add('show');
-            }
+            ngbRunTransition(
+                this._ngZone, panelElement, ngbCollapsingTransition,
+                {animation: false, runningTransition: 'stop', context: {direction: panel.isOpen ? 'show' : 'hide'}});
           }
         } else {
           // Classes must be initialized next time it will be in the dom
@@ -397,7 +395,7 @@ export class NgbAccordion implements AfterContentChecked {
     this.activeIds = this.panels.filter(panel => panel.isOpen && !panel.disabled).map(panel => panel.id);
   }
 
-  private _runTransitions(animation: boolean, emitEvent = true) {
+  private _runTransitions(animation: boolean) {
     // detectChanges is performed to ensure that all panels are in the dom (via transitionRunning = true)
     // before starting the animation
     this._changeDetector.detectChanges();
@@ -413,15 +411,13 @@ export class NgbAccordion implements AfterContentChecked {
           context: {direction: panel.isOpen ? 'show' : 'hide'}
         }).subscribe(() => {
           panel.transitionRunning = false;
-          if (emitEvent) {
-            const {id} = panel;
-            if (panel.isOpen) {
-              panel.shown.emit();
-              this.shown.emit(id);
-            } else {
-              panel.hidden.emit();
-              this.hidden.emit(id);
-            }
+          const {id} = panel;
+          if (panel.isOpen) {
+            panel.shown.emit();
+            this.shown.emit(id);
+          } else {
+            panel.hidden.emit();
+            this.hidden.emit(id);
           }
         });
       }

diff --git a/src/carousel/carousel-transition.ts b/src/carousel/carousel-transition.ts
@@ -27,7 +27,7 @@ const removeClasses = ({classList}) => {
 };
 
 export const ngbCarouselTransitionIn: NgbTransitionStartFn<NgbCarouselCtx> =
-    (element: HTMLElement, {direction}: NgbCarouselCtx) => {
+    (element: HTMLElement, animation: boolean, {direction}: NgbCarouselCtx) => {
       const {classList} = element;
       if (isAnimated(element)) {
         // Revert the transition
@@ -46,7 +46,7 @@ export const ngbCarouselTransitionIn: NgbTransitionStartFn<NgbCarouselCtx> =
     };
 
 export const ngbCarouselTransitionOut: NgbTransitionStartFn<NgbCarouselCtx> =
-    (element: HTMLElement, {direction}: NgbCarouselCtx) => {
+    (element: HTMLElement, animation: boolean, {direction}: NgbCarouselCtx) => {
       const {classList} = element;
       //  direction is left or right, depending on the way the slide goes out.
       if (isAnimated(element)) {

diff --git a/src/collapse/collapse.ts b/src/collapse/collapse.ts
@@ -54,14 +54,11 @@ export class NgbCollapse implements OnInit, OnChanges {
     this.animation = config.animation;
   }
 
-  ngOnInit() {
-    this._element.nativeElement.classList.add('collapse');
-    this._runTransition(this.collapsed, false, false);
-  }
+  ngOnInit() { this._runTransition(this.collapsed, false); }
 
   ngOnChanges({collapsed}: SimpleChanges) {
     if (!collapsed.firstChange) {
-      this._runTransition(this.collapsed, this.animation);
+      this._runTransitionWithEvents(this.collapsed, this.animation);
     }
   }
 
@@ -76,21 +73,21 @@ export class NgbCollapse implements OnInit, OnChanges {
   toggle(open: boolean = this.collapsed) {
     this.collapsed = !open;
     this.ngbCollapseChange.next(this.collapsed);
-    this._runTransition(this.collapsed, this.animation);
+    this._runTransitionWithEvents(this.collapsed, this.animation);
+  }
+
+  private _runTransition(collapsed: boolean, animation: boolean) {
+    return ngbRunTransition(
+        this._zone, this._element.nativeElement, ngbCollapsingTransition,
+        {animation, runningTransition: 'stop', context: {direction: collapsed ? 'hide' : 'show'}});
   }
 
-  private _runTransition(collapsed: boolean, animation: boolean, emitEvent = true) {
-    ngbRunTransition(this._zone, this._element.nativeElement, ngbCollapsingTransition, {
-      animation,
-      runningTransition: 'stop',
-      context: {direction: collapsed ? 'hide' : 'show'}
-    }).subscribe(() => {
-      if (emitEvent) {
-        if (collapsed) {
-          this.hidden.emit();
-        } else {
-          this.shown.emit();
-        }
+  private _runTransitionWithEvents(collapsed: boolean, animation: boolean) {
+    this._runTransition(collapsed, animation).subscribe(() => {
+      if (collapsed) {
+        this.hidden.emit();
+      } else {
+        this.shown.emit();
       }
     });
   }

diff --git a/src/modal/modal-backdrop.ts b/src/modal/modal-backdrop.ts
@@ -4,6 +4,7 @@ import {Observable} from 'rxjs';
 import {take} from 'rxjs/operators';
 
 import {ngbRunTransition} from '../util/transition/ngbTransition';
+import {reflow} from '../util/util';
 
 @Component({
   selector: 'ngb-modal-backdrop',
@@ -24,9 +25,12 @@ export class NgbModalBackdrop implements OnInit {
 
   ngOnInit() {
     this._zone.onStable.asObservable().pipe(take(1)).subscribe(() => {
-      ngbRunTransition(
-          this._zone, this._el.nativeElement, ({classList}) => classList.add('show'),
-          {animation: this.animation, runningTransition: 'continue'});
+      ngbRunTransition(this._zone, this._el.nativeElement, (element: HTMLElement, animation: boolean) => {
+        if (animation) {
+          reflow(element);
+        }
+        element.classList.add('show');
+      }, {animation: this.animation, runningTransition: 'continue'});
     });
   }
 

diff --git a/src/modal/modal-window.ts b/src/modal/modal-window.ts
@@ -21,6 +21,7 @@ import {getFocusableBoundaryElements} from '../util/focus-trap';
 import {Key} from '../util/key';
 import {ModalDismissReasons} from './modal-dismiss-reasons';
 import {ngbRunTransition, NgbTransitionOptions} from '../util/transition/ngbTransition';
+import {reflow} from '../util/util';
 
 @Component({
   selector: 'ngb-modal-window',
@@ -96,11 +97,15 @@ export class NgbModalWindow implements OnInit,
   }
 
   private _show() {
-    const {nativeElement} = this._elRef;
     const context: NgbTransitionOptions<any> = {animation: this.animation, runningTransition: 'continue'};
 
     const windowTransition$ =
-        ngbRunTransition(this._zone, nativeElement, () => nativeElement.classList.add('show'), context);
+        ngbRunTransition(this._zone, this._elRef.nativeElement, (element: HTMLElement, animation: boolean) => {
+          if (animation) {
+            reflow(element);
+          }
+          element.classList.add('show');
+        }, context);
     const dialogTransition$ = ngbRunTransition(this._zone, this._dialogEl.nativeElement, () => {}, context);
 
     zip(windowTransition$, dialogTransition$).subscribe(() => {

diff --git a/src/nav/nav-transition.ts b/src/nav/nav-transition.ts
@@ -1,10 +1,14 @@
 import {NgbTransitionStartFn} from '../util/transition/ngbTransition';
+import {reflow} from '../util/util';
 
 export const ngbNavFadeOutTransition: NgbTransitionStartFn = ({classList}) => {
   classList.remove('show');
   return () => classList.remove('active');
 };
 
-export const ngbNavFadeInTransition: NgbTransitionStartFn = (element: HTMLElement) => {
+export const ngbNavFadeInTransition: NgbTransitionStartFn = (element: HTMLElement, animation: boolean) => {
+  if (animation) {
+    reflow(element);
+  }
   element.classList.add('show');
 };
diff --git a/src/toast/toast-transition.ts b/src/toast/toast-transition.ts
@@ -1,7 +1,16 @@
 import {NgbTransitionStartFn} from '../util/transition/ngbTransition';
+import {reflow} from '../util/util';
+
+export const ngbToastFadeInTransition: NgbTransitionStartFn = (element: HTMLElement, animation: true) => {
+  const {classList} = element;
+
+  if (!animation) {
+    classList.add('show');
+    return;
+  }
 
-export const ngbToastFadeInTransition: NgbTransitionStartFn = ({classList}: HTMLElement) => {
   classList.remove('hide');
+  reflow(element);
   classList.add('showing');
 
   return () => {

diff --git a/src/util/transition/ngbCollapseTransition.ts b/src/util/transition/ngbCollapseTransition.ts
@@ -29,10 +29,25 @@ function measureCollapsingElementHeightPx(element: HTMLElement): string {
 }
 
 export const ngbCollapsingTransition: NgbTransitionStartFn<NgbCollapseCtx> =
-    (element: HTMLElement, context: NgbCollapseCtx) => {
+    (element: HTMLElement, animation: boolean, context: NgbCollapseCtx) => {
       let {direction, maxHeight} = context;
       const {classList} = element;
 
+      function setInitialClasses() {
+        classList.add('collapse');
+        if (direction === 'show') {
+          classList.add('show');
+        } else {
+          classList.remove('show');
+        }
+      }
+
+      // without animations we just need to set initial classes
+      if (!animation) {
+        setInitialClasses();
+        return;
+      }
+
       // No maxHeight -> running the transition for the first time
       if (!maxHeight) {
         maxHeight = measureCollapsingElementHeightPx(element);
@@ -55,14 +70,8 @@ export const ngbCollapsingTransition: NgbTransitionStartFn<NgbCollapseCtx> =
       element.style.height = direction === 'show' ? maxHeight : '0px';
 
       return () => {
+        setInitialClasses();
         classList.remove('collapsing');
-        classList.add('collapse');
-        if (direction === 'show') {
-          classList.add('show');
-        } else {
-          classList.remove('show');
-        }
-
         element.style.height = '';
       };
     };
diff --git a/src/util/transition/ngbTransition.spec.ts b/src/util/transition/ngbTransition.spec.ts
@@ -3,6 +3,7 @@ import createSpy = jasmine.createSpy;
 import {Component, ElementRef, NgZone, ViewChild} from '@angular/core';
 import {ComponentFixture, TestBed} from '@angular/core/testing';
 import {isBrowser, isBrowserVisible} from '../../test/common';
+import {reflow} from '../util';
 
 /**
  * This is sometimes necessary only for IE when it fails to recalculate styles synchronously
@@ -31,6 +32,7 @@ if (isBrowserVisible('ngbRunTransition')) {
       component = TestBed.createComponent(TestComponent);
       component.detectChanges();
       element = component.componentInstance.element.nativeElement;
+      reflow(element);
       spyOn(component.componentInstance, 'onTransitionEnd');
 
       zone = TestBed.inject(NgZone);
@@ -299,11 +301,12 @@ if (isBrowserVisible('ngbRunTransition')) {
     });
 
     it(`should create and allow modifying context when running a new transition`, (done) => {
-      const startFn = ({classList}: HTMLElement, context: any) => {
-        classList.remove('ngb-test-show');
-        expect(context.number).toBe(123);
-        context.number = 456;
-      };
+      const startFn: NgbTransitionStartFn<{number}> =
+          ({classList}: HTMLElement, animation: boolean, context: {number}) => {
+            classList.remove('ngb-test-show');
+            expect(context.number).toBe(123);
+            context.number = 456;
+          };
 
       element.classList.add('ngb-test-fade');
 
@@ -321,21 +324,22 @@ if (isBrowserVisible('ngbRunTransition')) {
 
     it(`should create and allow modifying context when running multiple transitions`, (done) => {
       const contextSpy = createSpy();
-      const startFn = ({classList}: HTMLElement, context: any) => {
-        classList.add('ngb-test-during');
-        if (!context.counter) {
-          context.counter = 0;
-        }
-        context.counter++;
-        contextSpy({...context});
-
-        return () => {
-          classList.remove('ngb-test-during');
-          classList.add('ngb-test-after');
-          context.counter = 999;
-          contextSpy({...context});
-        };
-      };
+      const startFn: NgbTransitionStartFn<{counter?, text?}> =
+          ({classList}: HTMLElement, animation: boolean, context: {counter?, text?}) => {
+            classList.add('ngb-test-during');
+            if (!context.counter) {
+              context.counter = 0;
+            }
+            context.counter++;
+            contextSpy({...context});
+
+            return () => {
+              classList.remove('ngb-test-during');
+              classList.add('ngb-test-after');
+              context.counter = 999;
+              contextSpy({...context});
+            };
+          };
 
       element.classList.add('ngb-test-before');
 
@@ -358,7 +362,7 @@ if (isBrowserVisible('ngbRunTransition')) {
     });
 
     it(`should pass context with 'animation: false'`, () => {
-      const startFn: NgbTransitionStartFn<{flag: number}> = (_, context) => { expect(context.flag).toBe(42); };
+      const startFn: NgbTransitionStartFn<{flag: number}> = (_, __, context) => { expect(context.flag).toBe(42); };
 
       const nextSpy = createSpy();
       const errorSpy = createSpy();
@@ -490,6 +494,8 @@ if (isBrowserVisible('ngbRunTransition')) {
       const fixture = TestBed.createComponent(TestComponentNested);
       fixture.detectChanges();
 
+      reflow(fixture.nativeElement);
+
       const outerEl = fixture.componentInstance.outer.nativeElement;
       const innerEl = fixture.componentInstance.inner.nativeElement;
 

diff --git a/src/util/transition/ngbTransition.ts b/src/util/transition/ngbTransition.ts
@@ -3,9 +3,10 @@ import {EMPTY, fromEvent, Observable, of, race, Subject, timer} from 'rxjs';
 import {endWith, filter, takeUntil} from 'rxjs/operators';
 import {getTransitionDurationMs} from './util';
 import {environment} from '../../environment';
-import {reflow, runInZone} from '../util';
+import {runInZone} from '../util';
 
-export type NgbTransitionStartFn<T = any> = (element: HTMLElement, context: T) => NgbTransitionEndFn | void;
+export type NgbTransitionStartFn<T = any> = (element: HTMLElement, animation: boolean, context: T) =>
+    NgbTransitionEndFn | void;
 export type NgbTransitionEndFn = () => void;
 
 export interface NgbTransitionOptions<T> {
@@ -50,11 +51,8 @@ export const ngbRunTransition =
             }
           }
 
-          // A reflow is required here, to be sure that everything is ready,
-          // Without reflow, the transition will not be started for some widgets, at initialization time
-          reflow(element);
-
-          const endFn = startFn(element, context) || noopFn;
+          // Running the start function
+          const endFn = startFn(element, options.animation, context) || noopFn;
 
           // If 'prefer-reduced-motion' is enabled, the 'transition' will be set to 'none'.
           // If animations are disabled, we have to emit a value and complete the observable