kirbydesign / designsystem

Kirby Design System
https://cookbook.kirby.design
MIT License
83 stars 22 forks source link

[BUG] Large performance overhead in floating directive #3351

Closed Tannex closed 6 months ago

Tannex commented 7 months ago

Describe the bug

There is a very large performance overhead in using multiple instances of components that use the floating directive due to position calculations running while the floating ref is hidden.

Describe how to reproduce the bug

Place some kirby-menu's or other components using the floating directive on a page and start scrolling. The performance hit will be visible in the browser performance profiler

Which Kirby version was used?

9.0.0 ## What was the expected behavior?

A much lower performance impact from updating positions of hidden floating elements.

Add any screenshots

Below image is a profiling of a table with a kirby-menu in each row. The areas with red bars are when the table is scrolled. with-kirby-menu

Below is the same table and same interaction pattern, but with the kirby-menu components removed. without-kirby-menu

Please complete the following information:

Are there any additional context?

The following diff should fix most of the issues as far as i can tell. You may wish to investigate further though.

diff --git a/libs/designsystem/shared/floating/src/floating.directive.ts b/libs/designsystem/shared/floating/src/floating.directive.ts
index 265f1e0d4..29add60b8 100644
--- a/libs/designsystem/shared/floating/src/floating.directive.ts
+++ b/libs/designsystem/shared/floating/src/floating.directive.ts
@@ -253,6 +253,8 @@ export class FloatingDirective implements OnInit, OnDestroy {
       return;
     }

+    this.autoUpdatePosition();
+
     this.attachDocumentClickEventHandler();
     this.attachHostClickEventHandler();
     this.renderer.setStyle(this.elementRef.nativeElement, 'display', 'block');
@@ -266,6 +268,8 @@ export class FloatingDirective implements OnInit, OnDestroy {
       return;
     }

+    this.removeAutoUpdaterRef();
+
     this.tearDownDocumentClickEventHandling();
     this.renderer.setStyle(this.elementRef.nativeElement, 'display', 'none');
     this.isShown = false;
@@ -302,7 +306,7 @@ export class FloatingDirective implements OnInit, OnDestroy {
   }

   private updateHostElementPosition(): void {
-    if (!this.reference) {
+    if (!this.reference || !this.isShown) {
       return;
     }

Checklist:

The following tasks should be carried out in sequence in order to follow the process of contributing correctly.

Verification

To make sure the bug is not intended behaviour; it should be verified by a member of team Kirby before moving on to implementation.

Implementation

The contributor who wants to implement this issue should:

Review

Once the issue has been implemented and is ready for review: