Skip to content
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

pkg/lib: fix context menu component popdown #20650

Merged

Conversation

allisonkarlitskaya
Copy link
Member

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

(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

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 cockpit-project#20649
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks! I assume you interactively tested this, as we obviously don't cover this with tests.

@allisonkarlitskaya
Copy link
Member Author

Ya, I did a fair amount of interactive testing to bisect the issue in the first place and then to decide what the correct behaviour ought to be, and I did test this result.

I'd probably need some help to develop a proper test case for this...

@allisonkarlitskaya allisonkarlitskaya merged commit d599d62 into cockpit-project:main Jun 24, 2024
80 checks passed
@allisonkarlitskaya allisonkarlitskaya deleted the context-popdown branch June 24, 2024 06:54
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.

Context menu component no longer pops down
2 participants