Skip to content

Commit

Permalink
Move/cache DOM queries to prevent menu layout trashing
Browse files Browse the repository at this point in the history 
The DOM queries will execute before subsequent DOM modifications
to avoid layout trashing when opening or switching menus.
  • Loading branch information
@krassowski
krassowski committed Oct 5, 2022
1 parent 48e8f32 commit 58f3374
Show file tree
Hide file tree
Showing 6 changed files with 157 additions and 40 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.updateWindowData();

// Clear the current contents of the context menu.
this.menu.clearItems();

Expand Down
111 changes: 87 additions & 24 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 @@ -908,6 +915,17 @@ export class Menu extends Widget {
}
}

/**
* Update window data used for menu positioning in window data 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).
*/
static updateWindowData(): void {
Private.updateWindowData();
}

private _childIndex = -1;
private _activeIndex = -1;
private _openTimerID = 0;
Expand Down Expand Up @@ -1372,6 +1390,50 @@ namespace Private {
*/
export const SUBMENU_OVERLAP = 3;

/**
* How long the window data cache should be kept (ms).
*/
export const WINDOW_DATA_CACHE_DURATION = 1000;

let latestWindowData: IWindowData | null = null;
let latestWindowDataTimestamp: number;

function getLatestWindowData(): IWindowData | null {
const currentTimestamp = Date.now();
if (isWindowDataCacheValid(currentTimestamp)) {
return latestWindowData;
}
return null;
}

function isWindowDataCacheValid(timestamp: number) {
return timestamp - latestWindowDataTimestamp < WINDOW_DATA_CACHE_DURATION;
}

/**
* Update window data cache.
*
* Note: should be called before any DOM modifications.
*/
export function updateWindowData(): void {
const currentTimestamp = Date.now();
if (isWindowDataCacheValid(currentTimestamp)) {
// If cache is still valid, try again as soon as it expires and layout is be ready,
// which drastically reduces the chance of cache timeout occurring just as we start
// showing or switching the menu.
setTimeout(() => {
requestAnimationFrame(() => {
latestWindowData = _getWindowData();
latestWindowDataTimestamp = currentTimestamp;
});
}, WINDOW_DATA_CACHE_DURATION - (currentTimestamp - latestWindowDataTimestamp));
} else {
// Update immediately
latestWindowData = _getWindowData();
latestWindowDataTimestamp = currentTimestamp;
}
}

/**
* Create the DOM node for a menu.
*/
Expand Down Expand Up @@ -1471,6 +1533,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 @@ -1485,10 +1556,11 @@ namespace Private {
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;
const windowData = getLatestWindowData() || _getWindowData();
let px = windowData.pageXOffset;
let py = windowData.pageYOffset;
let cw = windowData.clientWidth;
let ch = windowData.clientHeight;

// Compute the maximum allowed height for the menu.
let maxHeight = ch - (forceY ? y : 0);
Expand All @@ -1498,11 +1570,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,11 +1594,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 All @@ -1541,10 +1608,11 @@ namespace Private {
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;
const windowData = getLatestWindowData() || _getWindowData();
let px = windowData.pageXOffset;
let py = windowData.pageYOffset;
let cw = windowData.clientWidth;
let ch = windowData.clientHeight;

// Compute the maximum allowed height for the menu.
let maxHeight = ch;
Expand All @@ -1554,11 +1622,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 +1654,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
63 changes: 56 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.updateWindowData();
// 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.updateWindowData();

// 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,28 @@ 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 +947,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
14 changes: 6 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,28 @@ 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).to.equal('translate(0px, 10000px)');
});

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 @@ -679,6 +679,7 @@ export class Menu extends Widget {
readonly renderer: Menu.IRenderer;
get rootMenu(): Menu;
triggerActiveItem(): void;
static updateWindowData(): void;
}

// @public
Expand Down

0 comments on commit 58f3374

Please sign in to comment.