-
Notifications
You must be signed in to change notification settings - Fork 229
fix: scrolling the context menu shouldn't close it COMPASS-9584 #7172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where scrolling within a context menu would incorrectly close the menu. The fix involves checking if the scroll event originates from within the context menu itself before closing it.
- Adds a CSS class name constant for identifying context menu elements
- Modifies the scroll event handler to ignore scroll events from within the context menu
- Applies the CSS class to the context menu component for identification
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/compass-context-menu/src/index.ts | Exports the new contextMenuClassName constant |
packages/compass-context-menu/src/context-menu-provider.tsx | Updates scroll event handler to check if scroll target is within context menu |
packages/compass-context-menu/src/consts.ts | Defines the contextMenuClassName constant |
packages/compass-components/src/components/context-menu.tsx | Applies the contextMenuClassName to the menu component |
(e) => { | ||
const isCompassContextMenu = | ||
e.target instanceof HTMLElement && | ||
e.target.classList.contains(contextMenuClassName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check only verifies if the direct target has the contextMenuClassName. Consider using element.closest() to check if the scroll target is within any ancestor element that has the contextMenuClassName, as scroll events might originate from child elements within the context menu.
e.target.classList.contains(contextMenuClassName); | |
e.target.closest('.' + contextMenuClassName); |
Copilot uses AI. Check for mistakes.
Assigned |
Yeah the capture was intentional because we generally want to close the right click menu whenever it gets scrolled on any other item. Otherwise it'd end up floating awkwardly as you scroll. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me! Thanks for looking into it.
Description
stopPropagation doesn't work because the eventListener on window gets triggered first due to the capture option. I assume the capture was intentional and shouldn't be removed (correct me if I'm wrong @gagik ), but it's okay because we can just do this - ignore based on the target.
Screen.Recording.2025-08-06.at.22.49.15.mov
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes