fix!: debounce updating overflow to improve performance (#9618) (#9639)

Author: web-padawan

packages/menu-bar/src/vaadin-menu-bar-mixin.js

@@ -10,7 +10,9 @@ import { DisabledMixin } from '@vaadin/a11y-base/src/disabled-mixin.js';
 import { FocusMixin } from '@vaadin/a11y-base/src/focus-mixin.js';
 import { isElementFocused, isKeyboardActive } from '@vaadin/a11y-base/src/focus-utils.js';
 import { KeyboardDirectionMixin } from '@vaadin/a11y-base/src/keyboard-direction-mixin.js';
+import { microTask } from '@vaadin/component-base/src/async.js';
 import { ControllerMixin } from '@vaadin/component-base/src/controller-mixin.js';
+import { Debouncer } from '@vaadin/component-base/src/debounce.js';
 import { I18nMixin } from '@vaadin/component-base/src/i18n-mixin.js';
 import { ResizeMixin } from '@vaadin/component-base/src/resize-mixin.js';
 import { SlotController } from '@vaadin/component-base/src/slot-controller.js';
@@ -347,13 +349,7 @@ export const MenuBarMixin = (superClass) =>
       const container = this.shadowRoot.querySelector('[part="container"]');
       container.addEventListener('click', this.__onButtonClick.bind(this));
       container.addEventListener('mouseover', (e) => this._onMouseOver(e));
-
-      // Delay setting container to avoid rendering buttons immediately,
-      // which would also trigger detecting overflow and force re-layout
-      // See https://github.com/vaadin/web-components/issues/7271
-      queueMicrotask(() => {
-        this._container = container;
-      });
+      this._container = container;
     }
 
     /**
@@ -382,7 +378,7 @@ export const MenuBarMixin = (superClass) =>
      * @override
      */
     _onResize() {
-      this.__detectOverflow();
+      this.__scheduleOverflow();
     }
 
     /**
@@ -395,7 +391,7 @@ export const MenuBarMixin = (superClass) =>
     _themeChanged(theme, overflow, container) {
       if (overflow && container) {
         this.__renderButtons(this.items);
-        this.__detectOverflow();
+        this.__scheduleOverflow();
 
         if (theme) {
           overflow.setAttribute('theme', theme);
@@ -415,7 +411,7 @@ export const MenuBarMixin = (superClass) =>
      */
     _reverseCollapseChanged(_reverseCollapse, overflow, container) {
       if (overflow && container) {
-        this.__detectOverflow();
+        this.__scheduleOverflow();
       }
     }
 
@@ -451,7 +447,7 @@ export const MenuBarMixin = (superClass) =>
       if (items !== this._oldItems) {
         this._oldItems = items;
         this.__renderButtons(items);
-        this.__detectOverflow();
+        this.__scheduleOverflow();
       }
 
       if (disabled !== this._oldDisabled) {
@@ -563,11 +559,14 @@ export const MenuBarMixin = (superClass) =>
     }
 
     /** @private */
