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

Prevent submenus from closing if mouse leaves and re-enters within leaveDelay ms #300

Closed
wants to merge 1 commit into from

Conversation

synthetiv
Copy link
Contributor

Description

This fixes #299, at least for my use case, by storing and clearing the leaveDelay timeout that is set when the user's cursor exits a submenu.

This has not yet been tested with second-level submenus (i.e. sub-submenus).

Related Issues

#299

@NickDJM NickDJM added bug Something isn't working needs tests This issue or pull request requires tests to be written labels Mar 15, 2024
@NickDJM
Copy link
Owner

NickDJM commented Mar 15, 2024

I've done some manual testing in the demo environment (npm run dev).

This fix works well with Disclosure Menus, Menubars, and Treeviews with single and multi-level menus. It does not work for Top Link Disclosure Menus though. Top Link Disclosure Menus overwrite the base _handleHover() method, so this will need to be added there as well.

Tests will also need to be updated/added to to check for this case.

@synthetiv
Copy link
Contributor Author

Thank you, Nick! I should have some time to tackle this next week.

@NickDJM
Copy link
Owner

NickDJM commented May 8, 2024

@synthetiv I have some time this week to work on the project. Are you still planning on implementing this fix? Or do you want me to hop on it as well?

@NickDJM
Copy link
Owner

NickDJM commented May 10, 2024

I've been looking into this and I've run into a handful of interesting cases that make this a little bit of a bigger issue than expected.

  • Hovering out and then back in to the open menu works fine with this change.
  • Hovering out of the menu, in to the menu's toggle causes the menu to close and then reopen (I have this fixed locally, just need to commit).
  • Hovering out of the menu in to one of the menu's toggle's siblings that is not a submenu toggle and then back in works fine.
  • Hovering out of the menu in to one of the menu's toggle's siblings that is a submenu toggle then back in, closes the menu (I have tried to fix this, but my first attempt broke hover on treeviews, which obviously isn't great).

We'll need to get all those working for all menus and then figure out all the automated tests to write for those... Just a bit more complex than I immediately thought.

@synthetiv
Copy link
Contributor Author

Ah, thanks for picking this thread back up. I've only had time to address this issue for the specific use case where I observed it; back in March I began writing some tests for the other menu types but never got around to finishing them, and it sounds like you've found a few more specific scenarios that need testing.

If/when I do have a moment to dig in further, I will let you know – sorry I haven't so far.

@NickDJM
Copy link
Owner

NickDJM commented May 12, 2024

@synthetiv no worries. I'll be puttering away at this as well (I have a MR against your branch with changes I'm making).

@NickDJM
Copy link
Owner

NickDJM commented May 12, 2024

I was discussing this with a colleague on Friday and we think there might be a need to move the timeout functions to be owned by the toggles themselves. We might need to track which toggle is opening/closing in which case it makes more sense for the toggles to own the events entirely.

I'll need to play around with this a little more though.

@NickDJM
Copy link
Owner

NickDJM commented Jun 20, 2024

Ok I have it all worked out I think. I moved _hoverTimeout to the menu items. This allows each individual menu item to control it's own timeouts, which looks like it works excellently.

Given this is moving a protected field that shouldn't really be used outside of the menu itself, I think this doesn't really warrant a breaking change.

This is going to be a breaking change, so it'll need to be a new major.

The following still need to be done:

  • Add documentation for the new hoverTimeout (theres a getter/setter in menuItem now so menus can properly access it).
  • Add tests for the new getter/setters.
  • Add tests for the new interactions for entering/leaving menu items.
    • Mouse enters item, leaves, re-enters item.
    • Mouse enters item, leaves, enters non-submenu sibling, leaves sibling, re-enters item.
    • Mouse enters item, leaves, enters submenu toggle sibling, leaves sibling, re-enters item.
    • Mouse enters item, leaves, enters parent's non-submenu sibling, leaves sibling, re-enters item.
    • Mouse enters item, leaves enters parent's submenu sibling, leaves sibling, re-enters item.

@NickDJM
Copy link
Owner

NickDJM commented Jun 20, 2024

@synthetiv Are you able to give me commit privileges to your repo? It'll make it easier to run the automated tests.

@NickDJM
Copy link
Owner

NickDJM commented Jun 24, 2024

Moving all work to #314

@NickDJM
Copy link
Owner

NickDJM commented Jun 24, 2024

Closing this. I pulled all the changes made here into the other MR and it has been merged.

@NickDJM NickDJM closed this Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs tests This issue or pull request requires tests to be written
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: disclosure submenus close unexpectedly when hover is on and mouse leaves and re-enters
2 participants