Skip to content

Commit

Permalink
pkg/lib: fix context menu component popdown
Browse files Browse the repository at this point in the history
f9b0166 changed the popdown behaviour of the context menu.

Before that commit, the previous code was wrong.  It was:

     const wasOutside = !(event.target.contains === root.current);

     if (wasOutside)
         setVisible(false);

but because `event.target.contains` is a function, the comparison was
always false and wasOutside would always be true.  This meant that any
left click would always pop the menu down.

The replacement code was:

    const wasOutside = !event.target.contains(root.current);

which at least looks like it does something meaningful.  Unfortunately
this is also wrong, but in the other direction: this means that the menu
will only pop down if it's not contained inside of the item you've
clicked on.  If you click on a large top-level <div> (ie: something
which effectively contains "most things") then the menu will remain
shown.  That's definitely wrong.

We seemed to be happy with the behaviour before the original change, but
the code has a purpose: it allows clicking on content of the menu (like
on a divider, for example) without dismissing it.  Let's try to
implement the intended original behaviour: we should dismiss the menu
only if the element that we clicked on is outside of the menu:

    const wasOutside = root.current && !root.current.contains(event.target);

Fixes #20649
  • Loading branch information
allisonkarlitskaya committed Jun 24, 2024
1 parent 6c5597f commit 5581e5d
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion pkg/lib/cockpit-components-context-menu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export const ContextMenu = ({ parentId, children } : {

const _handleClick = (event: MouseEvent) => {
if (event && event.button === 0 && event.target instanceof HTMLElement) {
const wasOutside = !event.target.contains(root.current);
const wasOutside = root.current && !root.current.contains(event.target);

if (wasOutside)
setVisible(false);
Expand Down

0 comments on commit 5581e5d

Please sign in to comment.