-    __detectOverflow() {
-      if (!this._container) {
-        return;
-      }
+    __scheduleOverflow() {
+      this._overflowDebouncer = Debouncer.debounce(this._overflowDebouncer, microTask, () => {
+        this.__detectOverflow();
+      });
+    }
 
+    /** @private */
+    __detectOverflow() {
       const overflow = this._overflow;
       const buttons = this._buttons.filter((btn) => btn !== overflow);
       const oldOverflowCount = this.__getOverflowCount(overflow);

packages/menu-bar/test/dom/__snapshots__/menu-bar.test.snap.js

@@ -15,7 +15,7 @@ snapshots["menu-bar basic"] =
     aria-expanded="false"
     aria-haspopup="true"
     role="menuitem"
-    tabindex="-1"
+    tabindex="0"
   >
     Reports
   </vaadin-menu-bar-button>
@@ -31,7 +31,7 @@ snapshots["menu-bar basic"] =
     class="help"
     last-visible=""
     role="menuitem"
-    tabindex="-1"
+    tabindex="0"
   >
     <vaadin-menu-bar-item aria-selected="false">
       <strong>
@@ -46,7 +46,7 @@ snapshots["menu-bar basic"] =
     hidden=""
     role="menuitem"
     slot="overflow"
-    tabindex="-1"
+    tabindex="0"
   >
     <div aria-hidden="true">
       ···

packages/menu-bar/test/dom/menu-bar.test.js

@@ -1,5 +1,6 @@
 import { expect } from '@vaadin/chai-plugins';
-import { fixtureSync, nextRender } from '@vaadin/testing-helpers';
+import { fixtureSync, nextRender, nextResize } from '@vaadin/testing-helpers';
+import '../not-animated-styles.js';
 import '../../src/vaadin-menu-bar.js';
 import '../not-animated-styles.js';
 
@@ -32,7 +33,7 @@ describe('menu-bar', () => {
         className: 'help',
       },
     ];
-    await nextRender();
+    await nextResize(menu);
   });
 
   it('basic', async () => {

packages/menu-bar/test/overflow.test.js

@@ -1,5 +1,6 @@
 import { expect } from '@vaadin/chai-plugins';
 import { arrowRight, fixtureSync, nextRender, nextResize, nextUpdate } from '@vaadin/testing-helpers';
+import sinon from 'sinon';
 import './menu-bar-test-styles.js';
 import '../src/vaadin-menu-bar.js';
 
@@ -29,8 +30,8 @@ describe('overflow', () => {
         </div>
       `);
       menu = wrapper.querySelector('vaadin-menu-bar');
-      await nextRender(menu);
       menu.items = [{ text: 'Item 1' }, { text: 'Item 2' }, { text: 'Item 3' }, { text: 'Item 4' }, { text: 'Item 5' }];
+      await nextResize(menu);
       buttons = menu._buttons;
       overflow = buttons[buttons.length - 1];
     });
@@ -107,9 +108,10 @@ describe('overflow', () => {
       expect(overflow.item.children.length).to.equal(0);
     });
 
-    it('should hide overflow button and reset its items when all buttons fit after changing items', () => {
+    it('should hide overflow button and reset its items when all buttons fit after changing items', async () => {
       // See https://github.com/vaadin/vaadin-menu-bar/issues/133
       menu.items = [{ text: 'Item 1' }, { text: 'Item 2' }];
+      await nextResize(menu);
       buttons = menu._buttons;
       overflow = buttons[2];
       assertVisible(buttons[1]);
@@ -123,7 +125,7 @@ describe('overflow', () => {
       await nextResize(menu);
       expect(overflow.hasAttribute('hidden')).to.be.true;
       menu.setAttribute('theme', 'big');
-      await nextUpdate(menu);
+      await nextResize(menu);
       assertHidden(buttons[3]);
       assertHidden(buttons[4]);
       expect(overflow.hasAttribute('hidden')).to.be.false;
@@ -171,8 +173,9 @@ describe('overflow', () => {
     });
 
     describe('reverse-collapse', () => {
-      beforeEach(() => {
+      beforeEach(async () => {
         menu.reverseCollapse = true;
+        await nextUpdate(menu);
       });
 
       it('should show overflow button and hide the buttons which do not fit', () => {
@@ -194,8 +197,9 @@ describe('overflow', () => {
         expect(overflow.item.children[2]).to.deep.equal(menu.items[2]);
       });
 
-      it('should update overflow when reverseCollapse changes', () => {
+      it('should update overflow when reverseCollapse changes', async () => {
         menu.reverseCollapse = false;
+        await nextUpdate(menu);
         assertVisible(buttons[0]);
         assertVisible(buttons[1]);
         assertHidden(buttons[2]);
@@ -211,7 +215,7 @@ describe('overflow', () => {
     beforeEach(async () => {
       menu = fixtureSync('<vaadin-menu-bar></vaadin-menu-bar>');
       menu.items = [{ text: 'Item 1' }, { text: 'Item 2' }, { text: 'Item 3' }, { text: 'Item 4' }];
-      await nextRender(menu);
+      await nextResize(menu);
       buttons = menu._buttons;
       overflow = buttons[buttons.length - 1];
     });
@@ -247,12 +251,13 @@ describe('overflow', () => {
       await nextResize(menu);
 
       menu.setAttribute('theme', 'big');
-      await nextUpdate(menu);
+      await nextResize(menu);
       expect(menu.hasAttribute('has-single-button')).to.be.true;
     });
 
-    it('should set when changing items to only have one button', () => {
+    it('should set when changing items to only have one button', async () => {
       menu.items = [{ text: 'Actions' }];
+      await nextResize(menu);
       expect(menu.hasAttribute('has-single-button')).to.be.true;
     });
 
@@ -261,13 +266,16 @@ describe('overflow', () => {
       await nextResize(menu);
 
       menu.items = [{ text: 'Actions' }];
+      await nextResize(menu);
       expect(menu.hasAttribute('has-single-button')).to.be.true;
     });
 
-    it('should remove when changing items to have more than one button', () => {
+    it('should remove when changing items to have more than one button', async () => {
       menu.items = [{ text: 'Actions' }];
+      await nextResize(menu);
 
       menu.items = [{ text: 'Edit' }, { text: 'Delete' }];
+      await nextResize(menu);
       expect(menu.hasAttribute('has-single-button')).to.be.false;
     });
   });
@@ -462,4 +470,20 @@ describe('overflow', () => {
       expect(item.classList.contains('test-class-1')).to.be.true;
     });
   });
+
+  describe('performance', () => {
+    let menu, spy;
+
+    beforeEach(() => {
+      menu = fixtureSync('<vaadin-menu-bar></vaadin-menu-bar>');
+      spy = sinon.spy(menu, '_hasOverflow', ['get', 'set']);
+    });
+
+    it('should only detect overflow twice on initial render', async () => {
+      menu.items = [{ text: 'Item 1' }, { text: 'Item 2' }, { text: 'Item 3' }, { text: 'Item 4' }, { text: 'Item 5' }];
+      await nextResize(menu);
+      await nextUpdate(menu);
+      expect(spy.set.callCount).to.equal(2);
+    });
+  });
 });