Skip to content

fix: add keyboard support for context menu#1172

Open
annahaa874 wants to merge 6 commits intoglideapps:mainfrom
Workfront:fix-context-menu-keyboard-support
Open

fix: add keyboard support for context menu#1172
annahaa874 wants to merge 6 commits intoglideapps:mainfrom
Workfront:fix-context-menu-keyboard-support

Conversation

@annahaa874
Copy link

@annahaa874 annahaa874 commented Jan 26, 2026

Problem Statement

The data grid previously supported opening context menus only via mouse right-click.
Keyboard users were unable to open context menus using standard shortcuts.

At first glance, handling the contextmenu event appears sufficient. However, this assumption breaks down on macOS, where keyboard shortcuts do not fire a contextmenu event.

Root Cause

The issue has two contributing factors:

Split event handling architecture

  • Keyboard events are handled on the canvas element
  • Context menu events are handled on the scroller div
  • Keyboard-triggered context menu events would target the canvas, which had no listener

Platform limitation on macOS

  • macOS does not fire contextmenu events from keyboard shortcuts

Relying solely on contextmenu listeners is insufficient

Changes

  • Added a Shift + F10 keybinding to explicitly open the context menu
  • The handler Invokes the existing onContextMenu callback with event args
  • Added unit tests to verify changes

@annahaa874 annahaa874 force-pushed the fix-context-menu-keyboard-support branch from 0ac9112 to a5c32ac Compare January 26, 2026 13:41
}
}
}
console.log({event, details}, "keys.contextMenu:", keys.contextMenu, overlayOpen )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this console log.

buttons: 0
}, cancel)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the else if block should be here. You put it in the rangeSelect === "rect" || rangeSelect === "multi-rect" if clause.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since rangeSelect can be "none" | "cell" | "rect" | "multi-cell" | "multi-rect", I placed the new condition above it.

Comment on lines 3397 to 3401
location: [col, row],
bounds: event.bounds,
ctrlKey: false,
metaKey: false,
shiftKey: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these properties (location, bounds, ctrlKey, metaKey, and shiftKey) exist in the event object. I recommend using those values instead.

The rowMarkerOffset needs to be considered for the col in location. But I think that is already handled in the location property in the event object.

Comment on lines 3395 to 3396
localEventX: event.bounds.width / 2,
localEventY: event.bounds.height / 2,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is not important for a keyboard event and there is no mouse pointer position, I suggest setting localEventX and localEventY to NaN.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a keyboard event and there is no mouse pointer position, localEventX and localEventY are used to control where the context menu opens. To ensure it opens in the center of the cell, I set them to the cell’s midpoint.

@annahaa874 annahaa874 force-pushed the fix-context-menu-keyboard-support branch from 1d7fe8e to 20afebe Compare February 3, 2026 10:51
@annahaa874 annahaa874 force-pushed the fix-context-menu-keyboard-support branch from 3a86574 to aa8e457 Compare February 3, 2026 11:04
@annahaa874 annahaa874 force-pushed the fix-context-menu-keyboard-support branch from 3c121fa to 8b65628 Compare February 3, 2026 11:49
Copy link
Contributor

@citizensas citizensas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants