Skip to content

Commit

Permalink
Avoid menu layout trashing by moving DOM queries (jupyterlab#432)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@krassowski
krassowski authored Oct 7, 2022
1 parent 5faf82f commit 55e87ca
Show file tree
Hide file tree
Showing 6 changed files with 152 additions and 44 deletions.
5 changes: 4 additions & 1 deletion packages/virtualdom/src/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
3 changes: 3 additions & 0 deletions packages/widgets/src/contextmenu.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
106 changes: 78 additions & 28 deletions packages/widgets/src/menu.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -826,6 +833,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();

Expand Down Expand Up @@ -908,6 +918,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;
Expand Down Expand Up @@ -1372,6 +1395,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.
*/
Expand Down Expand Up @@ -1471,6 +1520,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.
*/
Expand All @@ -1481,15 +1539,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);

Expand All @@ -1498,11 +1557,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.
Expand All @@ -1526,26 +1581,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;

Expand All @@ -1554,11 +1609,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.
Expand Down Expand Up @@ -1590,11 +1641,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';
}

/**
Expand Down
62 changes: 55 additions & 7 deletions packages/widgets/src/menubar.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -519,8 +519,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);
}
}

Expand All @@ -545,15 +548,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.
*/
Expand All @@ -570,7 +597,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) {
Expand All @@ -591,20 +618,27 @@ export class MenuBar extends Widget {
if (oldMenu) {
oldMenu.close();
} else {
this.addClass('lm-mod-active');
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');
}

// 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);
}
}

Expand Down Expand Up @@ -912,6 +946,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.
*/
Expand Down
19 changes: 11 additions & 8 deletions packages/widgets/tests/src/menu.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -582,30 +582,33 @@ 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.startsWith('translate(0px, ')
).to.equal(true);
expect(menu.node.style.transform.endsWith(', 10000px)')).to.equal(
false
);
});

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)');
});
});

Expand Down
1 change: 1 addition & 0 deletions review/api/widgets.api.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -678,6 +678,7 @@ export class Menu extends Widget {
removeItemAt(index: number): void;
readonly renderer: Menu.IRenderer;
get rootMenu(): Menu;
static saveWindowData(): void;
triggerActiveItem(): void;
}

Expand Down

0 comments on commit 55e87ca

Please sign in to comment.