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

Chore/update menu accessibility #631

Merged

Conversation

bennymi
Copy link
Contributor

@bennymi bennymi commented Dec 3, 2022

Before submitting the PR:

  • Does your PR reference an issue? If not, please chat to the team on Discord or GitHub before submission.
  • Did you update and run tests before submission using npm run test?
  • Does your branch follow our naming convention? If not, please amend the branch name using branch -m new-branch-name
  • Did you update documentation related to your new feature or changes?

What does your PR address?

Improves menu accessibility by including up and down arrow key functionality and being able to open and close the menu by clicking the menu button. This Resolves #616.

When the menu is opened, using the arrow down key starts at the top and allows the user the move to the next menu items downwards.

When the menu is opened with a click and the arrow up key is used, the last menu item gets focused first.

Tips

  • Tap "convert to draft" to indicate this is work in progress.
  • Link to an issue using the verbiage Fixes #XX
  • Linked issues will auto-close when the PR is merged.

@vercel
Copy link

vercel bot commented Dec 3, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
skeleton-docs ✅ Ready (Inspect) Visit Preview Dec 6, 2022 at 5:18PM (UTC)

@endigo9740
Copy link
Contributor

Awesome thanks @bennymi. It's Saturday for me which is my off day, but I'll give this a proper review tomorrow. Excited to try it out!

@bennymi
Copy link
Contributor Author

bennymi commented Dec 3, 2022

Hey, no worries Chris. Enjoy your day off!

@endigo9740
Copy link
Contributor

@bennymi Hey, maybe I'm doing something wrong here, but I can't seem to get this to work. From my understanding when a menu is open I should be able to use the up/down arrows to navigate up/down the list of focusable elements within the menu. Is this correct?

Currently we have a couple menus in the documentation that should support this. The "Features" drop down menu at the top of the page, in the App Bar. Plus the Navigation demo on the Menu docs page. Neither behave in this manner when tested though. The page just scrolls slightly up and down. A couple considerations:

  • I assumed it was a small bug that focus was not set correctly when the menu opens, but no, even manually adjusting focus I can't seem to get this to work.
  • It looks like you're only setting the list of focusable elements during init. However, you might make considerations to update this list each time the menu opens. What if the contents of the menu are reactive and change between viewing? Then your original list if focusable elements is outdated.
  • Additionally, if you are introducing keyboard interaction, make sure to update the documentation accordingly. No one will know about this feature if we don't tell them it's enabled.

Screen Shot 2022-12-05 at 10 41 12 AM

For now I'll drop this PR into a draft state. Tap the "ready for review" action on this page when adjustments have been made and this is ready for another review pass.

@endigo9740 endigo9740 marked this pull request as draft December 5, 2022 16:51
@bennymi
Copy link
Contributor Author

bennymi commented Dec 5, 2022

@endigo9740 thank you for the feedback. I updated the documentation and made sure that up and down arrow keys no longer cause scrolling when a menu is open. But I'm still stuck on one problem. The up and down arrow keys do work when clicking on a menu in both Firefox and Chrome in my testing, but the focused element is only highlighted in Firefox, not in Chrome.

This only happens when a click is used to open the menu. When Enter is used to open a menu after it was focused, then the menu items are also highlighted in Chrome.

Firefox after clicking on a menu and using the arrow up key:
image

Chrome after clicking on a menu and using the arrow up key:
image

Do you have an idea what could be the reason here?

@endigo9740
Copy link
Contributor

endigo9740 commented Dec 6, 2022

@bennymi Interesting, so I'll need to review our focus styling for these soon. That's odd because tab focus workings fine in Chrome. Ok I'm going to mark this ready for review and do another pass with this info.

Thanks for updating the keyboard interactions as well!

@endigo9740 endigo9740 marked this pull request as ready for review December 6, 2022 00:56
@bennymi
Copy link
Contributor Author

bennymi commented Dec 6, 2022

@endigo9740 yes it's strange behavior, because when I tab to a menu button and hit enter and the arrow up key I do get focus styling in Chrome as well:

image

It just doesn't work when the menu is clicked and then the arrows are used.

@bennymi
Copy link
Contributor Author

bennymi commented Dec 6, 2022

@endigo9740 if I add focus-visible:border-2 focus-visible:border-primary-500 to the anchor elements in the menu it also shows up in chrome:

image

image

Maybe we can adjust the focus styling for elements to have it work on Chrome as well.

@endigo9740
Copy link
Contributor

@bennymi I've added some focus styles for anchors limited to within the menu for now. This should work cross browser. We may need to expand the scope over time, but this was working for our AppBar dropdown menu. With that I'm good to merge this! Thanks for your contribution and working with me on this!

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.

Improve accessibility functions of Menus
2 participants