From e938ee1f866ee0fbd14a806ca2c1c81e006ed6f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micha=C5=82=20Krassowski?=
 <5832902+krassowski@users.noreply.github.com>
Date: Fri, 7 Oct 2022 08:55:02 +0100
Subject: [PATCH] Backport PR #432: Avoid menu layout trashing by moving DOM
 queries

* Move/cache DOM queries to prevent menu layout trashing

The DOM queries will execute before subsequent DOM modifications
to avoid layout trashing when opening or switching menus.

* Use simpler transient cache, move update requests

(cherry picked from commit 55e87caa277ac3ae3d15b8c5da75e78d54b069ff)
---
 packages/virtualdom/src/index.ts        |   5 +-
 packages/widgets/src/contextmenu.ts     |   3 +
 packages/widgets/src/menu.ts            | 106 +++++++++++++++++-------
 packages/widgets/src/menubar.ts         |  68 ++++++++++++---
 packages/widgets/tests/src/menu.spec.ts |  17 ++--
 review/api/widgets.api.md               |   1 +
 6 files changed, 153 insertions(+), 47 deletions(-)

diff --git a/packages/virtualdom/src/index.ts b/packages/virtualdom/src/index.ts
index 1f344edc6..c3c57b43c 100644
--- a/packages/virtualdom/src/index.ts
+++ b/packages/virtualdom/src/index.ts
@@ -1349,7 +1349,10 @@ namespace Private {
 
       // Handle the simplest case of in-place text update first.
       if (oldVNode.type === 'text' && newVNode.type === 'text') {
-        currElem!.textContent = newVNode.content;
+        // Avoid spurious updates for performance.
+        if (currElem!.textContent !== newVNode.content) {
+          currElem!.textContent = newVNode.content;
+        }
         currElem = currElem!.nextSibling;
         continue;
       }
diff --git a/packages/widgets/src/contextmenu.ts b/packages/widgets/src/contextmenu.ts
index d62ea1ab5..de1cf936b 100644
--- a/packages/widgets/src/contextmenu.ts
+++ b/packages/widgets/src/contextmenu.ts
@@ -78,6 +78,9 @@ export class ContextMenu {
    * position indicated by the event.
    */
   open(event: MouseEvent): boolean {
+    // Prior to any DOM modifications update the window data.
+    Menu.saveWindowData();
+
     // Clear the current contents of the context menu.
     this.menu.clearItems();
 
diff --git a/packages/widgets/src/menu.ts b/packages/widgets/src/menu.ts
index 9d51d9eca..f1613da23 100644
--- a/packages/widgets/src/menu.ts
+++ b/packages/widgets/src/menu.ts
@@ -32,6 +32,13 @@ import {
 
 import { Widget } from './widget';
 
+interface IWindowData {
+  pageXOffset: number;
+  pageYOffset: number;
+  clientWidth: number;
+  clientHeight: number;
+}
+
 /**
  * A widget which displays items as a canonical menu.
  */
@@ -829,6 +836,9 @@ export class Menu extends Widget {
       return;
     }
 
+    // Prior to any DOM modifications save window data
+    Menu.saveWindowData();
+
     // Ensure the current child menu is closed.
     this._closeChildMenu();
 
@@ -911,6 +921,19 @@ export class Menu extends Widget {
     }
   }
 
+  /**
+   * Save window data used for menu positioning in transient cache.
+   *
+   * In order to avoid layout trashing it is recommended to invoke this
+   * method immediately prior to opening the menu and any DOM modifications
+   * (like closing previously visible menu, or adding a class to menu widget).
+   *
+   * The transient cache will be released upon `open()` call.
+   */
+  static saveWindowData(): void {
+    Private.saveWindowData();
+  }
+
   private _childIndex = -1;
   private _activeIndex = -1;
   private _openTimerID = 0;
@@ -1439,6 +1462,32 @@ namespace Private {
    */
   export const SUBMENU_OVERLAP = 3;
 
+  let transientWindowDataCache: IWindowData | null = null;
+  let transientCacheCounter: number = 0;
+
+  function getWindowData(): IWindowData {
+    // if transient cache is in use, take one from it
+    if (transientCacheCounter > 0) {
+      transientCacheCounter--;
+      return transientWindowDataCache!;
+    }
+    return _getWindowData();
+  }
+
+  /**
+   * Store window data in transient cache.
+   *
+   * The transient cache will be released upon `getWindowData()` call.
+   * If this function is called multiple times, the cache will be
+   * retained until as many calls to `getWindowData()` were made.
+   *
+   * Note: should be called before any DOM modifications.
+   */
+  export function saveWindowData(): void {
+    transientWindowDataCache = _getWindowData();
+    transientCacheCounter++;
+  }
+
   /**
    * Create the DOM node for a menu.
    */
@@ -1541,6 +1590,15 @@ namespace Private {
     return result;
   }
 
+  function _getWindowData(): IWindowData {
+    return {
+      pageXOffset: window.pageXOffset,
+      pageYOffset: window.pageYOffset,
+      clientWidth: document.documentElement.clientWidth,
+      clientHeight: document.documentElement.clientHeight
+    };
+  }
+
   /**
    * Open a menu as a root menu at the target location.
    */
@@ -1551,15 +1609,16 @@ namespace Private {
     forceX: boolean,
     forceY: boolean
   ): void {
+    // Get the current position and size of the main viewport.
+    const windowData = getWindowData();
+    let px = windowData.pageXOffset;
+    let py = windowData.pageYOffset;
+    let cw = windowData.clientWidth;
+    let ch = windowData.clientHeight;
+
     // Ensure the menu is updated before attaching and measuring.
     MessageLoop.sendMessage(menu, Widget.Msg.UpdateRequest);
 
-    // Get the current position and size of the main viewport.
-    let px = window.pageXOffset;
-    let py = window.pageYOffset;
-    let cw = document.documentElement.clientWidth;
-    let ch = document.documentElement.clientHeight;
-
     // Compute the maximum allowed height for the menu.
     let maxHeight = ch - (forceY ? y : 0);
 
@@ -1568,11 +1627,7 @@ namespace Private {
     let style = node.style;
 
     // Clear the menu geometry and prepare it for measuring.
-    style.top = '';
-    style.left = '';
-    style.width = '';
-    style.height = '';
-    style.visibility = 'hidden';
+    style.opacity = '0';
     style.maxHeight = `${maxHeight}px`;
 
     // Attach the menu to the document.
@@ -1596,26 +1651,26 @@ namespace Private {
     }
 
     // Update the position of the menu to the computed position.
-    style.top = `${Math.max(0, y)}px`;
-    style.left = `${Math.max(0, x)}px`;
+    style.transform = `translate(${Math.max(0, x)}px, ${Math.max(0, y)}px`;
 
     // Finally, make the menu visible on the screen.
-    style.visibility = '';
+    style.opacity = '1';
   }
 
   /**
    * Open a menu as a submenu using an item node for positioning.
    */
   export function openSubmenu(submenu: Menu, itemNode: HTMLElement): void {
+    // Get the current position and size of the main viewport.
+    const windowData = getWindowData();
+    let px = windowData.pageXOffset;
+    let py = windowData.pageYOffset;
+    let cw = windowData.clientWidth;
+    let ch = windowData.clientHeight;
+
     // Ensure the menu is updated before opening.
     MessageLoop.sendMessage(submenu, Widget.Msg.UpdateRequest);
 
-    // Get the current position and size of the main viewport.
-    let px = window.pageXOffset;
-    let py = window.pageYOffset;
-    let cw = document.documentElement.clientWidth;
-    let ch = document.documentElement.clientHeight;
-
     // Compute the maximum allowed height for the menu.
     let maxHeight = ch;
 
@@ -1624,11 +1679,7 @@ namespace Private {
     let style = node.style;
 
     // Clear the menu geometry and prepare it for measuring.
-    style.top = '';
-    style.left = '';
-    style.width = '';
-    style.height = '';
-    style.visibility = 'hidden';
+    style.opacity = '0';
     style.maxHeight = `${maxHeight}px`;
 
     // Attach the menu to the document.
@@ -1660,11 +1711,10 @@ namespace Private {
     }
 
     // Update the position of the menu to the computed position.
-    style.top = `${Math.max(0, y)}px`;
-    style.left = `${Math.max(0, x)}px`;
+    style.transform = `translate(${Math.max(0, x)}px, ${Math.max(0, y)}px`;
 
     // Finally, make the menu visible on the screen.
-    style.visibility = '';
+    style.opacity = '1';
   }
 
   /**
diff --git a/packages/widgets/src/menubar.ts b/packages/widgets/src/menubar.ts
index 0a88bfb10..922dfef9f 100644
--- a/packages/widgets/src/menubar.ts
+++ b/packages/widgets/src/menubar.ts
@@ -523,8 +523,11 @@ export class MenuBar extends Widget {
       this._closeChildMenu();
       this.activeIndex = index;
     } else {
+      const position = this._positionForMenu(index);
+      Menu.saveWindowData();
+      // Begin DOM modifications.
       this.activeIndex = index;
-      this._openChildMenu();
+      this._openChildMenu(position);
     }
   }
 
@@ -549,15 +552,39 @@ export class MenuBar extends Widget {
       return;
     }
 
+    // Get position for the new menu >before< updating active index.
+    const position = this._positionForMenu(index);
+
+    // Before any modification, update window data.
+    Menu.saveWindowData();
+
+    // Begin DOM modifications.
+
     // Update the active index to the hovered item.
     this.activeIndex = index;
 
     // Open the new menu if a menu is already open.
     if (this._childMenu) {
-      this._openChildMenu();
+      this._openChildMenu(position);
     }
   }
 
+  /**
+   * Find initial position for the menu based on menubar item position.
+   *
+   * NOTE: this should be called before updating active index to avoid
+   * an additional layout and style invalidation as changing active
+   * index modifies DOM.
+   */
+  private _positionForMenu(index: number): Private.IPosition {
+    let itemNode = this.contentNode.children[index];
+    let { left, bottom } = (itemNode as HTMLElement).getBoundingClientRect();
+    return {
+      top: bottom,
+      left
+    };
+  }
+
   /**
    * Handle the `'mouseleave'` event for the menu bar.
    */
@@ -574,7 +601,7 @@ export class MenuBar extends Widget {
    * If a different child menu is already open, it will be closed,
    * even if there is no active menu.
    */
-  private _openChildMenu(): void {
+  private _openChildMenu(options: { left?: number; top?: number } = {}): void {
     // If there is no active menu, close the current menu.
     let newMenu = this.activeMenu;
     if (!newMenu) {
@@ -595,23 +622,30 @@ export class MenuBar extends Widget {
     if (oldMenu) {
       oldMenu.close();
     } else {
-      this.addClass('lm-mod-active');
-      /* <DEPRECATED> */
-      this.addClass('p-mod-active');
-      /* </DEPRECATED> */
       document.addEventListener('mousedown', this, true);
     }
 
     // Ensure the menu bar is updated and look up the item node.
     MessageLoop.sendMessage(this, Widget.Msg.UpdateRequest);
-    let itemNode = this.contentNode.children[this._activeIndex];
 
     // Get the positioning data for the new menu.
-    let { left, bottom } = (itemNode as HTMLElement).getBoundingClientRect();
+    let { left, top } = options;
+    if (typeof left === 'undefined' || typeof top === 'undefined') {
+      ({ left, top } = this._positionForMenu(this._activeIndex));
+    }
+    // Begin DOM modifications
+
+    if (!oldMenu) {
+      // Continue setup for new menu
+      this.addClass('lm-mod-active');
+      /* <DEPRECATED> */
+      this.addClass('p-mod-active');
+      /* </DEPRECATED> */
+    }
 
     // Open the new menu at the computed location.
     if (newMenu.items.length > 0) {
-      newMenu.open(left, bottom, this._forceItemsPosition);
+      newMenu.open(left, top, this._forceItemsPosition);
     }
   }
 
@@ -961,6 +995,20 @@ namespace Private {
     return node;
   }
 
+  /**
+   * Position for the menu relative to top-left screen corner.
+   */
+  export interface IPosition {
+    /**
+     * Pixels right from screen origin.
+     */
+    left: number;
+    /**
+     * Pixels down from screen origin.
+     */
+    top: number;
+  }
+
   /**
    * The results of a mnemonic search.
    */
diff --git a/packages/widgets/tests/src/menu.spec.ts b/packages/widgets/tests/src/menu.spec.ts
index ed8dc4199..f47729a3e 100644
--- a/packages/widgets/tests/src/menu.spec.ts
+++ b/packages/widgets/tests/src/menu.spec.ts
@@ -558,30 +558,31 @@ describe('@lumino/widgets', () => {
       it('should open the menu at the specified location', () => {
         menu.addItem({ command: 'test' });
         menu.open(10, 10);
-        expect(menu.node.style.left).to.equal('10px');
-        expect(menu.node.style.top).to.equal('10px');
+        expect(menu.node.style.transform).to.equal('translate(10px, 10px)');
       });
 
       it('should be adjusted to fit naturally on the screen', () => {
         menu.addItem({ command: 'test' });
         menu.open(-10, 10000);
-        expect(menu.node.style.left).to.equal('0px');
-        expect(menu.node.style.top).to.not.equal('10000px');
+        expect(menu.node.style.transform.indexOf('translate(0px, ')).to.equal(
+          0
+        );
+        expect(menu.node.style.transform.indexOf(', 10000px)')).to.equal(-1);
       });
 
       it('should accept flags to force the location', () => {
         menu.addItem({ command: 'test' });
         menu.open(10000, 10000, { forceX: true, forceY: true });
-        expect(menu.node.style.left).to.equal('10000px');
-        expect(menu.node.style.top).to.equal('10000px');
+        expect(menu.node.style.transform).to.equal(
+          'translate(10000px, 10000px)'
+        );
       });
 
       it('should bail if already attached', () => {
         menu.addItem({ command: 'test' });
         menu.open(10, 10);
         menu.open(100, 100);
-        expect(menu.node.style.left).to.equal('10px');
-        expect(menu.node.style.top).to.equal('10px');
+        expect(menu.node.style.transform).to.equal('translate(10px, 10px)');
       });
     });
 
diff --git a/review/api/widgets.api.md b/review/api/widgets.api.md
index da5c9d8c9..c8b242d69 100644
--- a/review/api/widgets.api.md
+++ b/review/api/widgets.api.md
@@ -584,6 +584,7 @@ export class Menu extends Widget {
     removeItemAt(index: number): void;
     readonly renderer: Menu.IRenderer;
     readonly rootMenu: Menu;
+    static saveWindowData(): void;
     triggerActiveItem(): void;
 }