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

Focus of menu button on close seems incorrect. #55

Closed
achambers opened this issue Jun 18, 2021 · 3 comments · Fixed by #58
Closed

Focus of menu button on close seems incorrect. #55

achambers opened this issue Jun 18, 2021 · 3 comments · Fixed by #58
Labels
bug Something isn't working
Milestone

Comments

@achambers
Copy link
Collaborator

When I click out side of the menu, say on another button, when the menu closes it sets focus back on the menu button whereas I'd expect the focus to remain on the element I just clicked on. I'm not clear if it makes sense to refocus the button at all. Maybe it does under some circumstances but unsure what they are. In short though, the example below doesn't make sense so there is some improvement that could be made:

In this screencast I'm doing the following:

  1. Click on the 3rd dropdown
  2. Click on the 2nd dropdown
  3. Press up/down keys which opens the 3rd dropdown again.
Screen.Recording.2021-06-18.at.10.36.56.mov

I think the offending code is here:

next(() => {
document.getElementById(this.buttonGuid).focus();
});

@achambers achambers added the bug Something isn't working label Jun 18, 2021
@alexlafroscia
Copy link
Collaborator

Maybe it should only re-focus the button on ESC (once #54 is fixed)? Or if the element clicked on is not itself interactive?

alexlafroscia added a commit to alexlafroscia/ember-headlessui that referenced this issue Jun 26, 2021
From the Headless UI documentation on the Menu:

> Clicking the `MenuButton` toggles the menu and focuses the `MenuItems` component.

Using the focus trap here is a little unintuitive, because the menu item elements themselves are not focusable. However, this is a useful approach because

* Focus should be locked within the menu anyway, per the accessibility requirements of a menu
* Focus needs to be returned to the previously-focused element when the menu closes, which the `focus-trap` modifier will do for us

The documentation for the `focus-trap` library describes how you should handle a case where the element that focus is trapped within has no focusable children, which is to focus the "wrapper" element itself using `initialFocus`, which is the approach taken here!

Closes GavinJoyce#55
alexlafroscia added a commit to alexlafroscia/ember-headlessui that referenced this issue Jun 26, 2021
From the Headless UI documentation on the Menu:

> Clicking the `MenuButton` toggles the menu and focuses the `MenuItems` component.

Using the focus trap here is a little unintuitive, because the menu item elements themselves are not focusable. However, this is a useful approach because

* Focus should be locked within the menu anyway, per the accessibility requirements of a menu
* Focus needs to be returned to the previously-focused element when the menu closes, which the `focus-trap` modifier will do for us

The documentation for the `focus-trap` library describes how you should handle a case where the element that focus is trapped within has no focusable children, which is to focus the "wrapper" element itself using `initialFocus`, which is the approach taken here!

Closes GavinJoyce#55
@achambers
Copy link
Collaborator Author

Maybe it should only re-focus the button on ESC (once #54 is fixed)? Or if the element clicked on is not itself interactive?

Both reasonable suggestions to me. Which do you prefer @alexlafroscia ?

@alexlafroscia
Copy link
Collaborator

#58 fixes this by allowing a focus-trap on the Menu::Items element (which should have a focus trap and currently does not) handle re-focusing the previous element, so the behavior becomes "whatever focustrap.js does by default", which is probably good enough! It seems to work nicely from what I can tell but I haven't tried multiple menus adjacent to each other where we bounce between them. Could you try pulling that PR and seeing if the way it handles things feels good in the example that you showed here?

@alexlafroscia alexlafroscia added this to the v1.0.0 milestone Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants